Diff Detail
- Repository
- R223 Okular
- Lint
Lint Skipped - Unit
Unit Tests Skipped
(Please fix the edit policy of the review, which is now too restricted)
Also:
https://community.kde.org/Policies/Commit_Policy#Always_add_descriptive_log_messages
Given you've been doing the same patch for multipple apps now, any chance this can be generated using an ECM macro, pernhaps populated from the app's desktop file or appstream data?
This change does more than just enabling hidpi support in the plist.
Given you've been doing the same patch for multipple apps now, any chance this can be generated using an ECM macro, pernhaps populated from the app's desktop file or appstream data?
Generating the correct XML from that data is maybe a bit of a tall order for cmake's language?
As to the hidpi support: why is it necessary to set that in the plist in addition to what's done in the code? This seems like something that should be fixed in Qt's cmake modules or even in cmake itself (wherever the default Info.plist model comes from).
FWIW, LSMultipleInstancesProhibited should of course not be set by default (should it for Okular which seems to launch in multiple instances on Linux?)
Lastly: please add kde-mac to the subscriber list for Mac-related reviews.
Actually, I realise this patch too is a somewhat stripped down version of a patch I've been using for a long time in my MacPorts packaging for Okular:
https://github.com/RJVB/macstrop/blob/master/kf5/kf5-okular/files/patch-open-docs-from-Finder.diff
This diff is simply named wrong. Hidpi (retina) support is only one of the purposes of this custom Info.plist . In addition to that it also adds support for configuring the copyright message, adds the indicator that Carbon is required (mostly for good measure) and above all, registers what kind of documents the application can handle with LaunchServices (read: the Finder). That was the initial purpose of using a custom Info.plist, the other changes are just icing on the cake. FWIW, that CFBundleDocumentTypes is pointless without a change to the code, handling QEvent::FileOpen (see the shell.cpp diff in the linked patch).