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

Sidebar item selection fix #4193

Merged
merged 4 commits into from
Aug 17, 2021
Merged

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Aug 11, 2021

Fixes some issues when navigating the sidebar with Up/Down keys, [Library],Move..., [Playlist],Select... controls (emulated key presses)

part1
fixes https://bugs.launchpad.net/mixxx/+bug/1939082

Keys can NOT select the pevious/next playlist (Playlists, History) but make the selection jump to Tracks (and sometimes apply the key press after that).

Looks like #2996 added the redundant and failing code (git bisect points to a8a93f0), though I didn't trace what exactly causes the fail (redundant sidebar item activation with invalid index)

En passant I added some debug assertions.

part 2
After creating a playlist/crate Up/Down keys would make the selection jump to the Tracks.
Fixed by adding setCurrentIndex() to WLibrarySidebar::selectIndex and WLibrarySidebar::selectChildIndex.
edit also prevents the jump when moving away from active external features' root item (Serato, Rekordbox ...)

part 3
Remove a crate and the selection jumps to the Crates root item.
Fix: Like with playlists, before deleting a crate, store its neighbour's Id to somewhat restore the selection afterwards, or rather scroll position.

Remove unnecessary & conflicting code.
Previously Up/Down key presses in the sidebar would NOT select the
pevious/next playlist (Playlists, History) but make the selection
jump to Tracks (and sometimes apply the key press after that).
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 11, 2021
@coveralls
Copy link

coveralls commented Aug 11, 2021

Pull Request Test Coverage Report for Build 1128717031

  • 0 of 27 (0.0%) changed or added relevant lines in 3 files are covered.
  • 133 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.005%) to 25.991%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/library/trackset/baseplaylistfeature.cpp 0 2 0.0%
src/widget/wlibrarysidebar.cpp 0 7 0.0%
src/library/trackset/crate/cratefeature.cpp 0 18 0.0%
Files with Coverage Reduction New Missed Lines %
src/library/trackset/baseplaylistfeature.cpp 1 0%
src/library/trackset/crate/cratefeature.cpp 2 0%
src/vinylcontrol/vinylcontrolxwax.cpp 130 0%
Totals Coverage Status
Change from base Build 1125408407: -0.005%
Covered Lines: 20015
Relevant Lines: 77006

💛 - Coveralls

@ronso0
Copy link
Member Author

ronso0 commented Aug 11, 2021

though I didn't trace what exactly causes the fail

Any select prev/next sidebar item command is either an actual or emulated key press Up/Down.
That is captured by the sidebar model's clickTimer, which then invokes pFeature->activateChild(index); =actual click (without timeout), so the additonal featureSelect and m_pSidebarWidget->selectChildIndex(index) stuff is entirely superflous (and wrong since it is invoked with an invalid index).

I'm puzzled that no one using main noticed the bug since the History tweaks were merged. Obviously we weren't using controllers for navigating the playlists or history.

src/library/trackset/crate/cratefeature.cpp Outdated Show resolved Hide resolved
src/library/trackset/crate/cratefeature.cpp Outdated Show resolved Hide resolved
src/library/trackset/baseplaylistfeature.cpp Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Aug 13, 2021

I fixed another bug: after creating, deleting or renaming a playlist/crate the selection also jumps unexpedtedly. This is due to missing setCurrentIndex() in WLibrarySidebar::selectIndex and WLibrarySidebar::selectChildIndex. I suppose this was the root cause for redundant featureSelect() to fail.

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.

After a manual test we can merge.

@ronso0
Copy link
Member Author

ronso0 commented Aug 13, 2021

I'm adding another selection fix right now, see the TODO
https://github.com/mixxxdj/mixxx/pull/4193/files#diff-784192df9ca0c20864aeee323d8c9f51193221742b69a1faf71b0ecf0bc5559bR398-R399

If you don't mind Iet's include that,too

@ronso0
Copy link
Member Author

ronso0 commented Aug 13, 2021

@uklotzde Please take a look at the last one.
The sidebar selection should now be usable after any crate or playlist operation.

@ronso0 ronso0 changed the title Sidebar playlist selection fix Sidebar item selection fix Aug 13, 2021
@Swiftb0y
Copy link
Member

I can confirm that part 1 is fixed. I had trouble reproducing part 2 & 3 on main so I can't comment on those.

@ronso0
Copy link
Member Author

ronso0 commented Aug 14, 2021

@Swiftb0y I updated the 1st post with steps how to reproduce. Both keyboard and Library/Playlist controls are affected, except for part3 where the selection always jumps.

@Swiftb0y
Copy link
Member

I'm sorry, my controller seems to be broken right now so I have no way to test this anymore.

@ronso0
Copy link
Member Author

ronso0 commented Aug 17, 2021

Library, Move.../Playlist,Select... controls are just emulated key presses in the sidebar widget, so a keyboard is sufficient for testing. that's also how I noticed the bugs in the first place.

@Swiftb0y
Copy link
Member

Thanks, I thought these issues were only present when navigating via a controller because I had trouble reproducing part 2 and 3.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Fixes confirmed to be working. Merge?

@ronso0
Copy link
Member Author

ronso0 commented Aug 17, 2021

Fixes confirmed to be working. Merge?

@uklotzde?

@Holzhaus
Copy link
Member

Uwe already approved this PR. I'll hit merge.

@Holzhaus Holzhaus merged commit 54a68c4 into mixxxdj:main Aug 17, 2021
@Swiftb0y
Copy link
Member

Uwe already approved this PR.

That was before 185e373 got committed.

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.

5 participants