feat: simplify waveform combobox

[acolombier]

This addresses ~#6428~ #13226

This current uses RGB only and the RGB L/R is unused.

By default, it prioritises the GL version if AllShader isn't available, is that the best approach?

https://github.com/mixxxdj/mixxx/assets/7086688/45c2a2b0-97c4-4504-aa9a-0cbcc2c45e12

[JoergAtGithub]

The idea is to merge https://github.com/mixxxdj/mixxx/pull/13151 and than retire all classic GL and GLSL waveforms.

Since https://github.com/mixxxdj/mixxx/pull/12994 we have also first waveform type specific sub options, I could imagine, that we use such an aub option for L/R support as well.

[acolombier]

Are we planning to remove MIXXX_USE_QOPENGL in this case? I guess that doesn't not invalidate this PR, since this is already decoupling the rendering backend complexity away from the waveform type. When it comes to RGB L/R, is it really something that people use? On LateNight default colour scheme, it just make the waveform even more rainbow looking and hard to follow. We could add a check box when RGB is selected, but since this task is to simplify the UX, I'm wondering if this is really necessary

---

Joerg replied

When it comes to RGB L/R, is it really something that people use?

Yes, we had the discussion already, this is why it got implemented. The old GLSL waveforms had this (but looking different), and to deprecated them requires feature-equality.

[acolombier]

(not sure why you edited my message?) It doesn't quite make sense as of why there will be two separate waveform renderer tho. Arguably, if this is used, the same option should be done for the HSV waveform. Anyway, I'm going to add another checkbox.

[acolombier]

I have merged the RGB L/R and RGB in a single renderer, and introduced rendering option, which can be reused with HSV in the future, giving slip mode renderer as a free bonus. Happy to move this commit in a dedicated PR. Would be great if @m0dB could have a quick look on that section!

[acolombier]

Right, I think this is pretty much ready. The only outstanding question I have is, should I hide the Acceleration checkbox is not available/only available? Since I do the following for the L/R option, it might keep it consistent

[JoergAtGithub]

(not sure why you edited my message?)

I did not edit it intentional - sorry - I just intended to reply

[acolombier]

Right, that should be ready for review now. Once #13151 (and #12641) are in, it should be straight forward to retire the legacy waveforms.

[ywwg]

this is a great quality-of-life fix! thank you

[m0dB]

This is nice.

[acolombier]

I've updated the UI and behaviour according to what #13226 mentioned. @m0dB I have already added the High details option so hopefully will make the integration with #13151 seamless.

Code is a bit dirty in some places, especially in the factory, but hopefully with your help, we can make it good enough.

Here is the demo:

https://github.com/mixxxdj/mixxx/assets/7086688/f4cc8728-7744-4099-a152-cacaf3716b24

[Swiftb0y]

I'd like to bring your attention on the fact that I tried to limit the amount of changes done in the factory. I know a few of your comment are about modernising the codebase, but I would appreciate to keep this PR focused on the (IMO already risky) UI and UX change.

I fully agree. I was only asking you to roughly scope the modernization to the lines you're touching already (such as the new functions you created). If the scope is not clear enough, I can also send you a PR with the changes I'm suggesting.

[acolombier]

I was only asking you to roughly scope the modernization to the lines you're touching already (such as the new functions you created)

It's unfortunately not that simple everywhere. Example with unique_ptr on new methods, the allocated widgets get passed around in a way which I can't guarantee who is in the fact owner of this data anymore. (as it currently with WaveformWidgetFactory::createWaveformWidget) I agree this is a bad state to be in, but I guess as @ywwg suggested, perhaps we could accept that the code will get worse before it gets better?

Edit: took note of all inputs in #13245

[ywwg]

A good way to manage incoming mess is to file issues and link them in code. so like

// TODO([link to issue]): This should be cleaned up by...

[acolombier]

https://github.com/mixxxdj/mixxx/assets/7086688/ed71f320-e448-41cf-aed8-980fc37e80bb

Note that I have added support for stereo split in RGB high details and slip animation so we have some consistency on the waveform type. I'll add the slip animation in follow up for other types

[acolombier]

A good way to manage incoming mess is to file issues and link them in code.

I have added TODO on new code, the rest feels counter intuitive to put a TODO on something I didn't write in a first place

---

I think this is pretty much ready for review. As you might have noticed, that PR sized snowballed to 3k line changes, so please try to keep in mind the iterations we discussed about today, and lets address legacy code quality in a follow up PRs asap :)

[ywwg]

I have added TODO on new code, the rest feels counter intuitive to put a TODO on something I didn't write in a first place

yeah I hear you! We do want to avoid "you touched the file, now you are responsible for it" but I think it's worthwhile to think about "we are looking at this file for the first time in a long time, let's not forget about it right away."

[acolombier]

this change defaulted me to plain RGB. We should try to preserve the users' existing choices where possible. Though, I am not sure if I had some other patch applied that messed up that migration.

Could you please share the value you had in mixxx.cfg before running that branch? I'll add some tests to make sure it handles migration correctly.

