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 warning when using positonal audio on mono output #5247

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

kartikkhullar
Copy link
Contributor

@kartikkhullar kartikkhullar commented Aug 28, 2021

As requested in #5238 this commit adds a warning message in the chat when positional audio is enabled on mono output device.

Implements #5238

Checks

@kartikkhullar kartikkhullar marked this pull request as draft August 28, 2021 02:13
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.

You should run clang-format on the source tree in order to bring your code changes into the same format as the rest of the codebase.

After you have integrated the formatting changes into your commit (amend the changes), you should also run scripts/updatetranslations.py in order to bring the translation files up to date.

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved
@Krzmbrzl Krzmbrzl added the feature-request This issue or PR deals with a new feature label Aug 29, 2021
@Krzmbrzl Krzmbrzl linked an issue Aug 29, 2021 that may be closed by this pull request
@kartikkhullar
Copy link
Contributor Author

I have added the suggested changes.

@kartikkhullar kartikkhullar marked this pull request as ready for review August 30, 2021 18:03
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Aug 31, 2021

Why are there 2 translation commits?

Also please squash the code-review changes into your original commit. And it seems that you did not run clang-format as requested 👀

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Sep 9, 2021

@kartikkhullar what's the status on this?

@kartikkhullar
Copy link
Contributor Author

I don't have access to my computer from last few days, most probably will get back to you by Monday.

@kartikkhullar
Copy link
Contributor Author

Why are there 2 translation commits?

Also please squash the code-review changes into your original commit. And it seems that you did not run clang-format as requested 👀

I have squashed all the changes into original commit. Also, I did ran clang-format as requested. I tried running it again but it says "no modified files to format". So I guess it did ran successfully previously.

@Krzmbrzl
Copy link
Member

I have squashed all the changes into original commit.

That is not what I wanted. We need to have all code changes in one commit and then all translation changes in another, separate commit.

And also there should be no merge commit in your branch.

I recommend the following steps:

  • Drop all commits that are unrelated to your code changes (the merge and translation commits)
  • squash your code commits into a single one (this commit must then include all your code changes but nothing else)
  • run ./scripts/updatetranslations.py
  • force push to your remote branch on GitHub in order to update this PR

@Krzmbrzl Krzmbrzl force-pushed the positonalaudiowarning branch from 67bcebe to 19bcdbe Compare September 8, 2022 18:33
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.

I took the liberty of tidying your commit history for you

@Krzmbrzl Krzmbrzl force-pushed the positonalaudiowarning branch 2 times, most recently from df9208a to 2ab5818 Compare September 8, 2022 18:43
kartikkhullar and others added 2 commits September 9, 2022 10:04
As requested in mumble-voip#5238 this commit adds a warning message in the chat
when positional audio is enabled on mono output device.

Fixes mumble-voip#5238
@Krzmbrzl Krzmbrzl force-pushed the positonalaudiowarning branch from 2ab5818 to 70082df Compare September 9, 2022 08:04
@Krzmbrzl Krzmbrzl merged commit 264eac3 into mumble-voip:master Sep 9, 2022
Hartmnt added a commit to Hartmnt/mumble that referenced this pull request Jul 15, 2024
Merge request mumble-voip#5247 introduced a warning that is triggered
when Mumble is configured to use positional audio, if
only a single channel output is configured.

However, this warning is emitted from the audio thread and
therefore causes a crash. This is because Qt does not want to
add child QObjects from any other thread than main.

This commit makes sure the static version of ``logOrDefer`` is
called and also adds a check to ``Log::log`` to forward the call
to the main thread, if necessary.

Fixes mumble-voip#6507
Hartmnt added a commit to Hartmnt/mumble that referenced this pull request Jul 16, 2024
Merge request mumble-voip#5247 introduced a warning that is triggered
when Mumble is configured to use positional audio and
only a single channel output is configured.

However, this warning is emitted from the audio thread and
therefore causes a crash. This is because Qt does not want to
add child QObjects from any other thread than main.

This commit makes sure the static version of ``logOrDefer`` is
called and also adds a check to ``Log::log`` to forward the call
to the main thread, if necessary.

Fixes mumble-voip#6507
davidebeatrici pushed a commit to Hartmnt/mumble that referenced this pull request Aug 20, 2024
Merge request mumble-voip#5247 introduced a warning that is triggered
when Mumble is configured to use positional audio and
only a single channel output is configured.

However, this warning is emitted from the audio thread and
therefore causes a crash. This is because Qt does not want to
add child QObjects from any other thread than main.

This commit makes sure the static version of ``logOrDefer`` is
called and also adds a check to ``Log::log`` to forward the call
to the main thread, if necessary.

Fixes mumble-voip#6507
Krzmbrzl pushed a commit that referenced this pull request Aug 20, 2024
Merge request #5247 introduced a warning that is triggered
when Mumble is configured to use positional audio and
only a single channel output is configured.

However, this warning is emitted from the audio thread and
therefore causes a crash. This is because Qt does not want to
add child QObjects from any other thread than main.

This commit makes sure the static version of ``logOrDefer`` is
called and also adds a check to ``Log::log`` to forward the call
to the main thread, if necessary.

Fixes #6507

(cherry picked from commit 6374856)
Hartmnt added a commit that referenced this pull request Sep 16, 2024
Merge request #5247 introduced a warning that is triggered
when Mumble is configured to use positional audio and
only a single channel output is configured.

However, this warning is emitted from the audio thread and
therefore causes a crash. This is because Qt does not want to
add child QObjects from any other thread than main.

This commit makes sure the static version of ``logOrDefer`` is
called and also adds a check to ``Log::log`` to forward the call
to the main thread, if necessary.

Fixes #6507

(cherry picked from commit 6374856)
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 positional audio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn when using positional audio on mono output
3 participants