fixed favorite and size issues on main detail view. #26552

mjobst-necls · 2016-11-05T15:28:36Z

Description

The missing information on detail view is being reloaded from now. Because there is no action for the click on the star button registered at this point we need to create a local action. This makes the favorite button work on the detail view.
I also did some refactoring of the property names in the javascript code to provide a better overview and to avoid misspellings in the future.

Related Issue

#26437
#26241 : The names of the share recipients still diseappeard on view Shared with others

Motivation and Context

The size and the favorite star were not shown correctly on the details view when openened by any share list. By this commit, the needed information is reloaded when the detail view is open and the information is missing.

How Has This Been Tested?

Tests during development in the browser on current master branch version.
Running the javascript unitTest

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

enhanced js test file removed css superscript attribute for version size

some refactoring

mention-bot · 2016-11-05T15:28:37Z

@mjobst-necls, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @DeepDiver1975 and @MorrisJobke to be potential reviewers.

DeepDiver1975 · 2016-11-05T19:19:25Z

@PVince81 js-fu - your discipline - THX

mjobst-necls · 2016-11-07T10:58:56Z

I just realized that the test has to be modified due to the additional getSize call. I'm going to change it today evening.

The method getSize can be called when this property is requested.

PVince81 · 2016-11-07T15:56:45Z

core/js/files/client.js

+	OC.CLIENT.NS_DAV = 'DAV:';
+
+	OC.CLIENT.PROPERTY = {};
+	OC.CLIENT.PROPERTY.ACTORDISPLAYNAME		= '{' + OC.CLIENT.NS_OWNCLOUD + '}actorDisplayName';


we can't list all possibly Webdav properties here. Some are supposed to be only known by apps/plugins like the comments app.

PVince81 · 2016-11-07T15:57:22Z

core/js/files/client.js

@@ -59,43 +59,83 @@
 		this._client = new dav.Client(clientOptions);
 		this._client.xhrProvider = _.bind(this._xhrProvider, this);
 	};
+	OC.CLIENT = {};


Capitalized class name doesn't look good. Can we put these constants on OC.Files.Client instead ?

Yep, you're right. I'm going to change this.

PVince81 · 2016-11-07T15:59:40Z

core/js/files/client.js

+	OC.CLIENT.PROPERTY.USERASSIGNABLE		= '{' + OC.CLIENT.NS_OWNCLOUD + '}user-assignable';
+	OC.CLIENT.PROPERTY.USERVISIBLE			= '{' + OC.CLIENT.NS_OWNCLOUD + '}user-visible';
+	OC.CLIENT.PROPERTY.CHECKSUMS			= '{' + OC.CLIENT.NS_OWNCLOUD + '}checksums';
+	OC.CLIENT.PROPERTY.DATA_FINGERPRINT		= '{' + OC.CLIENT.NS_OWNCLOUD + '}data-fingerprint';


while I appreciate the intention of declaring constants for every property, I'd still prefer if we keep the constants in the scope of the respective apps. So in the OC.Files.Client namespace only define the base properties known for files.

Then every app define their own constants. They could optionally extend this namespace so the props still all appear in the same one:
_.extend(OC.Files.Client, { PROPERTY_COMMENTS_UNREAD: '...', ...})

Not sure if it's a good idea though.

Ok, I'll check whether this is possible ... but it should be ...
I just want to avoid duplicate code =)

PVince81 · 2016-11-07T16:01:36Z

apps/files/js/mainfileinfodetailview.js

@@ -117,6 +126,80 @@
 			return baseUrl + OC.generateUrl('/f/{fileId}', {fileId: fileId});
 		},

+		_createLocalFavoriteActionIfNotExisting: function(actionName){


The problem with this is that the MainFileInfoView now needs to know about the favorite plugin/action.

Ideally we need a mechanism to inject elements into this view from outside so that the TagsPlugin can inject the star itself.
The current impl didn't do that yet unfortunately.

PVince81 · 2016-11-07T16:05:17Z

apps/files/js/mainfileinfodetailview.js

+				}
+
+				if( properties.length > 0){
+					this._fileList.reloadProperties(fileInfo, properties);


It shouldn't be needed to reload the properties.

Whenever a model properties gets modified with "model.set()", a "change" event is triggered in which the list of properties to update can be seen.

I think the TagsPlugin needs to listen to the model's event:
model.on('change:tags', function() { ... } and in the handler it should tell the file list to rerender the row with the updated information by calling fileList.updateRow(...).

Actually there was already such code for other operations but a bit hacky: https://github.com/owncloud/core/blob/master/apps/files/js/filelist.js#L445. Not sure why that one doesn't work when updating tags ?

The problem is, that the size and the tags (also favorite tag) are not already in the file info, because they were not retrieved by the list fill api call. I think that behavior is correct until now, because the information is not shown in the list. It's just needed for the details view.
( Ok, we also need the favorite information because of issue #19753. I'm going to work on that for this PR, but shouldn't we use the favoritesplugin in this case? Or, at least, extract the actions to reuse them on the sharing views?)

The reloadProperties() function retrieves the missing properties from the server by webdav and adds them to the model.

The change event is getting triggered and the row gets updated and rerendered after reloading the properties.
Can you please verify whether my approach about reloading missing file properties is good or not or do you know another method how to do this?

Ah, right... The favorite info is usually available on the regular PROPFIND but not when calling the share API.

Here there are three solutions:

Your solution, but it involves doing a PROPFIND for every entry in the list, it will become expensive when the list is long

Modify the OCS Share API to also return size and favorite in its response. What I don't like here is that one might not always need the "favorite" info and that one is more expensive to resolve on DB level. The advantage of PROPFIND is that you can choose what you need.

Create a new Webdav API for sharing which aligns with the Webdav files API, which would make it possible to add such properties.

regarding 1) That's not correct. My approach just reloads the missing properties for the selected entry, when the Details view is open.
regarding 2) Considering #19753 this would be the best approach for a quick solution without need to create a new API because the information is needed for every entry in the list. It should be possible to hide/show the favorite column as a user setting and add a parameter to the OCS Share API to control which values should be sent.
regarding 3) Would be the best approach for a long term solution.

