Add KFileItemActionPlugin for copying Google URL to clipboard.
ClosedPublic

Authored by barchiesi on Sep 11 2019, 10:16 AM.

Details

Summary

When right clicking on a Google Drive file or folder, show a 'Copy Google URL to clipboard' context menu action.

Diff Detail

Repository
R219 KIO GDrive
Branch
arcpatch-D23874
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16586
Build 16604: arc lint + arc unit
barchiesi requested review of this revision.Sep 11 2019, 10:16 AM
barchiesi created this revision.

@elvisangelaccio care to answer the following?

src/CMakeLists.txt
51–54

Is this the correct way of reusing the same logging category?

src/copyurlitemaction.cpp
78

How should this error be handled? A notification perhaps?

elvisangelaccio requested changes to this revision.Sep 13 2019, 8:00 PM
elvisangelaccio added inline comments.
src/CMakeLists.txt
51–54

It works so it's not "wrong". The alternative is to create a private library with all the gdrive sources and link the new plugin to this library. That would also avoid to build gdriveurl.cpp twice, which this patch currently does.

Question: do we really need debug in this plugin? Maybe it can be avoided.

Question 2: do we really need the GDriveUrl class here? It's basically used only to compare the scheme with the "gdrive" string...

src/copyurlitemaction.cpp
70

Why are we stating the URL again? We already have the KFileItem for it which can give us access to its UDSEntry object.

75

This doesn't seem to work on Wayland. Haven't tested on X11 yet.

src/copyurlitemaction.json
10

Please do not add translations manually, they will be added by a bot when ready.

src/gdriveurl.h
33 ↗(On Diff #65842)

IMHO this is overkill, we can just use UDS_EXTRA in the fastInsert() call.

This revision now requires changes to proceed.Sep 13 2019, 8:00 PM
barchiesi updated this revision to Diff 66025.Sep 14 2019, 9:00 AM
barchiesi marked 3 inline comments as done.

Removed unnecessary stat(), removed translations, replaced UDS_ALTERNATE_LINK_FIELD with UDS_EXTRA.

barchiesi added inline comments.Sep 14 2019, 9:12 AM
src/CMakeLists.txt
51–54

In this scenario I guess usage of both debug and GDriveUrl can be avoided, however the private library might need to be considered. In the future I would like to add a way to change file permissions after right clicking and to do so I will very probably need access to other parts of kio-gdrive. Any ideas?

src/copyurlitemaction.cpp
75

I just tried it out simply selecting "Plasma (Wayland)" in SDDM and it seems to work, I think the bug is elsewhere. What were you right clicking on?

src/CMakeLists.txt
51–54

I would create the library when we cannot actually avoid it. Simple as that :)

src/copyurlitemaction.cpp
48

Missing const; prefer at(0).

63

I'd call it "Copy Google URL to clipboard", since it can be a "drive" url (drive.google.com) but also a "document" url (docs.google.com).

75

Nevermind, it works. It just doesn't show up in the clipboard plasmoid, but that's surely not a bug introduced by this patch.

src/copyurlitemaction.cpp
69

Can the link be empty? In this case we might not show the action at all.

barchiesi updated this revision to Diff 66076.Sep 14 2019, 5:48 PM
barchiesi marked 3 inline comments as done.

Remove debug and gdriveurl dependency, renamed context menu entry.

barchiesi added inline comments.Sep 14 2019, 5:49 PM
src/CMakeLists.txt
51–54

I removed debug and GDriveUrl class from this plugin.

src/copyurlitemaction.cpp
75

Ah, then in that case it probably has something to do with T4449: [kwayland] Clipboard Manager protocol

barchiesi retitled this revision from Add KFileItemActionPlugin for copying google drive url to clipboard. to Add KFileItemActionPlugin for copying Google URL to clipboard..Sep 14 2019, 6:14 PM
barchiesi edited the summary of this revision. (Show Details)
barchiesi added a project: KIO GDrive.
barchiesi added a subscriber: KIO GDrive.

Almost there!

src/copyurlitemaction.cpp
23

Not needed

24

QGuiApplication

30

Not needed

49

QLatin1String for comparisons

72

QGuiApplication should be enough

src/copyurlitemaction.json
9

"Copy Google URL to clipboard" also here ;)

barchiesi updated this revision to Diff 66131.Sep 15 2019, 3:40 PM
barchiesi marked 6 inline comments as done.

Remove unnecessary includes, use QGuiApplication instead of QApplication, use QLatin1String for string comparison, fix name in service descriptor.

elvisangelaccio accepted this revision.Sep 15 2019, 6:48 PM

Feel free to push after fixing the last two nitpicks.

src/copyurlitemaction.h
23–24

Sorry, these are also not needed

27

Can be moved to the .cpp file

This revision is now accepted and ready to land.Sep 15 2019, 6:48 PM
This revision was automatically updated to reflect the committed changes.