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): Add option to toggle TTS in settings menu #5565

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

BombJovi
Copy link
Contributor

@BombJovi BombJovi commented Feb 11, 2022

Checks

Added option to enable/disable TTS from within settings menu.

Closes issue #5561

src/mumble/Log.ui Outdated Show resolved Hide resolved
src/mumble/Log.cpp Outdated Show resolved Hide resolved
src/mumble/Log.cpp Outdated Show resolved Hide resolved
Copy link
Member

@davidebeatrici davidebeatrici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only cosmetic stuff, the code itself looks good to me!

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.

As already written on IRC: The commits need to be adjusted to match our commit guidelines and the code needs to be formatted using clang-format 10. If you don't have that, I can do the formatting for you.

Furthermore, you'll have to run the script scripts/updatetranslations.py to update the translation source strings so that they can be properly translated on our external platform (Weblate).

Note that in the end you should end up with only 2 commits:

  1. The commit that makes the changes to add the new checkbox in the settings (that should be a plain FEAT(client): ... commit as it adds a feature, but doesn't change an existing one (the old checkbox for TTS in the dropwdown continues to exist) what would be implied by FEAT/CHANGE
  2. The commit created by the translation script. No changes necessary for that one (the scripts will commit it in the proper format already)

That means that after you have taken care of the formatting, you squash all your commits into a single one, reword the commit message for that to match our guideline and then you can run the script for the translations. After that you perform a git pull -f to update your remote branch and with that, this PR :)

@BombJovi BombJovi changed the title FEAT/CHANGE(client) Adds the ability to enable/disable Text-to-Speech in settings menu FEAT(client) Adds the ability to enable/disable Text-to-Speech in settings menu Feb 12, 2022
@Krzmbrzl
Copy link
Member

Alright - we still have to sort out the commit history. As you can see, this PR now consists of 6 instead of 2 commits. In order to change the commit history, you'll have to perform a rebase.
For that, open a terminal at the repository root (while the correct branch is checked out) and execute git rebase -i 89f65c152542f5428a68cb019da216eaab2119e8~. That cryptic number is the commit hash of the first commit contained in this PR.
This should drop you into a text editor that contains a line for each of your 6 commits. They are all listed as pick followed by the respective commit hash and the commit title.

The first thing to do, would be to replace pick with drop for the merge commit. We don't want that one, so we tell git to drop it.
Then, we bring the other commits into proper order. In order to reorder commits, just copy&paste the lines into a different order. We want the TRANSLATION commit to be last and keep the other commits in their respective order. So essentially just copy&paste the TRANSLATION line and insert it at the end of the commit list (whether you put it before or after the dropped merge commit shouldn't matter).

For the remaining 4 commits, ignore the topmost one and for the others, replace pick with squash. That will cause git to squash all of these 4 commits together into a single one.
Once you have performed these changes, save and exit your text editor. Git will now start working and for the squashed commit, it will ask you to edit the commit message. Delete everything that is pre-filled and replace it with

FEAT(client): Add option to toggle TTS in settings menu

Previously TTS could only be toggled from the Configure dropdown menu, but not
from within the settings. This was confusing, especially since other TTS related settings
were editable from within the settings UI.

This commit introduces a new checkbox into the settings UI that allows users to toggle
TTS right then and there, which should take care of the created confusion.

Fixes #5561

Save and exit.

After this, git should tell you, that the rebase has been performed successfully.

The last step to do is now to update your remote branch (and with that this PR). Since you have performed a rebase, you will have to use a force-push via git push -f.

If you want to update your branch to the latest and greatest changes from our upstream master branch, you can rebase your changes on top of that, before pushing to your remote branch, via git pull --rebase https://github.com/mumble-voip/mumble.git master.

@BombJovi
Copy link
Contributor Author

BombJovi commented Feb 15, 2022

Is the code I added Clang-10 compliant now?

Also - big thank you to Krzmbrzl, davidebeatrici and Terry137 from the Matrix channel for guiding me through this process! I won't lie, git is still pretty confusing to me, I hope to understand it more in time. Thank you and let me know if there is anything else I need to change!

BombJovi and others added 2 commits February 15, 2022 08:38
Previously TTS could only be toggled from the Configure dropdown menu,
but not from within the settings. This was confusing, especially since
other TTS related settings were editable from within the settings UI.

This commit introduces a new checkbox into the settings UI that allows
users to toggle TTS right then and there, which should take care of the
created confusion.

Fixes mumble-voip#5561
@Krzmbrzl Krzmbrzl added client feature-request This issue or PR deals with a new feature ui labels Feb 15, 2022
@Krzmbrzl Krzmbrzl linked an issue Mar 16, 2022 that may be closed by this pull request
@Krzmbrzl Krzmbrzl changed the title FEAT(client) Adds the ability to enable/disable Text-to-Speech in settings menu FEAT(client): Add option to toggle TTS in settings menu Mar 16, 2022
@Krzmbrzl Krzmbrzl merged commit 2dab6a2 into mumble-voip:master Mar 16, 2022
@Krzmbrzl
Copy link
Member

Kinda forgot about this PR... Sorry for that. Thanks for working on this, though 👍

@BombJovi
Copy link
Contributor Author

BombJovi commented Mar 16, 2022

For sure! Thank you for code reviewing and assisting me with this merge request!

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.

Add option to enable TTS to settings page
3 participants