I think vsynctest should not be available in Release builds

AFAIK vsynctest is included if you run with --developer. I'll make sure that remains the case

[acolombier]

@ywwg I have added full coverage on the waveform type upgrade. Please let me know if I have covered your usecase correctly. Also removed the VSync if not in dev mode

[m0dB]

I have created a PR on top of this PR (I hope that works, git wise) to get rid of all the code duplication in the allshader waveformwidget, reducing it to a single one + some cleanup of waveformwidgetfactory.

https://github.com/acolombier/mixxx/pull/2

[ywwg]

I hope that works, git wise

that should work, yes

[acolombier]

I have hidden the acceleration checkbox on Mac. Note that it will still be show if running with --developer or if the acceleration backend isn't the recommended one (all-shader, happens if you override the setting in INI)

@m0dB could you confirm it hides properly?

[m0dB]

could you confirm it hides properly?

Not entirely, the combobox is a bit smaller than the other full-width comboboxes.

That said, I created https://github.com/acolombier/mixxx/pull/3 and checked that the non-accelerated waveform types work correctly on macOS, so we might as well enable them? Not very useful on macOS I guess, but at least we keep the UI and the behaviour identical across platforms.

We don't seem to have stacked and simple software waveform types though. I guess they should be disabled in the combobox when non-accelerated is selected?

[acolombier]

Not entirely, the combobox is a bit smaller than the other full-width comboboxes.

Which backend were you using? All-shader? Otherwise, it is expected it shows it.

we might as well enable them?

As our Mac developer, I'll follow your call the matter! :) We mentioned in our sync Monday that macs have a fairly unified set of hardware, which all support the all-shader one, so I thought it might make sense to default to all shader and simplify the UI, but I also like the unified UI across platform so happy either way.

We don't seem to have stacked and simple software waveform types though. I guess they should be disabled in the combobox when non-accelerated is selected?

I went the other way around and made the combobox leading the decision with I think is a better UX: if you selected Simple or Stacked it will enable the checkbox (enable hardware) and disable the option. This way, a user can understand no software fallback is available but also don't have to toggle the checkbox first before selecting the new waveform type. I guess I could add a tooltip too?

[acolombier]

@ Mixxx's Core Team could we look at merging that PR ASAP, and potentially to a thorough retroactive review? I agree to address comments and concerns in one or more follow up PRs, but this would help me and @m0dB to stop that PR to grow out of proportion

[ywwg]

sorry, which PR?

[acolombier]

I think we got everything in order now

[ronso0]

I didn't look into the ui file in detail, just want to share a few thoughts on the UI/UX:

• I think it'd look better if Acceleration, Stereo and High Details are in one row
• the label "This function requires ..." should only be shown if waveforms are enabled and acceleration is disabled
• for consistency, all waveform-related options should be greyed out (end-of-track, play position, ...)
• while working on #13273 I noticed we should move the overview options into their own section, because if we do disable all waveform options, the overview options are in between those disabled options which is a bit confusing IMO Maybe move the Overview options to the top so they're more discoverable than below the Waveform option? Maybe use QGroupBoxes for waveform and overview?

I'll check if I can provide some demo commits tomorrow.

[acolombier]

Thanks for your input @ronso0 Are you okay with this being improved in a follow up PR?

[ronso0]

Sure.

[Swiftb0y]

@acolombier do you have time to update the upgrade.cpp code? Then we can merge.

[Swiftb0y]

also @ywwg please double check your review and approve/dismiss so we can get this cleared formally.

[acolombier]

I should get the time sometime today or tomorrow hopefully.

[ronso0]

This has really finally been bumped to 2.6? I though there was the goal to clean this up for 2.5

[acolombier]

This has really finally been bumped to 2.6? I though there was the goal to clean this up for 2.5

This is only the first part of the clean up, more is to come. I'm afraid it might not be ready in time and we probably don't want to delay 2.5 for that

[ywwg]

(I am not sure if we have to explicitly wait for @m0dB and @ronso0 since they did not make must-address comments? I am still unsure how we handle multi-reviewer PRs. It sounded like Ronso0 was lgtm)

[acolombier]

Ideally, I just would like confirmation that this is fine from @m0dB. Alternatively, I think it is small enough to be fixed in later PRs if it isn't.

[Swiftb0y]

I think as long as they aren't requests for changes its fine. I think thats the only justification anyways for there being a difference between "Comment" and "Request Changes" anyways

[acolombier]

Could you provide more information? How to reproduce? What is the crash? Message log? Are you able to reproduce this on main using the same allshader waveform?

[Swiftb0y]

Whoops. I'm sorry for the false alarm. I did some more digging and I'm pretty sure I suffered from https://gitlab.gnome.org/GNOME/mutter/-/issues/3462. After carefully ensuring I've upgraded to gnome 46.2 all the waveforms work again (at least without crashes and other regressions).

[Swiftb0y]

Thank you for your continued efforts and sticking around enough to get this huge PR merged btw @acolombier Also thank you for your simplification PRs @m0dB