Add minute markers on horizontal waveform overview

[ChrisTallon]

Some users would like a visual indication of minutes on the waveform overview.

There is a discourse thread here: https://mixxx.discourse.group/t/do-minute-markers-exist-in-this-software/27477

There is a bug about it #5843.

Without minute markers:

With minute markers:

An extra setting in the preferences dialog waveform section to control minute markers:

It defaults to off to maintain current Mixxx behaviour. There is no support for vertical waveform overviews - I don't know of a skin to use to write and test it with.

[ronso0]

Thank you for this PR, this looks good so far! Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

Just a quick note: WaveformWidgetFactory only deals with the scrolling waveforms. I see it's handy to use that for getting the minute markers bool in every paint event, but you may also use static enableMinuteMarkers(bool enable) in WOverview or create a ControlObject in the preferences and the respective ControlProxy in WOverview (like I did recently for the overview type switch). Will do a more thorough review soonish.

[ChrisTallon]

Thanks for the attention so far. It feels like this is on track.

@Swiftb0y I believe I have incorporated all your suggestions manually. I didn't use the one-click commit as it would have left an unbuildable commit.

I think there are two things left - I want to investigate @ronso0 's comment about creating ControlObject / ControlProxy objects. I didn't appreciate WaveformWidgetFactory only dealt with the scrolling waveforms - it doesn't make sense for the Overview minute markers code to be in there.

Also I want to see how to trigger a WWaveformOverview GUI paint when clicking apply / OK on the preferences dialog. It seems like an annoying loose end that if nothing is happening the minute marks don't get painted. This could be confusing for someone looking to see what that option changed.

[ronso0]

Also I want to see how to trigger a WWaveformOverview GUI paint when clicking apply / OK on the preferences dialog

This instant update would be one of the benefits of the ControlObject + ControlProxy. You just need to find a way to trigger a repaint if the control value changes. Maybe just update();?

[Swiftb0y]

I believe I have incorporated all your suggestions manually. I didn't use the one-click commit as it would have left an unbuildable commit.

Yes, thank you. That is usually the desired workflow (or accept and then pull, fix the commit, amend, force push). Whatever works for you.

it doesn't make sense for the Overview minute markers code to be in there.

:+1:

[ronso0]

WaveformWidgetFactory only deals with the scrolling waveforms.

Ahmm, I was wrong: it already provides the overview waveform data and handles the [Waveform], OverviewNormalized setting. So actually, the alternative to the ControlProxy would be WaveformWidgetFactory emitting a minuteMarkersEnabled(bool) signal, and connect to that in WOverview::setup.

[ChrisTallon]

So which should I go for... Are you moving towards a ControlProxy architecture and so that is desirable, or do I just do the WaveformWidgetFactory solution?

[ronso0]

I think the WaveformWidgetFactory signal solution is easier to implement.

First I preferred to keep the overview stuff out of WaveformWidgetFactory, but actually (and since it already handles overview settings) the opposite might be better: consolidate all waveform-related settings (main and overview) in the WaveformWidgetFactory singleton, which would result in one place to maintain all that (like get rid of the overview type proxy).

@Swiftb0y wdyt?

[Swiftb0y]

not sure tbh. WaveformWidgetFactory is one ugly class, idk if we want that to get further out of control. I would prefer to put it somewhere else if there is a better alternative.

[ChrisTallon]

Contributor agreement signed.

I have switched the config to ControlObject/Proxy. This PR now doesn't use WaveformWidgetFactory at all.

As far as I know this is ready for a final(?) review.

[Swiftb0y]

Please set up pre-commit and make sure to manually apply the required changes using pre-commit run --from-ref main --to-ref HEAD. Also look at the other CI failures and address those. Thank you.

[ChrisTallon]

Sorry, I set up a new git tree earlier today and forgot to install pre-commit. I fixed it for this CI run but I'll look out for any more fails.

[ChrisTallon]

I can't get my compiler to generate those same errors so I'm fixing in the blind now...

[Swiftb0y]

I can't get my compiler to generate those same errors so I'm fixing in the blind now...

Yeah... that's usually the case when fixing CI issues with foreign compilers. Just try, push and if that didn't work, amend and force push until you have a commit that works.

[ChrisTallon]

Hi, what happens next?

[Swiftb0y]

Hey there, thank you for reminding me of this. Can you do me a favor and squash your review commits down? Then it be good if someone could test this (idk when I'll find time) and then we can merge. I may also do another review round to make sure I didn't miss anything.

[ChrisTallon]

Commits are squashed. Thanks!

[Swiftb0y]

Thanks. @ronso0 do you have time for a smoketest by any chance?

[ronso0]

This looks good, I'm also fine with white as default marker color. There are some marker offsets with long tracks, probably due to rounding

In vertical mode, the right markers are drawn somewhere on the overview, should be fixed by 6ed9a1ae8f feel free to adjust the var names. maybe length for the longitudinal direction and width for the amplitude direction?

commit with vertical waveforms (dirty): 1507f35b27

The relative length for the markers makes sense, but for tall (vertical: wide) overviews the markers get unnecesarily long IMO. Shall we limit the length? Or would that limit the skin designers freedom for let's say a skin with huge overviews where long markers are desired?

[Swiftb0y]

Lets just get it right without adding the customization complexity yet. The vertical markers were untested anyways so I'm not surprised they're wrong. @ChrisTallon can you pull in the vertical waveform commit ronso0 linked?

[ChrisTallon]

https://github.com/mixxxdj/mixxx/commit/6ed9a1ae8febde99933fd01a733db73ef38e1df9 is committed.

[ChrisTallon]

Mixxx is awesome.

[JoergAtGithub]

Mixxx is awesome.

And you just made it a bit more awesome!