Fixed size issues on main detail view and disappearing of share recipients by mjobst-necls · Pull Request #26603 · owncloud/core

mjobst-necls · 2016-11-10T17:53:52Z

This is a clean PR refering to #26552

…ients

mention-bot · 2016-11-10T17:53:54Z

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

PVince81

Code looks good otherwise.

I haven't tested yet (will need to retest all affected components).

See me comments and let me know what you think.

PVince81 · 2016-11-10T18:01:26Z

+		PROPERTY_OBJECTID:	'{' + OC.Files.Client.NS_OWNCLOUD + '}objectId',
+		PROPERTY_OBJECTTYPE:	'{' + OC.Files.Client.NS_OWNCLOUD + '}objectType',
+		PROPERTY_ACTORDISPLAYNAME:	'{' + OC.Files.Client.NS_OWNCLOUD + '}actorDisplayName',
+		PROPERTY_CREATEIONDATETIME:	'{' + OC.Files.Client.NS_OWNCLOUD + '}creationDateTime'


creation, not create-ion ? 😉

PVince81 · 2016-11-10T18:02:06Z

+			'creationDateTime': OC.Files.Client.PROPERTY_CREATEIONDATETIME,
+			'objectType': 		OC.Files.Client.PROPERTY_OBJECTTYPE,
+			'objectId':			OC.Files.Client.PROPERTY_OBJECTID,
+			'isUnread':			OC.Files.Client.PROPERTY_ISUNREAD


weird alignment here, I suggest to not tab or indent after the ":" and let it flow 😄

Ok, in the editor it looks better with an tab size of 4. I'll change this and take it in future works into account.

PVince81 · 2016-11-10T18:03:53Z

 				return $displayName;
 			});
