Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Overlays] Use "Share" overlays #6098

Closed
SamuAlfageme opened this issue Oct 16, 2017 · 15 comments
Closed

[Overlays] Use "Share" overlays #6098

SamuAlfageme opened this issue Oct 16, 2017 · 15 comments
Assignees
Labels
Milestone

Comments

@SamuAlfageme
Copy link
Contributor

SamuAlfageme commented Oct 16, 2017

While checking #6094 I found out that the "Share" overlay icon is not being used in the shell integations neither in Linux nor in Windows, while in macOS is correctly set right after sharing from the desktop/receiving a shared file.

Expected behavior

When a file is shared, the overlay icon should be the right one

Actual behavior

The "OK" overlay is the one displayed on the file explorer.

Client configuration

Client version: 2.3.3/2.4.0 (current master)
Operating system: Ubuntu 16.04(Nautilus)/Windows 7

@ogoffart
Copy link
Contributor

What are the step to reproduce?

Is it different on mac and windows? I see no reason why it could be different.

@ogoffart
Copy link
Contributor

I guess the problem is that when sharing, the database still has the old cache information weather the file is cached or not. I do not believe sharing change the etag of the parent folder, which means we won't notice the change until there is an actual change in that folder.

@guruz
Copy link
Contributor

guruz commented Oct 17, 2017

I do not believe sharing change the etag of the parent folder, which means we won't notice the change until there is an actual change in that folder.

CC @DeepDiver1975 @PVince81

@PVince81
Copy link
Contributor

We already fixed etag propagation a while ago with OC 9.0. Every mount point has an etag and the etag of the parents is computed based on all child etags. So if anything changes within a received share, all receivers will have their etag changes.

If the etag did not change then it's a bug.

@SamuAlfageme
Copy link
Contributor Author

SamuAlfageme commented Oct 17, 2017

@ogoffart yeah, I think it differs since setting up folders with shared items from scratch still displays ✅ (while in macOS you get the shared one)

Otherwise, the behavior with the parent etags is erratic, agreed. Could we retrigger an individual PROPFIND after editing shared properties of a file? ~> a831164

@ckamm
Copy link
Contributor

ckamm commented Oct 18, 2017

@SamuAlfageme I'll check this out on linux/nautilus.

Edit: Can you be more specific? When a file is shared with my user, the nautilus integration does use the expected icon in the scenario I tried. When my user shares a file with someone, the icon doesn't change. (possibly because I'm testing against an older server right now, this only works with servers >=10)

Is that different on OSX?

@SamuAlfageme
Copy link
Contributor Author

SamuAlfageme commented Oct 18, 2017

@ckamm sorry, I think I'm not being very clear. This is what I see on Ubuntu 16.04 with Nautilus - note the 2 files being shared by link on the server after recreated locally and getting the wrong overlay. Is it different to what happens on your side?

When my user shares a file with someone, the icon doesn't change. (possibly because I'm testing against an older server right now, this only works with servers >=10)

Yes, in macOS it does not change instantly either even in servers >= 10 (issue #4788) (you can try with admin:[email protected]). However after restarting the client/resetting the local sync folder, I get the right ones (that's the original reason of this issue).

If the etag did not change then it's a bug.

@PVince81 do you have a link to the core issue so I can verify if it's a regression or a client bug?

@SamuAlfageme SamuAlfageme changed the title [Overlays][Windows/Linux] Use "Share" overlays [Overlays][Linux] Use "Share" overlays Oct 18, 2017
@SamuAlfageme
Copy link
Contributor Author

SamuAlfageme commented Oct 18, 2017

(edited the title since I was mixing the 2 issues related to the share overlay - the one described in #6098 (comment) only happens on Linux, If we confirm the etags are changing but the client is not reactive, will open a different one track in #4788)

@ogoffart
Copy link
Contributor

@PVince81 I tested with 10.x, and the etag of parent directory is not changed if an item within the directory becomes shared. (I'm talking about the user sharing this item with another user)
However, i'm going to work around this in the client. The share dialog is going to invalidate the etags for the recently modified item, and re-trigger a sync.

ogoffart added a commit that referenced this issue Oct 23, 2017
... even if the file is not changed.

We get an UPDATE_METADATA in that case, so make sure we let the
SyncFileStatusTracker know about it.
That means we need to filter out UPDATE_METADATA in the other listeners
of this signal.

Issue #6098
ogoffart added a commit that referenced this issue Oct 23, 2017
This allow the sync engine to query the new metadata and update the
overlay icons.

Note: we also need to invalidate the etags because the server does not
change the etag of parent directories that see their share-types changed.

Issue #6098
@PVince81
Copy link
Contributor

@ogoffart wait, are you saying we need to change the etag also for outgoing shares ?

So far we only change it for incoming shares.

This was by design so far because an outgoing share doesn't change any file/folder data or structure, unlike incoming shares. If you are saying that you now need an etag change, likely because of metadata change, then we need to talk and find a way to get this in. (enhancement)

I believe there was a similar topic about having permission changes trigger an etag change but I think this was never implemented. (I need to find the ticket).

@PVince81
Copy link
Contributor

Related: owncloud/core#16589 and owncloud/core#20474

@ogoffart by the way, you said back then that no etag change for metadata: owncloud/core#20474 (comment)

I'm fine reconsidering this as it might be a requirement when clients process more and more metadata.

Introduce a "metadata etag" ?

@ogoffart
Copy link
Contributor

@PVince81 The etag of the file itself must not change. But the etag of the parent directories should change for metadata of the files within that directory.
Permissions change indeed must trigger an etag change of the parent directories, but not of the file.

@PVince81
Copy link
Contributor

Ok, I think we don't have this currently, we need to continue working on owncloud/core#16589 and do this for outgoing shares as well.

ogoffart added a commit that referenced this issue Oct 24, 2017
... even if the file is not changed.

We get an UPDATE_METADATA in that case, so make sure we let the
SyncFileStatusTracker know about it.
That means we need to filter out UPDATE_METADATA in the other listeners
of this signal.

Issue #6098
ogoffart added a commit that referenced this issue Oct 24, 2017
This allow the sync engine to query the new metadata and update the
overlay icons.

Note: we also need to invalidate the etags because the server does not
change the etag of parent directories that see their share-types changed.

Issue #6098
@ckamm
Copy link
Contributor

ckamm commented Oct 25, 2017

@jturcotte I assume that the merged PR means this is ready to test. Though it's only a workaround that triggers when you create a new share from within the client.

@ckamm ckamm added ReadyToTest QA, please validate the fix/enhancement Linux labels Oct 25, 2017
@ckamm ckamm changed the title [Overlays][Linux] Use "Share" overlays [Overlays] Use "Share" overlays Oct 25, 2017
@ckamm
Copy link
Contributor

ckamm commented Oct 26, 2017

I've tested by checking the icon on incoming and outgoing shares, and as long as I trigger the outgoing share from the shell integration the icon will update quickly after. Further improvements will happen with the folder etag updates on the server that @PVince81 linked.

@ckamm ckamm closed this as completed Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants