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

Effectchain tooltip #11902

Merged
merged 3 commits into from Sep 6, 2023
Merged

Effectchain tooltip #11902

merged 3 commits into from Sep 6, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Aug 31, 2023

picking another low-hanging fruit to polish the new effects frontend:
show chained effects in tooltip of

  • WEffectChainPresetSelector
  • WEffectChainPresetButton menu
    image
    image

Close #10605

@github-actions github-actions bot added the ui label Aug 31, 2023
@ronso0 ronso0 added effects gui and removed ui labels Aug 31, 2023
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 31, 2023
@github-actions github-actions bot added the ui label Aug 31, 2023
populateMenu();
}

void WEffectChainPresetButton::populateMenu() {
m_pMenu->clear();

// Chain preset items
const auto bem = m_pEffectsManager->getBackendManager();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const auto bem = m_pEffectsManager->getBackendManager();
const auto* pBem = m_pEffectsManager->getBackendManager();

Copy link
Member

Choose a reason for hiding this comment

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

Else the pointer is const, not the object it is pointing to.
Below the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

This returns EffectsBackendManagerPointer, I don't understand how EffectsBackendManager could be const here.
Now it's explicit const EffectsBackendManagerPointer instead of const auto.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes I missed that. Thank you.

tooltip.append(bem->getDisplayNameForEffectPreset(pEffectPreset));
}
}
QAction* pAction(new QAction(title, this));
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 parented_ptr?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, done.

@daschuer
Copy link
Member

daschuer commented Sep 1, 2023

Thank you for this nice addition.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

@Swiftb0y
Copy link
Member

Swiftb0y commented Sep 3, 2023

@daschuer please resolve your review.

@daschuer
Copy link
Member

daschuer commented Sep 3, 2023

We have the issue that Chains with a single effect are looking wrong, like this:

Filter
Filter

Can we use the pManifest->setDescription() instead?

Filter
Allows only high or low frequencies to play.

Or just skip the bold headline?

@ronso0
Copy link
Member Author

ronso0 commented Sep 3, 2023

or just show
Filter ?

  • no tooltip feels like a bug
  • showing the description of the only effect is inconsistent with multi-effect chains
  • first line is the preset name, hence first (or only) line not bold also feels wrong

@daschuer
Copy link
Member

daschuer commented Sep 3, 2023

Single Bold Filter works for me.

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.

LGTM, Thank you.
Manual Test are also good. Just waiting for CI results.

@Swiftb0y Swiftb0y merged commit f7877f3 into mixxxdj:2.4 Sep 6, 2023
12 of 13 checks passed
@ronso0 ronso0 deleted the effectchain-tooltip branch September 6, 2023 12:58
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.

None yet

3 participants