+			$propFind->handle(self::SIZE_PROPERTYNAME, function() use ($node) {
+				return $node->getSize();


The "oc:size" property is only used for folders. For files we use "DAV:getcontentlength" instead.

Did you add it for convenience ?

No, I just thought the oc:size is the correct property to retrieve the size of an object.
I added the oc:size property to the webdav request payload to retrieve the size and nothing was returned. It must be dependent of another property.

oc:size is also in the request payload of the webdav API:
<?xml version="1.0"?> <d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns"> <d:prop> <d:getlastmodified /> <d:getetag /> <d:getcontenttype /> <d:resourcetype /> <oc:fileid /> <oc:permissions /> <oc:size /> <d:getcontentlength /> <oc:tags /> <oc:favorite /> <oc:comments-unread /> <oc:owner-display-name /> <oc:share-types /> </d:prop> </d:propfind>

What exactly does the oc:size property express, then? What is the difference of oc:size and d:getcontentlength?

Can I use d:getcontentlength for reloading the size property of shared files and folders?

Yes, you can add "d:getcontentlength" to the requested properties and then check later which one of "oc:size" or "d:getcontentlength" was set in the response and use that.

The reason why we needed to invent "oc:size" is because the Webdav spec's "d:getcontentlength" isn't suitable for folders/collections and also because we needed something that represents the size of the complete contents (basically the same as the size shown in the properties of a folder in a file manager, the size of ALL included files). This is not exactly the same as checking the "quota-used-bytes" as received shared folders do not use any quota for the current user.

PVince81 · 2016-11-10T18:05:23Z

+					properties: properties
+				})
+				.then(function(status, data) {
+					// the following lines should be extracted to a mapper


yeah, or use the backbone model's ".fetch()" method instead of getFileInfo, but that's WIP here: #25909. In that PR fetching the properties of the model would automatically make the list's entry refresh.

Ok, cool. I'll try it.

Don't try it on your PR, it won't work...

Here you can see it in action in the other PR: https://github.com/owncloud/core/pull/25909/files#diff-dafefd9519924bec4efa9f5af4af8e68L2262

PVince81 · 2016-11-10T18:06:36Z

+					deferred.resolve(status, data);
+				})
+				.fail(function(status) {
+					OC.Notification.showTemporary(t('files', 'Could not create file "{file}"', {file: name}));


"could not load info for file" ?

Yeah. For sure =/

PVince81 · 2016-11-10T18:06:57Z

+					// the following lines should be extracted to a mapper
+					if( properties.indexOf(OC.Files.Client.PROPERTY_SIZE) !== -1){
+						fileInfo.set('size', data.size);
+					}


See discussion above about oc:size vs DAV:getcontentlength. You'd need to also check the content length property here I think.

PVince81 · 2016-11-10T18:08:00Z

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


yeah, if we could at some point just call "this.model.fetch()" like I'd like to have in this PR #25909 then we could remove the dependency with the file list completely.

PVince81 · 2016-11-14T09:09:50Z

@mjobst-necls do we actually still need that reload call ?
Your other PR changed the OCS Share API to already return the correct values.
So if someone changes the favorite in the sidebar all we need to do is set it on the model, which itself will (should) trigger a row update with the star state applied.
So I don't think we need to do the extra reloads any more.

mjobst-necls · 2016-11-14T18:57:29Z

@PVince81 Yes, the property reload is still needed because of the file size, which is not contained in the share API response. In big data applications only the information needed for immediate display should be retrieved, minimizing the response data size. This is the point here, because the file size is not shown in the file list.

Otherwise, if we want to show e.g. the file/content size summary at the bottom or in a column, the file size should be retrieved by the share api immediately.

Of course, we can add the size/contentlength to the share api, but this adds additional overhead for not necessarily needed information until now.

The size/contentlength property is only reloaded once per file on demand for the following position on details view:

Well, you are the boss and I want the file size to be displayed correctly on the main details view. Which method do you prefer?

PVince81 · 2016-11-14T20:33:55Z


+		/**
+		 * Reloads missing properties from server and set them in the model.
+		 * @param {OCA.Files.FileInfo} fileInfo file info


should be FileInfoModel ?

Yes, you're right... I'm gonna change this.

Uh, the FileInfoModel doesn't have a client in this version, yet.

Anyway. I added the client to the FileInfoModel so that we are closer to what we wanna have by PR #25909.

I tested the result and it works =)

PVince81 · 2016-11-14T20:35:06Z

Ah, damn... I forgot that the sharing file list didn't have the size. Well, if the PROPFIND only happens for the sidebar, then this is fine.

Too bad we can't have "reloadCallback" directly on the model.
Anyway, I think if we ever get to merge #25909 we'll be able to use "model.fetch({davProperties: []})` with the stuff that the sidebar needs.

mjobst-necls · 2016-11-14T22:54:15Z

I had to add the Client to the FileInfoModel, but this should be ok, because we are closer to what we want to have by Issue #25909, now.

mjobst-necls · 2016-11-16T20:57:28Z

Conclusion of changes:

The size on main details view (triggered by share views) get displayed correctly. There is a reload of the size property on demand, but only once per file until page reload.
The share recipients on view Shared with others stay visible on after favorite change
The Expiration date on view Shared by link stay visible on after favorite change

PVince81 · 2016-11-18T16:47:50Z

Great, so I guess ready to review again ?
Will check it next week. Thanks.

mjobst-necls · 2016-11-18T17:45:53Z

@PVince81 Yes, this PR should be complete, now.

PVince81 · 2016-11-21T14:03:42Z

Tested, works 👍
Thanks!

lock · 2019-08-04T08:02:39Z

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.

fixed size issues on main detail view and disappearing of share recip…

41717b0

…ients

PVince81 reviewed Nov 10, 2016

View reviewed changes

Changes due to code comments

33711ec

mjobst-necls added the 3 - To Review label Nov 11, 2016

mjobst-necls added this to the 9.2 milestone Nov 11, 2016

mjobst-necls added app:files feature:sharing labels Nov 11, 2016

PVince81 reviewed Nov 14, 2016

View reviewed changes

mjobst-necls added 2 commits November 14, 2016 23:31

Moved reloadProperties() to FileInfoModel

ee8e936

Solved Scrutinizer issues

0a41cc6

Bugfix: undefined value used on error

5f818f6

mjobst-necls self-assigned this Nov 14, 2016

check if options are set for FileInfoModel.initialize()

46ce507

PVince81 merged commit c1b3702 into owncloud:master Nov 21, 2016

blizzz mentioned this pull request Dec 23, 2016

Fixed size issues on main detail view and disappearing of share recipients (#26603) nextcloud/server#2850

Merged

PVince81 mentioned this pull request Jan 13, 2017

[OC 8.2.1] Sidebar shared file no filesize is displayed #20667

Closed

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

Uh oh!

Conversation

mjobst-necls commented Nov 10, 2016

Uh oh!

mention-bot commented Nov 10, 2016

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 14, 2016

Uh oh!

mjobst-necls commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 14, 2016

Uh oh!

mjobst-necls commented Nov 14, 2016

Uh oh!

mjobst-necls commented Nov 16, 2016

Uh oh!

PVince81 commented Nov 18, 2016

Uh oh!

mjobst-necls commented Nov 18, 2016

Uh oh!

PVince81 commented Nov 21, 2016

Uh oh!

lock Bot commented Aug 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjobst-necls commented Nov 14, 2016 •

edited

Loading