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 library selection, save header configuration per crate/playlist #3063

Closed

poelzi wants to merge 36 commits into mixxxdj:main from poelzi:fix_selection

Contributor

poelzi commented Aug 31, 2020 •

edited by ronso0

Loading

This branch fixes multiple selection issues in the library.

When switching between library feature, the table may show a entry selected but currentIndex will not reflect the selection anymore. Move selection now actually queries the model selection and uses those row values for calculating the next entry.
Wrap around when using the move COs. There is no benefit on being stuck at the end/top of the list.
Use a signal to initiate a state save on the view.
Use a key provided by the model for associating the model index with a scrollposition. This way, a model can be reused for different views and trigger a state save.

edit (ronso0) fixes https://bugs.launchpad.net/mixxx/+bug/1808472
https://bugs.launchpad.net/mixxx/+bug/1448400

Member

ronso0 commented Aug 31, 2020

Wrap around when using the move COs. There is no benefit on being stuck at the end/top of the list.

I'm not sure abou this. Sure with [Libary],MoveVertical you'll notice soon enough that you approach the end of the table.
But with [Libary],ScrollVertical wrapping will cause irritation IMO.

Member

ronso0 commented Aug 31, 2020

Oh, I see this only affects the Select[Prev/Next]Track controls.

ronso0 reviewed

View reviewed changes

src/widget/wlibrarytableview.cpp Outdated Show resolved Hide resolved

ronso0 reviewed

View reviewed changes

src/widget/wlibrarytableview.cpp Outdated Show resolved Hide resolved

ronso0 reviewed

View reviewed changes

src/widget/wlibrarytableview.cpp Outdated Show resolved Hide resolved

ronso0 reviewed

View reviewed changes

src/widget/wlibrarytableview.cpp Outdated Show resolved Hide resolved

Member

ronso0 commented Aug 31, 2020

Oh, I see this only affects the Select[Prev/Next]Track controls.

..which don't require the table to be :focused. So it doesn't replace my fix for keyboard and ..MoveVertical controls in #3049 , and afaict can tell it doesn't conflict either, right?

Be-ing changed the title ~~[WIP] Fix selection~~ [WIP] Fix library selection

poelzi mentioned this pull request

Tracks table selection fix for [Library],MoveVertical controls #3049

Merged

Member

ronso0 commented Sep 1, 2020 •

edited

Loading

For further improving the library navigation (in another PR):
as mentioned earlier (elsewher) It would be nice if [Library], MoveVertical would use moveSelection() (instead of key press emulation) if the tracks table is focused / was the last focused library widget. That way both [Playlist],SelectTrack and [Library], MoveVertical would benefit from the fixes introduced here.

ronso0 mentioned this pull request

WTrackTableView: keep selected tracks in view after sorting #3092

Merged

Be-ing changed the base branch from master to main

October 23, 2020 23:11

poelzi added 3 commits

December 8, 2020 03:55


          Query selectionModel for selected rows

cc585bd

currentIndex is not necesarry in sync with the selected rows the user
sees. Query the selectionModel and using it results fixes those problems.

Also implement wrap-around behaviour.

Example:
Go to Library Tracks, select track. Go to AutoDJ select track,
go back to library. Now use moveSelection controlls will always
jump back to the first row because currentIndex returns invalid index.

The new implementation fixes this bug.


          Use signal to initiate state save

b161ee9

Use a signal in Library or LibraryFeature objects to
save the scroll position of the trackmodel currently shown.
The key to store the scroll position is no longer defined by the
model pointer but a value provided by the model itself.
This way, the same model can be reused for different views like
the crates feature does.


          Add save/restore functionality for library views

9a5f9c8

* Allow LibraryViews to save/restore their state like
scroll position, selection and currentIndex.
* Add state cache to baseplaylist
* Add special modelKey implementations for Playlists, Crates, Computer, Proxy
* Selection is now search aware and saved different pointers for different searches
* Clear Cache regularly

