D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin abandoned this revision.
Herald edited subscribers, added: okular-devel; removed: Okular.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: okular-devel, ltoscano, rjvbb, ngraham, darcyshen, aacid, #okular

D11074: Fix opening files via a file manager on Mac

[René J . V . Bertin]

rjvbb added a comment.


  >   I mean this patch D11075  (the Dock 
menu). setAsDockMenu() is a Mac-only function.
  
  Ah, yes of course, that one is.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin added a comment.


  In D11074#220231 , @rjvbb wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[René J . V . Bertin]

rjvbb added a comment.


  >   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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin added a comment.


  In D11074#220158 , @rjvbb wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[René J . V . Bertin]

rjvbb added a comment.


  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.

INLINE COMMENTS

> CMakeLists.txt:26-28
> +set(okular_SRCS
> +${okular_SRCS}
> +macapplication.cpp)

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

> macapplication.h:24
> +
> +public:
> +MacApplication(int , char **argv);

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[René J . V . Bertin]

rjvbb added a comment.


  >   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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[René J . V . Bertin]

rjvbb added a comment.


  >   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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin added a comment.


  In D11074#219505 , @ltoscano wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin added a dependent revision: D11075: Make Okular show the list of opened 
windows in the Dock menu.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Luigi Toscano]

ltoscano added a comment.


  In D11074#219504 , @sbragin wrote:
  
  > In D11074#219502 , @ltoscano 
wrote:
  >
  > > 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).

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin added a comment.


  In D11074#219502 , @ltoscano wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Luigi Toscano]

ltoscano added a comment.


  So each application
  
  In D11074#219501 , @rjvbb wrote:
  
  > >   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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[René J . V . Bertin]

rjvbb added a comment.


  >   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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin added a comment.


  In D11074#219475 , @ltoscano wrote:
  
  > 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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Luigi Toscano]

ltoscano added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D11074

To: sbragin
Cc: ltoscano, rjvbb, #okular, michaelweghorn, ngraham, aacid

D11074: Fix opening files via a file manager on Mac

[Sergio Bragin]

sbragin created this revision.
Restricted Application added a project: Okular.
sbragin requested review of this revision.

REVISION 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

REPOSITORY
  R223 Okular

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11074

AFFECTED FILES
  shell/CMakeLists.txt
  shell/macapplication.cpp
  shell/macapplication.h
  shell/main.cpp

To: sbragin
Cc: rjvbb, #okular, michaelweghorn, ngraham, aacid