Fix data flow#43
Conversation
|
|
||
| void WStarRating::mouseReleaseEvent(QMouseEvent* /*unused*/) { | ||
| emit ratingChanged(m_starRating.starCount()); | ||
| slotSetRating(m_visualStarRating.starCount()); |
There was a problem hiding this comment.
This feels wrong: we now store the stars without confirmation, as if it was an update from the owner/mediator. IMO it was correct before: emit the change (don't store it) and wait for the owner to confirm/reject/reset.
There was a problem hiding this comment.
Result of this implementation is that you can now set stars in the rating widget of an empty deck.
| m_starCount = starCount; | ||
| resetVisualRating(); | ||
| emit ratingChanged(starCount); |
There was a problem hiding this comment.
shouldn't this be updateVisualrating()?
Also, emitting the signal again with the value we may just received doesn't make sense to me. What for?
It seems emit is required here only because you use slotSetRating instead of emit in mouseReleaseEvent.
|
Thank you for the cleanup/renaming and the StarRating helpers. |
This comment was marked as outdated.
This comment was marked as outdated.
|
I revisited the code again, but couldn't come up with a consistent solution. You round trip argument doesn't hold, since the mouse release event explicitly sets a new value implicitly. Intermediate, temporary changes when hovering with the mouse over the widget should not be emitted. Giving up, not wasting my time with this convoluted legacy code. Probably better to wait for QML. Since I don't use this feature I cannot even test it properly. |
|
Okay then, thanks for your time anyway!
Exactly, just the clicked star count is emitted. Imagining the signals are very slow, on LeaveEvent the rating is reset to the previously confirmed value. Then the return signal from BaseTrackPlayer (or DlgTrackInfo) sets (or clears) the visual rating. |
3 participants
Only the actual rating drives the signal, not the (temporary) visual number of stars.