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): Implemented new Global Shortcuts #6260

Merged
merged 2 commits into from
Dec 31, 2023

Conversation

Globostofo
Copy link
Contributor

@Globostofo Globostofo commented Nov 4, 2023

As mentionned in #6229, this commit implements new global shortcuts linked to the menu actions. The following actions are now available:

  • Connect to a server
  • Disconnect from server
  • Open server information
  • Open server tokens
  • Open server user list
  • Open server ban list
  • Toggle priority speaker
  • Open recording dialog
  • Change comment
  • Change avatar
  • Remove avatar
  • Register on the server
  • Audio statistics
  • Open settings
  • Start audio wizard
  • Start certificate wizard
  • Toggle TTS
  • Open about dialog
  • Open about Qt dialog
  • Check for update

Implements #6229

Checks

@Globostofo
Copy link
Contributor Author

I'm completely open to your recommendations and changes.
I'm not a native English speaker, so don't hesitate to correct me if you see any mistakes in my code (strings or variable/method names).

Thank you in advance for your consideration :^)

@Hartmnt
Copy link
Member

Hartmnt commented Nov 4, 2023

Thanks for your contribution! I will look into this in more detail soon.
Just skimming over the new actions, would you also be able to implement #3932 ? That is actually a feature request we received multiple times. But it would require you to add user data similar to the whisper/shout shortcut. Maybe that is out of the scope of this MR 🤔

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.

@Globostofo
Copy link
Contributor Author

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?

@Hartmnt
Copy link
Member

Hartmnt commented Nov 5, 2023

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.

@Globostofo Globostofo force-pushed the 6229-add-new-global-shortcuts branch from 12cf5df to 41333bc Compare November 5, 2023 13:35
@Globostofo
Copy link
Contributor Author

I've patched the formatting errors, but I'm not sure what to do in the translation commit you mentioned. Can you explain?

@Hartmnt
Copy link
Member

Hartmnt commented Nov 5, 2023

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 updatetranslations.py script in the scripts folder from the repository root now. Alternatively, just wait for the reviews and if there are no more complaints you can add the translation commit last.

Copy link
Member

@Krzmbrzl Krzmbrzl left a 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.

@Krzmbrzl Krzmbrzl added client ui feature-request This issue or PR deals with a new feature labels Dec 29, 2023
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
@Globostofo Globostofo force-pushed the 6229-add-new-global-shortcuts branch from 41333bc to 7ffe0a7 Compare December 30, 2023 11:18
@Globostofo
Copy link
Contributor Author

Globostofo commented Dec 30, 2023

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 :^)

@Krzmbrzl Krzmbrzl linked an issue Dec 31, 2023 that may be closed by this pull request
@Krzmbrzl Krzmbrzl merged commit ead6fc9 into mumble-voip:master Dec 31, 2023
15 checks passed
@Krzmbrzl
Copy link
Member

Awesome! Thank you very much! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make more functions available in the list of global shortcuts
3 participants