Last Played At: Add UI for last_played_at column #3457

ywwg · 2020-12-17T00:50:15Z

No description provided.

ywwg · 2020-12-17T00:51:13Z

src/library/rekordbox/rekordboxfeature.cpp

@@ -386,15 +386,18 @@ void insertTrack(
    query.bindValue(":bitrate", bitrate);
    query.bindValue(":analyze_path", anlzPath);
    query.bindValue(":device", device);
-    query.bindValue(":color", mixxx::RgbColor::toQVariant(colorFromID(static_cast<int>(track->color_id()))));
+    query.bindValue(":color",


the code formatter went to town on this file.

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

src/library/rekordbox/rekordboxfeature.cpp

@@ -1093,6 +1189,9 @@ void RekordboxPlaylistModel::initSortColumnMapping() {
    m_columnIndexBySortColumnId[static_cast<int>(
            TrackModel::SortColumnId::TimesPlayed)] =
            fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED);
+    m_columnIndexBySortColumnId[static_cast<int>(


here is the actual new code in this file

src/track/track.cpp

src/library/trackset/setlogfeature.cpp

uklotzde

Thank you for finishing this feature! Code changes seem sensible, less than expected. Not yet test.

uklotzde · 2020-12-18T18:18:51Z

Conflicts. Should be rebased after #3465 has been merged into main.

src/library/basetracktablemodel.cpp

uklotzde · 2020-12-21T19:24:17Z

Just one minor suggestion for renaming the column header. Otherwise ready to merge.

daschuer

The play column has an offset of UTC-Local Time.

In the sqlite file we use UTC to store the date added. That is OK to see a consecutive order independent of the time zones.

The last played column is stored in local time an I think the display code here assumes UTC, leading to the issue.

I am not DJing in different time Zones, but I can imagine that here is a use case of sorting the tracks by hour last played.
If I had a gig China for instance and back in Europa I see the tracks played at 14:00 MET which has been played in China at 22:00 local time, it will be confusing.

lt;dr:

Let's stick with storing local time and remove the offset for displaying here.

daschuer · 2020-12-22T23:40:11Z

src/library/basetracktablemodel.cpp

@@ -613,6 +618,7 @@ QVariant BaseTrackTableModel::roleValue(
            return QString("(%1)").arg(timesPlayed);
        }
        case ColumnCache::COLUMN_LIBRARYTABLE_DATETIMEADDED:
+        case ColumnCache::COLUMN_LIBRARYTABLE_LAST_PLAYED_AT:


COLUMN_LIBRARYTABLE_LAST_PLAYED_AT is not UTC. We need an extra case here.

@daschuer It actually is stored as UTC, so this code correct. Please double check!

No, not with current upstream/main.

If we decide for UTC in the library, the code her is correct.

We cannot decide about this if the system already decided for us. At least for my setup the timestamps are stored in UTC (see comments).

uklotzde · 2020-12-23T00:03:20Z

The play column has an offset of UTC-Local Time.

In the sqlite file we use UTC to store the date added. That is OK to see a consecutive order independent of the time zones.

The last played column is stored in local time an I think the display code here assumes UTC, leading to the issue.

I am not DJing in different time Zones, but I can imagine that here is a use case of sorting the tracks by hour last played.
If I had a gig China for instance and back in Europa I see the tracks played at 14:00 MET which has been played in China at 22:00 local time, it will be confusing.

lt;dr:

Let's stick with storing local time and remove the offset for displaying here.

CURRENT_TIMSTAMP generates an UTC timestamp for me. Please try this SELECT CURRENT_TIMSTAMP FROM library. If the behavior differs depending on the system configuration we are in big trouble.

Holzhaus · 2020-12-23T00:13:53Z

I think @daschuer meant that we wants to store the local timezone the track was played in, not UTC. Personally I think if you travel a lot, it would be even more confusing if the last time played is ahead of your current local time. Therefore I think storing UTC makes sense and will probably save us a lot of headaches later on.

daschuer · 2020-12-23T01:02:47Z

CURRENT_TIMSTAMP generates an UTC timestamp for me. Please try this SELECT CURRENT_TIMSTAMP FROM library. If the behavior differs depending on the system configuration we are in big trouble.

This works for me in UTC and is used for time added.

It looks like that the "last_played_at" column using local time, which is also OK for me.

I think @daschuer meant that we wants to store the local timezone the track was played in, not UTC.

It is already stored in local time. The issue her is that the displaying code assumes UTC and does a unnecessary and wrong conversion.

Personally I think if you travel a lot, it would be even more confusing if the last time played is ahead of your current local time.

Can this actually happen? How?

Therefore I think storing UTC makes sense and will probably save us a lot of headaches later on.

Yes, I think storing the value in UTC is reasonable, since all other times are also UTC. This is currently not the case.

If we only store UTC without the current Time zone. We can't do time only queries like "Tracks played at dinner timer" 18:00 - 21:00 or such. Because the dinner time in MET is not related to dinner time in CST.
I am unsure if this is a real use case though, but we have the chance to collect the data for future use.

A time zone field for each history playlist would be sufficient since it is unlikely that a DJ move the time zone during a gig.
On a cruise vessel maybe, but even then we don't want a gap/overlap when crossing a time zone.

uklotzde · 2020-12-23T07:57:25Z

I disagree. Storing plain UTC is valid and appropriate only for system-level events and as an optimization for sorting/ordering.

For user-level events that are often related to a geographical location you need to store local time with the time offset. This allows both, consistent ordering and meaningful display. Without the time offset (representing the time zone) you lose information!

This discussion is out of scope. Please report unambiguously what time is actually stored in your database and post an example timestamp. Please remember, for SQLite this is just plain text.

Holzhaus · 2020-12-23T08:00:51Z

Personally I think if you travel a lot, it would be even more confusing if the last time played is ahead of your current local time.

Can this actually happen? How?

Yes, it can happen. Random example:

When the plane took off from Japan at 4:00 PM on October 24, it was 11 PM on October 23 in Los Angeles. Adding 9 and a half hours to this, the landing time would be 8:30 AM on October 24.

uklotzde · 2020-12-23T08:09:44Z

CURRENT_TIMESTAMP = '2020-12-21 17:35:48' generated and stored in UTC (system clock: UTC, display time zone: CET/GMT+01:00 with german locale settings)

uklotzde · 2020-12-23T08:10:27Z

I suppose the content of pl_datetime_added depends on the system clock. This is unfortunate.

daschuer · 2020-12-23T10:01:21Z

@uklotzde:

I disagree. Storing plain UTC is valid and appropriate only for system-level events and as an optimization for sorting/ordering.

I am not sure where you disagree to. We have here a bug that the time "last_played_at column" shows the wrong time, that is the root issue. What is your proposal to solve this?

@Holzhaus

Yes, it can happen. Random example:

When the plane took off from Japan at 4:00 PM on October 24, it was 11 PM on October 23 in Los Angeles. Adding 9 and a half hours to this, the landing time would be 8:30 AM on October 24.

Yes of cause. My considerations are if your did play a DJ gig just before and after start and the user expectationion when looking at a history playlist recorded in a different time zone.

But anyway, this would be "feature" of the history playlist. Here we are taking about last_played_at.

Please report unambiguously what time is actually stored in your database and post an example timestamp. Please remember, for SQLite this is just plain text.

In DB:
last_played_at:
2020-12-23T10:37:46.946
datetime_added:
2020-12-23T09:37:25.373Z

in GUI
Zeitstempel
23.12.20 10:37
Hinzugefügt
23.12.20 10:37
Last Played At
23.12.20 11:37 <- Wrong

uklotzde · 2020-12-23T12:47:38Z

#3477 tries to address the time zone confusions by relying on the DB for all the date/time formatting and simply copying the generated time stamps. Those values should only be read, but never written programmatically from C++. Otherwise we have to ensure that we are writing the exact same format as SQLite does.

uklotzde · 2020-12-23T12:51:48Z

datetime_added:
2020-12-23T09:37:25.373Z

Is this really the exact column value in PlaylistTracks.pl_datetime_added? I get a different format, i.e. no ISO 8601.

daschuer · 2020-12-23T13:07:15Z

2020-12-23T09:37:25.373Z is the value from library.datetime_added
2020-12-23 09:37:46 is the value from PlaylistTracks.pl_datetime_added
2020-12-23T10:37:46.946 is the value from library.last_played_at

All different formats :-/

uklotzde · 2020-12-23T13:29:25Z

library.datetime_added is generated by Mixxx. Using UTC is reasonable for this use case, although using local time + offset would be more appropriate and consistent.

PlaylistTracks.pl_datetime_added is generated by SQLite. It is UTC although the time zone is not specified. Moreover using UTC for the history playlists doesn't fit the use case as already discussed, local time + offset would be more appropriate.

library.last_played_at is parsed by Qt from SQLite and then stringified by Qt again. Since the SQLite timestamp (in UTC) doesn't mention a time zone Qt assumes local time which is wrong. Nevertheless writing another format than that generated by and copied from PlaylistTracks.pl_datetime_added mixes up the sorting in database queries.

Be-ing · 2020-12-23T16:33:01Z

IMO times should always be stored as UTC and transformed to local time when displayed to the user. This is simple and easy to keep correct.

ywwg · 2020-12-23T17:09:07Z

IMO times should always be stored as UTC and transformed to local time when displayed to the user. This is simple and easy to keep correct.

completely agree -- DB times should all be UTC and adjusted for current local time. If that means someone gets on a plane and the times look wonky, so be it.

ywwg · 2020-12-23T17:11:21Z

I think timestamp display issues are out of scope for this PR. This code displays the timestamp in a standard way, and that is good enough here. If we want to adjust the way we display times, let's do that in a followup

daschuer · 2020-12-23T17:31:11Z

I think timestamp display issues are out of scope for this PR. This code displays the timestamp in a standard way, and that is good enough here. If we want to adjust the way we display times, let's do that in a followup

Unfortunately right now the bug is here. The time is read as UTC even though the library stores local time.

If you like to fix this this, It can be merged.

If we decide to change the time format in the library to UTC which seems to be reasonable, this can be done in this PR or in another PR that needs to be merged before this. In the later case this PR don't need to be changed.

If we change the time format we have the issue with alpha users. I think it is the best to deal with them by dropping the current column and create a new one.

uklotzde

@ywwg Sorry for the noise. The bug is unrelated to this PR and must be fixed separately, i.e. by #3477.

Let's just merge this PR as is.

uklotzde · 2020-12-23T22:29:09Z

@daschuer Please ignore the column values that are displayed after Mixxx has modified them. Instead re-execute the statement from schema migration 35 and then test if the values are displayed correctly. Anything else is out of scope right now.

Why should we delay this PR that only affects the display of those values?

uklotzde · 2020-12-23T22:42:38Z

IMO times should always be stored as UTC and transformed to local time when displayed to the user. This is simple and easy to keep correct.

Using plain UTC for system time stamps and M2M communication is perfect. But it is inappropriate for many user-related use cases and especially contracts. I had to learn this after many years designing data models and interfaces for business services. The current local time zone is arbitrary and should not affect an event that happened in the past and that is associated with a place or geographical location.

daschuer · 2020-12-23T22:51:10Z

After executing the migration statement the last_played_at column values are displayed correct with this branch.
All newly played tracks are displayed with an offset.

Why should we delay this PR that only affects the display of those values?

Because we than don't bother bleeding edge users with a broken feature. I would prefer to have it fixed before.
Since the feature can't be used right now, we have the option to move to a new column and not present users the mess we have produced since now.

The current local time zone is arbitrary and should not affect an event that happened in the past and that is associated with a place or geographical location.

I agree to that for the history view. This is an issue in the tracks table though, because we would have than Times from different time zones in on column. Do we agree that this should be transformed to local time?

Holzhaus · 2020-12-23T23:05:01Z

This is an issue in the tracks table though, because we would have than Times from different time zones in on column. Do we agree that this should be transformed to local time?

We could work around that issue by displaying values like "5 minutes ago", "2 days ago", etc. Not sure if that is a good idea.

daschuer · 2020-12-23T23:11:41Z

This will work for the major use case of finding tracks that have not been been played lets say since a year. For older tracks the wall clock time is not relevant.

I am still not clear if there is a use case for finding "dinner" or "late night" tracks. This sound appealing on the first sight, but is probably unreliable so that users better skim through there history itself. We have also crates for this.

What do you think?

uklotzde · 2020-12-24T00:03:18Z

After executing the migration statement the last_played_at column values are displayed correct with this branch.
All newly played tracks are displayed with an offset.

Why should we delay this PR that only affects the display of those values?

Because we than don't bother bleeding edge users with a broken feature. I would prefer to have it fixed before.
Since the feature can't be used right now, we have the option to move to a new column and not present users the mess we have produced since now.

The current local time zone is arbitrary and should not affect an event that happened in the past and that is associated with a place or geographical location.

I agree to that for the history view. This is an issue in the tracks table though, because we would have than Times from different time zones in on column. Do we agree that this should be transformed to local time?

Due to lack of knowledge about the time zone or offset we are not able to migrate existing values. I am not investing any time on migrating to a new format. We only have to fix the different formats by storing all values in the SQLite-generated format consistently.

Displaying historic data transformed into the current local time zone is confusing. But since everything is stored in UTC we don't have a choice here and need to make a reasonable assumption. If you don't travel between different time zones frequently then the current local time zone is a good guess that should be valid for the majority of users.

uklotzde · 2020-12-25T08:59:55Z

@ywwg Please review ywwg#10

An ugly workaround to bridge the gaps. The handling of date/time values from the database through the caching layer is brittle and inconsistent, much worse than what I expected. Take it or leave it.

uklotzde · 2020-12-25T09:01:27Z

ywwg#10 also splits the database migration to clean up the inconsistent values that have emerged until now.

QStringBuilder doesn't work for constants!

daschuer · 2020-12-27T01:04:46Z

Thank you very much @uklotzde ywwg#10 does the trick and solved the issue completely for me.
@ywwg Once you have merged it, this PR can be merged to finally enable the feature for alpha users.

Fix reading/writing of library.last_played_at

daschuer · 2020-12-28T21:13:06Z

Thank you for joining forces :-)
LGTM.

Last Played: Add UI for last_played_at column

e74c466

ywwg requested a review from uklotzde December 17, 2020 00:50

github-actions bot added the library label Dec 17, 2020

ywwg commented Dec 17, 2020

View reviewed changes

src/track/track.cpp Show resolved Hide resolved

ywwg commented Dec 17, 2020

View reviewed changes

src/library/trackset/setlogfeature.cpp Show resolved Hide resolved

ywwg mentioned this pull request Dec 17, 2020

Library: Add last_played_at column #3140

Merged

uklotzde approved these changes Dec 17, 2020

View reviewed changes

Last Played: merge and resolve conflicts

9a44f0b

uklotzde reviewed Dec 21, 2020

View reviewed changes

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

uklotzde mentioned this pull request Dec 21, 2020

TrackDAO: Do not write timesplayed/last_played_at #3477

Closed

daschuer requested changes Dec 22, 2020

View reviewed changes

daschuer reviewed Dec 22, 2020

View reviewed changes

Last played ui: shorten header title

70b0738

uklotzde approved these changes Dec 23, 2020

View reviewed changes

uklotzde added 3 commits December 25, 2020 14:54

Fix reading/writing of last_played_at values

e9bd24e

Split database migration steps 35/36 to fix last_played_at

86d1576

Add another constant for the date/time separator

9de685d

QStringBuilder doesn't work for constants!

uklotzde and others added 3 commits December 27, 2020 11:22

Add debug assertion for date/time formats

1f370b2

Rename namespace mixxx::sqlite

2ffbd01

Merge pull request #10 from uklotzde/last-played-squashed

57ac9c8

Fix reading/writing of library.last_played_at

github-actions bot added the build label Dec 28, 2020

daschuer approved these changes Dec 28, 2020

View reviewed changes

daschuer merged commit 8ba1a80 into mixxxdj:main Dec 28, 2020

mixxxbot mentioned this pull request Aug 22, 2022

last played column #8211

Closed

Last Played At: Add UI for last_played_at column #3457

Last Played At: Add UI for last_played_at column #3457

Conversation

ywwg commented Dec 17, 2020

ywwg Dec 17, 2020

Choose a reason for hiding this comment

ywwg Dec 17, 2020

Choose a reason for hiding this comment

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Dec 18, 2020

uklotzde commented Dec 21, 2020

daschuer left a comment

Choose a reason for hiding this comment

daschuer Dec 22, 2020

Choose a reason for hiding this comment

uklotzde Dec 23, 2020

Choose a reason for hiding this comment

daschuer Dec 23, 2020

Choose a reason for hiding this comment

uklotzde Dec 23, 2020

Choose a reason for hiding this comment

uklotzde commented Dec 23, 2020

Holzhaus commented Dec 23, 2020

daschuer commented Dec 23, 2020

uklotzde commented Dec 23, 2020 • edited Loading

Holzhaus commented Dec 23, 2020

uklotzde commented Dec 23, 2020 • edited Loading

uklotzde commented Dec 23, 2020

daschuer commented Dec 23, 2020

uklotzde commented Dec 23, 2020 • edited Loading

uklotzde commented Dec 23, 2020

daschuer commented Dec 23, 2020

uklotzde commented Dec 23, 2020 • edited Loading

Be-ing commented Dec 23, 2020

ywwg commented Dec 23, 2020 • edited Loading

ywwg commented Dec 23, 2020

daschuer commented Dec 23, 2020

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Dec 23, 2020

uklotzde commented Dec 23, 2020

daschuer commented Dec 23, 2020

Holzhaus commented Dec 23, 2020

daschuer commented Dec 23, 2020

uklotzde commented Dec 24, 2020

uklotzde commented Dec 25, 2020

uklotzde commented Dec 25, 2020 • edited Loading

daschuer commented Dec 27, 2020

daschuer commented Dec 28, 2020

uklotzde commented Dec 23, 2020 •

edited

Loading

uklotzde commented Dec 23, 2020 •

edited

Loading

uklotzde commented Dec 23, 2020 •

edited

Loading

uklotzde commented Dec 23, 2020 •

edited

Loading

ywwg commented Dec 23, 2020 •

edited

Loading

uklotzde commented Dec 25, 2020 •

edited

Loading