Ark: Mac UI adaptations
ClosedPublic

Authored by rjvbb on May 27 2017, 5:42 PM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Ark
KDE Applications
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).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.May 27 2017, 5:42 PM
Restricted Application added a subscriber: kde-utils-devel. · View Herald TranscriptMay 27 2017, 5:42 PM
rjvbb updated this revision to Diff 14891.May 27 2017, 5:49 PM

fix an oversight in the kerfuffle patch

rjvbb set the repository for this revision to R36 Ark.May 27 2017, 5:49 PM

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

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?

rjvbb added a comment.May 27 2017, 9:39 PM

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

You want separate review requests?

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.

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

rjvbb added a comment.May 28 2017, 7:39 AM

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.

app/main.cpp
156

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.

325–326

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.

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?

app/main.cpp
323–324

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

new OpenFileEventHandler(&application, window);

this way we don't need Q_UNUSED, no?

325

Please replace "we" with "MacOS"

326

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

kerfuffle/pluginmanager.cpp
273 ↗(On Diff #14891)

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

274–280 ↗(On Diff #14891)

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

kerfuffle/pluginmanager.cpp
273 ↗(On Diff #14891)

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

rjvbb updated this revision to Diff 14915.May 28 2017, 1:17 PM

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?

rjvbb retitled this revision from Ark: Mac adaptations to Ark: Mac UI adaptations.May 28 2017, 1:18 PM
rjvbb set the repository for this revision to R36 Ark.
elvisangelaccio requested changes to this revision.May 29 2017, 9:28 AM

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

This revision now requires changes to proceed.May 29 2017, 9:28 AM
rjvbb updated this revision to Diff 14947.May 29 2017, 4:17 PM
rjvbb edited edge metadata.

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?

rjvbb set the repository for this revision to R36 Ark.May 29 2017, 4:18 PM
elvisangelaccio requested changes to this revision.May 29 2017, 4:45 PM
elvisangelaccio added inline comments.
app/main.cpp
324

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

This revision now requires changes to proceed.May 29 2017, 4:45 PM

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 e932b113b2d9

elvisangelaccio accepted this revision.May 29 2017, 5:26 PM

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

This revision is now accepted and ready to land.May 29 2017, 5:26 PM
rjvbb added a comment.May 29 2017, 7:21 PM

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