Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WStarRating: don't update track directly, use signals/slots #11565

Merged
merged 5 commits into from
Jul 2, 2023

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 17, 2023

fixes #11540

changes to the star rating are now handled via change signals, like with the QLineEdits

edit: instead of working around the symptom, as per @uklotzde's suggestion, this PR now adds slots and signals for track rating. Now it's consistent with other track properties and WStarRating now much smaller.

Note: updating the rating (or any other track property) elsewhere emits the trackCganged signal and will therefore still wipe pending changes in the properties dialog. This is not a bug.

@@ -166,7 +174,11 @@ void WStarRating::mouseReleaseEvent(QMouseEvent* /*unused*/) {
return;
}

m_pCurrentTrack->setRating(m_starRating.starCount());
m_currRating = m_starRating.starCount();
if (m_updateTrack) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing these issues. QML will hopefully help to prevent those mistakes in the (far away) future.

The widget should never directly update the track state on its own. Instead the receiver of the signal could do that if desired or otherwise reject the proposed new value by resetting the widget -> unidirectional data flow.

Offering both options is confusing and not maintainable. But I am afraid that trying to avoid this mixed behavior could open another Pandora's Box.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the widget would only offer a slot for setting the rating (input, write-only) and emit a signal when the rating changed (output, read-only). Maybe a getter for reading the current rating on demand. No track pointer, no implicit context management.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you're saying, and I agree that WStarRating becomes more complex and bit harder to maintain. Looks like this can be changed without too much hazzle, with a few changes to Track and BaseTrackPlayer(Impl) (and mediators like LegacySkinParser, and DlgTrackinfo, of course), and WStarRating would be quite clean then.

I think I'll take a stab at this for 2.4 beta, though it's out of scope for this quick bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then try to add the basic building blocks to the current design and (important!) TODO comments, i.e. to remove m_pCurrentTrack. Otherwise the direction is unclear.

@ronso0
Copy link
Member Author

ronso0 commented May 18, 2023

@uklotzde That was easier than I thought, though tedious.
I went back to square one and reverted the previous commit.
Please take a look.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to hear that this was easier than expected. The refactoring avoids to pile up more legacy code.

src/widget/wstarrating.h Outdated Show resolved Hide resolved
src/mixer/basetrackplayer.h Outdated Show resolved Hide resolved
m_starRating.setStarCount(star);
update();
m_pCurrentTrack->setRating(star);
emit ratingChangeRequest(star);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signal should only be emitted if the value has actually changed to prevent cycles.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. This just handles the group,stars_up CO and changes the current value. The cycle prevention is in Track::setRating.

However, I changed this method to work with m_currRating, i.e. the stored value delivered by Track::ratingUpdated, not the visual state (which avoids confusion in the unlikely case a user hovers the rating and uses the CO).

}

