History improvements #2996

poelzi · 2020-08-05T12:51:24Z

The history feature has quite some usability issues. The list gets very long very quickly and the most useful entries are at the bottom of the list.

Edit(ronso0) https://bugs.launchpad.net/mixxx/+bug/1127120

Stop wasting the root window with information that are easily discoverable, but instead display the current playlist at the root entry. This way, the menu does not need to be open to jump to the current list.
Instead of putting the newest entry at the bottom, use descending order and show the last entries first.
Only show the last 5 playlists and group later entries by year.

Todo: highlight group if any child is highlighted

Swiftb0y · 2020-08-05T18:37:30Z

It would also be cool if there was a functionality to remove history playlists containing less than N tracks.

ronso0 · 2020-08-05T20:51:31Z

It would also be cool if there was a functionality to remove history playlists containing less than N tracks.

... as action in history list context menu > QDialog
max. tracks spinbox
Delete Cancel

Or simply being able to select multiple history items in the sidebar, then use Remove from the existing context menu.

But that's probably asked too much for this PR.
Though we should keep it in mind > https://bugs.launchpad.net/mixxx/+bug/1127120

daschuer

Great, this is really a nice improvement.
I have left some comments.

src/library/trackset/baseplaylistfeature.h

src/library/trackset/setlogfeature.cpp

ronso0 · 2020-08-05T20:58:53Z

also somehow related:
https://bugs.launchpad.net/mixxx/+bug/1766163 (update history tracks more frequently)

poelzi · 2020-08-05T22:32:37Z

It would also be cool if there was a functionality to remove history playlists containing less than N tracks.

I had a patch for this as well. Currently the cleanup for empty playlists is in the destructor of a class. Crashes or ctrl+c on a new session always creates empty playlists.

src/library/trackset/setlogfeature.cpp

daschuer · 2020-08-14T06:54:07Z

Do you consider this in a mergeable state now? If yes, please remove the draft state.

And just a hint for the future:

Implement requested changes.

It is not interesting in some years that the commit was a review request. So it is better just to describe the change and split it into separate commits if you do not find a headline that covers all.

ronso0 · 2020-09-17T15:38:08Z

I wanted to have this in my 2.3 builds I use live, so I tried to port it to 2.3 (rather hasty copy/paste): https://github.com/ronso0/mixxx/tree/history-improv-2.3

ronso0 · 2020-09-17T15:45:19Z

side note / small UX issue I noticed while testing this:

both regular playlists and history playlists can have numbers in parenthesis appended

for regular playlists this number indicates the number of tracks each list contains
for history playlists it's append only for the (n>1)-th playlist of a certain date

This is a bit confusing. Maybe we can use [ ] for the number of tracks?

foss- · 2020-09-17T17:37:13Z

YYYY-MM-DD_n (n2) where n is number of playlist on same date and n2 is the number of tracks on certain playlist. Using a suffix number for number of playlists on same date and parenthesis for number of tracks on tracklist would provide good readability and restore consistency with other playlists used in mixxx.

Edit to add: Allowing selection of more than a single entry when deleting unwanted entires would be highly welcome. But maybe that should be a separate PR then?

src/library/treeitemmodel.cpp

poelzi · 2020-09-20T22:49:57Z

I tried [] and other unicode characters and found YYYY-MM-DD / n looked the nicest.
Now the tree is properly set bold for the history entry and it's parent.

There is only one thing left that I would like to have:
If you delete a history entry, it would be nice if next item in the tree is selected after deletion. Currently the model is just redrawn and the tree is collapsed. I tried to use the scrollTo function on the Index, but since the index is based on the library feature model and not the sidebar model, the function crashes mixxx.

foss- · 2020-09-21T09:33:53Z

@poelzi can you attach a screenshot? Wouldn't that introduce an inconsistency with the way other library areas use n? how is n and n2 handled now?

poelzi · 2020-09-21T23:19:53Z

I can change it to something different, but this looks nice for me.
Export of a playlist now suggests a name in which the dir separator is replaced with a "-".

poelzi · 2020-09-21T23:23:48Z

There is only one bug left, but I think it's upstream. scrollTo(QModelIndex) does not work on the sidebar widget.

Holzhaus · 2020-09-21T23:33:47Z

There are also multiple unresolved review comments.

ronso0 · 2020-09-22T13:55:44Z

There is only one bug left, but I think it's upstream. scrollTo(QModelIndex) does not work on the sidebar widget.

scrollTo seems to be broken in general. There are multiple outdated Qt bugs for QTreeView but I didn't yet take the time to create a new one for our context with QTableView as selectRow() is a helpful workaround/aternative for selecting and scrolling to an index.

ronso0 · 2020-09-22T14:07:17Z

