Private links in share dialog and shell integrations by ckamm · Pull Request #5763 · owncloud/client

ckamm · 2017-05-10T07:54:30Z

This is an initial draft for comment.

Mostly the patch is quite straightforward. The exception is that storing the fileid in addition to the id adds some complications, see 875b9bb . In particular

We need to initially-populate the new column from the full id column. That shouldn't cause problems though.
PUT/MKCOL only give us the full id, not the fileid. If we extract the fileid by doing .left(8), we might as well not add the new column and just do that whereever needed.

The other option would be to have a transition plan where the server also replies with a new OC-FileIdLocal: <oc:fileid> header. Then oc:id and oc:fileid would be truly decoupled.

Opinions?

The last thing is that of course the OSX and Windows integrations also need to be updated to include the new action. Note that while I only added one action to the context menu for now, we might desire more, see discussion in #5023.

(I wonder whether we should generalize and have the socket api give a list of context menu commands and menu titles to the integrations - similar to GET_STRINGS - then we could adjust context menus without touching the integrations in the future)

guruz · 2017-05-10T14:44:10Z

Man that was confusing.. :)

guruz · 2017-05-10T14:50:40Z

yay for doing this in SQL :)

guruz · 2017-05-10T14:53:38Z

Would you have a way to autotest the migration step? Maybe in tx.pl or so?
(..basically make sure that if the old id is set, the new one should also be set).

guruz · 2017-05-10T14:54:40Z

FYI @PVince81 @DeepDiver1975 to keep you in the loop

guruz · 2017-05-10T15:01:38Z

Code wise it looks good.
But we should probably indeed have the server also return the fileid on the operations (like MKCOL) where it now returns the ID.

I just wonder how decoupled this can be. Could we merge this PR here and just schedule something for the server?

ckamm · 2017-05-11T11:09:28Z

@guruz Will look at tests.

The idea for decoupling id/fileid would be that we check for the presence of the new fileid header, and use left(8) of the full id if the new header isn't present. That way it works for old servers and future servers can have completely different id/fileid.

guruz · 2017-05-11T15:20:52Z

Any special reason to use reference for a qobject which was on heap before? (i don't think we do that anywhere else in the client)

Ah, I did this automatically because it can't be 0. But yes, it's not the style we use and will adjust it.

guruz · 2017-05-11T15:22:28Z

Should all the .left(8) magic (it's in other commits too right?) be moved to a function that has a fat comment to remove the whole stuff in a year or two?

I had it in a function, but then removed it when there was only a single use left. Then I later found this corner case (PropFind is only sent for servers <= 7.0.0). Yep, will put it into a function again.

guruz · 2017-05-11T15:24:52Z

So we assume that (future) servers will continue to always emit both "local" and full IDs?
@PVince81

The client at least needs both for now.

ckamm · 2017-05-12T08:40:04Z

Rebased now that the logging PR was merged.

phil-davis · 2017-05-12T09:07:18Z

How local is the local path? "local local" ;)

ckamm · 2017-05-12T10:40:27Z

@guruz This looks good to me now

Users of the 'id' property would like to be able to deduce the 'fileid' property from it. To allow that the 'id' property is now required to be made up from two parts: a purely numeric one, plus one that starts with a letter. The first part shall be the (potentially padded) fileid. The background is that clients need the fileid to be able to create private link urls, but they currently only have the full 'id' property available. See owncloud/client#5763

ckamm · 2017-06-23T09:51:26Z

@guruz @davivel To move this forward I've created this PR against core: owncloud/core#28193 - ideally we can drop the discovery and storage of the short numeric ids.

It existed to work around a Qt bug that has been fixed https://bugreports.qt.io/browse/QTBUG-48953 owncloud#3300

ckamm · 2017-06-23T10:56:25Z

I've updated this PR to remove the fileid discovery and storage and instead generate the fileid from the id we store already! From my point of view this is ready to merge.

Note: Since I rebase-dropped a bunch of unnecessary commits now, the remainder doesn't compile individually anymore. Let me flatten and merge this once it is approved.

jturcotte

I've pushed a few commits to refactor and make the branch work on macOS and Windows, feel free to rebase, squash (like the build fixes) or edit commits. Feel free to give feedback on those and I can fix them too.

To be able to fix an issue on Windows I added GET_STRINGS:BEGIN and GET_STRINGS:END messages, the linux code might have to be adjusted.

jturcotte · 2017-07-03T17:11:55Z

+        { "APPNAME", Theme::instance()->appNameGUI() },
+        { "CONTEXT_MENU_TITLE", Theme::instance()->appNameGUI() },
+        { "COPY_PRIVATE_LINK_TITLE", tr("Copy private link to clipboard") },
+        { "EMAIL_PRIVATE_LINK_TITLE", tr("Send private link by email...") },


Since I also name variables to match those, it would be nice if the _MENU_TITLE suffix was consistent.

jturcotte · 2017-07-04T15:50:59Z

Is this string going to be used?

Not right now, it seemed like a string we might need - can drop for now.

jturcotte · 2017-07-04T17:39:21Z

+void SocketApi::command_GET_STRINGS(const QString &, SocketListener *listener)
+{
+    static auto map = std::initializer_list<std::pair<const char *, QString>>{
+        { "SHARE_MENU_TITLE", tr("Share with %1...", "parameter is ownCloud").arg(Theme::instance()->appNameGUI()) },


Putting the appName in both the submenu title and the share entry looks a bit redundant now:

Maybe we could name it just "Share..."?

guruz · 2017-07-05T10:30:33Z

@SamuAlfageme The contents should now also be in local_links branch in normal client repo..

ckamm · 2017-07-06T09:40:27Z

@jturcotte I think I have addressed the remaining issues and tests look good (dolphin only tomorrow though).

@SamuAlfageme I also updated the local_links branch in the main client repo in case you want to test early.

jturcotte

Went through the changes again and everything looks good. If you can also go through my commits I guess it's legal that I approve this PR.

jturcotte · 2017-07-06T14:54:10Z

+            return []

+        # Currently 'sharable' also controls access to private link actions,
+        # and we definitely don't want to show them for IGNORED.


Nit: 'sharable' -> 'shareable'

ckamm · 2017-07-07T08:53:15Z

Merged manually, squashing various review and fixup commits together.

ckamm added the DO NOT MERGE YET label May 10, 2017

ckamm added this to the 2.4.0 milestone May 10, 2017

ckamm self-assigned this May 10, 2017

ckamm requested review from guruz and ogoffart May 10, 2017 07:54

ckamm mentioned this pull request May 10, 2017

Use "Local Links" on the client #5023

Closed

3 tasks

guruz reviewed May 10, 2017

View reviewed changes

Comment thread csync/src/csync_update.c Outdated

guruz May 10, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man that was confusing.. :)

guruz reviewed May 10, 2017

View reviewed changes

Comment thread src/libsync/syncjournaldb.cpp Outdated

guruz May 10, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay for doing this in SQL :)

guruz reviewed May 11, 2017

View reviewed changes

ckamm force-pushed the local_links branch 2 times, most recently from a9d6cf1 to e2c7305 Compare May 12, 2017 08:39

phil-davis reviewed May 12, 2017

View reviewed changes

Comment thread src/gui/owncloudgui.h Outdated

phil-davis May 12, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How local is the local path? "local local" ;)

