FEAT(client): Toggle positional audio shortcut #6133

[GrossTrevor]

Constructed framework of a new shortcut to toggle positional audio, but do not know what functions to use or where to go from here. This is a work in progress that I would need some help with. Forgive me, I'm afraid this is my first open-source commit like this. This is concerning issue #6133 to which I am assigned.

• Added new Global Shortcut type in GlobalShortcutTypes.h
• Remade new assertion in same file
• Initialized shortcut as a new GlobalShortcut in MainWindow.cpp
• Created a new function that corresponds to the new shortcut (no real code in there yet, this is where I need help)
• Associated definitions in MainWindow.h

This is for a school assignment due Friday, February 9th, so please provide some guidance as soon as possible if you can. Thanks!

[Hartmnt]

Otherwise, your merge request looks good so far. In the end you will need to add a translation commit. But first you just need to worry about implementing the logic :)

[GrossTrevor]

I added another commit that mimics the function on_gsAudioTTS_triggered(bool down, QVariant) as suggested. Some of the checks are still failing and, as a novice, I really have no idea what that means for my commit. Are there errors in my addition that need to be fixed?

[Hartmnt]

I added another commit that mimics the function on_gsAudioTTS_triggered(bool down, QVariant) as suggested. Some of the checks are still failing and, as a novice, I really have no idea what that means for my commit. Are there errors in my addition that need to be fixed?

Please squash your two existing commits into one. While you are at it you can also include a Fixes #6133 in the commit message.
And then add a translation commit using the Python script updatetranslations.py in our repo. That script will automatically add a translation commit to your branch without further user interaction. After that you can update this merge request by force pushing your branch git push origin positional-audio-shortcut --force-with-lease. The force-push is necessary, because of the squashed commits.

[GrossTrevor]

I added another commit that mimics the function on_gsAudioTTS_triggered(bool down, QVariant) as suggested. Some of the checks are still failing and, as a novice, I really have no idea what that means for my commit. Are there errors in my addition that need to be fixed?

Please squash your two existing commits into one. While you are at it you can also include a Fixes #6133 in the commit message. And then add a translation commit using the Python script updatetranslations.py in our repo. That script will automatically add a translation commit to your branch without further user interaction. After that you can update this merge request by force pushing your branch git push origin positional-audio-shortcut --force-with-lease. The force-push is necessary, because of the squashed commits.

I've been messing with this for a while now and I cannot get the script to run because it cannot find Lupdate. I have QT for open source on Windows but I have never used the program before. I already squished the commits. Is there a way I can perform the function of updatetranslations.py manually?

[Hartmnt]

I've been messing with this for a while now and I cannot get the script to run because it cannot find Lupdate. I have QT for open source on Windows but I have never used the program before. I already squished the commits. Is there a way I can perform the function of updatetranslations.py manually?

Not sure what the issue is, but I certainly have no idea how to get this to work on windows. No problem though, I will gladly add the translation commit for you. You just need to update your branch to include the single squashed commit :)

[Hartmnt]

One line of your code change was not following the clang-10 format:

diff --git a/src/mumble/MainWindow.cpp b/src/mumble/MainWindow.cpp
index 9c8d8479c..dae803de1 100644
--- a/src/mumble/MainWindow.cpp
+++ b/src/mumble/MainWindow.cpp
@@ -412,8 +412,8 @@ void MainWindow::createActions() {
        gsHelpVersionCheck->setObjectName(QLatin1String("gsHelpVersionCheck"));
        gsHelpVersionCheck->qsWhatsThis = tr("This will check if mumble is up to date");
 
-       gsTogglePositionalAudio =
-               new GlobalShortcut(this, GlobalShortcutType::TogglePositionalAudio, tr("Toggle positional audio", "Global Shortcut"));
+       gsTogglePositionalAudio = new GlobalShortcut(this, GlobalShortcutType::TogglePositionalAudio,
+                                                                                                tr("Toggle positional audio", "Global Shortcut"));
        gsTogglePositionalAudio->setObjectName(QLatin1String("gsTogglePositionalAudio"));
        gsTogglePositionalAudio->qsWhatsThis = tr("This will turn on/off positional audio");

For the test to pass, that needs to be changed as well. I pushed the translation commit already. Is your git-fu strong enough to edit the code commit without touching the translation commit 😅 ?
If not, just do the change and force push the branch without my translation commit. I can always re-add it.

[GrossTrevor]

One line of your code change was not following the clang-10 format:

diff --git a/src/mumble/MainWindow.cpp b/src/mumble/MainWindow.cpp
index 9c8d8479c..dae803de1 100644
--- a/src/mumble/MainWindow.cpp
+++ b/src/mumble/MainWindow.cpp
@@ -412,8 +412,8 @@ void MainWindow::createActions() {
        gsHelpVersionCheck->setObjectName(QLatin1String("gsHelpVersionCheck"));
        gsHelpVersionCheck->qsWhatsThis = tr("This will check if mumble is up to date");
 
-       gsTogglePositionalAudio =
-               new GlobalShortcut(this, GlobalShortcutType::TogglePositionalAudio, tr("Toggle positional audio", "Global Shortcut"));
+       gsTogglePositionalAudio = new GlobalShortcut(this, GlobalShortcutType::TogglePositionalAudio,
+                                                                                                tr("Toggle positional audio", "Global Shortcut"));
        gsTogglePositionalAudio->setObjectName(QLatin1String("gsTogglePositionalAudio"));
        gsTogglePositionalAudio->qsWhatsThis = tr("This will turn on/off positional audio");

For the test to pass, that needs to be changed as well. I pushed the translation commit already. Is your git-fu strong enough to edit the code commit without touching the translation commit 😅 ? If not, just do the change and force push the branch without my translation commit. I can always re-add it.

Huh that is weird. I copied and pasted the above shortcut initializations to ensure my formatting was the exact same, but I guess that did not work. It also looks correct on my machine, but who knows. Are the lines with pluses what I should change it to? Also, I can guarantee you that I will find a way to mess up your translation commit, but I will try lol.

Here is a picture of what I see:

[Hartmnt]

Huh that is weird. I copied and pasted the above shortcut initializations to ensure my formatting was the exact same, but I guess that did not work. It also looks correct on my machine, but who knows. Are the lines with pluses what I should change it to? Also, I can guarantee you that I will find a way to mess up your translation commit, but I will try lol.

The snippet above is a diff. Your lines (the ones with the -) are to be removed and the newly formatted ones (with the +) are to be added. Your code is functional as is, but we require it to be formatted according to the clang-10 specification.

For this line in particular that just means it is not happy with the line length. Since the name of the variable changed, it does not want it to be formatted as the shortcut above. There is a tool to automate this (clang-format), but I have no idea, if and how to use that on Windows ;) For this single line it is probably easier to just change it by hand.

If you follow the link of the failing test here on GitHub https://github.com/mumble-voip/mumble/actions/runs/7775343666/job/21201217927?pr=6326 you can also see where the test complains about the line.

[GrossTrevor]

Huh that is weird. I copied and pasted the above shortcut initializations to ensure my formatting was the exact same, but I guess that did not work. It also looks correct on my machine, but who knows. Are the lines with pluses what I should change it to? Also, I can guarantee you that I will find a way to mess up your translation commit, but I will try lol.

The snippet above is a diff. Your lines (the ones with the -) are to be removed and the newly formatted ones (with the +) are to be added. Your code is functional as is, but we require it to be formatted according to the clang-10 specification.

For this line in particular that just means it is not happy with the line length. Since the name of the variable changed, it does not want it to be formatted as the shortcut above. There is a tool to automate this (clang-format), but I have no idea, if and how to use that on Windows ;) For this single line it is probably easier to just change it by hand.

If you follow the link of the failing test here on GitHub https://github.com/mumble-voip/mumble/actions/runs/7775343666/job/21201217927?pr=6326 you can also see where the test complains about the line.

Just pushed it again. This is what the code looks like on my machine (hopefully it is correct):

[Hartmnt]

Just pushed it again. This is what the code looks like on my machine (hopefully it is correct):

Sometimes a project agrees to ditch subjective code beauty for pragmatic auto-formatting :) Mumble is such a project

[Hartmnt]

Compiled and tested*, LGTM

• The positional audio setting is correctly toggled. I was not able to test it with the manual link plugin in a real world scenario, because that is currently broken on master

[Krzmbrzl]

Suggested change

	gsTogglePositionalAudio->qsWhatsThis = tr("This will turn on/off positional audio");
	gsTogglePositionalAudio->qsWhatsThis = tr("This will toggle positional audio on/off");

[Krzmbrzl]

Why do you take the boolean parameter by reference? This seems unnecessary as the size of a boolean is quite a bit smaller than the size of a pointer (which is what a reference really is, under the hood) and therefore passing the boolean by value should be more efficient.
Premature optimizations aside, I think it would also be clearer to pass by value as passing a boolean by reference introduces the semantics that it shall act as some kind of out-parameter (though that is of course not possible here due to the const qualification).

So in short: Unless you have good reason to do so, I would prefer if we passed the boolean by value instead of by reference.

[Krzmbrzl]

This does not what the name suggests. If it is supposed to toggle the positional audio state, this function shouldn't take a parameter for the new state as the new state is simply the inverted old state (e.g. state = !state). If in fact you want to be able to set the state of positional audio, the function should be called setPositionalAudio or even better enablePositionalAudio.

[Krzmbrzl]

I just saw that the odd naming of toggle-functions is also present in existing functions. Guess that slipped under my radar. Nonetheless, we should strive to have new code match high standards :)

[Hartmnt]

@GrossTrevor Hey, I know it is past your assignment due date, but would you be able to apply the requested changes? We can do it for you, if you do not have the time or motivation for it, but your commits might lose some authorship information. Maybe you need the proof that your commit was actually merged or something 😅

Anyway, the other existing methods have already been refactored on master ( 9b9d88c ) so you can see exactly how it is now supposed to look :)