QML: Refactor Knob/Slider controls #3955
Conversation
| id: root | ||
|
|
||
| property string resetGroup: group | ||
| property string resetKey: key + "_set_default" |
There was a problem hiding this comment.
maybe instead of relying on _set_default of a ControlPotMeter control, it would be better to expose ControlProxyt::reset() to the qmlcontrolproxy?
There was a problem hiding this comment.
This is just a default value, you can override resetKey if you want.
There was a problem hiding this comment.
Right, but my point is that COs already seem have a concept of resetting themselves (slot ControlProxy::reset(), ControlObject::reset()) so it would make sense to expose that same slot on the QML proxy as well. That would be better than relying on the fact that there is some CO variant whose only purpose is to reset the control. The _set_default variant seems like a workaround that was needed because of the legacy skin and controller mapping system (before there was even JS engine).
There was a problem hiding this comment.
Good point. I'll look into that in a follow up PR, this one is already pretty large. I don't want to add C++ changes, too.
There was a problem hiding this comment.
I also spend some time on that already :)
| const change = (root.max - root.min) * Mixxx.MathUtils.clamp(delta, -100, 100) / 100; | ||
| const value = Mixxx.MathUtils.clamp(root.value + change, root.min, root.max); |
There was a problem hiding this comment.
the first clamp seems unnecessary to me.
There was a problem hiding this comment.
PLease make a concrete suggestion. Just removing the clamp will make the value go to the minimum/maximum on the slightest mouse move.
There was a problem hiding this comment.
That doesn't seem right. I'll try to debug whats going on here then.
There was a problem hiding this comment.
I can remove both clamps and the it works just as well. Could it be a screen resolution / pixel density issue?
In addition, there is also a deadzone which makes the control ignore the drag for small movements. You would have to set dragThreshold: 0 but thats only been added in 5.15.
Maybe using a DragHandler is a bad idea after all...
Can you elaborate on why you are replacing the mousearea with a draghandler at all?
There was a problem hiding this comment.
If I use a mousearea, double-click to reset doesn't work anymore due to event propagation issues.
I you have a simpler, working solution, let's merge know and you open a follow up PR.
There was a problem hiding this comment.
I now understand the issue. I don't think composing mouseareas together is a good idea. especially not for the purpose of being able to reset a control. IMO the better approach is to make the reset functionality part of the ControlProxy object and then just give the knob and slider a property bool resetable: false.
I know this is making a large part of this PR irrelevant so I can understand if you are opposed to it.
I would be ok with working on this myself though my schedule makes that difficult atm.
There was a problem hiding this comment.
The main issue is that there may be even other things you might want to do, like rightclicking support etc. Adding everything to the base componet is bad design IMHO. Trying to add config options for every single thing someone may want to use is futile and makes the code a complexity nightmare. Stuff should be flexible and composable.
Also, besides that the DragHanlder approach works, it also is the right thing to use semantically IMHO. If the only issue is the drag threshold, we can improve the precision in the JS code or wait for Qt 5.15 as minimum requirement. I doubt QML will be finished before that point in time anyway.
There was a problem hiding this comment.
I see your point about composability... it just seems a bit wrong how we do it. The code of the baseclass is not very maintainable like this as well. Maybe we are just missing something. I'm fine with merging as is though I'd like to look into some alternative soon.
There was a problem hiding this comment.
I mean we can also factor out the Arc feature into a separate ArcKnob if you think that helps. But other than that the Knob.qml in Mixxx.Controls is pretty slim already.
Everything that is not in the Mixxx subdirectory is skin-local and doesn't need to be reusable. We can later check which parts may be useful and move it into the Mixxx directory.
There was a problem hiding this comment.
That would make sense architectually, but there is no practical reason for doing that right now.
|
Let's just merge as-is. No commits breaks the build or is broken or something. |
Split Sliders and Knobs into 3 components each: