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

Effect parameter: briefly show value in parameter name widget #11032

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 2, 2022

Use the parameter name label to show the parameter value for .8 seconds after it changed.
Rounded to two decimals fo now. Maybe rounding can be made dependent on the actual range.
image

closes #9022

@github-actions github-actions bot added the ui label Nov 2, 2022
@daschuer
Copy link
Member

daschuer commented Nov 2, 2022

Nice idea.

@@ -9,12 +9,17 @@

namespace {
const QString kMimeTextDelimiter = QStringLiteral("\n");
// for rounding the value display to N decimals
const int kValDecimals = pow(10, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Can this become a constexpr? If not use the result as constexpr to avoid runtime overload.

Copy link
Member Author

Choose a reason for hiding this comment

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

meeh, pow() is not constexpr.
Instead of elaborate workarounds like

constexpr double kValDecimals() {
    return pow(10, 2);
}

I'll go with constexpr int kValDecimals = 100;

} // anonymous namespace

WEffectParameterNameBase::WEffectParameterNameBase(
QWidget* pParent, EffectsManager* pEffectsManager)
: WLabel(pParent), m_pEffectsManager(pEffectsManager) {
setAcceptDrops(true);
m_valDisplayTimer.setSingleShot(true);
m_valDisplayTimer.setInterval(800);
m_valDisplayTimer.callOnTimeout(this, &WEffectParameterNameBase::parameterUpdated);
Copy link
Member

Choose a reason for hiding this comment

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

I think it is required to split parameterUpdated() to
parameterSlotUpdated() an showParameterName() or such.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the connect part needs to be moved to setEffectParameterSlot() ... ?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, I totally overlooked that when reviving this old branch.
callOnTimeout(this, [this]() {setText(m_text);}) is sufficient.

src/widget/weffectparameternamebase.cpp Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Nov 2, 2022

Additionally it would be nice to show the actual snapped value for quantized parameters like Echo time and Tremolo rate, but that's rather elaborate...
Actually, it would be much better UX if the knobs would snap.
#8756

@ronso0 ronso0 force-pushed the effect-parameter-change-label branch 2 times, most recently from 0b9276e to 53cb324 Compare November 2, 2022 11:21
@ronso0 ronso0 force-pushed the effect-parameter-change-label branch from 53cb324 to d360922 Compare November 2, 2022 11:44
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. It works good.

The only issue is that the number allow looks naked and gives in many cases not more info than the knob position on on its own. I think to unleash the power of this feature we need to make use of the units feature. We have already an unused unitsHint() function we may use and adjust.

I will file an issue and merge this now.

@daschuer daschuer merged commit febc702 into mixxxdj:main Nov 2, 2022
@daschuer
Copy link
Member

daschuer commented Nov 2, 2022

Here is the issue #11033

@ronso0 ronso0 deleted the effect-parameter-change-label branch November 2, 2022 21:07
@daschuer
Copy link
Member

daschuer commented Nov 2, 2022

Ups, the duplicate connection issue was still there. Here is a fix: #11034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show ControlPotmeter and EffectParameter values in skins
2 participants