Fix opening files via a file manager on Mac
Needs ReviewPublic

Authored by sbragin on Mar 5 2018, 9:16 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

On Mac a file open event is sent to the QApplication instance if there is
a request from the operating system to open a file. This patch adds a new
class, derived from QApplication, which handles file open events.

Thanks to: René J.V. Bertin

Diff Detail

Repository
R223 Okular
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
sbragin created this revision.Mar 5 2018, 9:16 PM
Restricted Application added a project: Okular. · View Herald TranscriptMar 5 2018, 9:16 PM
sbragin requested review of this revision.Mar 5 2018, 9:16 PM

Isn't this another case of "should be fixed in QApplication or another lower level library"?

Isn't this another case of "should be fixed in QApplication or another lower level library"?

I do not quite understand, what you mean. I guess dispatching QFileOpenEvent is how file opening is supposed to be treated on Mac:
http://doc.qt.io/qt-5/qfileopenevent.html

rjvbb added a comment.Mar 5 2018, 9:40 PM
Isn't this another case of "should be fixed in QApplication or another lower level library"?

No, not anymore than opening files via the legacy argc,argv mechanism.

So each application

Isn't this another case of "should be fixed in QApplication or another lower level library"?

No, not anymore than opening files via the legacy argc,argv mechanism.

The legacy argc,argv is handled by QCommandLineParser.

Let me rephrase: each and every macOS application which needs to open file via a file manager needs to implement this same code?
If it does need to do that, then this is not the right place to implement it.

Let me rephrase: each and every macOS application which needs to open file via a file manager needs to implement this same code?

File opening via a file manager is handled via QFileOpenEvent.

Let me rephrase: each and every macOS application which needs to open file via a file manager needs to implement this same code?

File opening via a file manager is handled via QFileOpenEvent.

This states that a QFileOpenEvent instance is used, but it is not an answer to my question.

Does each and every macOS application which would like to open a file via a file manager need to implement the same macapplication.cpp (or so) again and again (which makes use of a QFileOpenEvent, yes, but that's not the point; the point is about the content of macapplication.cpp, which is not Okular-specific).

Does each and every macOS application which would like to open a file via a file manager need to implement the same macapplication.cpp (or so) again and again (which makes use of a QFileOpenEvent, yes, but that's not the point; the point is about the content of macapplication.cpp, which is not Okular-specific).

This is the way offered by Qt:
http://doc.qt.io/qt-5/qfileopenevent.html

One could try to reimplement this with Objective-C, I do not know whether it makes sense though.

rjvbb added a comment.Mar 5 2018, 10:37 PM
The legacy argc,argv is handled by QCommandLineParser.

Only partly, the annoying bit where you loop over the array to see which options are there. For the rest it only handles Qt-specific arguments, and files to be opened end up in the positionalArguments() list.

QApplication cannot know if your application can (or should be able to) open files, even less how to open them.
All Qt can do is provide an integrated Qt'ish mechanism through which get informed when the user intends to open a document via LaunchServices. That mechanism encapsulates the ObjC implementation Sergio referred to, and such events can occur at any moment, also when the application is already running.

And so yes, every Qt application that wants to be able to open files through the Finder and other LaunchServices mechanisms must implement a handler (event filter) for QFileOpenEvents.

rjvbb added a comment.Mar 5 2018, 10:39 PM
One could try to reimplement this with Objective-C, I do not know whether it makes sense though.

No, you'd be doing what Qt already does for you.

rjvbb added a comment.Mar 6 2018, 5:48 PM

In addition to the inline comments:

There is no specific reason to hide the QFileOpenEvent handling in a Mac-specific file and behind #ifdefs . I seem to recall that the same event mechanism was used on another, now defunct Qt platform. Other KDE applications (notably Kate) don't bother and just include the extra code unconditionally.

I'd suggest having a look at the QtSingleApplication class used by Kate (and KDevelop, plus undoubtedly others). That'd be a nod to Luigi's wish to defer the required functionality to an external library (QSA could probably be integrated in KGuiAddons or KWidgetsAddons at some point).

It also provides the additional benefit of allowing a single instance approach. I don't know what the Okular team thinks of that I find that it'd make sense (as a user option?), esp. with Sergio's other proposed feature to provide an open documents menu under the Dock tile. That feature would be much more user-friendly if you don't have to try each and every Okular Dock tile until you find the instance that has the document open you want to activate.

I'll hold off giving a +1 until someone from the Okular team reacts to this.

shell/CMakeLists.txt
26–28

You could use list(APPEND ...) here.

shell/macapplication.h
24

public, private etc. keywords are usually left-aligned in KDE code.

There is no specific reason to hide the QFileOpenEvent handling in a Mac-specific file and behind #ifdefs . I seem to recall that the same event mechanism was used on another, now defunct Qt platform. Other KDE applications (notably Kate) don't bother and just include the extra code unconditionally.

Would not it end with having #ifdefs scattered all over the QApplication anyway? I guess the dependent patch would need some #ifdefs then.

rjvbb added a comment.Mar 6 2018, 7:37 PM
Would not it end with having #ifdefs scattered all over the QApplication anyway?

No, none of the Qt APIs you're using are Mac-specific, they just do nothing on other platforms (you won't get any QFileOpen events). Not using #ifdefs also means that if ever Qt extends the API to other platforms (MS Windows, for instance) Okular will leverage it without further changes.

I haven't looked in detail at the additional goodies you added compared to my bare-bones implementation; could it have or be given a more general interest?

No, none of the Qt APIs you're using are Mac-specific, they just do nothing on other platforms (you won't get any QFileOpen events). Not using #ifdefs also means that if ever Qt extends the API to other platforms (MS Windows, for instance) Okular will leverage it without further changes.

I haven't looked in detail at the additional goodies you added compared to my bare-bones implementation; could it have or be given a more general interest?

I mean this patch D11075 (the Dock menu). setAsDockMenu() is a Mac-only function.

rjvbb added a comment.Mar 7 2018, 8:41 AM
I mean this patch D11075 <https://phabricator.kde.org/D11075> (the Dock menu). setAsDockMenu() is a Mac-only function.

Ah, yes of course, that one is.