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

Improve echo cancellation settings ui #4113

Merged

Conversation

Krzmbrzl
Copy link
Member

@Krzmbrzl Krzmbrzl commented Apr 29, 2020

This PR makes sure that the enablement of the echo cancellation ComboBox in the setting is done correctly (and on-the-fly: No need to press Apply for this to work) and if it is disabled, a tooltip will explain why.

Fixes #4110

Changelog

| Improved: Echo cancellation setting in the UI is now much more intuitive in regards to when and why it is disabled

This new extension adds a name property to each ConfigWidget that in
contrast to the title is not subject to localization.
…tent with what is currently set

Before the system didn't detect if the user had changed the audio output
interface and thus the enablement status of the echo cancellation
ComboBox did not reflect the current combination of set audio
interfaces.
@Krzmbrzl Krzmbrzl force-pushed the improve-echo-cancellation-settingsUI branch from 0b41dd8 to 28945ca Compare April 29, 2020 19:03
@davidebeatrici
Copy link
Member

Why not return the string directly instead of storing it in a variable?

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Apr 30, 2020

Why not return the string directly instead of storing it in a variable?

@davidebeatrici What code part are you talking about? 🤔

@davidebeatrici
Copy link
Member

For example:

const QString ASIOConfig::name = QLatin1String("ASIOConfig");

const QString &ASIOConfig::getName() const {
	return ASIOConfig::name;
}

I would replace it with:

const QString &ASIOConfig::getName() const {
	return QLatin1String("ASIOConfig");
}

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented May 3, 2020

Yeah you're right. I was thinking that I have to take special care here because I want to return a reference but as I'm using returning a const reference, I should be fine returning a string literal directly...

EDIT: Scratch that, it doesn't work. When I try to do that, I get

error: returning reference to temporary [-Werror=return-local-addr]

and if I try to make make the variable static inside the function body I get the same error.

Thus everything failed as I thought it would ☝️

This might be helpful for people that don't know that the combination of
input and output interface plays an important role for this feature.
@Krzmbrzl Krzmbrzl force-pushed the improve-echo-cancellation-settingsUI branch from 28945ca to 6789b05 Compare May 3, 2020 17:51
@Krzmbrzl Krzmbrzl merged commit aca3a44 into mumble-voip:master May 3, 2020
@davidebeatrici
Copy link
Member

Sorry, should've been:

const QString ASIOConfig::getName() const {
	return QLatin1String("ASIOConfig");
}

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented May 3, 2020

Yeah that I didn't do because I don't see the need to copy the names all the time as they are in fact static. Thus I want to allocate them once and then use that instance ☝️

@davidebeatrici
Copy link
Member

How many times is getName() called effectively?

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented May 3, 2020

Probably not often enough for this to make a difference but I still wanted to do it "the best way". And I don't think it clutters the code in the way I did it 🤔

@davidebeatrici
Copy link
Member

The memory usage is minimal anyway.

@Krzmbrzl Krzmbrzl deleted the improve-echo-cancellation-settingsUI branch November 9, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance echo cancellation seetings UI
2 participants