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): Adds a new shortcut which hides/shows the main window #4562

Merged
merged 2 commits into from Nov 3, 2020

Conversation

merua
Copy link
Contributor

@merua merua commented Oct 31, 2020

Implements #2328

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.

I would also remove the log messages.

src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
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.

Furthermore you have to update the translations to also include your new string.

In order to do so, please run scripts/updatetranslations.sh ☝️

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.

Thanks for the changes!

Now I'd only ask you to squash your first 2 commits (but keep the translation one separate) into a single one (the commit history of how the review has influenced the code is nothing that has to go into master).

After that I think we're all set 👍

if(down) {
if(g.mw->isVisible()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(down) {
if(g.mw->isVisible()) {
if (down) {
if (g.mw->isVisible()) {

Scanning directory './src'...
Scanning directory './src/mumble'...
Updating 'src/mumble/mumble_en.ts'...
    Found 1910 source text(s) (1 new and 1909 already existing)
@davidebeatrici davidebeatrici merged commit 1309874 into mumble-voip:master Nov 3, 2020
@davidebeatrici
Copy link
Member

Thank you very much for your contribution!

@merua
Copy link
Contributor Author

merua commented Nov 3, 2020

Thanks to you and all the other contributors.

I have started using Mumble since the lockdowns and it has really helped staying in touch with my friends, so keep up the good work! (Especially since I found out -- to my horror -- that a lot of problem tickets are written in a very toxic tone.)

And next time I will try to set up the formatter. Sorry for that...

@davidebeatrici
Copy link
Member

Thanks!

No worries about the formatting, we should probably add a CI task that runs clang-format and then shows the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants