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

FIX(client): Registered AudioOutputBuffer as qRegisterMetaType #6571

Merged
merged 1 commit into from
Sep 29, 2024

Conversation

jaggzh
Copy link
Contributor

@jaggzh jaggzh commented Sep 27, 2024

Checks

FIX(client): Registered the AudioOutputBuffer as qRegisterMetaType

Just a QT registration missing for the AudioOutputBuffer. Being only a single required registration I didn't make a separate function for it -- it's just in the constructor.

Fixes: #6569

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.

If you look at other occurrences of qRegisterMetaType https://github.com/search?q=repo%3Amumble-voip%2Fmumble%20qRegisterMetaType&type=code you can see that those mostly happen in the classes where the connections are being used. So I guess it would make sense to move your call of qRegisterMetaType in the constructor of ``AudioOutput.cpp` or something

Also please make sure your commit message follows our commit guidelines, otherwise the CI will not pass. The ChatGPT comment for the single line code change is also probably not needed.

@jaggzh jaggzh requested a review from Hartmnt September 28, 2024 10:50
@jaggzh jaggzh changed the title Registered the AudioOutputBuffer as qRegisterMetaType FIX(client): Registered AudioOutputBuffer as qRegisterMetaType Sep 28, 2024
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.

I assume you have checked if this still fixes the issue, right? Because you added the line to the AudioOutputRegistrar constructor instead of the AudioOutput constructor 😄
If both works functionally, I am more leaning towards putting it in the AudioOutput constructor.

The commit message you wrote in the description here on GitHub is fine, but you need to use it as your actual commit message. For that you will need to squash your commits and reword the message. See this for more information: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

@Hartmnt
Copy link
Member

Hartmnt commented Sep 28, 2024

In addition to that you did not remove the changes you made to main.cpp. So please make sure your branch only contains relevant changes.

@jaggzh jaggzh force-pushed the qt-register-audiooutputbuffer branch 4 times, most recently from 7d3e88e to 6027ac1 Compare September 28, 2024 14:28
@@ -6,6 +6,7 @@
#ifdef USE_OVERLAY
# include "Overlay.h"
#endif

Copy link
Member

@Krzmbrzl Krzmbrzl Sep 28, 2024

Choose a reason for hiding this comment

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

Please also removr this changed line in your commit

@Hartmnt
Copy link
Member

Hartmnt commented Sep 28, 2024

@Krzmbrzl Would you put this in the constructor of AudioOutputRegistrar or AudioOutput?

@Krzmbrzl
Copy link
Member

I guess technically putting it into the AudioOutput constructor makes this execute multiple times (once for every created audio output Backend). Bot sure right now whether that would also be the case with the registrar.

On the other hand, this does no harm and having the call as close as possible to where it is needed makes semantic sense, I guess.

In the end I don't have a real preference 🤔

@Hartmnt Hartmnt added client audio bug A bug (error) in the software labels Sep 28, 2024
@jaggzh jaggzh force-pushed the qt-register-audiooutputbuffer branch from 6027ac1 to 0213df5 Compare September 28, 2024 18:37
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.

Compiled and tested. Seems to fix the bug.

@Hartmnt Hartmnt merged commit 8f3d859 into mumble-voip:master Sep 29, 2024
15 checks passed
@Hartmnt
Copy link
Member

Hartmnt commented Sep 29, 2024

@jaggzh Thank you very much for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio bug A bug (error) in the software client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push to talk button works only once.
3 participants