ckamm removed the DO NOT MERGE YET label May 12, 2017

ckamm force-pushed the local_links branch from 624eda2 to c35d6e2 Compare May 12, 2017 10:49

ckamm force-pushed the local_links branch from c35d6e2 to bb49196 Compare May 17, 2017 10:25

ckamm changed the title ~~Local links (now: Private Links)~~ Private links in share dialog and shell integrations Jun 8, 2017

guruz added the p2-high Escalation, on top of current planning, release blocker label Jun 15, 2017

SamuAlfageme mentioned this pull request Jun 15, 2017

[Shell Extensions] [Windows, macOS] Provide context menu options for private link handling #5842

Closed

ckamm mentioned this pull request Jun 23, 2017

Node: Define relationship between id and fileid owncloud/core#28193

Merged

ckamm force-pushed the local_links branch from bd3bf70 to e40c26d Compare June 23, 2017 10:55

ckamm added a commit to ckamm/owncloud-client that referenced this pull request Jun 23, 2017

SocketAPI: Add 'email local link' action owncloud#5763

1f9b572

ckamm added a commit to ckamm/owncloud-client that referenced this pull request Jun 23, 2017

Nautilus plugin: Email local link action owncloud#5763

deaa9f0

ckamm added a commit to ckamm/owncloud-client that referenced this pull request Jun 23, 2017

Dolphin plugin: Email local link action owncloud#5763

ddcf636

ckamm added a commit to ckamm/owncloud-client that referenced this pull request Jun 23, 2017

Remove copyToClipboard owncloud#5763

5d8c3e3

It existed to work around a Qt bug that has been fixed https://bugreports.qt.io/browse/QTBUG-48953 owncloud#3300

ckamm added a commit to ckamm/owncloud-client that referenced this pull request Jun 23, 2017

Review fixes owncloud#5763

d4456c8

ckamm added a commit to ckamm/owncloud-client that referenced this pull request Jun 23, 2017

Rename 'local link' to 'private link' everywhere owncloud#5763

f57f203

jturcotte reviewed Jul 4, 2017

View reviewed changes

jturcotte force-pushed the local_links branch from c771a0f to 010664c Compare July 4, 2017 17:36

jturcotte reviewed Jul 4, 2017

View reviewed changes

jturcotte approved these changes Jul 6, 2017

View reviewed changes

ckamm closed this Jul 7, 2017

ckamm force-pushed the local_links branch from 35256d2 to 411621b Compare July 7, 2017 08:52

ckamm deleted the local_links branch July 7, 2017 08:52

michaelstingl mentioned this pull request Jul 14, 2017

Extend files context menu nextcloud/gsoc_client#3

Open

This was referenced Jul 27, 2017

[macOS] Drop .pkg in favor of .dmg/.app distribution #5926

Closed

Migrate sync journal table structure to support BIGINT switch on server's DB #5933

Closed

SamuAlfageme mentioned this pull request Sep 12, 2017

Adapt private link generation to core#28984 #6020

Closed

ogoffart mentioned this pull request Aug 28, 2018

Don't rely on instance id oc prefix when parsing file id #6745

Closed

Uh oh!

Conversation

ckamm commented May 10, 2017 • 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!

guruz commented May 10, 2017

Uh oh!

guruz commented May 10, 2017

Uh oh!

guruz commented May 10, 2017

Uh oh!

ckamm commented May 11, 2017

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!

ckamm commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckamm commented May 12, 2017

Uh oh!

ckamm commented Jun 23, 2017

Uh oh!

ckamm commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jturcotte 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!

guruz commented Jul 5, 2017

Uh oh!

ckamm commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jturcotte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckamm commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ckamm commented May 10, 2017 •

edited

Loading

ckamm commented Jun 23, 2017 •

edited

Loading

ckamm commented Jul 6, 2017 •

edited

Loading