D5990: Ark: Mac adaptations

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a project: Ark.
Restricted Application added a subscriber: kde-utils-devel.

REVISION SUMMARY
This is a patch addressing several features to improve Ark's Mac integration:

- allow opening files from LaunchServices (the Finder, the `open` commandline utility etc)
- prevent replacing the embedded app icon with the result of a failed lookup-from-theme
- ported `PluginManager::libarchiveHasLzo()`

Ideally the toplevel CMake file should check the clang version being used; the build fails for me with AppleClang 600 (= clang 3.5).

REPOSITORY
R36 Ark

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

AFFECTED FILES
app/CMakeLists.txt
app/MacOSXBundleInfo.plist.in
app/main.cpp
kerfuffle/pluginmanager.cpp

To: rjvbb, #ark, #kde_applications
Cc: kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb updated this revision to Diff 14891.
rjvbb added a comment.


fix an oversight in the kerfuffle patch

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5990?vs=14890&id=14891

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

AFFECTED FILES
app/CMakeLists.txt
app/MacOSXBundleInfo.plist.in
app/main.cpp
kerfuffle/pluginmanager.cpp

To: rjvbb, #ark, #kde_applications
Cc: kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb set the repository for this revision to R36 Ark.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio added a comment.


Could you please split the patch into self-contained commits? (it would be easier to review the changes)

Post by René J.V. Bertin
Ideally the toplevel CMake file should check the clang version being used; the build fails for me with AppleClang 600 (= clang 3.5).

What's the error? Can't we just fix it?

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb added a comment.

Post by Elvis Angelaccio
Could you please split the patch into self-contained commits? (it would be easier to review the changes)

You want separate review requests?

Post by Elvis Angelaccio

Post by René J.V. Bertin
Ideally the toplevel CMake file should check the clang version being used; the build fails for me with AppleClang 600 (= clang 3.5).

What's the error? Can't we just fix it?

There's an error about `return QString();` from the lambda in the kerfuffle plugin which can be fixed with a cast, but there are other issues which I think stem from incomplete C++11 support.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio added a comment.


