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

allow changing the waveform overview type without reloading the skin #13273

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 23, 2024

... by moving the RGB/LMH/HSV pixmap drawing into the base class WOveriew.
Type change is caught by a ControlProxy.

Switching works super smooth 😎

Partially fixes #9745
This is a bugfix IMO and can slip in 2.5 anytime.

Nice to have: enum for overview type

Hopefully not to many conflicts with the current waveform pref work

@ronso0
Copy link
Member Author

ronso0 commented May 26, 2024

I think this super low-risk and can go to 2.5

FWIW the only change I made here was to move the draw functions to the base class, no further adjustments.

@ronso0 ronso0 changed the base branch from main to 2.5 June 8, 2024 12:55
@ronso0 ronso0 force-pushed the overview-type-integration branch from d0e0e94 to 7080e9d Compare June 8, 2024 12:55
@github-actions github-actions bot added the build label Jun 8, 2024
@ronso0 ronso0 added this to the 2.5.0 milestone Jun 10, 2024
@ronso0 ronso0 force-pushed the overview-type-integration branch 2 times, most recently from 866de76 to 6c3ec64 Compare June 13, 2024 12:49
@ronso0 ronso0 force-pushed the overview-type-integration branch from 6c3ec64 to d4c1bc2 Compare June 13, 2024 13:20
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.

It works like a charm, Thank you. Somme comments:

const QString m_group;
UserSettingsPointer m_pConfig;
parented_ptr<ControlProxy> m_endOfTrackControl;

int m_type;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this an enum?

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, we can that's why I mentioned it as nice-to-have in the description, and I already took a stab at it.
But I gave up since it took to long and isn't mandatory IMO, even feels like this makes the preferences more complicated (or I simply messed something up there)

I can take another look, but actually I'd appreciate if we could this merged soonish and refine later on (before the 2.5 release)

@@ -1211,6 +1237,285 @@ void WOverview::drawPassthroughOverlay(QPainter* pPainter) {
}
}

bool WOverview::drawNextPixmapPart() {
ScopedTimer t(u"WOverview::drawNextPixmapPartHSV");
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
ScopedTimer t(u"WOverview::drawNextPixmapPartHSV");
ScopedTimer t(QStringLiteral("WOverview::drawNextPixmapPartHSV"));

Else it will break after merging 2.5 to main.

ConstWaveformPointer pWaveform,
const int nextCompletion) {
DEBUG_ASSERT(!m_waveformSourceImage.isNull());
ScopedTimer t(u"WOverview::drawNextPixmapPartHSV");
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
ScopedTimer t(u"WOverview::drawNextPixmapPartHSV");
ScopedTimer t(QStringLieral("WOverview::drawNextPixmapPartHSV"));

ConstWaveformPointer pWaveform,
const int nextCompletion) {
DEBUG_ASSERT(!m_waveformSourceImage.isNull());
ScopedTimer t(u"WOverview::drawNextPixmapPartLMH");
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
ScopedTimer t(u"WOverview::drawNextPixmapPartLMH");
ScopedTimer t(QStringLiteral("WOverview::drawNextPixmapPartLMH"));

ConstWaveformPointer pWaveform,
const int nextCompletion) {
DEBUG_ASSERT(!m_waveformSourceImage.isNull());
ScopedTimer t(u"WOverview::drawNextPixmapPartRGB");
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
ScopedTimer t(u"WOverview::drawNextPixmapPartRGB");
ScopedTimer t(QStringLiteral("WOverview::drawNextPixmapPartRGB"));

@ronso0 ronso0 force-pushed the overview-type-integration branch from 227973f to 3fd550b Compare June 13, 2024 22:04
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.

@daschuer daschuer merged commit 11dff74 into mixxxdj:2.5 Jun 14, 2024
13 checks passed
@ronso0 ronso0 deleted the overview-type-integration branch June 14, 2024 12:04
ronso0 added a commit to ronso0/mixxx that referenced this pull request Jun 14, 2024
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.

2 participants