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.

Sign up for GitHub

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

Jump to bottom

fix glitch in Star rating #12582

Merged
merged 9 commits into from Jan 19, 2024
Merged

fix glitch in Star rating #12582

merged 9 commits into from Jan 19, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 15, 2024 •
edited by daschuer

  • Reset last confirmed value
    • on leave event and when the track menu is opened
      (even though it works for other editors, the latter requires a hack if the editor is also selected)
    • when pointer position leaves the rating area
      (if parent is wider than the rating, e.g. stretched library column)
  • draw focus border on first click

Bonus:

  • center rating horizontally
    (if parent is wider, else still left-aligned and can be truncated)

Fixes apply to both WStarRating (decks, Track Info) and the library's StarDelegate/StarEditor.
I moved redundant code to StarRating, and some more small improvements along the way.

star-rating-fix.webm

Fixes #12576 #7304

@ronso0 ronso0 mentioned this pull request Jan 15, 2024
Closed
daschuer
daschuer requested changes Jan 16, 2024
View reviewed changes
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.

Thank you for taking care for this long standing bug. I need to do also manual tests.

@ronso0 ronso0 marked this pull request as draft January 16, 2024 13:20
@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2024

Back to draft, I discovered other small issues.

@github-actions github-actions bot added the ui label Jan 16, 2024
@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2024

Okay, this is now a roundhouse kick as it fixes the mentioned issues for all star ratings (decks, Track Info, library).

And it's horizontally centered if the parent is wider that the rating.
Though I can revert that if its not desired.

And, no need to rush, this can easily go to 2.4.1

@ronso0 ronso0 marked this pull request as ready for review January 16, 2024 14:30
@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2024

For WStarRating testing purpose I amend a TEST commit that makes the deck rating in LateNight a bit wider.

daschuer
daschuer reviewed Jan 16, 2024
View reviewed changes
src/library/stardelegate.cpp Outdated
@@ -54,8 +54,9 @@ QWidget* StarDelegate::createEditor(QWidget* parent,
void StarDelegate::setEditorData(QWidget* editor,
const QModelIndex& index) const {
StarRating starRating = index.data().value<StarRating>();
int stars = index.model()->data(index, Qt::ToolTipRole).toInt();
Copy link
Member

Choose a reason for hiding this comment

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

Why not using starRating.starCount()?
I am afraid that the tooltip may change and breaker the code here.
If there is a reason, please add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, so i didn't fully understand the StarRating class. index.model()->data(Qt::EditRole | Qt::TooltipRol | Qt::UserRole + 2) should all return the QVariant(rawValue.toInt()). But it's always 0.

Anyway, you're right, starCount() is it.

@github-actions github-actions bot added the skins label Jan 16, 2024
@ronso0 ronso0 linked an issue Jan 16, 2024 that may be closed by this pull request
Stars hidden even on mouseover of empty space; and remain hidden whilst right click menu is open #12576
Closed
@ronso0
Copy link
Member Author

ronso0 commented Jan 17, 2024 •
edited

on leave event and when the track menu is opened
(even though it works for other editors, the latter requires a hack if the editor is also selected)

That's not working reliably, probably due varying order of events? Like, on right click, sometimes the editor receives a LeaveEvent, sometimes it doesn't (for the same cell, tracks view unchanged).
I'll do a brief test if eventFilter() does the trick, else I'll just revert it and we may look into fixing it later on.

@ronso0
StarEditor: reset to last confirmed value when pointer leaves star area
c9b04b9
@ronso0
StarEditor: center horizontally if cell is wider than the editor
86f43e1
@ronso0
WStarRating: apply fixes + centering from StarEditor
731b091 
move duplicate starAtPosition() to shared class StarRating
@ronso0
StarEditor: move mouse event handling to eventFilter()
dc978e1 
to simplify maintenance a bit.
Catch QEvent::ContextMenu, too, to reliably reset the rating on right-click.
@ronso0
StarEditor: merge renderHelper() into paintEvent()
051aaf3 
no need for this to be static. Less lines, easier to grasp.
@ronso0
Library delegates: move drawing border to TableItemDelegate
47b2430
@ronso0
StarEditor: draw 'focus' border on first select click ...
8e85dbd 
a helper for visual consistency, the editor isn't actually be focused.
@ronso0
Copy link
Member Author

ronso0 commented Jan 19, 2024 •
edited

Yup, with eventFilter() I couldn't make it fail anymore, but it remains a mystery to me, because it should work the same (the event is not consumed by eventFilter).
Works now 🤷‍♂️ and with eventFilter() it's a bit easier to maintain IMO.

Okay, this is now a roundhouse kick

This has grown beyond a quick fix, so no stress to squeeze this into 2.4.0

@ronso0 ronso0 mentioned this pull request Jan 19, 2024
Merged
daschuer
daschuer approved these changes Jan 19, 2024
View reviewed changes
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.

Works perfectly and LGTM. Thank you very much for fixing alsohttps://github.com//issues/7304 form 2014

@daschuer daschuer merged commit 9ee9f53 into mixxxdj:2.4 Jan 19, 2024
13 checks passed
@daschuer daschuer added this to the 2.4.0 milestone Jan 19, 2024
@daschuer
Copy link
Member

daschuer commented Jan 19, 2024 •
edited

Ups, the TEST commit went again in. Sorry.
Fixed in dc22fd5

@ronso0
Copy link
Member Author

ronso0 commented Jan 19, 2024

Great, thanks for testing!

The test commit, forgot to request changes myself, I'll probably go with the Draft state next time.

@ronso0 ronso0 deleted the stars-leave-reset-fix branch January 19, 2024 08:42
@ronso0 ronso0 mentioned this pull request Jan 19, 2024
Merged
@ronso0
Copy link
Member Author

ronso0 commented Jan 20, 2024 •
edited

FYI I pushed three fix/polish commits directly to 2.4
5401f6b
ab2d9b6
923abdf

now trying to merge 2.4 into main ✔️

edit
and another one 5b1f477
Puuuh, now this is polished.

@ronso0 ronso0 mentioned this pull request Jan 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
library skins ui
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

Stars hidden even on mouseover of empty space; and remain hidden whilst right click menu is open
2 participants
@ronso0 @daschuer