and 3) are quite equal but 2) would be easier for me, because I don't know anything about creating a new api endpoint in owncloud, yet.

PVince81 · 2016-11-07T16:07:46Z

@mjobst-necls great to see you diving deeper into the mess that is the files app code 😄

However I think the approach you proposed will make it a bit more messy and introduce additional coupling. Have a look at my comments and let's see if we can find a better way to make it work.

mjobst-necls · 2016-11-07T17:23:49Z

@PVince81 I'm sometimes wondering why oc is doing quite everything on it's own. There are a plenty of solutions like AngularJS doing jobs better. JQuery, for example, is also used, so I guess you don't want to avoid using 3rd Party libraries. The design could also be more responsive by using bootstrap. Bootstrap and AngularJS are both under MIT license so that shouldn't be a problem, either.

I'm just interested in the why?.

mjobst-necls · 2016-11-07T17:35:36Z

@PVince81 I also realized that the file propiertes are added to the tr element as "data-..." attributes and the list reads that attributes to create a new FileInfo object when updating a File.

Are the "data-..." attributes really needed or
Would it be better just to update the model and recreate (or modify) the tr element by using the information of the model?

AngularJS, for example, provides two way binding. The whole setting and getting properties and also the rerendering is done without extra code.

PVince81 · 2016-11-07T19:18:41Z

I'm just interested in the why

Because this code is very old and never got to properly clean it up fully.
And Angular JS has a too steep learning curve which would make it difficult for new contributors, so rather use something easier.

I was looking at using Backbone JS a bit more in this PR #25909. Goal is at least to separate the model from the view.

If possible I'd eventually want to get rid of the "data-*" attributes and keep everything on the model.

Would it be better just to update the model and recreate (or modify) the tr element by using the information of the model?

Yes, would love that. See #25909

mjobst-necls · 2016-11-07T19:55:34Z

Because this code is very old and never got to properly clean it up fully.

Ok. that's an argument. Indeed, introducing a library has to be done step by step.

And Angular JS has a too steep learning curve which would make it difficult for new contributors, so rather use something easier.

Is it possible to use a user developed files-app instead of the provided files app in owncloud (selectable by the admin)?
Maybe, it would be an interesting study whether people rather use/contribute to low-level or higher-level developed apps.

…mjobst-necls/core into fix_favorite_from_sidebar-patch

Added missing function description

DeepDiver1975 · 2016-11-10T08:16:18Z

That PR is based on #26583

is merged - @mjobst-necls can you please rebase this branch? THX a lot! Nice work!

some refactoring

The method getSize can be called when this property is requested.

some refactoring

DeepDiver1975 · 2016-11-10T12:12:48Z

please try (asuming you have this repo setup as remote names upstream 😉 )

git fetch upstream
git rebase upstream/master

Duplicates because of rebase mistage

mjobst-necls · 2016-11-10T12:20:29Z

It is already rebased, isn't it?

DeepDiver1975 · 2016-11-10T12:23:50Z

It is already rebased, isn't it?

Now yes - I guess old commits have been shown to be due to missing update on the github page - I'm sorry

mjobst-necls · 2016-11-10T12:25:29Z

Oh. wait. there still seems to be an rebase mistake ...

mjobst-necls · 2016-11-10T12:28:34Z

Wrong alert. I just haven't pulled the client.js so that karma test failed.

mjobst-necls · 2016-11-10T15:02:50Z

Now yes - I guess old commits have been shown to be due to missing update on the github page - I'm sorry

The old commits still seem to show up on rebase, but on files changed only the real changes for this issue are shown.

PVince81 · 2016-11-10T15:06:21Z

If they show up it still means that on merge they're likely to appear in the log, additionally.

PVince81 · 2016-11-10T15:06:55Z

@mjobst-necls easiest would be to restart with a new PR, then cherry-pick only the new commits there.

DeepDiver1975 · 2016-11-10T15:07:36Z

@mjobst-necls easiest would be to restart with a new PR, then cherry-pick only the new commits there.

