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

add AutoDJ xfader recenter option (default off) #13303

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 29, 2024

adopting #13156 but cleaning up the "buffer" settings before
(that dlg wrote some "...Buff" settings to config for storing temp pending settings...)

Fixes #11571

TODO:

@Eve00000
Copy link
Contributor

Eve00000 commented May 30, 2024

Hi Ronso,
I was almost ready to upload my adapted version, but you already did the changes. You cleaned up the whole routine. Thank you for the work and congratulations it surely will be better then mine .
I will learn by comparing your version with what I did.

If it's possible I just want to add the lines in the ui with some explanation (tooltip),
I don't know how to add that in the pr so I'll write it here.

<property name="toolTip">
<string>When checked, the Crossfader will be set to the default center position after disabling the Auto DJ.
This can be handy when you're working with an external mixer and don't need to see the Mixxx-mixer and crossfader 
(mixer and/or crossfader disabled in the skin-options).

Please mind that the crossfader adds some volume to the pointed playing deck in the main output, 
so centering the crossfader will cause a volumedrop in the main output.
</string>
</property>

greetings

@ronso0 ronso0 marked this pull request as ready for review May 30, 2024 12:23
@ronso0
Copy link
Member Author

ronso0 commented May 30, 2024

Thanks for your proposal, I already added the hint as static label (no as tooltip which may not be discovered).

Ready for review!

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.

couple comments, looks pretty good so far. I can't really review the ui file though

Comment on lines 581 to 582
// if (m_pConfig->getValue<bool>(ConfigKey(kAutoDJGroup,
if (m_pConfig->getValue<bool>(ConfigKey(kConfigKey,
QStringLiteral("center_xfader_when_disabling")))) {
Copy link
Member

Choose a reason for hiding this comment

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

stray comment

Suggested change
// if (m_pConfig->getValue<bool>(ConfigKey(kAutoDJGroup,
if (m_pConfig->getValue<bool>(ConfigKey(kConfigKey,
QStringLiteral("center_xfader_when_disabling")))) {
if (m_pConfig->getValue<bool>(ConfigKey(kConfigKey,
QStringLiteral("center_xfader_when_disabling")))) {

Copy link
Member Author

Choose a reason for hiding this comment

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

whoopsy

Comment on lines 84 to 85
RequeueIgnoreTimeEdit->setTime(QTime::fromString(
"23:59", RequeueIgnoreTimeEdit->displayFormat()));
Copy link
Member

Choose a reason for hiding this comment

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

no need to implicitly depend on the display format or construct the time in an an unnecessarily slow fashion.

Suggested change
RequeueIgnoreTimeEdit->setTime(QTime::fromString(
"23:59", RequeueIgnoreTimeEdit->displayFormat()));
RequeueIgnoreTimeEdit->setTime(QTime(23, 59));

// if (m_pConfig->getValue<bool>(ConfigKey(kAutoDJGroup,
if (m_pConfig->getValue<bool>(ConfigKey(kConfigKey,
QStringLiteral("center_xfader_when_disabling")))) {
m_pCOCrossfader->set(0);
Copy link
Member

Choose a reason for hiding this comment

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

would it make more sense to set use crossfader_set_default here?

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, butfor that we'd need another control, either via ControlObject::get() or another member.
I'd prefer to stick to the crossfader CO.

Copy link
Member

Choose a reason for hiding this comment

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

fair. I just thought the _set_default was more expressive.

Comment on lines +62 to +63
CenterXfaderCheckBox->setChecked(m_pConfig->getValue(
ConfigKey("[Auto DJ]", "center_xfader_when_disabling"), false));
Copy link
Member

@Swiftb0y Swiftb0y May 30, 2024

Choose a reason for hiding this comment

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

I'd be nice if these defaults were constexpr statics. Currently they're duplicated a bunch of times.

Copy link
Member Author

Choose a reason for hiding this comment

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

Jep, moving all repeatedly used pref ConfigKeys (preferences, and respective engine units) to other files.
I have some changes stashed already, but I prefer to do that in a separate PR.

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. feel free to squash and force push

@Swiftb0y Swiftb0y merged commit 8be2e89 into mixxxdj:main Jun 6, 2024
13 checks passed
@Swiftb0y
Copy link
Member

Swiftb0y commented Jun 6, 2024

Sorry, forgot about this.

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.

Re-add Crossfader re-center when disabling auto DJ
3 participants