Refactor ValueTransformer and WBaseWidget
#13853
Conversation
Swiftb0y
force-pushed
the
refactor/valuetransformer
branch
from
edb85f9
to
3974c37
Compare
src/skin/legacy/legacyskinparser.cpp
Outdated
| if (directionOption & ControlParameterWidgetConnection::DIR_TO_WIDGET) { | ||
| pLastLeftOrNoButtonConnection = pConnection; | ||
| pLastLeftOrNoButtonConnection = pConnection.get(); |
There was a problem hiding this comment.
This requires at least a comment. Using a unique pointer on one hand an then passing it as raw pointer to an object that stores it for the whole lifetime looks wrong even if it is the same object. Maybe we can find a way to move the logic into the widget, that we have only one call?
There was a problem hiding this comment.
it only stores it for the lifetime of the function. Its just an artifact that the code wants to use this at the end of the function again. I can easily move that part into the branch here so it acts on the unique_ptr directly and we don't need to retain it until later.
There was a problem hiding this comment.
I think I misunderstood. Instead you meant to move the .get() into the class, right? I tried to implement that in 2c78d23, wdyt?
There was a problem hiding this comment.
That should work, however the name
pWidget->setDisplayConnection() is confusing, because it also adds and keeps the connection. Only the last one becomes than the display connection. addConnectionAndUseForDisplay() or such.
The alternative would be to integrate the whole logic in addConnection() or add a new call that calculated the display connection after all connections are added.
There was a problem hiding this comment.
well I initially changed it to addDisplayConnection but reverted it again since that implies that you could add an arbitrary amount. I'm not sure how making this part of addConnection would solve the issue exactly. Do you have a more concrete suggestion? I feel like it should suffice to simply document above the function that only the last one is used as the DisplayConnection. I don't think the naming here is particularly important as the function is not used very often (only legacyskinparser and whotcuebutton).
There was a problem hiding this comment.
Okay, After looking at this I'm unsure how good of an idea it is.
My understanding is the following: The fact that last connection is automatically the displayConnection is (as far as I can tell by the comment) legacy behavior that is only there to provide stability for skins. WBaseWidget is however not intrinsically coupled to the skins thus moving this hack into the class mixes concerns. Moreover, the legacy behavior would now be backed into the class, so opting out of it would be impossible. I think that is a worse tradeoff than a bad or unwieldy long name. Wdyt?
There was a problem hiding this comment.
The comment about "Legacy" means "Keep it like that, to not introduce regressions."
The related review can be found here: #146 (diff)
@rryan suggested to explicit introduce a <Display> connection. But this has never been introduced. Since no one ask to have that in 10 years and we now have a dedicated hotcue button anyway, the legacy behavior is just fine.
We may consider to adjust the comment accordingly. I don't mind if you go for a lengthy function name or for the widget move. The Widget move will match a similar call in the Hotcue button:
mixxx/src/widget/whotcuebutton.cpp
Line 85 in 028e3ff
There was a problem hiding this comment.
so to clarify, you are okay with a better comment or name?
There was a problem hiding this comment.
The core issue is that the current name is misleading and should be changed. The issue is that becoming a display connection is only a side effect of the function call. Currently it looks like it is it's main purpose. If you don't find a perfect name, a different name AND a comment is fine.
There was a problem hiding this comment.
I changed the name and added a comment. Please review. If you don't like it, please make a concrete suggestion.
This is prep for the follow up commit to make it lighter.
Swiftb0y
force-pushed
the
refactor/valuetransformer
branch
2 times, most recently
from
c9979c1
to
afb744c
Compare
This ensures that if a connection is a DisplayConnection then it is also added to the other connections. This improves memory safety as well as usability because it makes it impossible to accidentally set a connection as DisplayConnection that is not tracked by the WBaseWidget.
Swiftb0y
force-pushed
the
refactor/valuetransformer
branch
2 times, most recently
from
b31a99d
to
26ca2b0
Compare
Swiftb0y
force-pushed
the
refactor/valuetransformer
branch
from
26ca2b0
to
abf5fe6
Compare
extracted a commit from #13687 and fixed a TODO there (which in turn led to far bigger changes again).