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

fix the preferences menu and opening the settings directory on macOS #11679

Merged

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Jun 23, 2023

  • fix the Preferences menu by avoiding text-heuristics treating "Settings directory" as Preferences (macOS), by change the default menu role QAction::TextHeuristicsRole to QAction::NoRole
  • fix opening the folder by using QUrl::fromLocalFile (this required changing the argument of slotVisitUrl from QString to QUrl, which makes more sense anyway)

@JoergAtGithub
Copy link
Member

I confirm, that this PR works on Windows as 2.4.
This is a string change which affects translation - but it seems we need to change the string to fix this bug. Are we in string freeze phase?
Regarding the string itself, I would suggest to use "Mixxx settings directory" instead of "User settings directory", since we point to a Mixxx specific subdirectory of the (Windows) user settings directory. Other tools I know, use terms like "Open file location" for similar operations. Maybe "Open Mixxx settings directory" is better to recognize therefore?

@m0dB
Copy link
Contributor Author

m0dB commented Jun 24, 2023

I agree that Mixxx settings directory sounds better.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 24, 2023

I don't know about string freeze, but mind you that this was added only days ago.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 24, 2023

Let me see if I can avoid the Settings / Preferences mix-up without changing the string at all.

…u item, as it makes take the role of Preferences; fix opening the folder by using QUrl::fromLocalFile
@m0dB m0dB force-pushed the fix-preferences-menu-and-settings-dir-url branch from dd292aa to 7c67396 Compare June 24, 2023 10:11
@m0dB
Copy link
Contributor Author

m0dB commented Jun 24, 2023

Ok, fixed without having to change the string (aka fixed correctly)

pHelpSettingsDir->setMenuRole(QAction::NoRole);
did the trick.

By default the menuRole is QAction::TextHeuristicRole which means that "This action should be put in the application menu based on the action's text as described in the QMenuBar documentation."

Which lead to "&Settings directory" being treated as Preferences.

@JoergAtGithub
Copy link
Member

LGTM! Works on Windows11 as expected. Waiting for CI. Thank you!

@ronso0
Copy link
Member

ronso0 commented Jun 24, 2023

@m0dB Thanks for debugging this! No regression on Linux afaict. LGTM

Lesson learned: any PR touching the menubar should be tested manually on macOS @mixxxdj/developers

@JoergAtGithub Feel free to rename to "Mixxx settings directory". Better sooner than later, before translators start to work on 2.4-beta.

@m0dB
Copy link
Contributor Author

m0dB commented Jun 24, 2023

Now that this has been fixed without renaming, I don't think renaming to "Mixxx settings ..." is necessary and it actually seems a bit redundant. It should be pretty obvious what settings it refers too!

@JoergAtGithub
Copy link
Member

"Settings directory" and "Mixxx settings directory" work both for me. Just "User settings directory" sounds wrong to me.

@JoergAtGithub JoergAtGithub merged commit bd3a6f5 into mixxxdj:2.4 Jun 24, 2023
13 checks passed
@Swiftb0y
Copy link
Member

Sorry introducing the regression. Many thanks for fixing it this quickly.

Lesson learned: any PR touching the menubar should be tested manually on macOS https://github.com/orgs/mixxxdj/teams/developers

Agree. I'll be more careful next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants