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

FEAT(client): Introduce GlobalShortcutButtons, for better shortcut management #4722

Merged

Conversation

davidebeatrici
Copy link
Member

@davidebeatrici davidebeatrici commented Jan 28, 2021

GlobalShortcutButtons is a dialog that allows to efficiently view/add/remove buttons for a specific shortcut.

The tree design allows to easily identify the buttons and the device they're attached to.

The capture process can be started by pressing on Add.
Once all buttons are released the process stops automatically.

There's only a cosmetic issue, that should be fixed in our theme: no distinction between active and focused QPushButton.


GlobalShortcut::buttonName() and GlobalShortcut::buttonText() are removed in favor of GlobalShortcut::buttonInfo().

The new function returns a ButtonInfo structure, which contains two strings: one identifies the device, the other one the button.

ShortcutKeyWidget is removed, as replaced by GlobalShortcutButtons.




@Krzmbrzl
Copy link
Member

I would prefer showing the actual key combination instead of the number of buttons that it consists of. 'cause in the end the user wants to know which keys to press and not how many 🤷

@davidebeatrici
Copy link
Member Author

Should I show the key combination without specifying the device?

For example: f + e + y + r + Print + 0 + 2

@Krzmbrzl
Copy link
Member

Yes that LGTM. Are there situations in which the device information could get important?

@davidebeatrici
Copy link
Member Author

Definitely, for example when multiple devices may have a button with the same name.

However, you can just open the dialog to have detailed info.

@davidebeatrici
Copy link
Member Author

Done.

@Krzmbrzl
Copy link
Member

3 things:

  1. I think we should either omit the + between the button names or wrap the actual button names in quotes. Otherwise it could be weird if a plus sign is actually involved in the key combo. I actually think that simply using spaces to separate the keys like a b is fine.
  2. I think we should try to not display mouse buttons as simple digits since that should be reserved for the actual number keys on the keyboard. Instead we could display something like M1, M2 and so on
  3. How are special keys (spacebar, backspace, enter, control, alt, etc.) displayed? I think for those we should display space, bspace, enter, ctrl, alt, ... 🤔

Definitely, for example when multiple devices may have a button with the same name.

In that case I think buttons from these devices should get a special prefix. Thus only regular keyboard keys should be displayed without a prefix while all other keys should get a short prefix identifying the device they are living on (similar to my suggestion for the mouse keys). Otherwise there will probably be a lot of confusion there.

@davidebeatrici
Copy link
Member Author

  1. Wrapping each button name in quotes is definitely the way to go, as a button name may contain spaces.
  2. Excellent point.
  3. It strictly depends on the backend. For example, on Linux:

In that case I think buttons from these devices should get a special prefix. Thus only regular keyboard keys should be displayed without a prefix while all other keys should get a short prefix identifying the device they are living on (similar to my suggestion for the mouse keys). Otherwise there will probably be a lot of confusion there.

In that case I would add another QString member to ButtonInfo, called devicePrefix.

That way we can decide the appropriate prefix for each device in each backend. What do you think?

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Feb 1, 2021

Wrapping each button name in quotes is definitely the way to go, as a button name may contain spaces.

Ah yes in that case we should definitely use quotes. I think using single quotes is best for this purpose. Except for the single quote character itself. Using double quotes for that is ugly as well though. Not sure what we should do with that. I guess for the time being it would be fine to just also use single quotes around it since it is probably unlikely that someone maps this key anyways 🤔

I still think we should additionally drop the + sign between the buttons in order to save screen space.

It strictly depends on the backend. For example, on Linux:

That shouldn't be the case. Do you think it would be possible to create a mapping from backend specific representations to unified representations in Mumble?
Having something like this would also greatly facilitate something like #3244.

In that case I would add another QString member to ButtonInfo, called devicePrefix.
That way we can decide the appropriate prefix for each device in each backend. What do you think?

In principle that sounds good. I am not entirely sure though what exactly you have in mind with your last sentence. Could you give a quick example?

@davidebeatrici
Copy link
Member Author

Ah yes in that case we should definitely use quotes. I think using single quotes is best for this purpose. Except for the single quote character itself. Using double quotes for that is ugly as well though. Not sure what we should do with that. I guess for the time being it would be fine to just also use single quotes around it since it is probably unlikely that someone maps this key anyways 🤔

I agree, consider that the single quote shows up as apostrophe on Linux.

I still think we should additionally drop the + sign between the buttons in order to save screen space.

Absolutely.

That shouldn't be the case. Do you think it would be possible to create a mapping from backend specific representations to unified representations in Mumble?
Having something like this would also greatly facilitate something like #3244.

Mumble saves the raw button info in the configuration, not the button name. As such it's not possible to import/export shortcuts in different operating systems, because each backend identifies buttons its own way.

In principle that sounds good. I am not entirely sure though what exactly you have in mind with your last sentence. Could you give a quick example?

  • Mouse:
    ButtonInfo info;
    info.device = tr("Mouse");
    info.devicePrefix = QLatin1String("M");
  • Keyboard:
    ButtonInfo info;
    info.device = tr("Keyboard");
    info.devicePrefix = QLatin1String("K");
  • Xbox controller:
    ButtonInfo info;
    info.device = tr("Xbox controller");
    info.devicePrefix = QLatin1String("Xbox");
  • Other HID device:
    ButtonInfo info;
    info.device = tr("[abcd:1234] My HID device");
    info.devicePrefix = QLatin1String("[abcd:1234]");

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Feb 1, 2021

Mumble saves the raw button info in the configuration, not the button name. As such it's not possible to import/export shortcuts in different operating systems, because each backend identifies buttons its own way.

That wasn't even my main point. My main point was that we should not depend on the backend for naming of buttons. That can be used as a fallback but in general I would prefer having buttons named consistently across backends.
And then we could indeed perform a name to backend specific ID mapping that'd allow for import/export of shortcuts. But as I said: that's secondary. We should try to use a consistent naming of buttons throughout different backends (imo).

The suggestion with the additional prefix fields LGTM 👍
Only keyboard should not get a prefix as that should kinda be the default and thus I don't think that it needs (nor should have) a prefix 🤔

@davidebeatrici
Copy link
Member Author

Sure, no problem.

@davidebeatrici davidebeatrici force-pushed the globalshortcutbuttons branch 2 times, most recently from 8b5cc42 to 683d8b9 Compare February 2, 2021 01:35
@davidebeatrici
Copy link
Member Author

Updated.

…nagement

This is a dialog that allows to efficiently view/add/remove buttons for a specific shortcut.

The tree design allows to easily identify the buttons and the device they're attached to.

The capture process can be started by pressing on "Add".
Once all buttons are released the process stops automatically.

There's only a cosmetic issue, that should be fixed in our theme: no distinction between active and focused QPushButton.
…tcut

GlobalShortcut::buttonName() and GlobalShortcut::buttonText() are removed in favor of GlobalShortcut::buttonInfo().

The new function returns a ButtonInfo structure, which contains two strings: one identifies the device, the other one the button.

ShortcutKeyWidget is removed, as replaced by GlobalShortcutButtons.
Scanning directory './src'...
Scanning directory './src/mumble'...
Updating 'src/mumble/mumble_en.ts'...
    Found 1937 source text(s) (15 new and 1922 already existing)
    Removed 4 obsolete entries
@davidebeatrici davidebeatrici merged commit 002a527 into mumble-voip:master Feb 5, 2021
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.

None yet

2 participants