add History cleanup options #4726

ronso0 · 2022-04-15T18:51:10Z

After the History rework #2996 I'm finally adding some options to Preferences > Library for configuring how the history playlists are cleaned up.

configure tracks threshold for histories to keep
~~allow to keep locked playlists with fewer tracks~~
always keep locked playlists
allow adjusting the "recently played" threshold (tracks that are NOT added twice to the history) lp:1766163
reset "recently played" list when starting a new session manually ("Finish current and start new") lp:1969217
reorder / re-group the library options in the GUI

ronso0 · 2022-04-16T09:17:10Z

I'm done here. Options work as expected, GUI looks good.
Any hints regarding the wording in the preferences are welcome, especially from native speakers @Be-ing @ywwg

foss- · 2022-04-19T08:09:30Z

Very useful changes that improve usability a lot. Is allowing to select more than a single entry, analogue to how multiple files are selected in file manager, in order to bulk delete history entries, out of scope for this PR?

ronso0, you already commented on https://bugs.launchpad.net/mixxx/+bug/1968503 which should also be updated once this is merged. In your screenshot the UI looks misaligned when looking at the text rows and the setting to the right.

ronso0 · 2022-04-19T08:21:03Z

Is allowing to select more than a single entry, analogue to how multiple files are selected in file manager, in order to bulk delete history entries, out of scope for this PR?

yes, I'm not going to work on that now.
This PR only adds the interface for existing functionality and fixes bugs.

ronso0 · 2022-04-19T08:29:42Z

In your screenshot the UI looks misaligned when looking at the text rows and the setting to the right.

That is already fixed, the note is now in the tooltips of the spinbox and the checkbox. I updated the screenshot.

foss- · 2022-04-19T08:32:51Z

Is the fields width expected? This will likely not be more than two digits, thus it looks a bit too wide for its purpose.

ronso0 · 2022-04-19T08:40:25Z

Sure, but if the translation string is rather short there will be a lot of whitespace in between the labels on the left and the spinboxes. Keeping them closer together improves readability, like avoiding long lines in continuous text to ease finding the beginning of the next line. That's also the reason why the row height and search timeout labels are right-aligned.

ronso0 · 2022-04-19T11:41:27Z

select more than a single entry, analogue to how multiple files are selected in file manager, in order to bulk delete history entrie

FWIW I'd not do this from in the sidebar. We'd need to switch from single- to multi-selection and I think it's tricky to get the UX right.
I'd prefer the simpler approach of adding a seperate dialog for this, for example a listw view with checkable items, probably grouped by year and month.

daschuer · 2022-04-19T20:42:09Z

Is there a need for the checkbox to delete locked lists? I would prefer to dispose it and never delete locked playlists.

daschuer

The code looks and works good. Only the naming can be improved. Thank you.

daschuer · 2022-04-19T20:25:05Z

src/library/library_prefs.cpp

+const ConfigKey mixxx::library::prefs::kHistoryRecentlyPlayedThresholdConfigKey =
+        ConfigKey{
+                mixxx::library::prefs::kConfigGroup,
+                QStringLiteral("history_recently_played_threshold")};


This one is hard to understand. It is the number of tracks that has to be tracked, before a track is re-entered.
It has been introduced to not list a track many times when scratching.
Maybe "history_reenter_distance" .. "history_rerecord_inhibit_gap" or something better.
The same applies to the GUI.

history_track_readd_threshold?
If you don't consider that suitable either I'd rather add a comment here than creating a foot-long configkey.

In the preferences the spinbox tooltip explains the meaning. Do you think that's not sufficient?

what about
'Recently played' threshold to prevent duplicate entries
?

"history_track_readd_threshold" works better for me, while "threshold" can be anything. That's why I had proposed "gap" or "distance"

'Recently played' threshold to prevent duplicate entries

This can be improved because it will not "prevent duplicates" it just sets the minimal distance of duplicates.

"Track re-add threshold" works also in the GUI

but how about:
"Minimum distance of repeated tracks"?

and for the config key

"history_track_repeat_distance"

"history_duplicate_distance"

Thanks, I wasn't really in the shape to come up with a proper term.
I'll go with history_track_duplicate_distance and for the History preference "Track duplicate distance"

daschuer · 2022-04-19T20:27:40Z

src/library/library_prefs.cpp

+const ConfigKey mixxx::library::prefs::kHistoryCleanupMinTracksConfigKey =
+        ConfigKey{
+                mixxx::library::prefs::kConfigGroup,
+                QStringLiteral("history_cleanup_min_tracks")};


Isn't this the max length that is cleaned up?
how about something like: "max_tracks_auto_delete"
Or the other way around "min_entries_to_keep"

