Library: Add last_played_at column #3140

uklotzde · 2020-09-29T14:04:46Z

I need this property to complete the aoide integration in #2282.

The new field is stored in the PlayCounter
The field is searchable by "lastplayed", short and concise, similar to "timesplayed"
The temporary AutoDJ table is populated with the new property instead of initializing it with an unknown/empty value
A new view column will be added by Add "Last Played" column to library #2670, not included here

After this permanent addition, #2670 should become much simpler. I have reused the database query for the migration.

Note: The first 2 commits contain a temporary, transient approach before I decided to make this property persistent. Included for the sake of completeness and to document the development process.

TODO: Store the time stamp in the database instead of the useless boolean flag.

daschuer

Thank you. Some comments.

res/schema.xml

src/library/dao/autodjcratesdao.cpp

src/library/dao/trackdao.cpp

daschuer · 2020-09-30T07:38:06Z

src/library/dao/trackdao.cpp


-bool setTrackArtist(const QSqlRecord& record, const int column,
-                    TrackPointer pTrack) {
+bool setTrackArtist(const QSqlRecord& record, const int column, TrackPointer pTrack) {


Accordeing to the iso cpp guide these functions should use a plain pointer because they do not store it.

Not sure if this is a topic for this PR but it pops up during the mass reformat.

Same here, unrelated

src/library/dao/trackdao.cpp

src/track/playcounter.h

ywwg · 2020-09-30T12:47:26Z

Because my patch relies on the history playlists, it's possible to delete a history playlist and the last-played time automatically updates. Is that possible with this approach?

uklotzde · 2020-09-30T14:14:18Z

Because my patch relies on the history playlists, it's possible to delete a history playlist and the last-played time automatically updates. Is that possible with this approach?

Yes: Rerun the migration query for all tracks in the playlist after it has been deleted. The last_played_at column is just a cached value.

Even with the new indexes the migration might take a few seconds, it is a heavy-weight operation. The join required for the temporary view adds additional costs. Therefore I suggest to cache this value permanently in this new column instead of in a temporary view that needs to be recreated on startup.

The play count is also affected by deleting history playlists and would need to be updated if you want to implement such a feature consistently.

ywwg · 2020-09-30T14:33:19Z

in my version, I hook in to the playlist deletion signals to trigger the updates to the table. This should be easy to reimplement here if you reference my code. This is a pretty important facet of this particular feature for me so I'd appreciate if it could be included.

uklotzde · 2020-09-30T15:57:53Z

@daschuer I have force pushed after reverting all unintended changes. All additional changes will follow as separate commits.

VSCode is configured to only reformat changed parts and works as expected. The reformatting has probably been caused by the pre-commit hook, because I usually review all changes before creating a commit. If the pre-commit hook complains then I simply stash all additional changes created by the hook without further review. Will keep an eye on this in the future.

uklotzde · 2020-09-30T20:21:10Z

I have moved the creation of indexes into a separate schema migration step. Not only for playlists but also for crates.

uklotzde · 2020-09-30T23:43:29Z

@ywwg Automatic updates implemented

daschuer

Thank you.

src/library/dao/trackdao.cpp

daschuer · 2020-10-01T06:44:37Z

src/library/dao/trackdao.cpp

+                            QString::number(PlaylistDAO::PLHT_SET_LOG)));
+    VERIFY_OR_DEBUG_ASSERT(!query.hasError()) {
+        return false;
+    }


I think we need also consider the played flag.

Maybe we can dispose it internally by comparing the current play count with the value the session starts. That sounds less redundant.

I did not have a look, but we have also a feature to join two set logs, the played flag needs to be considered than as well.

The played flag should not be a property of the (persistent) track object. The set of tracks that are considered as (manually) played during a session should be managed separately. The complicated and inconsistent logic in the PlayCounter is already screaming out loud. I refuse to add more magic to it that fits some use cases but induces unexpected behavior for others. If the user decides to rewrite history they could be bothered with updating the played flag manually. It will be reset anyway at some time.

For my aoide view I don't have a played flag and already use the logic you proposed for an implicit played flag. This could be overridden with a session played flag, once Mixxx supports this concept.

