Remove menu icons from non-document menu actions on macOS/OS X

[follower]

(Originally this bug was this but see #3362:

As previously mentioned (#2890 (comment)) the new white menu icons do not display correctly on Mac OS X 10.8 with Qt5:

Related version info:

I assume if they display on later OS X versions it's due to a change in menu background colour?
)

Now:

FWIW, I think on Mac OS X / macOS icons should not be displayed on menus at all, as it's not "standard practice" on the platform and IMO makes lmms appear "less native". Further details on request. :)

It appears specifying Qt::AA_DontShowIconsInMenus would enable this but I've not tested it yet.

[tresf]

IMO makes lmms appear "less native"

Probably the more important part of this is the UI experience. OSs which can't align text in a menu should have the icons turned off, period. Please issue a Pull Request with Qt::AA_DontShowIconsInMenus activated for MacOS only and we can finally put this issue to rest. 👍

[follower]

Once I started work on this I realised that files not having icons in the "New from template" and "Recently Opened Projects" sub-menus looks a bit weird. By way of comparison, "Preview.app" shows icons in its "Open Recent" sub-menu.

This led me to wonder what the official Apple Human Interface Guidelines say: "Icons and Symbols in Menus".

My reading of the HIG suggests that keeping the icons in the aforementioned sub-menus is legit, so I'll see how much effort it is to retain them while eliminating the rest. It seems like QAction::iconVisibleInMenu may enable this behaviour.

Other than that, things look nice and clean. :)

[tresf]

Sounds good. I noticed the icons don't seem so cosmetically weird if the icons aren't white. For reference here's a side-by-side with Google Chrome:

[tresf]

I just read a bug report with users complaining about this identical behavior, except when in dark mode. Apparently there's an unmerged feature to help... https://codereview.qt-project.org/#/c/115120/

Linked from https://bugreports.qt.io/browse/QTBUG-42109.

For the short-term, we can use a quick hack, such as inverting the color palette for these icons. http://doc.qt.io/qt-5/qimage.html#invertPixels

[follower]

I managed to get the submenu icons to display (still as white icons for default theme, as icons for classic theme) by using setIconVisibleInMenu(true) but (surprise, surprise) there appears to be a bug in Qt that means the change in visibility doesn't propagate correctly (in spite of the fact that the original issue was marked as "Cannot Reproduce") apparently when addAction() is used.

I've worked out a few variations of a workaround for the apparent bug and plan to use the one which is least disruptive to the existing code and requires least additional code.

[follower]

So, once QApplication::setAttribute(Qt::AA_DontShowIconsInMenus, true); has been used, in theory we can turn individual icons back on using setIconVisibleInMenu(true) .

Let's look at the different approaches we can use by using this example from MainWindow::updateRecentlyOpenedProjectsMenu() in src/gui/MainWindow.cpp:

m_recentlyOpenedProjectsMenu->addAction(embed::getIconPixmap( "project_file" ), *it );

Making icons visible

Chained method calls

The simplest approach would be to use:

m_recentlyOpenedProjectsMenu->addAction(embed::getIconPixmap( "project_file" ), *it )->setIconVisibleInMenu(true);

However, I wasn't sure if "chained" method calls like that were within the lmms "style" (it's seems they're used sometimes but not frequently) and while (in theory) it should be a no-op for other platforms, since we're wanting to make this change be Apple only it would be a little "messy" to make the compilation conditional.

That consideration becomes a somewhat moot point however due to the bug mentioned in my previous comment.

Using actions().last()

With the bug in mind and the desire for conditional compilation I think the preferable approach is to add this as a separate line:

m_recentlyOpenedProjectsMenu->actions().last()->setIconVisibleInMenu(true);

Using the returned object

Another alternative would be to avoid the actions()/last() method calls and do this but again it's a more complex change:

QAction *newAction = m_recentlyOpenedProjectsMenu->addAction(embed::getIconPixmap( "project_file" ), *it );
newAction->setIconVisibleInMenu(true)

Creating the QAction first and avoiding the bug/workaround

AFAICT the bug occurs when using .addAction() rather than creating the QAction first, so we could change the existing code to be something like this but IMO doing this creates too many changes in the existing code:

QAction *newAction = new QAction(embed::getIconPixmap( "project_file" ), *it );
newAction->setIconVisibleInMenu(true);
m_recentlyOpenedProjectsMenu->addAction(newAction);

The bug

The change in icon visibility doesn't get propagated (using sendDataChanged()) due to the test(s) here: https://github.com/qt/qtbase/blob/99b6eb4382e135815b53b567297be9cd4ecfd96a/src/widgets/kernel/qaction.cpp#L1278

        // Only send data changed if we really need to.
        if (oldValue != -1
            || (oldValue == -1
                && visible == !QApplication::instance()->testAttribute(Qt::AA_DontShowIconsInMenus))) {
            d->sendDataChanged();
        }

It's a little unclear what the correct test should be but that discussion belongs in the bug report "[QTBUG-44565] CLONE - QAction::setIconVisibleInMenu does not work."

Bug workarounds

Toggle approach

While using the actions().last() approach works for our purpose, due to the bug this change in visibility doesn't get propagated so we need to workaround it by doing setting the value to false first (when first instantiated the value is -1 i.e. default/global setting):

m_recentlyOpenedProjectsMenu->actions().last()->setIconVisibleInMenu(false);
m_recentlyOpenedProjectsMenu->actions().last()->setIconVisibleInMenu(true);

Manual event propagation trigger

Doing the above works around the bug in the "quickest" way but doesn't actually result in the same flow of events.

To actually cause the "correct" event flow to happen we could manually trigger the event ourselves like this but this seems overkill for our purposes:

m_recentlyOpenedProjectsMenu->actions().last()->setIconVisibleInMenu(true);
QActionEvent::QActionEvent event(QEvent::ActionChanged, m_recentlyOpenedProjectsMenu->actions().last());
QApplication::sendEvent(m_recentlyOpenedProjectsMenu, &event);

Conclusion

Anyway, this is mostly a record for my benefit but might be useful in case other people encounter the same issue.

PR with functionality + fix will be forthcoming.

[follower]

Would've been a good idea to create a separate issue earlier but oh well... :)

#3362 is now for handling icon inversion but I'm not planning to touch that for the moment.

This is now just for removing the menu icons.

[follower]

Here's how the menus look with the patches...

Classic theme (projects & templates submenus):

Default theme (projects & templates submenus):

[follower]

Closed by #3363.