Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround Qt5 bug that fails to remove separator at end of File/Help menus #3350

Merged

Conversation

follower
Copy link
Contributor

See #3345 for details.

When the Quit menu item is relocated to Application Menu on
Mac OS X / macOS the separator above it should be removed. While
Qt4 removes the separator, Qt5 does not remove the separator.

For details see:
 * <LMMS#3345>
 * <https://bugreports.qt.io/browse/QTBUG-40071>
When the About menu item is relocated to Application Menu on
Mac OS X / macOS the separator above it should be removed. While
Qt4 removes the separator, Qt5 does not remove the separator.

For details see:
 * <LMMS#3345>
 * <https://bugreports.qt.io/browse/QTBUG-40071>
@follower
Copy link
Contributor Author

Note: I've only tested this fix on Qt5 + Mac OS X 10.8.x.

@tresf
Copy link
Member

tresf commented Feb 13, 2017

@follower thanks, your test results are sufficient.

I think your comment could be shorted some.

Conditional compilation to workaround Qt5 bug is redundant since you're using precompiler flags and the flag has Qt in the name. I'd shrink it to simply:

// Per https://bugreports.qt.io/browse/QTBUG-40071

Or if you prefer to leave an explanation:

// remove dangling separator at end of menu per https://bugreports.qt.io/browse/QTBUG-40071

Also note that removing the specificity of Quit also allows others to copy and past this patch in other areas moving forward without fear of contextual comment typos.

@follower
Copy link
Contributor Author

Thanks for the review.

I suspected I might get that feedback--I've never been accused of being excessively terse. :)

I can make the change as suggested but wanted to mention part of the motivation for the wording was to make it clear that it was the preprocessor conditional that was the workaround not the addition of the separator. I'd normally put the comment on the #if line but figured it'd make the line too long in this case.

Will go with:

// Prevent dangling separator at end of menu per https://bugreports.qt.io/browse/QTBUG-40071

@follower
Copy link
Contributor Author

Should now exclude Qt 5.6+. Tested with Qt 5.5.1.

@tresf tresf merged commit b6441b7 into LMMS:master Feb 19, 2017
@tresf
Copy link
Member

tresf commented Feb 19, 2017

@follower thanks!!!

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants