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.

Sign up for GitHub

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

Jump to bottom

DlgPrefSound: fix setting keylock engine from combobox #11047

Merged
merged 1 commit into from
Nov 8, 2022
Merged

DlgPrefSound: fix setting keylock engine from combobox #11047

merged 1 commit into from
Nov 8, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 7, 2022

previously the new engine wasn't applied due to wrong type conversion 😬

@ronso0 ronso0 requested a review from Swiftb0y November 7, 2022 22:20
@github-actions github-actions bot added the ui label Nov 7, 2022
daschuer
daschuer reviewed Nov 7, 2022
View reviewed changes
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.

I do not understand the problem that is fixed here ...

src/preferences/dialog/dlgprefsound.cpp Outdated Show resolved Hide resolved
Swiftb0y
Swiftb0y reviewed Nov 7, 2022
View reviewed changes
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.

The changes seem like a noop to me too. Are you sure this is fixing the problem?

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 7, 2022

Thanks for looking into it btw.

@ronso0
Copy link
Member Author

ronso0 commented Nov 7, 2022

I do not understand the problem that is fixed here ...

https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Rubberband.203.2E0.20setting.20not.20applied.2Fsaved
The value from mixxx.cfg is loaded correctly, but no matter what new value you select in the preferences, in slotApply() keylockComboBox->currentData().toDouble() would always return 0 and you'd be stuck with SoundTouch.

@ronso0
Copy link
Member Author

ronso0 commented Nov 7, 2022 •
edited
Loading

The changes seem like a noop to me too. Are you sure this is fixing the problem?

Nope.
Yep.

Just try with main. Select one of the RubberBand versions, apply and check the value in dev tools / mixxx.cfg

@ronso0 ronso0 force-pushed the rubberband-pref-fix branch from e6e7ae7 to eb19e10 Compare November 7, 2022 22:45
Swiftb0y
Swiftb0y reviewed Nov 7, 2022
View reviewed changes
src/preferences/dialog/dlgprefsound.cpp
Comment on lines +315 to +317
const auto keylockEngine =
keylockComboBox->currentData().value<EngineBuffer::KeylockEngine>();
m_pKeylockEngine->set(static_cast<double>(keylockEngine));
Copy link
Member

Choose a reason for hiding this comment

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

interesting, apparently QVariant::toDouble doesn't work if the contained type is an enum? https://doc.qt.io/qt-5/qvariant.html#toDouble

Am I reading that correctly?

Swiftb0y
Swiftb0y approved these changes Nov 7, 2022
View reviewed changes
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, The broken QVariant<enum_t>::toDouble is a bad tripping hazard though, we might want to file a QTBUG.

daschuer
daschuer approved these changes Nov 8, 2022
View reviewed changes
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 for the fix and your fast responds

@daschuer daschuer merged commit 233dcad into mixxxdj:main Nov 8, 2022
@ronso0 ronso0 deleted the rubberband-pref-fix branch November 8, 2022 10:26
napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023
@daschuer @napaalm
Merge pull request mixxxdj#11047 from ronso0/rubberband-pref-fix
da8a47c 
DlgPrefSound: fix setting keylock engine from combobox
napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023
@daschuer @napaalm
Merge pull request mixxxdj#11047 from ronso0/rubberband-pref-fix
5014f06 
DlgPrefSound: fix setting keylock engine from combobox
napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023
@daschuer @napaalm
Merge pull request mixxxdj#11047 from ronso0/rubberband-pref-fix
79a9591 
DlgPrefSound: fix setting keylock engine from combobox
napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 10, 2023
@daschuer @napaalm
Merge pull request mixxxdj#11047 from ronso0/rubberband-pref-fix
92886da 
DlgPrefSound: fix setting keylock engine from combobox
napaalm pushed a commit to napaalm/mixxx that referenced this pull request Jul 11, 2023
@daschuer @napaalm
Merge pull request mixxxdj#11047 from ronso0/rubberband-pref-fix
6bef121 
DlgPrefSound: fix setting keylock engine from combobox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

@Swiftb0y Swiftb0y Swiftb0y approved these changes

Assignees
No one assigned
Labels
ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ronso0 @Swiftb0y @daschuer