we can also squash on merge ....

mjobst-necls · 2016-11-10T15:09:41Z

Ok. I create a new PR ...

lock · 2019-08-04T10:02:09Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mjobst-necls added 6 commits October 29, 2016 21:14

Display changes in file versions tab view and detailsView

80a90f0

versions tab enhancements

863c339

enhanced js test file removed css superscript attribute for version size

Replaced spaces with tabs

999db0f

Merge remote-tracking branch 'upstream/master'

b9c3fd2

Fixed disappearing of share information

c9ed0c3

fixed favorite and size issues on main detail view.

9bcb215

some refactoring

mjobst-necls added 2 commits November 5, 2016 17:24

shared by link fix

93e6ba8

fixed Scrutinizer issue

580f3a1

PVince81 added the 3 - To Review label Nov 7, 2016

Update FilesPluginTest.php

dd43d28

The method getSize can be called when this property is requested.

PVince81 reviewed Nov 7, 2016

View reviewed changes

mjobst-necls added 5 commits November 8, 2016 18:52

Refactoring client api property constants

c2f1d9f

Merge branch 'fix_favorite_from_sidebar-patch' of https://github.com/…

f76df27

…mjobst-necls/core into fix_favorite_from_sidebar-patch

share api expanded by tags

59f8c3a

Modified files_sharing JS Unit tests

926fd86

modified tests. renamed request parameter. refactoring

2d0ab56

mjobst-necls mentioned this pull request Nov 9, 2016

share api expanded by tags #26583

Merged

9 tasks

Update Share20OCS.php

ff1d63a

Added missing function description

DeepDiver1975 added this to the 9.2 milestone Nov 10, 2016

mjobst-necls and others added 12 commits November 10, 2016 11:56

Merge branch 'master' of https://github.com/owncloud/core

e895eb6

fixed favorite and size issues on main detail view.

1f723ff

some refactoring

shared by link fix

41565e7

fixed Scrutinizer issue

5d74ac2

Update FilesPluginTest.php

b10c3d0

The method getSize can be called when this property is requested.

Refactoring client api property constants

06d9322

share api expanded by tags

29eee69

modified tests. renamed request parameter. refactoring

e35a236

fixed favorite and size issues on main detail view.

bc6440c

some refactoring

Refactoring client api property constants

4335902

Rebased on fix_show_favorites_indicator_in_shared_views-patch branch

f3ab4b3

merge

d58167d

Update client.js

f11d9a2

Duplicates because of rebase mistage

mjobst-necls closed this Nov 10, 2016

This was referenced Nov 10, 2016

Fix favorite and size in details view patch #26602

Closed

Fixed size issues on main detail view and disappearing of share recipients #26603

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed favorite and size issues on main detail view. #26552

fixed favorite and size issues on main detail view. #26552

mjobst-necls commented Nov 5, 2016 •

edited

Loading

mention-bot commented Nov 5, 2016

DeepDiver1975 commented Nov 5, 2016

mjobst-necls commented Nov 7, 2016

PVince81 Nov 7, 2016

PVince81 Nov 7, 2016

mjobst-necls Nov 7, 2016

PVince81 Nov 7, 2016

mjobst-necls Nov 7, 2016

PVince81 Nov 7, 2016

PVince81 Nov 7, 2016

mjobst-necls Nov 7, 2016

PVince81 Nov 8, 2016

mjobst-necls Nov 8, 2016 •

edited

Loading

PVince81 commented Nov 7, 2016

mjobst-necls commented Nov 7, 2016

mjobst-necls commented Nov 7, 2016

PVince81 commented Nov 7, 2016

mjobst-necls commented Nov 7, 2016 •

edited

Loading

DeepDiver1975 commented Nov 10, 2016

DeepDiver1975 commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

DeepDiver1975 commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

PVince81 commented Nov 10, 2016

PVince81 commented Nov 10, 2016

DeepDiver1975 commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

lock bot commented Aug 4, 2019

fixed favorite and size issues on main detail view. #26552

fixed favorite and size issues on main detail view. #26552

Conversation

mjobst-necls commented Nov 5, 2016 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Nov 5, 2016

DeepDiver1975 commented Nov 5, 2016

mjobst-necls commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjobst-necls Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

PVince81 commented Nov 7, 2016

mjobst-necls commented Nov 7, 2016

mjobst-necls commented Nov 7, 2016

PVince81 commented Nov 7, 2016

mjobst-necls commented Nov 7, 2016 • edited Loading

DeepDiver1975 commented Nov 10, 2016

DeepDiver1975 commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

DeepDiver1975 commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

PVince81 commented Nov 10, 2016

PVince81 commented Nov 10, 2016

DeepDiver1975 commented Nov 10, 2016

mjobst-necls commented Nov 10, 2016

lock bot commented Aug 4, 2019

mjobst-necls commented Nov 5, 2016 •

edited

Loading

mjobst-necls Nov 8, 2016 •

edited

Loading

mjobst-necls commented Nov 7, 2016 •

edited

Loading