-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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): Implemented new Global Shortcuts #6260
FEAT(client): Implemented new Global Shortcuts #6260
Conversation
I'm completely open to your recommendations and changes. Thank you in advance for your consideration :^) |
Thanks for your contribution! I will look into this in more detail soon. Anyway, the checks are currently failing for a few code formatting issues. You can check the logs of the "PR-Checks" run. Do not worry about the translation check failing that can be fixed when everything else is done :) Edit: Feel free to ask if you need help updating the existing pull request using git. Quite a few people have trouble with that at first. |
Thanks for your feedback! After taking a look at the issue you're talking about and the code it would require to modify, I think it's out of the scope of this MR whose goal is the implementation of new shortcuts. However, I'll be happy to work on it once this MR is closed :^) I'll start solving the check problems right away 👍. Would you prefer me to create a new commit on my branch or amend the one I've already done? |
In the end we want one commit for your cumulative changes and one translation commit. So amending the fixes into your existing commit would be the way to go right now. |
12cf5df
to
41333bc
Compare
I've patched the formatting errors, but I'm not sure what to do in the translation commit you mentioned. Can you explain? |
Mumble translations are community contributions outside of the Mumble GitHub repository. To enable the translations for new strings, you will have to add a "translation commit" using the scripts located in this repo. They will perform all the necessary steps to get the new strings up for translation automagically. You can add the translation commit now, but remember: if someone reviews this MR and tells you there is some more work to do, you will have to squash that changes into your existing commit (amending will not work that easy, because the translation commit will be the last). But if you feel good about your git skills, you can run the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I assume that you have also verified that these new shortcuts work as expected and that the menu controls also still work as expected? If so, then please add the translations commit now (if you prefer I can also add it for you) and then this can be merged.
As mentionned in mumble-voip#6229, this commit implements new global shortcuts linked to the menu actions. The following actions are now available: - [x] Connect to a server - [x] Disconnect from server - [x] Open server information - [x] Open server tokens - [x] Open server user list - [x] Open server ban list - [x] Toggle priority speaker - [x] Open recording dialog - [x] Change comment - [x] Change avatar - [x] Remove avatar - [x] Register on the server - [x] Audio statistics - [x] Open settings - [x] Start audio wizard - [x] Start certificate wizard - [x] Toggle TTS - [x] Open about dialog - [x] Open about Qt dialog - [x] Check for update Implements mumble-voip#6229
41333bc
to
7ffe0a7
Compare
Yes of course, I've tested all the shortcuts I've implemented. I did my best to avoid bugs, but if anyone finds a problem in the future, please let me know so I can help fix it :^) |
Awesome! Thank you very much! 👍 |
As mentionned in #6229, this commit implements new global shortcuts linked to the menu actions. The following actions are now available:
Implements #6229
Checks