+1

What about non-unix systems? e.g. on Windows we don't have dbus but the stash ioslave could still be functional (or maybe not, @shortstheory what do you say?)

In D9094#174235, @elvisangelaccio wrote:

on Windows we don't have dbus

Why is it so? Craft installs and packages dbus...

In D9094#174312, @alexeymin wrote:

In D9094#174235, @elvisangelaccio wrote:

on Windows we don't have dbus

Why is it so? Craft installs and packages dbus...

Oh that's news to me. Then I guess the patch is fine.
Let's wait for @shortstheory opinion.

Sorry for the late reply, I have endsem college exams atm.

How do daemons register themselves on Windows' KDED5 module (if there is one)? Otherwise, the patch looks good.

 QDBusMessage msg;
 QDBusMessage replyMessage;
 msg = QDBusMessage::createMethodCall(
           "org.kde.kio.StashNotifier", "/StashNotifier", "", "pingDaemon");
 replyMessage = QDBusConnection::sessionBus().call(msg);
 if (replyMessage.type() == QDBusMessage::ErrorMessage) {
     qDebug() << "Launching fallback daemon";
     const QString program = "./testdaemon";
     stashDaemonProcess->start(program);
}

I've been doing it like this for testing if the stash daemon is active for the unit tests. This makes sure that the caller is able to use DBus and get replies from it successfully. Perhaps that should change to the approach used in this patch as well...

I'm not expert but I don't think this breaks Windows.

I need a ship-it ... :)

-2

You shouldn't depend on the implementation details of ioslaves. KIO has a well-defined interface to determine if an protocol can be handled.
And if the implementation of this interface is not good enough, then it needs to be improved (e.g. decrease number of file system look-ups, better cache invalidation, ...).

It's tricky. On the face of it you're right, because what the action does is open a view using the ioslave, and yes, it shouldn't be aware of the implementation details.

Here's the best argument for it: The protocol being known isn't the same as kio-stash actually working, because if the service isn't registered then even if the protocol is known the stash won't work anyway. You can therefore argue that this code better captures intent - the action will actually work if this requirement is satisfied.

It would be nicer if the "aware of implementation details" part were encapsulated within kio-stash itself, i.e. if it could supply this action as a plugin to Dolphin. But Dolphin isn't currently extensible in this way, so given that this action being in Dolphin code is suboptimal to begin with (it's coded against an ioslave it doesn't install itself), I still think this patch is a pragmatic way forward as it improves both performance and reliability.

To sum it up: This should be done in a more proper way (kio-stash adding this action to Dolphin in some way knowing when it can be enabled), but in the meantime there's no good reason for it to be slower and less reliable.

ping

Alright, my college exams finally got over so I have some time to think about this.

I feel that Dolphin should have some more configurability and having dedicated code just for showing a button for a not-very-commonly used ioslave is probably not the best idea. Perhaps it's better if this is moved to the add-on "Services" which can be opened from a Context menu? @emmanuelp would know if it's possible to add a toolbar button that way (should be because it's just a QAction at the end of the day).

@shortstheory Have a look at KAbstractFileItemActionPlugin, it definitely allows to show context menu actions depending on some custom logic.