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 popup when mute cue is activated for the first time #6155

Merged
merged 2 commits into from
Jan 1, 2024

Conversation

kbanushi
Copy link
Contributor

@kbanushi kbanushi commented Jun 22, 2023

As requested in #6094 this commit implements a popup to inform the user that the mute cue just played while also giving them the option to keep it enabled or not.

For that a new slot function was added in the MainWindow Class along with a signal function in the AudioInput class. These use a new boolean variable in the Settings Class to check if the popup has been shown and a boolean variable in the Global Class to check if the Configuration UI is open.

The popup works by emitting a signal from the AudioInput class to the MainWindow Class.

Fixes #6094

Checks

@kbanushi
Copy link
Contributor Author

Sorry about the new pull request, I got confused on the squashing and ended up with 6 commits that I could not get to squash. I believe I addressed all of the concerns from the previous code review but the PR still seems to be failing even though I ran clang-format version 10.

@Hartmnt
Copy link
Member

Hartmnt commented Jun 22, 2023

Sorry about the new pull request, I got confused on the squashing and ended up with 6 commits that I could not get to squash. I believe I addressed all of the concerns from the previous code review but the PR still seems to be failing even though I ran clang-format version 10.

For future reference what you would have wanted to do is for example: git rebase HEAD~N --interactive
where N is the number of commits in the history you want to edit. Then choose "squash" instead of "pick" for all commits you want to squash. After that you would have force-pushed your branch for example with git push --force-with-lease.

@Hartmnt
Copy link
Member

Hartmnt commented Jun 22, 2023

This is a continuation of #6153

@Hartmnt Hartmnt added client ui feature-request This issue or PR deals with a new feature labels Jun 22, 2023
@Hartmnt
Copy link
Member

Hartmnt commented Jun 22, 2023

I tested this and it works as advertised. From my perspective the MR looks good. But the keen eyes of @Krzmbrzl might disagree.

The only thing missing is the translation commit. This can be automatically generated by running the update translation script in the Mumble repo
Do you want to try to run the script yourself, @kbanushi ? If not, I could run it for you real quick. (Note: the translation commit is not supposed to be squashed)

@kbanushi
Copy link
Contributor Author

I tested this and it works as advertised. From my perspective the MR looks good. But the keen eyes of @Krzmbrzl might disagree.

The only thing missing is the translation commit. This can be automatically generated by running the update translation script in the Mumble repo Do you want to try to run the script yourself, @kbanushi ? If not, I could run it for you real quick. (Note: the translation commit is not supposed to be squashed)

Perfect, I am happy to make any changes if any suggestions come up too. If you could run the script that would be very helpful. Since rebuilding I am having issues with python in vcpkg.

@Hartmnt Hartmnt requested a review from Krzmbrzl June 22, 2023 20:36
Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

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

Also, when you added the braces, please run clang-format-10 again to make sure everything is correctly formatted.

To add the change to your existing commit you could do it like this: (assuming you did not change your branch in the meantime)

  • git fetch
  • git pull
  • Commit the small changes in a new commit
  • git rebase HEAD~3 --interactive
  • This opens a text editor. Move the line with your new commit under the line with the first commit. Change the action of the new commit to squash
  • Save and exit the text editor
  • Change the commit message to only include your original text and not the new commit message from you fix commit
  • git push <your remote name e.g. origin> <your branch name e.g. master> --force-with-lease

src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
@Hartmnt
Copy link
Member

Hartmnt commented Jun 30, 2023

Well, you almost got it right :D Now you squashed the bracket commit into the translation commit.

@Hartmnt
Copy link
Member

Hartmnt commented Jun 30, 2023

What I meant by saying "Move the line with your new commit under the line with the first commit" was this:

pick FEAT(client): Add popup...
squash Fix
pick TRANSLATION

what you did was

pick FEAT(client): Add popup...
pick TRANSLATION
squash Fix

Do you want me to fix this for you real quick, or do you want to give it a go yourself?

@kbanushi
Copy link
Contributor Author

What I meant by saying "Move the line with your new commit under the line with the first commit" was this:

pick FEAT(client): Add popup...
squash Fix
pick TRANSLATION

what you did was

pick FEAT(client): Add popup...
pick TRANSLATION
squash Fix

Do you want me to fix this for you real quick, or do you want to give it a go yourself?

Haha yeah I got a little confused with that. I also forgot to run the clang format before which might make things more complicated too. Sorry about that, I will see if I can fix it quickly.

@kbanushi
Copy link
Contributor Author

To undo these changes would I use git reset with the commit that I overwrote (7a7e9b3)? I am hoping this would undo the changes I introduced in the translation commit. Thanks for the help!

@Hartmnt
Copy link
Member

Hartmnt commented Jun 30, 2023

To undo these changes would I use git reset with the commit that I overwrote (7a7e9b3)? I am hoping this would undo the changes I introduced in the translation commit. Thanks for the help!

I guess the easiest way is to reset the translation commit entirely and rerun the .py script.
So: reset the translation commit such that your branch is on a120e82 without any additional unstaged/changed files. Then add the brackets to a120e82 either via git commit --amend or the rebase interactive thingy from above. Then re-run the .py script. If you want, I can run the .py script again.

@Hartmnt
Copy link
Member

Hartmnt commented Sep 11, 2023

@kbanushi Would you like me to rebase your changes and restore the correct order of your commits :) ?

@kbanushi
Copy link
Contributor Author

My apologies! I completely forgot about the commit issues. If you could do the rebasing that would be great, I remember getting confused and messing up my commits again. If not, I am willing to give it another go!

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.

Sorry for having taken this long to have a look at this 🙏

src/mumble/SettingsKeys.h Outdated Show resolved Hide resolved
src/mumble/MainWindow.cpp Show resolved Hide resolved
kbanushi and others added 2 commits January 1, 2024 19:12
As requested in mumble-voip#6094 this commit implements a popup to inform the user
that the mute cue just played while also giving them the option to keep
it enabled or not.

For that a new slot function was added in the MainWindow Class along
with a signal function in the AudioInput class.  These use a new boolean
variable in the Settings Class to check if the popup has been shown and
a boolean variable in the Global Class to check if the Configuration UI
is open.

The popup works by emitting a signal from the AudioInput class to the
MainWindow Class.

Implements mumble-voip#6094
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Jan 1, 2024

I took the liberty of fixing the merge conflicts and incorporating the little nitpick of my late review.

@Krzmbrzl Krzmbrzl merged commit bc03c56 into mumble-voip:master Jan 1, 2024
15 checks passed
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Jan 1, 2024

Thank you for working on this 👍

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 dialog to disable mute cue when it is first activated
3 participants