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

Move connection to setEffectParameterSlot() to avoid double connection #11034

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Nov 2, 2022

This is follow up to #11032.
Unfortunately I realized this issue not before merge.

@@ -35,6 +35,14 @@ void WEffectParameterNameBase::setEffectParameterSlot(
&EffectParameterSlotBase::updated,
this,
&WEffectParameterNameBase::parameterUpdated);
if (qobject_cast<EffectKnobParameterSlot*>(m_pParameterSlot.data())) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this preferred over
if (m_pParameterSlot->parameterType() == EffectParameterType::Knob)
and use data() twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

data() has no cost because it is an inline function just returning the pointer.
https://github.com/qt/qtbase/blob/067b53864112c084587fa9a507eb4bde3d50a6e1/src/corelib/tools/qsharedpointer_impl.h#L271

I have decided to use the qobject_cast, because it makes it obvious that the result will not change during the lifetime of
m_pParameterSlot and the connection in this place is sufficient. Does this work for you?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I was just curious.

@ronso0
Copy link
Member

ronso0 commented Nov 2, 2022

Btw for both the previous and this connection the value is shown for Meta-linked parameters when the effect is changed (Meta-knob update I guess). Dunno if users will find this confusing. Probably not if we stick to .8 seconds.

@ronso0
Copy link
Member

ronso0 commented Nov 2, 2022

I'll take a closer look tomorrow to understand when exactly those slots are called.

@ronso0
Copy link
Member

ronso0 commented Nov 3, 2022

okay, got it:

  • updated is emitted when an effect is un/loaded and when parameters are dragged
  • setEffectParameterSlot is only called during setup

LGTM, thank you!

@ronso0 ronso0 merged commit e9ecc28 into mixxxdj:main Nov 3, 2022
@daschuer daschuer deleted the effect-parameter-change-label branch November 16, 2022 08:22
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.

2 participants