Separator shouldn't display at bottom of "File" menu on Mac OS X

[follower]

This seems to be a regression related to the move from Qt4 to Qt5.

Notice there is a separator at the bottom of the "File" menu which is unnecessary and shouldn't be there:

Version information (this is on Mac OS X 10.8.x):

For comparison here is the same menu with Qt4:

And version information for Qt4 build on same system:

The reason this occurs is because Qt automatically moves the "Quit" menu item from the file menu to the "Apple Application Menu" as is usual for the platform. Normally any redundant separator is also removed at the same time.

This appears to be a known issue for Qt: "[QTBUG-40071] separatorsCollapsible does not work when menu items are moved to the application menu on Mac OS X".

I've added further detail to the QTBUG entry but have been unable to determine an underlying cause so far.

Before I realised it was an issue with Qt I developed a simple workaround which results in:

Given that it appears to be a known issue that's not been resolved in nearly 3 years I'd suggest using the workaround in lmms in the meantime.

If desired I can make a PR with the workaround (with further documentation):

diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp
index 4a5bacb..aa943c8 100644
--- a/src/gui/MainWindow.cpp
+++ b/src/gui/MainWindow.cpp
@@ -315,7 +315,9 @@ void MainWindow::finalize()
                                        SLOT( exportProjectMidi() ),
                                        Qt::CTRL + Qt::Key_M );*/
 
+#ifndef LMMS_BUILD_APPLE
        project_menu->addSeparator();
+#endif
        project_menu->addAction( embed::getIconPixmap( "exit" ), tr( "&Quit" ),
                                        qApp, SLOT( closeAllWindows() ),
                                        Qt::CTRL + Qt::Key_Q );

[tresf]

@follower please issue a pull request and we'd happily merge. Also; if Qt5 specific also add #if QT_VERSION >= 0x050000 ... Nevermind that last part, I realize this is a removal which should work fine on both Qt4/5

[follower]

Thanks for taking a look.

I'm working on a PR and decided it was probably still wise to keep the version check as we know the workaround is not needed on 4 and my impression of Qt so far is that assumptions it won't break anything on Qt 4 may be misplaced. :)

Also, I've just realised that the "Help" menu also has the same issue, so will do the same there.

[follower]

The original Qt bug has now been closed as "Cannot Reproduce" as of Qt 5.6, are you able to confirm this with one of your builds with a later Qt @tresf?

(Side note: As you might've guessed I finally managed to get Qt 5.5.1 + lmms building okay, so hopefully I can now contribute something more to lmms than requests for builds for obsolete OS. :D )

[tresf]

are you able to confirm [QTBUG-40071 is fixed] with one of your builds with a later Qt @tresf?

Yes, confirmed as fixed in Qt 5.7

[follower]

Thanks for checking. :)

So that's fixed within Qt 5.7 and without needing a workaround in lmms?

If that's the case, would you like me to add an upper bound to the Qt version check for the workaround?

[tresf]

So that's fixed within Qt 5.7 and without needing a workaround in lmms?

Sounds like a noble idea. We're probably releasing 1.2 against Qt5.5 on Mac due to the macdeployqt bug that prevents us from packaging on newer versions (I'm currently working around this here but haven't merged the changes in yet.

[follower]

I guess I'll take Qt's word for it and make the check be >= 5.0 and < 5.6. Feel free to verify 5.6. :)

[follower]

Closed by merge of #3350.

[follower]

FWIW recent tests & most recent comments on the original issue show that this issue still occurs in 5.6+.

This suggests the upper-bound version check should probably just be removed. (The linked comment also provides an alternate approach example of SLOT(onHelpAbout()) which might be worth considering.)

[tresf]

This suggests the upper-bound version check should probably just be removed.

Agreed. I see no harm in this. 👍