Yes, please (phabricator allows a single review for a branch with more than one commits, but the UI doesn't let you see which commit does what...).

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb added a comment.

Post by Elvis Angelaccio
Yes, please (phabricator allows a single review for a branch with more than one commits, but the UI doesn't let you see which commit does what...).

Either way I don't submit off locally committed changes. I've added inline explanations to the 2 small changes to main.cpp that I would have committed directly if there hadn't been other changes too. I think there isn't much to review about them but let me know if you want to do a full review (or 2!) anyway. If not that saves all of us some work.

Also, please let me know if you think I should reduce the #ifdefs to the strict minimum in main.cpp . A priori none of them is required, even the event filter could be installed as-is on all supported platforms if you don't mind invoking such a filter for nothing.

INLINE COMMENTS

Post by Elvis Angelaccio
main.cpp:162
KAboutData::setApplicationData(aboutData);
- application.setWindowIcon(QIcon::fromTheme(QStringLiteral("ark")));
+ application.setWindowIcon(QIcon::fromTheme(QStringLiteral("ark"), application.windowIcon()));

Specify the current application icon as the fallback for the lookup from theme. It will be empty like the default fallback when there is no embedded app icon and so doesn't change anything. Same thing on platforms where there is an embedded icon but no support for icon themes.

Post by Elvis Angelaccio
main.cpp:331-332
+ Q_UNUSED(openFileEventHandler);
+ // we don't provide a fullscreen mode
+ window->setWindowFlags(window->windowFlags() & ~Qt::WindowFullscreenButtonHint);
+#endif

This removes the fullscreen button in the window titlebar. It is a bit cumbersome to exit the resulting fullscreen mode when there is no menu action of keyboard shortcut to accomplish this - which I think must be because there is little need for such a mode in an application like Ark. (Especially not knowing that the native Mac fullscreen mode can black out all other monitors on a multi-head setup.)
The zoom/maximise button is not affected.

The `WindowFullscreenButtonHint` flag is Mac-specific as far as I know and thus doesn't require an #ifdef. The QOpenFileEvent handler stuff doesn't need it either.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio added a comment.

Post by René J.V. Bertin
Also, please let me know if you think I should reduce the #ifdefs to the strict minimum in main.cpp . A priori none of them is required, even the event filter could be installed as-is on all supported platforms if you don't mind invoking such a filter for nothing.

Yes, the less the ifdefs the better. So, the `QFileOpenEvent` event won't be sent on non-mac platforms?

INLINE COMMENTS

Post by René J.V. Bertin
main.cpp:329-330
+#ifdef Q_OS_MACOS
+ volatile const OpenFileEventHandler *openFileEventHandler = new OpenFileEventHandler(&application, window);
+ Q_UNUSED(openFileEventHandler);
+ // we don't provide a fullscreen mode

While `volatile`? Also, couldn't we just use:

new OpenFileEventHandler(&application, window);

this way we don't need Q_UNUSED, no?

Post by René J.V. Bertin
main.cpp:331
+ Q_UNUSED(openFileEventHandler);
+ // we don't provide a fullscreen mode
+ window->setWindowFlags(window->windowFlags() & ~Qt::WindowFullscreenButtonHint);

Please replace "we" with "MacOS"

Post by René J.V. Bertin
main.cpp:332
+ // we don't provide a fullscreen mode
+ window->setWindowFlags(window->windowFlags() & ~Qt::WindowFullscreenButtonHint);
+#endif

That's the only thing that really needs an ifdef, right?

Post by René J.V. Bertin
pluginmanager.cpp:273
// Step 2: ldd the libarchive plugin to figure out the absolute libarchive path.
QProcess ldd;
+#ifdef Q_OS_MACOS

Please rename this variable, since it's not only about "ldd" anymore.

Post by René J.V. Bertin
pluginmanager.cpp:274-280
+#ifdef Q_OS_MACOS
+ QRegularExpression regex(QStringLiteral("/.*/libarchive.*.dylib"));
+ ldd.start(QStringLiteral("otool"), {QStringLiteral("-L"), pluginPath});
+#else
+ QRegularExpression regex(QStringLiteral("/.*/libarchive.so"));
ldd.start(QStringLiteral("ldd"), {pluginPath});
+#endif

This ifdef could go away, use `QStandardPaths::findExecutable()` to look for "otool" and use a regex that matches both types of path.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

elvisangelaccio wrote in pluginmanager.cpp:273
Please rename this variable, since it's not only about "ldd" anymore.

...and also adjust the ldd mention in the comment above

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb updated this revision to Diff 14915.
rjvbb added a comment.


The kerfuffle changes (lowlevel) were moved to their own revision, making this a patch that affects the user experience directly.

The #ifdefs were all removed, since the additional code should not have any effect elsewhere but on Mac (QFileOpenEvent might in the future be supported elsewhere too, btw).

The event handler class instance is declared volatile to decrease the chance that the linker will optimise the assignment away because the variable isn't used. Maybe wouldn't happen anyway when using `new` to initialise a local variable?

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5990?vs=14891&id=14915

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

AFFECTED FILES
app/CMakeLists.txt
app/MacOSXBundleInfo.plist.in
app/main.cpp

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb retitled this revision from "Ark: Mac adaptations" to "Ark: Mac UI adaptations".
rjvbb set the repository for this revision to R36 Ark.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.

The event handler class instance is declared volatile to decrease the chance that the linker will optimise the assignment away because the variable isn't used. Maybe wouldn't happen anyway when using new to initialise a local variable?

Correct, see https://stackoverflow.com/questions/27741698/is-c-compiler-allowed-to-optimize-out-unreferenced-local-objects
So please remove the `volatile`.

In fact, the `new Foo();` pattern is quite common in Qt code, e.g. for dbus adaptor one would just do `new MyAdaptor(this);`.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb updated this revision to Diff 14947.
rjvbb added a comment.


patch updated as requested.

FWIW I'm now seeing `Unhandled container to remove : MainWindow` when I exit Ark, but that must be unrelated it seems?

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5990?vs=14915&id=14947

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

AFFECTED FILES
app/CMakeLists.txt
app/MacOSXBundleInfo.plist.in
app/main.cpp

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb set the repository for this revision to R36 Ark.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

main.cpp:324
+ // Ark doesn't provide a fullscreen mode; remove the corresponding window button
+ window->setWindowFlags(window->windowFlags() & ~Qt::WindowFullscreenButtonHint);
window->show();

One last thing and then it's good to go: unset this flag in the MainWindow constructor, since main.cpp is already quite crowded.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio added a comment.

FWIW I'm now seeing Unhandled container to remove : MainWindow when I exit Ark, but that must be unrelated it seems?

Yes, seems I introduced that with https://phabricator.kde.org/R36:e932b113b2d9ee7b792c23057ea2e15f8ea4f9c5

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.
This revision is now accepted and ready to land.


@rjvbb For future reference, you should end the commit message with this line:

`Differential Revision: https: //phabricator.kde.org/DXXX`

otherwise phab won't autoclose the review.

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb added a comment.


Yeah, keep forgetting the exact syntax - I must have the equivalent of a hemineglect for the term "differential revision" :)

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara

[René J.V. Bertin]

rjvbb closed this revision.
rjvbb added a comment.


https://commits.kde.org/ark/e3ff8c3b7cceb98684600d611a7b8bc1173d91fb

REPOSITORY
R36 Ark

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

To: rjvbb, #ark, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, kde-utils-devel, kde-mac, tctara