WLibraryTableView: Refocus or reselect on FocusIn events (continued)

[ronso0]

based on #3086

fixes unintended side-effects caused by de62420 from #3049

[ronso0]

unfortunately 3ec08c1 created another issue after switching features:

• the previous selection is restored (okay)
• Tab to focus tracks table = selection shrinks to first item (okay)
• no table item has focus = impossible to move selection with Up/Down (oooh!)
• press Right = left-most cell in selected row get focused = Up/Down works again

the last commit fixes this.

[ronso0]

I'm not at all sure if all the above checks are necessary, but this last line fixes the issue described above and allows arrow key navigation again.

[ronso0]

Please test with controllers and keyboard!

if there are no issues I'd like to squash this to the essence and merge asap.

[ronso0]

@uklotzde It would be nice if you could have a look at this when you have some spare minutes..

[MarcPlace]

Just compiled and tested commit 9828a41 (Gentoo Linux 64-Bit). No issues found on Denon MC3000 and MC7000 controllers. Also the bug mentioned in lp:1894901 seems to be fixed now. LGTM. Thanks for fixing this, ronso0.

unfortunately 3ec08c1 created another issue after switching features

I was trying to reproduce this issue here using commit 3ec08c1 but with no luck. Maybe I did something wrong?

[ronso0]

* the previous selection is restored (okay)

* Tab to focus tracks table = selection shrinks to first item (okay)

* no table item has focus = impossible to move selection with Up/Down (oooh!)

* press Right = left-most cell in selected row get focused = Up/Down works again

I tested with a keyboard only and 458aad1 to check wether there's a cell focused.
You tested with a controller? If so, which controls do you use for moving the selection?

[MarcPlace]

@ronso0 AFAIR the MC7000 uses "MoveVertical" / "ScrollVertical". I'm not quite sure what the MC3000 uses for moving the selection. Maybe the more deprecated "SelectNextTrack" / "SelectPrevTrack"? Nevertheless, I wasn't able to reproduce this issue here.

[ronso0]

SelectNextTrack / SelectPrevTrack are out of question anyway: they don't require the table to have focus and it uses the selection directly that was restored previously already.

Ready for merge if all addressed issues are fixed.

[daschuer]

It works as probably intended, so it is mergable from this point of view.
However this is only a band aid for the underlying issue that the selection context is invalidated in Crates and Playlist when out of focus.

If you select a track in a Playlist, select a track in the Track's list and than move back the first track in the playlist is selected.
If you go back to Tracks the old track is selected.

I am unsure if it is feasible to store the selected track in the playlist. Maybe for at least one?

What do you think?

[daschuer]

When you switch between two long playlists, the scollbar position remains.
Which is somehow senseless for the second playlist, but at least it preserves the scrollbar position when coming from the track list.
I think the same pattern is true for the selection.

Looking at

void WLibraryTableView::saveVScrollBarPos(TrackModel* key){
    m_vScrollBarPosValues[key] = verticalScrollBar()->value();
}

The current behavior is probably a bug, because the address of the TrackModel does not change when changing the playlist.
Maybe it is an unnoticed regression?
I think we need to find something different as a key.

Like a hash code created from "playlist_path" in or such.
void BaseExternalPlaylistModel::setPlaylist(QString playlist_path) {

Once this is fixed, we can store the selection in the same way.

[uklotzde]

LGTM. The remaining issues cannot address here.

[ronso0]

When you switch between two long playlists, the scollbar position remains.
Which is somehow senseless for the second playlist, but at least it preserves the scrollbar position when coming from the track list.
I think the same pattern is true for the selection.

Exactly. That was the issue I attempted to fix with #2378 which unfortunately doesn't save the state/selection for multiple Crates but just for each library feature instead. Building upon #2046 still seems viable to me.