I agree that the played flag is a slightly different idea and should remain separate. (Also note that Played status is maintained across crashes so the user doesn't lose state :))

Mangling the played flag as a column of the Track entity table was a bad idea. Instead the set of played tracks must be stored separately by introducing the concept of a Session entity. Out of scope of this PR.

ywwg

some small nits and notes. thanks for doing this!

src/database/mixxxdb.cpp

ywwg · 2020-10-12T02:28:01Z

src/library/dao/autodjcratesdao.cpp

@@ -15,6 +15,11 @@
 #include "mixer/playerinfo.h"
 #include "mixer/playermanager.h"

+#if !defined(VERBOSE_DEBUG_LOG)


please don't invent a new log level. "debug" is our verbose level, just use that.

We need this to prevent log spam. DEBUG is currently used as INFO. Explained here #2782. Otherwise I would have to delete all those extra logs.

Agree that we don't have a good story for Info vs Debug. But this feels like a one-off change that will increase tech debt. If you are going to do this, please create a bug and assign it to yourself to remove this special casing once we have better logging code.

src/library/dao/playlistdao.cpp

src/library/dao/trackdao.cpp

src/track/playcounter.h

poelzi · 2020-10-20T12:39:49Z

When this is merged, we can implement proper waveform cache cleanup based on LRU

uklotzde · 2020-10-29T11:55:46Z

How can we make progress here??

ywwg · 2020-11-13T02:50:19Z

what's the sticking point for this?

daschuer · 2020-11-13T06:45:54Z

We need to verify if the merge history and updating of the played flag still works.

This applies if the user restarts Mixxx during a gig for some reasons.

daschuer · 2020-11-13T07:40:42Z

build fails now:

/home/daniel/workspace/advanced_autodj_2/src/library/dao/trackdao.cpp: In member function ‘bool TrackDAO::updatePlayCounterFromPlayedHistory(QSet<TrackId>) const’:
/home/daniel/workspace/advanced_autodj_2/src/library/dao/trackdao.cpp:2211:32: error: passing ‘const TrackDAO’ as ‘this’ argument discards qualifiers [-fpermissive]
     emit tracksChanged(trackIds);
                                ^
In file included from /home/daniel/workspace/advanced_autodj_2/src/library/dao/trackdao.cpp:1:0:
/home/daniel/workspace/advanced_autodj_2/src/library/dao/trackdao.h:85:10: note:   in call to ‘void TrackDAO::tracksChanged(QSet<TrackId>)’
     void tracksChanged(QSet<TrackId> trackIds);
          ^~~~~~~~~~~~~

…last_played_at

daschuer · 2020-11-13T22:37:15Z

Since #2670 is closed now, we need to add the column addition here.

daschuer · 2020-11-13T22:40:58Z

The play flag handling still works as expected and the code looks good so far.

ywwg · 2020-11-13T22:47:14Z

Since #2670 is closed now, we need to add the column addition here.

I can rework my change to go from this PR instead. I'd prefer to get this db change in now and do the UI in a separate PR.

daschuer

OK, than LGTM.

uklotzde · 2020-11-13T23:09:20Z

Since #2670 is closed now, we need to add the column addition here.

I can rework my change to go from this PR instead. I'd prefer to get this db change in now and do the UI in a separate PR.

Thanks for taking over!

ywwg · 2020-12-17T00:53:25Z

UI: #3457

uklotzde added 2 commits September 29, 2020 09:28

Store (transient) last played time stamp in PlayCounter

3df10ad

TODO: Store the time stamp in the database instead of the useless boolean flag.

PlayCounter: Populate lastPlayedAt from database on load

ef647fc

uklotzde added the library label Sep 29, 2020

uklotzde added this to the 2.4.0 milestone Sep 29, 2020

uklotzde requested a review from ywwg September 29, 2020 14:04

This was referenced Sep 29, 2020

[Don't merge / Obsolete] Custom Tags #2656

Closed

[WiP] aoide - External music library #2282

Closed

daschuer requested changes Sep 30, 2020

View reviewed changes

uklotzde added 3 commits September 30, 2020 17:50

Add last_played_at column to library table

ab83cd1

Add "lastplayed" search field

f936a34

Use library.last_played_at for AutoDj

43e5e63

uklotzde added 2 commits September 30, 2020 21:56

PlayCounter: Rename played flag and fix outdated comments

3ecff63

Create indexes for tracks in playlists and crates

98cc65f

uklotzde added 5 commits September 30, 2020 22:31

Log composed SQL query on demand

ab7f187

Update required schema version

2ae67da

Delete obsolete code after reverting unintended changes

cc48052

Delete redundant '='

7c3b16c

Update play counter after removing tracks from playlists

4f9d766

TrackDAO: Delete more obsolete left-over code

66ea808

daschuer requested changes Oct 1, 2020

View reviewed changes

Explicitly update play counter from played history

46a9857

ywwg requested changes Oct 12, 2020

View reviewed changes

Merge branch 'master' into library_last_played_at

cd86537

ywwg mentioned this pull request Oct 16, 2020

Add "Last Played" column to library #2670

Closed

Merge branch 'master' into library_last_played_at

0625288

github-actions bot added the code quality label Oct 20, 2020

daschuer changed the base branch from master to main October 21, 2020 21:47

daschuer marked this pull request as draft October 21, 2020 21:48

Merge branch 'main' into library_last_played_at

a01c978

uklotzde marked this pull request as ready for review October 22, 2020 19:48

Holzhaus requested review from daschuer and ywwg October 24, 2020 13:17

uklotzde added 3 commits October 24, 2020 22:34

Merge branch 'main' into library_last_played_at

5e40a18

Merge branch 'main' into library_last_played_at

8baa952

Add missing emit keywords

36b2dd5

uklotzde added 2 commits November 7, 2020 18:09

Merge branch 'main' into library_last_played_at

b4b96fe

Merge branch 'main' into library_last_played_at

582b43f

uklotzde added 2 commits November 13, 2020 11:14

Merge branch 'main' into library_last_played_at

59371b6

Merge branch 'main' of git@github.com:mixxxdj/mixxx.git into library_…

4ee3e85

…last_played_at

daschuer approved these changes Nov 13, 2020

View reviewed changes

daschuer merged commit 7b81a58 into mixxxdj:main Nov 13, 2020

uklotzde deleted the library_last_played_at branch November 13, 2020 23:12

ronso0 added the changelog This PR should be included in the changelog label Nov 30, 2021

mixxxbot mentioned this pull request Aug 22, 2022

last played column #8211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library: Add last_played_at column #3140

Library: Add last_played_at column #3140

uklotzde commented Sep 29, 2020

daschuer left a comment

daschuer Sep 30, 2020

uklotzde Sep 30, 2020

ywwg commented Sep 30, 2020

uklotzde commented Sep 30, 2020

ywwg commented Sep 30, 2020

uklotzde commented Sep 30, 2020

uklotzde commented Sep 30, 2020 •

edited

Loading

uklotzde commented Sep 30, 2020

daschuer left a comment

daschuer Oct 1, 2020

uklotzde Oct 1, 2020

ywwg Oct 12, 2020

uklotzde Oct 29, 2020 •

edited

Loading

ywwg left a comment

ywwg Oct 12, 2020

uklotzde Oct 12, 2020

ywwg Oct 16, 2020

poelzi commented Oct 20, 2020

uklotzde commented Oct 29, 2020

ywwg commented Nov 13, 2020

daschuer commented Nov 13, 2020

daschuer commented Nov 13, 2020

daschuer commented Nov 13, 2020

daschuer commented Nov 13, 2020

ywwg commented Nov 13, 2020

daschuer left a comment

uklotzde commented Nov 13, 2020

ywwg commented Dec 17, 2020

Library: Add last_played_at column #3140

Library: Add last_played_at column #3140

Conversation

uklotzde commented Sep 29, 2020

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

ywwg commented Sep 30, 2020

uklotzde commented Sep 30, 2020

ywwg commented Sep 30, 2020

uklotzde commented Sep 30, 2020

uklotzde commented Sep 30, 2020 • edited Loading

uklotzde commented Sep 30, 2020

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

uklotzde Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

ywwg 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

poelzi commented Oct 20, 2020

uklotzde commented Oct 29, 2020

ywwg commented Nov 13, 2020

daschuer commented Nov 13, 2020

daschuer commented Nov 13, 2020

daschuer commented Nov 13, 2020

daschuer commented Nov 13, 2020

ywwg commented Nov 13, 2020

daschuer left a comment

Choose a reason for hiding this comment

uklotzde commented Nov 13, 2020

ywwg commented Dec 17, 2020

uklotzde commented Sep 30, 2020 •

edited

Loading

uklotzde Oct 29, 2020 •

edited

Loading