-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
CO Renaming (Pt. 3): Add ControlPotMeter
aliasing and move audio_latency_*
COs
#11998
Conversation
I couldn't find any use of the additional |
4cf33cf
to
206f477
Compare
src/control/controlpotmeter.cpp
Outdated
m_controlMinusToggle.addAlias(ConfigKey( | ||
key.group, QString(key.item) + QStringLiteral("_minus_toggle"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is duplicated across the constructor, I'm worried that we might forget to update both places in case we add another suffix, would this be possible to deduplicate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know how to do that in an elegant way. I pushed a little helper function to make the ConfigKey creation less verbose though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I came up with this: https://compiler-explorer.com/z/xxE83Gf8r
I find it very elegant, just don't know how easy it is to understand.
Frankly, now that I've written it, I don't think we should use it here. Its very seldom that we would add another suffix for ControlPotMeter
and the complexity doesn't warrant it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next best thing would just be a comment in both parts reminding the reader of the other part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
206f477
to
e5fe0f3
Compare
Great! So we don't need Potmeter aliasing at all? If yes, why keep that commit? |
There are other COs that might need it. We didn't rename all of them. And I thinks it's good if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm otherwise, thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you.
Merge? |
Thanks. |
This PR is part of the ongoing effort to remove offensive language from the Mixxx codebase (#11931).
I had to add proper support for adding aliases to
ControlPotMeter
due toaudio_latency_usage
andaudio_latency_overload
. However, I'm wondering why these controls areControlPotMeter
objects in the first place. These should be read-only IIRC, so there is no need for all these setter-COs.[Master],audio_latency_usage
[App],audio_latency_usage
[Master],audio_latency_overload
[App],audio_latency_overload
[Master],audio_latency_overload_count
[App],audio_latency_overload_count