S4 MK3 improvements

[acolombier]

This is a break down of #13653, in order to include only the HID changes.

Although these changes need to come at once due to intertwining and testing being done on the whole set, I tried my best to break down those changes into atomic comments.

These includes:

• Improve experience with hotcue (ability to change the colour, deletion mode feedback)
• Beatjump pad mode
• Stem controls
• Some stability improvement
• Support of for unofficial shared API (Ability for controller to share data at runtime #12199)

Manual is still to do! is available here: mixxxdj/manual#703

[Swiftb0y]

not loving all of this, but I didn't see any gigantic issues and I understand you want to get this merged...

[acolombier]

not loving all of this, but I didn't see any gigantic issues and I understand you want to get this merged...

Appreciate the effort here. If you would want, feel free to raise the aspect you feel may be suboptimal so I can try and see if there are better tradeoffs.
Or is it mainly that it embeds that unofficial API?

Manual is still to do!

Looks like I forgot I had already wrote that manual a year ago... mixxxdj/manual#703

[Swiftb0y]

Or is it mainly that it embeds that unofficial API?

No thats mostly fine (I guess you can add some comments that explains the situation for posterity).
My primary concern is rather the hasSelectedStem() and related code because it worsens the if else mess. But I haven't worked through this in enough detail to make concrete improvement suggestions.

[ronso0]

I'll give this a shot when I'll have my controller back --should be around Thursday/Friday-- and check if all makes sense with the new manual.

Thanks for your effort with the screens and the mapping itself 🙏
it's a great base for mods 😉

[acolombier]

My primary concern is rather the hasSelectedStem() and related code because it worsens the if else mess

I tried to improve the situation with f0e2672 by removing all the early return to use a more consistent condition structure when it make sense. Hopefully that somehow improves it! (Noticed a duplicate as well!)

[ywwg]

• Right now it seems that loop moving is always a single beat at a time -- is that intended? I'm used to the loops moving at the beatjump value. (Maybe that could be an option?)
• when I push the fx buttons, all of them light up, not just the one I'm pressing.
• "filter" should select the first selected quick effect to match other controllers (for instance, upgrading from my s3), rather than the fifth one. I will almost always want the Filter effect on the deck, because that's the best one for musical adjustment. But sometimes I want a different quickeffect. Part of the problem here is that Mixxx does not have a good UX for choosing quickeffects. But for now, I think it would actually be more intuitive to have Filter select the 0th quickeffect (the top of the list), and "1" select the next one down, etc.
• I do not see a way to toggle quantize mode per deck -- this is a feature I need for some mixing situations (for example: mixing two tracks, but then having an ambient sound effect track on a third deck that is not subject to sync). Holding quantize and then pushing a deck select button could be one way to do that.

[acolombier]

Right now it seems that loop moving is always a single beat at a time -- is that intended? I'm used to the loops moving at the beatjump value. (Maybe that could be an option?)

Agree the suggested approach, but this is already the existing behaviour (see in main)

when I push the fx buttons, all of them light up, not just the one I'm pressing.

Same, not changed as part of this PR. If this is using the 1st FX, this is expected (not sure why the text is duplicated 😅 )

"filter" should select the first selected quick effect to match other controllers

This should already be the case (Filter aliases to 0 - first quick FX configured). No behavioural change should have occurred in this PR

I do not see a way to toggle quantize mode per deck -- this is a feature I need for some mixing situations

There is indeed no options for this currently. This should however be a separate feature request IMO

[acolombier]

Thanks for your review @ywwg
As your change request appears to be related to existing, unmodified behaviour, as you okay to lift your change request review?

[acolombier]

Thanks for the approval - I took the opportunity to squash so if all tests from @ronso0 are conclusive, we can go ahead and merge!

[ronso0]

why is this dupe needed?
isn't it identical to that a few lines below?

[acolombier]

Hmmm I am not 100% sure tbh...

I likely wrote than more than a year ago, but I somewhat remember the issue being that component.outDisconnect() will usually act based of certain key that might have been changed by shift hooks. So we unsure the component is unshifted before we start disconnecting and updating one last time LED status and CO connections - that I'm pretty confident of

However, I cannot remember why I would have kept the one after - could be a failed conflict resolution some longtime ago... Should we take the risk and remove it? Feels fairly easy to add back in case it was inideed justified. And this time we can add a comment as of why 😅

[ronso0]

Hehe..
sure, remove and add a comment.

Unfortunately I don't really find time right now to test this thoroughly.
Will do tomorrow.

[ronso0]

I just don't manage to use the sparse time I have with controller to thoroughly test, so trusting the other ✔️ here.

Let's go!