Fix library track selection, save model state #4177

ronso0 · 2021-08-04T23:04:47Z

This is an attempt to cherry-pick commits from @poelzi's #3063 to fix the library track selection only
(save/restore selection, focus and scroll position per model view).

I guess this will take some time due to short free time, some merge conflicts, building each commit etc.

TODO

Browse/computer: store selected directory
fix inconsistent save/restore when activating active sidebar item
make use of noSearch parameter

coveralls · 2021-08-04T23:53:02Z

Pull Request Test Coverage Report for Build 1137229790

0 of 189 (0.0%) changed or added relevant lines in 25 files are covered.
27 unchanged lines in 12 files lost coverage.
Overall coverage decreased (-0.06%) to 25.909%

Changes Missing Coverage	Changed/Added Lines	%
src/library/recording/recordingfeature.cpp	1	0.0%
src/library/baseexternalplaylistmodel.cpp	2	0.0%
src/library/mixxxlibraryfeature.cpp	2	0.0%
src/library/proxytrackmodel.cpp	2	0.0%
src/library/recording/dlgrecording.cpp	2	0.0%
src/library/rhythmbox/rhythmboxfeature.cpp	2	0.0%
src/library/serato/seratofeature.cpp	2	0.0%
src/library/trackset/crate/cratefeature.cpp	2	0.0%
src/library/browse/browsefeature.cpp	3	0.0%
src/library/traktor/traktorfeature.cpp	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/engine/enginevumeter.cpp	1	90.24%
src/library/baseexternalplaylistmodel.cpp	1	0%
src/library/browse/browsetablemodel.cpp	1	0%
src/library/library.cpp	1	0%
src/library/recording/recordingfeature.cpp	1	3.33%
src/library/trackset/baseplaylistfeature.cpp	1	0%
src/library/trackset/crate/cratetablemodel.cpp	1	0%
src/widget/wlibrary.cpp	1	0%
src/library/browse/browsefeature.cpp	2	0.42%
src/widget/wlibrarytableview.cpp	3	0%

Totals
Change from base Build 1137013267:	-0.06%
Covered Lines:	19985
Relevant Lines:	77136

💛 - Coveralls

ronso0 · 2021-08-07T01:27:10Z

Alrighty, I think this is now in a good shape.
ToDos are in the 1st post, primarily restore functionality to re/store the selected Browse directory.
Anyone who has reviewed #3063 is invited to take a look at this : )

While debugging a crash I noticed that restoreTrackModelState
a is called on already selected models
~~b is called twice~~ fixed for Tracks

Another issue (needs to be double-checked, also with main):
. select any track in Tracks
. go to AutoDJ, fill the list so it has more tracks than fit in the view, scroll down, select some track near the bottom
. go to any crate that has more tracks than fit in the view
= initial view is scrolled down

Apart from that it works great!

@ywwg Feel free to build the header functions on top (sort order, individual headers etc.)

ronso0 · 2021-08-07T01:35:41Z

right now the commit history still contains a lot of back and forth experiments.
I'll wrap my head around how to merge those to the essentials. If desired...

src/widget/wlibrary.cpp

src/library/playlisttablemodel.h

src/library/baseexternalplaylistmodel.h

src/library/trackmodel.h

src/widget/wlibrarytableview.cpp

src/library/baseexternalplaylistmodel.h

ronso0 · 2021-08-09T00:29:30Z

I'm splitting the overall final changes into easily digestible bits, so don't start reviewing, yet.

uklotzde · 2021-10-24T21:44:51Z

Please fix the conflicts for testing.

daschuer

Thank you for picking this up.

I have added som suggestions, not found real issues. I will do also a manual test soon.

src/library/baseexternalplaylistmodel.cpp

daschuer · 2021-10-28T08:22:03Z

src/library/basesqltablemodel.cpp

+    if (noSearch) {
+        return QStringLiteral("table:") + m_tableName;
+    }
+    return QStringLiteral("table:") + m_tableName +


The same here.
We also may consider to move this function into the base class and fetch only the prefix via an overloaded virtual function.

yes, the modelKey() function is identical for most models, except Browse, Crate, BaseExternalPlaylistModel and Proxy..
So I wonder if it's worth the effort.
Fo rmy taste it's optimized sufficiently if the model prefix becomes a const in the namespaces.

src/library/browse/browsetablemodel.cpp

daschuer · 2021-10-28T08:51:28Z

src/widget/wlibrarytableview.cpp

+    VERIFY_OR_DEBUG_ASSERT(!key.isEmpty()) {
+        return;
+    }
+    ModelState* state = m_modelStateCache.take(key);


I think we can use .object() her which keeps the state object int the cache but moves it to the top.
If it returns null we can immediately insert the new element.
This avoids to move the ownership of the created object around and is probably a bit faster.

