[dolphin] : Mac integration
AcceptedPublic

Authored by rjvbb on Apr 25 2020, 12:56 PM.

Details

Reviewers
elvisangelaccio
feverfew
Group Reviewers
Dolphin
Summary

Dolphin has a few advantages over the Mac Finder, not the least of which being its support for various remote file access protocols including Samba. (Older Mac OS versions fail to connect to MSWin 10 shares because of their system Samba libraries.)

This patch improves the Dolphin integration on Mac:

  • by making certain helper applications are launched (including dolphin itself, using the assumption that "cloning" a window should be perfect and thus use the same Dolphin executable).
  • by making certain the service menu installer is found. For now this is still done by installing it as a regular POSIX executable in a location that is assumed to be on the PATH.
  • by registering the URIs Dolphin can handle with LaunchServices. This allows to open items by dragging them onto the application icon.
Test Plan

Works as intended: helper applications open (they don't without this patch unless you install symlinks in system directories) and items can be opened via the Mac GUI. For folders this is limited to dragging them onto the Dolphin application icon (in a folder or the one in the Dock); I'm not aware of a way to indicate that these should be opened preferentially by Dolphin.

Opening folders in individual windows and thus in multiple application instances means you get as many Dolphin icons in the Dock and AppSwitcher. I don't think anything can be done against that, other than adding MDI support to dolphin.

Dragging files and folders from the Finder into a Dolphin window already works as it does on Linux. The reverse operation works too but only for the file:/ protocol.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 25880
Build 25898: arc lint + arc unit
rjvbb created this revision.Apr 25 2020, 12:56 PM
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptApr 25 2020, 12:56 PM
rjvbb requested review of this revision.Apr 25 2020, 12:56 PM
rjvbb updated this revision to Diff 81171.Apr 25 2020, 12:57 PM
rjvbb edited the test plan for this revision. (Show Details)

Remove spurious hunk.

rjvbb set the repository for this revision to R318 Dolphin.Apr 25 2020, 12:58 PM
meven added a subscriber: meven.

I cannot test, by the patch seems correct and not intrusive.

Did you test on other platforms ?

broulik added inline comments.
src/global.cpp
56

I think you need a KShell::quoteArgs in case there's spaces in the path

src/main.cpp
62

This doesn't need to be a warning

88

Make those private and add proper getters

rjvbb added a comment.Apr 25 2020, 7:32 PM
I cannot test, by the patch seems correct and not intrusive.

Yes, on Linux. Transparent would be the correct term (except when opening a new window in a dolphin version that's not on the path; with this patch the new window will be provided by the same version; I consider that a bonus side-effect).

rjvbb added a comment.Apr 25 2020, 7:46 PM

+ QString command = QStringLiteral("%1 --new-window").arg(QCoreApplication::applicationFilePath());

I think you need a KShell::quoteArgs in case there's spaces in the path

Wouldn't it be enough to do simply "\"%1\" --new-window"?

I do have a question about a little glitch I see only on Mac. Most of the time the Dolphin window looses focus after the directory listing is done being drawn. It looks like this might be because of the use of a kioslave. I'd like to remedy this but I haven't yet had any luck finding a signal or event that gets sent when the window (re)draw is complete; all the completion signals I checked until now are sent when I can still see the folder list being drawn. I've never had to worry about something like this before; does Qt provide a signal or event I could use?

feverfew requested changes to this revision.Apr 25 2020, 10:49 PM
feverfew added a subscriber: feverfew.
feverfew added inline comments.
src/main.cpp
224–225

This goes against this diff: https://phabricator.kde.org/D23672

mainWindow needs to be created before these two classes are instantiated (well, in particular KDBusService but cleaner to do both at the same time). See the note in the class description: https://api.kde.org/frameworks/kdbusaddons/html/classKDBusService.html

This revision now requires changes to proceed.Apr 25 2020, 10:49 PM
rjvbb added inline comments.Apr 26 2020, 8:16 AM
src/main.cpp
224–225

Not that I am against this change because it means the main window is created even quicker, but just how many tabs do you need to open (and how, exactly) to observe the hanging?

FWIW, having the "open folders in tabs" settings under startup and not under general/behaviour is confusing and illogical to me...

feverfew added inline comments.Apr 26 2020, 8:37 AM
src/main.cpp
224–225

Please see the test plan in the diff that I mentioned. Should be readily reproducible in that scenario. Even if it isn't,my point still stands, the race exists.

Feel free to open another patch regarding moving the location of the setting and add Dolphin and VDG as reviewers I guess.

rjvbb added a comment.Apr 26 2020, 8:42 AM

Please see the test plan in the diff that I mentioned. Should be readily reproducible in that scenario.

Nope, which is why I asked. If you need to open 200 folders for hanging to occur I will probably not be able to see any hanging on top of the time it takes to open so many things on my machine anyway ...

rjvbb updated this revision to Diff 81215.Apr 26 2020, 9:04 AM
rjvbb marked 4 inline comments as done.

Updated as requested.

src/main.cpp
88

No need for getters beyond the already existing popUrls(), in fact.

src/dolphintabwidget.cpp
337

Why is this needed? Can't we fix KIO::CommandLauncherJob to also make it work on MacOS? Or at least fix its documention to recommend the usage of CoreApplication::applicationFilePath() ?

src/main.cpp
222–224

I don't get it, why do we need to move this here?

rjvbb planned changes to this revision.Apr 26 2020, 10:28 PM
rjvbb added inline comments.
src/main.cpp
210–211

Note to self: fix this, there's need for a getter after all.

222–224

To enable normal runtime processing of QFileOpen events as soon as possible after we've handled the first events.

LaunchServices, the Mac's service that handles launching applications and handing off documents to be opened doesn't use argc,argv but sends ObjC messages (one per document) that Qt forwards as QFileOpenEvents.
This is an asynchronous mechanism and there's no guarantee that the events for all documents/folders you selected for opening will have been processed after line 215 above, even if they probably have been.
The longer opening the main window is postponed, the bigger the chance that we'll need to check the event handler's URL queue for file open events that arrived after we flushed that queue.

Opening the main window a bit earlier seems like the best way to prevent this and I don't see anything suggesting I can't move the operaton as I did.

rjvbb marked 2 inline comments as done.Apr 27 2020, 8:51 AM
rjvbb added inline comments.
src/dolphintabwidget.cpp
337

I don't see how we can fix the CommandLauncherJob; how is it to know that it needs to run applicationFilePath() instead of the provided command string? Or how is it to know, even on Mac, that a given command cannot be run as a POSIX executable but needs to be started through LaunchServices, or that it needs to dig out the "BundleExecutable"?

Not to mention the fact that LaunchServices will launch *a* copy of a given application if you only specify the name and not a full path. That's worse than "the first hit on the $PATH will be launched".

Using applicationFilePath() here works around all that and has the added benefit that it ensures on all platforms that the newly opened window really behaves the same way as the existing window (as far as it's supposed to). That seems so useful to me that I would have presented it in a dedicated PR if the change hadn't been required for this patch.

As to fixing the documentation: I think that anyone developing on/for Mac (and esp. working on KDEis intimately aware of the fact that there are 2 ways to start applications, and what that implies. You'd need to fix the documentation for all APIs that can start applications.

rjvbb updated this revision to Diff 81312.Apr 27 2020, 8:53 AM
rjvbb marked an inline comment as done.

Fixed.

rjvbb set the repository for this revision to R318 Dolphin.Apr 27 2020, 8:53 AM
meven added inline comments.Apr 27 2020, 9:21 AM
src/main.cpp
87

Remove

rjvbb added inline comments.Apr 27 2020, 9:25 AM
src/main.cpp
87

???

alex added a subscriber: alex.Apr 27 2020, 1:39 PM
alex added inline comments.
src/global.cpp
56

This causes a compile error: ‘quoteArgs’ is not a member of ‘KShell’
What @broulik meant was:
QString command = QStringLiteral("%1 --new-window").arg(KShell::quoteArg(QCoreApplication::applicationFilePath()));

meven added inline comments.Apr 27 2020, 1:51 PM
src/main.cpp
87

The are two empty lines, one should suffice

rjvbb added a comment.Apr 27 2020, 3:09 PM

This causes a compile error: ‘quoteArgs’ is not a member of ‘KShell’

And I'd told myself I wouldn't make any more errors updating this patch while on Linux ...

What @broulik meant was:
QString command = QStringLiteral("%1 --new-window").arg(KShell::quoteArg(QCoreApplication::applicationFilePath()));

I guess that's the same as passing the entire command to quoteArg, or will it in fact mess up the new-window argument on MSWin?

alex added a comment.EditedApr 27 2020, 3:49 PM

Nope, that is something different, to give you an example:

QStringLiteral("cat %1").arg(KShell::quoteArg("/my/crappy filepath/"));  => cat '/my/crappy filepath/' 
KShell::quoteArg(QStringLiteral("cat %1").arg("/my/crappy filepath/")); => 'cat /my/crappy filepath/'

What you suggested will create shell command like:
'/usr/bin/dolphin --new-window'

And this would cause an error, because you specify that the --new-window parameter is a part of the executable name. Running this command will show:
bash: /usr/bin/dolphin --new-window: No such file or directory

rjvbb added a comment.Apr 27 2020, 4:37 PM
In D29178#658443, @alex wrote:

Nope, that is something different, to give you an example:

QStringLiteral("cat %1").arg(KShell::quoteArg("/my/crappy filepath/"));  => cat '/my/crappy filepath/' 
KShell::quoteArg(QStringLiteral("cat %1").arg("/my/crappy filepath/")); => 'cat /my/crappy filepath/'

Ah... Evidently that won't work, no. I must have misread the function documentation then, overestimating its capabilities (beyond reason, in hindsight).

rjvbb marked 3 inline comments as done.Apr 28 2020, 8:39 AM
rjvbb updated this revision to Diff 81417.Apr 28 2020, 8:41 AM

I think these were the last outstanding corrections to make?

Does anyone have an idea how to catch a window-is-done-refreshing signal/event so I can see if adding an activateWindow there fixes the focus issue I mentioned?

rjvbb set the repository for this revision to R318 Dolphin.Apr 28 2020, 8:42 AM

@feverfew Does it look good now for you?

feverfew resigned from this revision.Jul 7 2020, 6:48 PM

@feverfew Does it look good now for you?

My comment was addressed, but I don't feel confident to approve the rest, although I don't disapprove either.

elvisangelaccio accepted this revision.Jul 12 2020, 10:48 PM

Feel free to ship it.

This revision is now accepted and ready to land.Jul 12 2020, 10:48 PM