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?

Kooha-2024-05-08-18-15-40-10MB.mp4

[JoergAtGithub]

The idea is to merge #13151 and than retire all classic GL and GLSL waveforms.

Since #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:

Kooha-2024-05-12-16-58-50.mp4

[acolombier]

I think we got everything in order now

[Swiftb0y]

LGTM. Any1 else wanting to take a look (just in case I missed any grave errors, not for trivial stuff like I nitpicked in this round)?

[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 allow changing the waveform overview type without reloading the skin #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]

lgtm!

[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

[Swiftb0y]

Yeah. so all the additional refactorings here leave this in a difficult state. The software fallback waveforms of course work, but the hardware waveforms sometimes crash my gnome-shell completely. That has already been the case previously (with the legacy GLSL waveforms IIRC), but now it has become more difficult to tell when this happens. also the Stacked and the Simple waveforms don't work at all (though not producing any crashes).

This is on fedora 39.

[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]

Did some tests, now that mutter fixed the issue, it works well

[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