D5990: Ark: Mac adaptations

[Elvis Angelaccio]

elvisangelaccio added a comment.


  > 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

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

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

Please replace "we" with "MacOS"

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

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

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20170528/28ddbf92/attachment-0001.html>