Add Share action to Dolphin context menu
ClosedPublic

Authored by nicolasfella on May 25 2018, 10:36 PM.

Details

Summary

Use Purpose to enable sharing files using various services. Other KDE applications like Okular and Spectacle
already have that capability.

This feature overlaps with the various FileItemActionPlugins but is more generic as it is used by multiple
applications. At least some FileItemActionPlugins, such as the "Send via Bluetooth" plugin, should become
Purpose plugins instead. In other cases, such as KDE Connect, both approaches are supported at the moment.

Test Plan

Select one or multiple files, right-click, Share, select e.g. Telegram or KDE Connect, check for
successful transfer.

Diff Detail

Repository
R495 Purpose Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.May 25 2018, 10:36 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMay 25 2018, 10:36 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
nicolasfella requested review of this revision.May 25 2018, 10:36 PM
nicolasfella edited the summary of this revision. (Show Details)May 25 2018, 10:38 PM
nicolasfella edited the test plan for this revision. (Show Details)
  • Remove unrelated change
nicolasfella edited the test plan for this revision. (Show Details)May 25 2018, 10:41 PM
apol added a comment.May 25 2018, 11:13 PM

I have the feeling that it should be a KAbstractFileItemActionPlugin? It's how we implemented it for KDE Connect at least and it should be more generic.

src/dolphincontextmenu.cpp
318 ↗(On Diff #34889)

const QUrl &?

Yay, I've wanted this for ages!

At the same time, your screenshot shows a somewhat absurd situation: two "Send to" entries, the "Share" menu, and half a dozen other submenus. With the default plugin settings, there will also be another "Actions" sub-menu. We'll definitely want to move the Bluetooth and KDE Connect plugins into Purpose.

At the same time, your screenshot shows a somewhat absurd situation: two "Send to" entries, the "Share" menu, and half a dozen other submenus. With the default plugin settings, there will also be another "Actions" sub-menu. We'll definitely want to move the Bluetooth and KDE Connect plugins into Purpose.

That leaves the question of how to deal with the double entries. One could totally remove the FileItemActionPlugins from KDE Connect and Bluedevil, which would break things for users that don't have Purpose installed. For Dolphin this can be solved by requiring Purpose. Are there other applications that use (not provide) FileItemActionPlugins? Alternatively one could hide a subset of FIAPs when purpose is detected.

In D13124#268426, @apol wrote:

I have the feeling that it should be a KAbstractFileItemActionPlugin? It's how we implemented it for KDE Connect at least and it should be more generic.

I thought about that, but I'll leave the decision to Dolphin maintainers.

First I thought that Purpose might completely replace FIAPs, but that's not a good idea because some FIAPs like Ark's don't fit well into the "Share" terminology

elvisangelaccio requested changes to this revision.May 26 2018, 4:12 PM
elvisangelaccio added a subscriber: elvisangelaccio.

If possible, I also vote for a KFIA plugin which would allow us to not hard-depend on Purpose. Dolphin already has many dependencies, we should be very careful before adding new ones.

Right now this patch adds an optional dependency on Purpose, but it uses it unconditionally in the context menu. This breaks the build if purpose is not installed.

This revision now requires changes to proceed.May 26 2018, 4:12 PM

Make it a FileItemActionPlugin

Restricted Application added a project: Frameworks. · View Herald TranscriptMay 26 2018, 6:07 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
  • Set mimetype to */* if reported mimetype is empty
nicolasfella marked an inline comment as done.May 26 2018, 6:15 PM

Have you seen D12895?

Have you seen D12895?

I looked for similar patches with the Dolphin tag, but I didn't see yours

I like yours better, I can abandon mine if you want to finish this :)

markg added a subscriber: markg.May 28 2018, 4:13 PM

I like this :)
Thanks a lot for working on it!

I like yours better, I can abandon mine if you want to finish this :)

I would have abandoned mine because you were first, but fine with me :)

Friendly ping?

markg added a comment.Jun 3 2018, 2:06 PM

Friendly ping?

?

elvisangelaccio added inline comments.Jun 3 2018, 6:06 PM
src/fileitemactionplugin/CMakeLists.txt
13–14

Please consider using json metadata and installing the plugin into the kf5/kfileitemaction namespace.

nicolasfella added inline comments.Jun 18 2018, 7:20 PM
src/fileitemactionplugin/CMakeLists.txt
13–14

Sorry, I don't quite understand what I am supposed to do

src/fileitemactionplugin/CMakeLists.txt
13–14

I recommend to use the kcoreaddons_add_plugin cmake macro. Something like this:

kcoreaddons_add_plugin(purposefileitemaction
    SOURCES sendfileitemaction.cpp
    JSON sendfileitemaction.json
    INSTALL_NAMESPACE "kf5/kfileitemaction")

then drop both the install() calls.
Also, the plugin should use K_PLUGIN_FACTORY_WITH_JSON rather than K_PLUGIN_FACTORY.

Use JSON metadata and install in kf5/fileitemaction namespace

nicolasfella marked 3 inline comments as done.Sat, Jun 23, 5:40 PM

@apol As soon as we land this we have two ways of sharing with KDE Connect. Should we just remove the FileItemActionPlugin from KDE Connect? Are there more applications than Dolphin using FIAPs?

apol added a comment.Sun, Jul 1, 11:49 PM

Looks good to me overall.

Maybe it would make sense to call it ShareFileItemAction everywhere? "purpose" is an implementation detail here.

  • Rename to ShareFileItemAction
nicolasfella retitled this revision from [RFC] Add Share action to Dolphin context menu to Add Share action to Dolphin context menu.Mon, Jul 2, 10:26 AM
apol added a comment.Mon, Jul 2, 10:58 AM

Looks good to me, would appreciate if someone from dolphin could accept.

markg added a comment.Mon, Jul 2, 1:41 PM

This looks great!
But i don't get how it is magically included in Dolphin.. Perhaps someone could explain?
I get that it's a fileitemaction plugin and that it's being loaded by dolphin (somehow), i take that for granted.
But even so, if i look in the dolphin code that builds up the context menu for an item: https://cgit.kde.org/dolphin.git/tree/src/dolphincontextmenu.cpp#n307 i see absolutely nothing that can be loading this plugin.
And as this plugin isn't touching that code at all, how does it do that?

The line linked above adds the "Properties" entry. Right above that is "Move To" and "Copy To" submenus. There is nothing in between that loads plugins.. Yup, i'm really confused as i just don't see how this works.

Someone? :)

Check out KFileItemActions::addServiceActionsTo

elvisangelaccio accepted this revision.Mon, Jul 2, 8:08 PM

Check out KFileItemActions::addServiceActionsTo

Actually, KFileItemActions::addPluginActionsTo ;)

This revision is now accepted and ready to land.Mon, Jul 2, 8:08 PM
markg added a comment.Mon, Jul 2, 8:50 PM

Check out KFileItemActions::addServiceActionsTo

Actually, KFileItemActions::addPluginActionsTo ;)

Thank you both :)
After looking at that code (KFileItemActions::addPluginActionsTo that is), it's clear to me where and how it happens. That's an interesting approach Dolphin does there with calls to KFileItemActions. It fooled me as it's a local variable.

What i still don't get is how "Share" appears below "Move To" as the call to add the "Share" menu is done before the "Move To" menu is added. Which also fooled me as "Move To" is added using the this pointer.
The only reasoning i can think of is some sorting of menu items being done afterwards (which i don't see...) as the menu appears to be sorted alphabetically.

This revision was automatically updated to reflect the committed changes.