poelzi force-pushed the fix_selection branch from dcc9ceb to 9a5f9c8 Compare

December 8, 2020 03:18

github-actions bot added code quality library ui labels

poelzi changed the base branch from main to 2.3

December 8, 2020 03:20

Contributor Author

poelzi commented Dec 8, 2020

@ronso0 would like you give this version a test ?
@uklotzde is this still a candidate for 2.3 after some polishing or should I rebase on main ?

Unfortunately the small changes from the beginning got a bit larger but otherwise I could not implement it in a generic and working way for all the features. Now the selection of tracks, scrollposition and current Index is popery conserved between switches of different subsystems. Each playlist/crate/folder + search query has it's own state and will be garbage collected after LRU schema. (combined with the search combobox this is top).

Contributor

Be-ing commented Dec 8, 2020

Did you rebase on 2.3? I don't see the recently added GitHub Actions checks.

Member

ronso0 commented Dec 8, 2020

I guess I can test that in a few days with my controller, latest by the end of the week.
If it reliably saves the scroll position and selection for each feature this is a huge UX improvement and should definitely go into 2.3! Even more so if the navigation improvements also work for [Lib],Move.. controls (or if those can be mapped to the [Playlist] navi controls internally).

poelzi added 2 commits

December 8, 2020 11:09


          Add state save/restore to recordings/analyisis/missing features

dacc6b0


          Save/Restore support for externalplaylistmodel

6440f9e

Contributor Author

poelzi commented Dec 8, 2020

Analyze/Recordings/Missing Tracks now work as well.
I would not test externalplaylist based features because rythmbox feature crashes here and I don't have any other tools to test.

poelzi marked this pull request as ready for review

December 8, 2020 11:02

ronso0 changed the title ~~[WIP] Fix library selection~~ Fix library selection

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

4966ec3

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

53b3a45

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

89671ee

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 removed this from the 2.4.0 milestone

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

853baef

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

2544b88

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

c870d12

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

5daaf28

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

0c993a3

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

20e27fe

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

12798bb

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added a commit to ronso0/mixxx that referenced this pull request


          Library: allow views to save/restore the current state

96240ac

library views store states in a QCache in WTrackTableView incl.
 * track selection
 * current index (focused track)
 * scroll positions, horizontal + vertical
with a unique key composed of
 * feature (playlist/crate/browse ...)
 * child item (playlistId/crateId/directory ...)
 * current search term

Credits for this and the required commits go to
https://github.com/poelzi <git@poelzi.org>
I just boiled down some picks from stalled mixxxdj#3063

ronso0 added the stale label

ronso0 changed the title ~~Fix library selection~~ Fix library selection, save header configuration per crate/playlist

ronso0 mentioned this pull request

Library track selection tweaks #4536

Merged

Member

ronso0 commented Dec 23, 2021

marking this as draft.
@ywwg Maybe you want to take a look a the commits saving the table header per view.

ronso0 marked this pull request as draft

December 23, 2021 14:21

github-actions bot removed the stale label

Member

ywwg commented Dec 26, 2021

yeah I'll take a look

github-actions bot commented Mar 27, 2022

This PR is marked as stale because it has been open 90 days with no activity.

github-actions bot added the stale label

This was referenced Aug 22, 2022

Scroll position is lost when switching between playlists #7984

Closed

center selected track in library table #8091

Open

store selected table item index when switching library features #9547

Closed

crates should remember their own sort method #9606

Open

Crate/playlist specific column layouts? #10281

Closed

ronso0 mentioned this pull request

Tracks table: try to restore selection position after track removal #11196

Merged

Member

ronso0 commented Feb 8, 2023

Closing this stale PR now since the track view changes have been adopted by now, due to the enormous amount of conflicts and to clean up the list of open PRs.

ronso0 closed this

ronso0 mentioned this pull request

same sort-settings are used for all crates / playlists,, can it be list independent please? #12763

Open

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

Labels

code quality library stale ui