well, the original commit 49d263e is tagged "Make QCache usage safe against concurrent deletion", though I don't see when that would happen.
I guess @poelzi can explain his motivation for this change.

This one is open. I think we should use the normal pattern for QCache here unless we can remember the reason and put that into the code.

It follow the logic that the Object in the QCache is only ever owned by one owner, rust style. It is either in the update handler or in the QCache object, but it is never in the update handler while the QCache thinks it needs to drop this object.

See the warning in the QCache docs, if you just get a pointer:
Warning: The returned object is owned by QCache and may be deleted at any time.
If I remember correctly, I could provoke a crash by very fast switching between different models, I was able to cause a crash.

okay. could you add a review/code suggestion that adds a comment with this explanation?

The Warning applies applies, if you store the pointer in a member variable, or if we use the cache from multiple threads.
The QCrash has no magic to do something at laterally any time. It can do something at any call.
All this does not apply here. So I am not able to write a reasonable comment. I assume the crashes was cased by something else.

However since this is only a minor issue, and not on a performance critic path, I think we should keep this tested variant, but not add a comment with probably wrong assumptions.

daschuer · 2021-10-31T15:07:50Z

I have just give this a brief test. I think it works like it should from the code, but it does not feel correct.

Let's say I select a track "Nickelblack".
Than I search for "ABBA" and select an ABBA track.
Than I want to check which track was released at the same time.
Sort for date
Remove the search filter
"Nickelblack" is selected :-(

This not a regression here, but I like to know if you can confirm it.
Do we need also consider other use cases?

ronso0 · 2021-10-31T17:50:46Z

can't test this right now, but yeah I think that's the current behavior of onSearch() and @poelzi addressed the issue with 7b9e405
I didn't pick that commit because I experienced issues with it which I didn't debug yet.
and I didn't wrap my head around the actual query parser functions tbh.

poelzi · 2021-11-01T12:07:49Z

yes. with that commit it is decided if a search term is a reduced search, aka, is less sepecific. In this case, the selected track will be in the selection and therefor should keep it's selection. This felt correct when testing.

ronso0 · 2021-11-03T23:15:46Z

If reviews of the current state don't bring any issues to light I can continue to pick more of @poelzi's commits to improve the situation further. Though, I'll have to delve into the search parser stuff to somewhat know what I'm doing, and debug issues I ran into during previous attempts.
Or we do that in another PR. I don't mind.

daschuer · 2021-11-04T07:36:55Z

I would prefer to merge this soon and go in smaller steps. This has no regression to main so it should be possible.

The QCache usage is open and the CI failure.

ronso0 · 2021-11-04T13:02:09Z

re CI failure
why does glang-format scan unchanged lines? it didn't do so when run locally, and it didn't complain about the missing line breaks in the newly added lines.
Also I don't think the line break is helpful. the one-line, empty virtual functions are much easier to read without it.

Be-ing · 2021-11-13T23:39:50Z

Is there anything left to do here?

ronso0 · 2021-11-15T00:05:52Z

adding a comment explaining why the state is taken from the QHash, populated and re-inserted afterwards.
#4177 (comment)
@daschuer You noticed anything else?

ronso0 · 2021-11-20T00:06:13Z

ping @daschuer

daschuer

LGTM,
Thank you @ronso0 for picking it up.

daschuer · 2021-11-20T01:17:01Z

src/widget/wlibrarytableview.cpp

+    VERIFY_OR_DEBUG_ASSERT(!key.isEmpty()) {
+        return;
+    }
+    ModelState* state = m_modelStateCache.take(key);


The Warning applies applies, if you store the pointer in a member variable, or if we use the cache from multiple threads.
The QCrash has no magic to do something at laterally any time. It can do something at any call.
All this does not apply here. So I am not able to write a reasonable comment. I assume the crashes was cased by something else.

However since this is only a minor issue, and not on a performance critic path, I think we should keep this tested variant, but not add a comment with probably wrong assumptions.

ronso0 · 2021-11-20T01:46:35Z

🎉

github-actions bot added library ui labels Aug 4, 2021

ronso0 added this to the 2.4.0 milestone Aug 4, 2021

ronso0 marked this pull request as draft August 4, 2021 23:05

ronso0 force-pushed the lib-selection-state branch 5 times, most recently from 4b98db8 to 10bda6b Compare August 7, 2021 01:19

ronso0 marked this pull request as ready for review August 7, 2021 01:27

ronso0 changed the title ~~[WIP] Fix library track selection, save model state~~ Fix library track selection, save model state Aug 7, 2021

ronso0 requested a review from uklotzde August 7, 2021 01:27

ronso0 force-pushed the lib-selection-state branch from 10bda6b to 79a6d8f Compare August 7, 2021 01:33

ronso0 marked this pull request as draft August 7, 2021 10:54