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

Preferences waveforms: recall correct waveform type when selecting an overview type #12231

Merged
merged 3 commits into from Jan 11, 2024
Merged

Preferences waveforms: recall correct waveform type when selecting an overview type #12231

merged 3 commits into from Jan 11, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 27, 2023 •
edited

Closes #12226

The waveform type was written to config but the local variable m_configType holding this type for updating the widgets after skin (re)load was not updated accordingly.

Note that I consider this just a quick fix.

IMO we should not store the value in m_configType but read it from the config in setWidgetTypeFromConfig (move the logic from setConfig(). AFAICT this is required only in the constructor and in slotSkinLoaded().
A commit that should do th trick is 80bb79c, though I'm not motivated to verify this actually works under all conditions / on all OS. If someone wants to check that and do the testing, please go ahead.
Maybe also call DlgPrefWaveforms::slotUpdate when MixxxMainWindow::rebootMixxxView succeeded.

@ronso0 ronso0 mentioned this pull request Oct 27, 2023
Open
@ronso0 ronso0 linked an issue Oct 27, 2023 that may be closed by this pull request
Changing the waveform overview type after changing the main waveform type resets the main waveform type #12226
Open
@ronso0 ronso0 modified the milestone: 2.4.0 Oct 27, 2023
@ronso0 ronso0 added this to the 2.4.0 milestone Dec 3, 2023
ywwg
ywwg reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

I made some style notes, but the basic code looks good! thanks for this fix

src/waveform/waveformwidgetfactory.cpp Outdated
@@ -537,6 +538,8 @@ bool WaveformWidgetFactory::setWidgetType(
WaveformWidgetType::Type type,
WaveformWidgetType::Type* pCurrentType) {
if (type == *pCurrentType) {
qDebug() << "WaveformWidgetFactory::setWidgetType - type"
Copy link
Member

Choose a reason for hiding this comment

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

this does not seem like a debug statement we need -- ok to delete it?

src/waveform/waveformwidgetfactory.cpp
int empty = findHandleIndexFromType(WaveformWidgetType::EmptyWaveform);
int desired = findHandleIndexFromType(m_configType);
if (desired == -1) {
qDebug() << "WaveformWidgetFactory::setWidgetTypeFromConfig"
Copy link
Member

Choose a reason for hiding this comment

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

(whereas, this one seems useful)

src/waveform/waveformwidgetfactory.cpp Outdated
desired = empty;
}
// qDebug() << "found valid waveform type" << getDisplayNameFromType(m_configType);
Copy link
Member

Choose a reason for hiding this comment

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

can you remove the commented-out debug lines? Unless you think they will be super useful in the future they tend to add clutter

src/waveform/waveformwidgetfactory.cpp Outdated
for (int i = 0; i < m_waveformWidgetHandles.size(); i++) {
WaveformWidgetAbstractHandle& handle = m_waveformWidgetHandles[i];
int index = 0;
for (const auto& handle : std::as_const(m_waveformWidgetHandles)) {
Copy link
Member

Choose a reason for hiding this comment

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

if you're going to maintain an index like this, you might as well keep the original style of loop (and use .get() to get a const ref instead of mutable)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I reverted this.
Though this is a QVector, not sure what you mean by get().

@ronso0
Copy link
Member Author

ronso0 commented Dec 18, 2023

Thanks for your review. Feel free to edit this branch, I'm ill and probably won't fix it this year.

@ywwg
Copy link
Member

ywwg commented Dec 22, 2023

Thanks for your review. Feel free to edit this branch, I'm ill and probably won't fix it this year.

<3 hope you feel better and happy new year

@ronso0
Copy link
Member Author

ronso0 commented Dec 22, 2023

Thanks, doing better already.
Either you edit this as you desire, or I find time to do this, whatever happens earlier. After all I need some distraction from the usual xmas madness..

@ronso0
Copy link
Member Author

ronso0 commented Dec 23, 2023

I addressed your comments with !fixups. Please check and give 👍 if it's okay, then I'll squash and force-push.

@ronso0 ronso0 requested a review from ywwg December 29, 2023 15:08
@ywwg ywwg requested a review from daschuer January 2, 2024 15:39
@ronso0 ronso0 marked this pull request as draft January 2, 2024 15:54
@ronso0
Copy link
Member Author

ronso0 commented Jan 2, 2024

Draft again to avoid merging the separate fixups.

@ywwg
Copy link
Member

ywwg commented Jan 8, 2024

Let me know how you want to treat this PR, not quite clear how you wanted to treat the fixups?

@ronso0
Copy link
Member Author

ronso0 commented Jan 8, 2024

I want to squash them but give @daschuer a chance to review them separately.

@ywwg
Copy link
Member

ywwg commented Jan 8, 2024

Can you mark this as ready for review, and then add a note that it needs to be squashed before merge? that way it gets on his radar

@ronso0 ronso0 marked this pull request as ready for review January 8, 2024 14:29
@ronso0
Copy link
Member Author

ronso0 commented Jan 8, 2024

@daschuer I pushed to fixups and left them separate for now so you can easily review those.
Please give me a hint when you consider this okay, then I'll squash and force-push.
Thanks!

daschuer
daschuer approved these changes Jan 11, 2024
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 can confirm the issue is fixed and the Code looks good. Thank you.
Please give a hint once I can press merge.

@ronso0
WaveformWidgetFactory: tweak debug output, add getDisplayNameFromType()
85b1f9d
@ronso0
WaveformWidgetFactory: store type in member when writing to config
3db3585 
This is necessary to recall the stored type when repopulating the widgets after
(re)loading a skin, for example when selecting an overview type in the preferences.
@ronso0
WaveformWidgetFactory: range-based loops, std:as_const
afed5f7
@ronso0
Copy link
Member Author

ronso0 commented Jan 11, 2024

Ready!

@daschuer daschuer merged commit ebaa8bd into mixxxdj:2.4 Jan 11, 2024
13 checks passed
@daschuer
Copy link
Member

Thank you.

@ronso0 ronso0 deleted the pref-waveforms branch January 11, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywwg ywwg ywwg approved these changes

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
bug preferences usability waveform
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

Changing the waveform overview type after changing the main waveform type resets the main waveform type
3 participants
@ronso0 @ywwg @daschuer