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 &argc, 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

Reply via email to