void WStarRating::fillDebugTooltip(QStringList* debug) {
WWidget::fillDebugTooltip(debug);

QString currentRating = "-";
QString currentRating;
currentRating.setNum(m_currRating);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between m_currRating and m_starRating? The widget should not store any redundant state that could get inconsistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_starRating.starCount() is the current value of StarRating, i.e. what you see (for example while hovering).
m_currRating is the confirmed value of the track / pending track record value in DlgTrackInfo, after LeaveEvent.

Copy link
Contributor

@uklotzde uklotzde May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then m_currRating is redundant. The widget should maintain only a single value and don't care about the state of its clients or subscribers.

Copy link
Member Author

@ronso0 ronso0 May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then m_currRating is redundant.

Redundant with what?
The current value of StarRating (separate paint class!) (starCount()) just reflects the count of currently painted stars, be it final (clicked, not necessarily accepted by Track or DlgTrackInfo) or temporary (hovering, mouse moving).

Only m_currRating is storing the applied / real value and we need that for the COs and to restore the real value when leaving the widget (after mouse move, not clicked).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check it later. Only viewed the diff, not the whole code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevertheless my comments reveal that meaning and purpose of those member variables is unclear on first sight.

@@ -436,6 +436,7 @@ class Track : public QObject {
// adjusted in the opposite direction to compensate (no audible change).
void replayGainAdjusted(const mixxx::ReplayGain&);
void colorUpdated(const mixxx::RgbColor::optional_t& color);
void ratingUpdated(int rating);
Copy link
Contributor

@uklotzde uklotzde May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the neutral and agnostic changed terminology. That would be inconsistent with the existing signals, but we could add a TODO to rename those later.

@ronso0 ronso0 changed the title DlgTrackInfo: don't update from track when user changes star rating WStarRating: don't update track directly, use signals/slots May 18, 2023
@ronso0
Copy link
Member Author

ronso0 commented May 18, 2023

pushed some fixups which I'd like to squash if they're okay.

Comment on lines -178 to -180
if (m_pCurrentTrack) {
currentRating.setNum(m_pCurrentTrack->getRating());
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showing - / 5 in the debug tooltip with no track would be the only reason to track the "track loaded" state. not worth the effort IMO

@ronso0
Copy link
Member Author

ronso0 commented May 18, 2023

pushed some fixups which I'd like to squash if they're okay.

setting this to draft just to avoid accidental merge.

@uklotzde
Copy link
Contributor

pushed some fixups which I'd like to squash if they're okay.

Sure. I will take a final look when done.

@ronso0
Copy link
Member Author

ronso0 commented May 18, 2023

Go ahead, I'm done, I just addressed your review comments and packed the changes in small chunks for easy review.
Added a missing slotSetRating to DlgTrackInfo. All performs as expected.

@uklotzde
Copy link
Contributor

Stopping my review here. I wasn't able to fully understand what the code is supposed to do, already wasted too much time. If it works, then feel free to merge it. This has to be rewritten for QML anyway.

Previously, WStarRating updated the track directly. Undesired side effect was that
changing the rating in DlgTrackInfo causes Track::changed() to be emitted which in turn
would clear other pending changes in the current dialog.

Note that changing the rating or any other track property elsewhere (library or
another DlgTrackInfo instance still causes track metadata to be reloaded.
@ronso0
Copy link
Member Author

ronso0 commented May 20, 2023

@uklotzde Thanks for your time and ronso0#43.
Your renaming and cleanup commits are valuable so I'll pick them, I'll just revert (ammend) that one small thing I spotted.

It's working fine now: rating is synced if updated anywhere (library, decks, DlgTrackInfo), and (still) it's not possible to set the rating in an empty deck (or empty DlgTrackInfo, if that could be displayed).

@ronso0 ronso0 force-pushed the star-dlgtrackinfo branch 2 times, most recently from 5a7f496 to 169189a Compare May 20, 2023 13:07
@daschuer
Copy link
Member

@ronso0 do you still plan to rebase this or does it put to much work on us?

1 similar comment
@daschuer
Copy link
Member

@ronso0 do you still plan to rebase this or does it put to much work on us?

@ronso0
Copy link
Member Author

ronso0 commented Jun 13, 2023

Thanks for the reminder!
Yes, I can rebase onto 2.3

@ronso0
Copy link
Member Author

ronso0 commented Jun 13, 2023

Meeh, too many conflicts IMO. Resolving those now and when merging to 2.4 is a PITA.

As far as I can tell, unitl now there are only minor issues reported for 2.3 (fixed since 2.3.5) that don't justify another bugfix release, so let's merge to 2.4 and save the time for more important stuff.
What do you think?

@uklotzde
Copy link
Contributor

I also don't recommend rebasing. Too much has changed since then.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and works good, Thank you.

@daschuer daschuer merged commit b7911fe into mixxxdj:2.4 Jul 2, 2023
@ronso0
Copy link
Member Author

ronso0 commented Jul 2, 2023

Thanks for reviewing!

@ronso0 ronso0 deleted the star-dlgtrackinfo branch July 2, 2023 12:25
Q_UNUSED(trackId);
updateRatingFromTrack();
m_starCount = starCount;
updateVisualRating(starCount);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to add emit ratingChanged(m_starCount) in order to make the _up/_down controls to also update the track value
pushed a fix 0550e07

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating Star rating in track properties clears other unsaved properties
3 participants