I can change it to something different, but this looks nice for me.

I vote for @foss- proposal:
2020-07-26_n (#)

ronso0 · 2020-09-22T14:08:31Z

we could also have
2020-07-29 2/3 (#)

poelzi · 2020-09-22T14:20:09Z

we could also have
2020-07-29 2/3 (#)

That is not practical, because it would require to rename all old entries of the same date.

Holzhaus · 2020-09-22T14:33:43Z

What about YYYY-MM-DD - N (M)? Personally, I don't like the underscore (but I'd be okay with it if you prefer it) and the slash in incompatible with file names (when exporting).

foss- · 2020-09-22T21:13:42Z

@Holzhaus No strong feelings for either proposal.
YYYY-MM-DD_N (M) Idea with was to save space.
YYYY-MM-DD - N (M) Your proposal might be more human readable.

N: number of playlist for same day
M: number of tracks no playlist

poelzi · 2020-09-22T21:57:00Z

I also really dislike the underscore. I'm ok with the - , just wanted to not use the same as the date. I fixed the issue with exports in general, every directory seperator is replaced by "-" for all playlist exports.

ronso0 · 2020-09-23T14:54:11Z

I just don't fnd the - to be a good indicator for continous numbering.
what about YYYY-MM-DD #N (M) , would that work for export on all platforms?

Holzhaus · 2020-09-23T15:26:40Z

I guess: https://stackoverflow.com/a/31976060

daschuer · 2020-11-30T07:40:20Z

The "Join with previous" feature is now broken. Out of habit I expected that it deletes the current playlist and joins it with the one above. Unfortunately it now joins with the unrelated day below.
Can we rename that feature "Join with previous (below)" or explicit name the playlist joined?

daschuer

Thank you. Some comments.

src/library/trackset/baseplaylistfeature.cpp

daschuer · 2020-11-30T07:29:38Z

src/library/trackset/baseplaylistfeature.cpp

-        VERIFY_OR_DEBUG_ASSERT(playlistId >= 0) {
-            return;
-        }
+    int nextId = selectSiblingPlaylistId(m_lastRightClickedIndex);


Suggested change

int nextId = selectSiblingPlaylistId(m_lastRightClickedIndex);

int siblingId = selectSiblingPlaylistId(m_lastRightClickedIndex);

src/library/trackset/baseplaylistfeature.cpp

poelzi · 2020-11-30T22:10:23Z

The "Join with previous" feature is now broken. Out of habit I expected that it deletes the current playlist and joins it with the one above. Unfortunately it now joins with the unrelated day below.
Can we rename that feature "Join with previous (below)" or explicit name the playlist joined?

This is just habit and will change once used to it. It behaves correctly here and joins the entry with the previous entry which is below the current one, because it is further in time.

daschuer · 2020-11-30T23:18:27Z

This is just habit and will change once used to it. It behaves correctly here and joins the entry with the previous entry which is below the current one, because it is further in time.

The issue is that it can't be undone. As I hit the trap, many others will do.
So let's at least at least change the name to "Join with previous (below)"

ronso0 · 2020-11-30T23:33:25Z

The timeline is inverted now, top-to-bottom gors into the past, this feels very intuitive to me. I assume most users have one history per day so the date numbers make that clear instantly IMO.
So the "Join with previous" learning curve is quite steep ;) I would not add 'below' here.

Only activate playlists when already in focus. Add option to scroll but not select sidebar items.

poelzi · 2020-12-01T10:01:49Z

@daschuer Fixed your points

src/library/trackset/baseplaylistfeature.cpp

uklotzde

Works fine so far and is a huge usability improvement.

A nice extension for the future would be relative date folders:

today
yesterday
last week
last month
last year
...remaining playlists grouped by calendar year...

daschuer

LGTM, Thank you.

daschuer · 2020-12-02T22:15:35Z

@Holzhaus: merge?

Holzhaus · 2020-12-02T22:24:56Z

@Holzhaus: merge?

Feel free to merge, I didn't follow this PR.

uklotzde · 2020-12-02T23:35:04Z

All review comments have been addressed. Thank you! LGTM

ronso0 · 2020-12-03T12:55:53Z

🎉

Be-ing · 2020-12-10T17:53:04Z

The meaning of the number after the playlist date is unclear to me. Why does sometimes it have a # whereas other times it is in parentheses?

ronso0 · 2020-12-10T18:51:32Z

# is the prefix for the Nth playlist on that day.
number in parenthesis is the track count.

Be-ing · 2020-12-10T18:55:18Z

I think I got confused because there is no number in parentheses if the playlist only has 1 track.

Holzhaus · 2020-12-10T19:02:03Z

I think I got confused because there is no number in parentheses if the playlist only has 1 track.

I agree, we should always show the track count.

poelzi · 2020-12-11T10:36:24Z

I will open a followup

ronso0 added this to the 2.4.0 milestone Aug 5, 2020

ronso0 added library ui labels Aug 5, 2020

daschuer requested changes Aug 5, 2020

View reviewed changes

hacksdump suggested changes Aug 6, 2020

View reviewed changes

ronso0 added a commit to ronso0/mixxx that referenced this pull request Sep 17, 2020

TEST: port mixxxdj#2996 'poelzi:history' to 2.3

8f0436f

Holzhaus requested changes Sep 20, 2020

View reviewed changes

src/library/treeitemmodel.cpp Outdated Show resolved Hide resolved

daschuer requested changes Nov 30, 2020

View reviewed changes

poelzi added 2 commits December 1, 2020 02:07

use constexpr when possible. Clearify join wording

a05573b

Don't activate when not expected.

6cabebd

Only activate playlists when already in focus. Add option to scroll but not select sidebar items.

daschuer requested changes Dec 1, 2020

View reviewed changes

src/library/trackset/baseplaylistfeature.cpp Outdated Show resolved Hide resolved

use QChar instead of StringLiteral

4d69a40

poelzi requested a review from daschuer December 1, 2020 17:20

uklotzde approved these changes Dec 2, 2020

View reviewed changes

daschuer approved these changes Dec 2, 2020

View reviewed changes

uklotzde merged commit 049311a into mixxxdj:main Dec 2, 2020

ronso0 mentioned this pull request Aug 11, 2021

Sidebar item selection fix #4193

Merged

ronso0 mentioned this pull request Apr 15, 2022

add History cleanup options #4726

Merged

6 tasks

This was referenced Aug 22, 2022

Improve sorting of session playlists in History feature #6917

Closed

restore sidebar scroll position after playlist rename, un/lock or removal #9788

Closed

This was referenced Apr 6, 2023

Change History tree to show "Z to A" dated playlists instead of "A to Z" (current) #11447

Closed

Allow multiple selection for REMOVE and JOIN WITH PREVIOUS option in History feature #6469

Open

	int nextId = selectSiblingPlaylistId(m_lastRightClickedIndex);
	int siblingId = selectSiblingPlaylistId(m_lastRightClickedIndex);

History improvements #2996

History improvements #2996

Conversation

poelzi commented Aug 5, 2020 • edited by ronso0 Loading

Swiftb0y commented Aug 5, 2020

ronso0 commented Aug 5, 2020 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Aug 5, 2020 • edited Loading

poelzi commented Aug 5, 2020

daschuer commented Aug 14, 2020

ronso0 commented Sep 17, 2020

ronso0 commented Sep 17, 2020

foss- commented Sep 17, 2020 • edited Loading

poelzi commented Sep 20, 2020

foss- commented Sep 21, 2020

poelzi commented Sep 21, 2020 • edited Loading

poelzi commented Sep 21, 2020

Holzhaus commented Sep 21, 2020

ronso0 commented Sep 22, 2020

ronso0 commented Sep 22, 2020

ronso0 commented Sep 22, 2020

poelzi commented Sep 22, 2020

Holzhaus commented Sep 22, 2020 • edited Loading

foss- commented Sep 22, 2020 • edited Loading

poelzi commented Sep 22, 2020

ronso0 commented Sep 23, 2020

Holzhaus commented Sep 23, 2020

daschuer commented Nov 30, 2020

daschuer left a comment

Choose a reason for hiding this comment

daschuer Nov 30, 2020

Choose a reason for hiding this comment

poelzi commented Nov 30, 2020

daschuer commented Nov 30, 2020

ronso0 commented Nov 30, 2020 • edited Loading

poelzi commented Dec 1, 2020

uklotzde left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Dec 2, 2020

Holzhaus commented Dec 2, 2020

uklotzde commented Dec 2, 2020

ronso0 commented Dec 3, 2020

Be-ing commented Dec 10, 2020

ronso0 commented Dec 10, 2020 • edited Loading

Be-ing commented Dec 10, 2020

Holzhaus commented Dec 10, 2020

poelzi commented Dec 11, 2020

poelzi commented Aug 5, 2020 •

edited by ronso0

Loading

ronso0 commented Aug 5, 2020 •

edited

Loading

ronso0 commented Aug 5, 2020 •

edited

Loading

foss- commented Sep 17, 2020 •

edited

Loading

poelzi commented Sep 21, 2020 •

edited

Loading

Holzhaus commented Sep 22, 2020 •

edited

Loading

foss- commented Sep 22, 2020 •

edited

Loading

ronso0 commented Nov 30, 2020 •

edited

Loading

ronso0 commented Dec 10, 2020 •

edited

Loading