nope, it's the "minimum tracks count to keep playlist":

"SELECT id FROM Playlists " "WHERE (SELECT count(playlist_id) FROM PlaylistTracks WHERE " "Playlists.ID = PlaylistTracks.playlist_id) < :length AND " "Playlists.hidden = :hidden AND Playlists.locked = :locked"

https://github.com/mixxxdj/mixxx/pull/4726/files#diff-4112e31df339e252ee2f3f49b18e1daf2ce58aaf488f5f4db43ab98c4aa83ff7

Right, for me this combination is sensible "min + keep" or "max + cleanup/delete" is correct.
"min + clenaup" feels wrong, while cleanup is not well defined.

From your explanation we may use "history_min_tracks_to_keep" or such.

makes sense, I changed it.
The preferences UI is okay IMO

would it be possible to substitute the N with the number widget inline?

Do you fear the N / X / ? placeholder will not be understood?

Puuting the actual number in the label would work but I don't see the benefit of this redundancy tbh.
Inline spinbox is also possible but its position depends on the translation, and it screws up the layout. Sooo: tricky..

Mhmm makes sense. I just thought it would result in better UX, but considering the challenges it causes, I agree that its not worth the effort.

ronso0 · 2022-04-20T08:08:57Z

Is there a need for the checkbox to delete locked lists? I would prefer to dispose it and never delete locked playlists.

Good point 👍 I'll remove it.

daschuer

LGTM, Thank you.

github-actions bot added library ui labels Apr 15, 2022

ronso0 added the preferences label Apr 15, 2022

ronso0 added 6 commits April 15, 2022 22:48

History cleanup: make track threshold configurable

015674c

Preferences > Library: add spinbox for history cleanup track threshold

7b8a971

History cleanup: option to keep locked playlists

a1c9d9a

move some [Library] ConfigKeys to library_prefs.cpp

16f7565

History: reset 'recently played' list when starting new session

fbfc888

History: make 'recently played' threshold configurable

03ec672

ronso0 force-pushed the history-cleanup-options branch from 1ee63ac to 03ec672 Compare April 15, 2022 22:19

Pref > Library: re-group options

c59fc75

ronso0 marked this pull request as ready for review April 16, 2022 09:15

ronso0 added this to the 2.4.0 milestone Apr 17, 2022

ronso0 added the minor bug label Apr 18, 2022

daschuer requested changes Apr 19, 2022

View reviewed changes

ronso0 added 3 commits April 20, 2022 11:49

PlaylistDAO: always keep locked playlists

a1ea8fc

History managemanent: use 'Track duplicate distance'

25e580a

History managemanent: use history_min_tracks_to_keep

2ec1ceb

daschuer approved these changes May 9, 2022

View reviewed changes

daschuer merged commit 30cc5f1 into mixxxdj:main May 9, 2022

ronso0 deleted the history-cleanup-options branch May 12, 2022 21:11

This was referenced Aug 23, 2022

Track history window too large? #9259

Closed

Identify day of week and start time of histories #10708

Open

History: after splitting current history already played tracks are not recorded #10714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add History cleanup options #4726

add History cleanup options #4726

ronso0 commented Apr 15, 2022 •

edited

ronso0 commented Apr 16, 2022

foss- commented Apr 19, 2022

ronso0 commented Apr 19, 2022

ronso0 commented Apr 19, 2022

foss- commented Apr 19, 2022

ronso0 commented Apr 19, 2022 •

edited

ronso0 commented Apr 19, 2022

daschuer commented Apr 19, 2022

daschuer left a comment

daschuer Apr 19, 2022

ronso0 Apr 20, 2022

ronso0 Apr 20, 2022

daschuer Apr 20, 2022

daschuer Apr 20, 2022

ronso0 Apr 23, 2022

daschuer Apr 19, 2022

ronso0 Apr 20, 2022

daschuer Apr 20, 2022

ronso0 Apr 23, 2022

Swiftb0y Apr 25, 2022

ronso0 Apr 25, 2022

Swiftb0y Apr 25, 2022

ronso0 commented Apr 20, 2022

daschuer left a comment

add History cleanup options #4726

add History cleanup options #4726

Conversation

ronso0 commented Apr 15, 2022 • edited

ronso0 commented Apr 16, 2022

foss- commented Apr 19, 2022

ronso0 commented Apr 19, 2022

ronso0 commented Apr 19, 2022

foss- commented Apr 19, 2022

ronso0 commented Apr 19, 2022 • edited

ronso0 commented Apr 19, 2022

daschuer commented Apr 19, 2022

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Apr 20, 2022

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Apr 15, 2022 •

edited

ronso0 commented Apr 19, 2022 •

edited