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

User right group for "ViewMenu_ShowSkinSettings", fixes lp1851993. #2358

Merged
merged 2 commits into from
Nov 26, 2019

Conversation

daschuer
Copy link
Member

@daschuer daschuer added this to the 2.3.0 milestone Nov 15, 2019
@daschuer
Copy link
Member Author

This is also a 2.2 issue. Need to rebase.

@daschuer daschuer modified the milestones: 2.3.0, 2.2.3 Nov 16, 2019
@Holzhaus
Copy link
Member

I this a MacOS-only fix or is the issue reproducable on other OSes, too? I'd test your PR, but I have no Mac.

@ronso0
Copy link
Member

ronso0 commented Nov 16, 2019

At lest on linux it works perfectly without this PR: shortcut is displayed Ctrl + 1 and it toggles the menu.

@daschuer
Copy link
Member Author

Yes, this works in my case as well. The issue that is fixed here is that there are

[KeyboardShortcuts], ViewMenu_ShowSkinSettings entries in the keyboard mapping files:

ViewMenu_ShowSkinSettings Ctrl+1

Maybe they are used an MacOs and the default fails for some reason.

@Holzhaus
Copy link
Member

Hmm, I have the issue of missing keyboard shortcuts on Linux as well (even with this PR applied).
The problems seems to be locale-dependent:

With LANG=de_DE ./mixxx the problem persists:
2019-11-21-163824_643x366_scrot

With LANG=C ./mixxx the problem is gone:
2019-11-21-163706_596x410_scrot

@ronso0
Copy link
Member

ronso0 commented Nov 21, 2019

At lest on linux it works perfectly without this PR: shortcut is displayed Ctrl + 1 and it toggles the menu.

Aha. I use the system locale, which is en_US

@daschuer
Copy link
Member Author

In the German version the short cuts should be translated as well.
"Strg+1"

@esbrandt
Copy link
Contributor

In the German version the short cuts should be translated as well.

Hmm, they already are translated
https://www.transifex.com/mixxx-dj-software/mixxxdj/translate/#de/mixxx2-3/153124672?q=text%3Actrl.

Could it be, that not the translated shortcuts from res/translations/*.ts, but from the keyboard shortcut file res/keyboard/*.kbd.cfg are displayed? We have contradicting shortcuts in

ViewMenu_ShowSkinSettings Ctrl+1
ViewMenu_ShowMicrophone Ctrl+2
ViewMenu_ShowVinylControl Ctrl+3
ViewMenu_ShowPreviewDeck Ctrl+4
ViewMenu_ShowEffects Ctrl+5
ViewMenu_ShowCoverArt Ctrl+6
ViewMenu_MaximizeLibrary Space

I have never noticed on macOS, because there is always the Command key (⌘) on display, no matter the language .
Capto_Capture 2019-11-21_09-10-55_PM

@Holzhaus
Copy link
Member

Could it be, that not the translated shortcuts from res/translations/*.ts, but from the keyboard shortcut file res/keyboard/*.kbd.cfg are displayed? We have contradicting shortcuts in

ViewMenu_ShowSkinSettings Ctrl+1
ViewMenu_ShowMicrophone Ctrl+2
ViewMenu_ShowVinylControl Ctrl+3
ViewMenu_ShowPreviewDeck Ctrl+4
ViewMenu_ShowEffects Ctrl+5
ViewMenu_ShowCoverArt Ctrl+6
ViewMenu_MaximizeLibrary Space

Bingo, running this command fixed the issue for me:

$ sed -i 's/ViewMenu_ShowSamplers Ctrl+1/ViewMenu_ShowSkinSettings Ctrl+1/' res/keyboard/*.kbd.cfg

@uklotzde uklotzde mentioned this pull request Nov 25, 2019
@daschuer
Copy link
Member Author

Does this mean we can merge this now?

@Holzhaus
Copy link
Member

As I said, the issue is only fixed if you fix the keyboard shortcuts in res/keyboard/*.kbd.cfg. For some reason, Ctrl+1 is mapped to ShowSamplers instead of ShowSkinSettings there.

@daschuer
Copy link
Member Author

Ah got it. I did not understand it, because it was already fixed in master
4db75b6

@daschuer
Copy link
Member Author

OK, kbd.cfg files are fixed now.

@Holzhaus
Copy link
Member

I tested with LANG=cs_CZ, da_DK, de_CH, de_DE, el_GR, en_US, es_ES, fi_FI, fr_CH, fr_FR, it_IT and ru_RU and the Ctrl+1 shortcut shows up and works with all of them. LGTM.

@uklotzde
Copy link
Contributor

Also, LGTM.

@daschuer Could you please add a line in the changelog in #1789?

@uklotzde uklotzde merged commit a662abc into mixxxdj:master Nov 26, 2019
@uklotzde uklotzde modified the milestones: 2.2.3, 2.3.0 Nov 26, 2019
@uklotzde
Copy link
Contributor

The branch was targeted to master!!

@uklotzde
Copy link
Contributor

Follow-up #2372

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

Successfully merging this pull request may close these issues.

5 participants