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

Preferences > Key: restore custom notation #4136

Merged
merged 2 commits into from
Jul 27, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jul 23, 2021

reported in the forums
https://mixxx.discourse.group/t/custom-key-notation-not-saved-after-restart-in-2-3-0/

custom key notation was not restored correctly.
code is simpler now, too.

Proposed Changelog entry

- Fix custom key notation not restored correctly after restart #4136

@github-actions github-actions bot added the ui label Jul 23, 2021
@ronso0 ronso0 added this to the 2.3.1 milestone Jul 23, 2021
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.

I don't fully understand the code changes to be honest, but I was able to confirm that the issue exists in main and 2.3 and is fixed by this PR.
Do yo want to include a changelog entry? I don't know our current policy on what to include in the changelog when.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Only a minor typo.

return;
}

void DlgPrefKey::setNotation(KeyUtils::KeyNotation notation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole class and especially these methods are not so great. But at least the handling is consistent now.

for (auto it = m_keyLineEdits.constBegin();
it != m_keyLineEdits.constEnd(); ++it) {
it.value()->setEnabled(true);
it.value()->setText(KeyUtils::keyToString(it.key(), notation));
// QLineEdits are only enabled for Custon notation.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Member Author

Choose a reason for hiding this comment

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

thx, fixed

@ronso0 ronso0 force-pushed the pref-key-restore-custom-notation branch 2 times, most recently from a77f7fd to c270ba4 Compare July 25, 2021 14:47
@ronso0
Copy link
Member Author

ronso0 commented Jul 25, 2021

ready.

@@ -128,8 +128,11 @@ void DlgPrefKey::loadSettings() {
}
}

setNotation(notation_type);
// Store notation map for later rećall...
Copy link
Contributor

Choose a reason for hiding this comment

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

Why all the accents? Maybe one of your keys got stuck ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

naa, my display now has cracks all over the left side. so usually it's 'why that accent? oh, it's just a crack..'
here it was the other way around : )

KeyUtils::setNotation(notation);
// ... BEFORE invóking setNotation() which populates the QLineEdits from
// the map retreived from KeyUtils.
Copy link
Contributor

Choose a reason for hiding this comment

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

and a typo

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@ronso0 ronso0 force-pushed the pref-key-restore-custom-notation branch from c270ba4 to 761487c Compare July 25, 2021 20:51
@ronso0
Copy link
Member Author

ronso0 commented Jul 27, 2021

pa-ping

@uklotzde
Copy link
Contributor

Thank you! LGTM

@uklotzde uklotzde merged commit 3e11751 into mixxxdj:2.3 Jul 27, 2021
@ronso0 ronso0 deleted the pref-key-restore-custom-notation branch July 27, 2021 21:39
@Holzhaus Holzhaus added the changelog This PR should be included in the changelog label Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog minor bug preferences ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants