AutoDJ: Show total track time #4846

ninomp · 2022-07-06T19:13:35Z

This PR adds total track time to AutoDJ pane.

For now, it does NOT take intro/outro cues and AutoDJ mode into account. I am planning to change this when I add "Will Start At" column to track table, which is my next step.

ronso0 · 2022-07-06T20:12:00Z

I never use AutoDJ, would it be helpful to show the total remaining play time also in the sidebar item, like it's shown for playlists?
E.g. when you're in another feature for collecting tracks and need to know how much of your 2h set is left to be filled.
AutoDJ (28:43)

src/library/autodj/dlgautodj.cpp

uklotzde · 2022-07-06T22:28:28Z

src/library/autodj/dlgautodj.cpp

+
+    int rows = m_pAutoDJTableModel->rowCount();
+    for (int row = 0; row < rows; ++row) {
+        TrackPointer pTrack = m_pAutoDJTableModel->getTrack(m_pAutoDJTableModel->index(row, 0));


Loading and instantiating a heavy-weight track object just to obtain the duration is overkill and will certainly block the UI if many tracks are affected.

It somehow works for a quick and dirty prototype hack. But you shouldn't do this in production code.

Do you have a recommendation for a workaround?

I think the track duration will be already in the library cache.
However once we take the real duration into account, we need also the cues.

There are multiple options depending on what information is available in this context (which I am not aware of). This is just the worst option.

I think the best way forward is to introduce the same label we use per playlist.

mixxx/src/library/trackset/playlistfeature.cpp

Line 133 in f2d670e

QList<BasePlaylistFeature::IdAndLabel> PlaylistFeature::createPlaylistLabels() {

Since Auto DJ is in fact a "hidden" playlist, we can move this function into a common base class.

I suggest to use a "<" prefix to tell the user that the displayed time is the maximum.

In a next step we may calculate also the real runtime for a reasonable amount of tracks only. The main use case is to see I think not more than a time < 1 h ahead, for a timed show or a visit on the rest rooms or at the bar.
For longer times we can fall back to the original "<" display version.

What do you think?

daschuer · 2022-07-08T11:05:37Z

src/library/autodj/dlgautodj.cpp

+
+    QString label;
+    label.append(mixxx::DurationBase::formatTime(duration));
+    label.append(QString(" (%1)").arg(rows));


here we should reuse the

mixxx/src/library/trackset/playlistfeature.cpp

Line 26 in f2d670e

QString createPlaylistLabel(

in fact the playlists share the same issue that it shows the max time only and not the resulting time when put to Auto-DJ.

daschuer

Thank you for picking this up. I think we can merge a first version based on the Playlist solution.

Be-ing · 2022-07-09T19:16:47Z

src/library/autodj/dlgautodj.cpp

+    }
+
+    QString label;
+    label.append(mixxx::DurationBase::formatTime(duration));


This needs to communicate that the shown time is an estimate. Users making plans based on this will be mislead if they think this is accurate!

maybe a prefix like "Sum:" clarifies the overlaps/transitions are not considered.
Anyway, such info should be in the tooltip.

I disagree that it should be hidden in the tooltip. Also "sum" is unclear. "Estimate" would convey that this is not reliable.

Ping. The UI still presents this estimate as if it reflects the actual play time.

ronso0 · 2022-11-22T20:49:48Z

preempting the stale bot:
@ninomp are you motivated to pick this up again and consider the change requests?

ninomp · 2022-11-23T01:39:53Z

Hi @ronso0 Yes, I am, but I'm short on time lately. However, I will try to find some time to work on this. Stay tuned. ;-)

* Heavily inspired by CrateStorage * Move playlist summary computation from PlaylistFeature to it * PlaylistFeature no longer contains any raw SQL

ninomp · 2022-12-10T23:46:38Z

This is ready for review.

daschuer · 2022-12-13T22:09:28Z

The presentation looks somehow wrong, because one time we have parentheses and one time not.

daschuer

Thank you. This woks already good. Added some comments.

src/library/autodj/autodjfeature.cpp

daschuer · 2022-12-13T17:28:51Z

src/library/autodj/dlgautodj.cpp

@@ -32,7 +32,8 @@ DlgAutoDJ::DlgAutoDJ(WLibrary* parent,
                  parent->getTrackTableBackgroundColorOpacity(),
                  /*no sorting*/ false)),
          m_bShowButtonText(parent->getShowButtonText()),
-          m_pAutoDJTableModel(nullptr) {
+          m_pAutoDJTableModel(nullptr),
+          m_pTrackCollection(pLibrary->trackCollectionManager()->internalCollection()) {


This row of "->" looks scary. Can we move the initialization to the code body and add some null checkes?
Or are these checks in place at the calling code and we can pass the final pointer?
Using The not_null pointer might be also an option.

daschuer · 2022-12-13T17:30:12Z

src/library/autodj/dlgautodj.cpp

@@ -54,6 +55,13 @@ DlgAutoDJ::DlgAutoDJ(WLibrary* parent,
            this,
            &DlgAutoDJ::updateSelectionInfo);

+    connect(&pLibrary->trackCollectionManager()
+                     ->internalCollection()
+                     ->getPlaylistDAO(),


Another place of a "->" row.

src/library/autodj/dlgautodj.cpp

daschuer · 2022-12-13T17:36:40Z

src/library/autodj/dlgautodj.cpp

+
+    QString label;
+    label.append(summary.getTrackDurationText());
+    label.append(QString(" (%1)").arg(summary.getTrackCount()));


Suggested change

label.append(QString(" (%1)").arg(summary.getTrackCount()));

label.append(QStringLiteral(" (%1)").arg(summary.getTrackCount()));

src/library/autodj/dlgautodj.cpp

daschuer · 2022-12-13T22:14:36Z

src/library/autodj/dlgautodj.ui

@@ -219,6 +219,13 @@
        </property>
       </widget>
      </item>
+      <item>
+       <widget class="QLabel" name="labelTotalInfo">


All other features have the text left aligned. I think we should do it here as well.

Did you consider this comment?

Well, labelSelectionInfo does not have text alignment set, so I'm slightly confused why labelTotalInfo would be different.

This is because in the analysis view the spacer is on the very right.

I have no strong preferences for one of the options but they should be the same.

src/library/trackset/playlist/playliststorage.h

daschuer · 2023-01-30T13:44:23Z

I see now this:

daschuer · 2023-01-30T13:48:35Z

Did we have a conclusion how to handle the error due to cross-fading and cue points?
I thing using a "<" prefix or "~" would be reasonable.

daschuer · 2023-01-30T13:55:21Z

src/library/trackset/playlist/playlist.h

+#include "library/trackset/playlist/playlistid.h"
+#include "util/db/dbnamedentity.h"
+
+class Playlist : public DbNamedEntity<PlaylistId> {


Is this new class used? Can this be just a typedef? The name is IMHO ambiguous.

Yes, PlaylistSummary class is derived from this class.

Well, I was trying to follow and replicate how CrateStorage is implemented. The idea is that one day PlaylistStorage, which is introduced with this PR, will replace PlaylistDAO, just like CrateStorage replaced CrateDAO.

…otal_time

'=': conversion from 'size_t' to 'int', possible loss of data

daschuer

This looks a bit cluttered:

Maybe we can decorate it with icons or labels?
Another option is to use the total time if one track is selected and show the selected time if we have more selected.

<15:00 (3/15)

daschuer · 2023-04-29T08:34:42Z

src/library/autodj/dlgautodj.ui

@@ -219,6 +219,13 @@
        </property>
       </widget>
      </item>
+      <item>
+       <widget class="QLabel" name="labelTotalInfo">


This is because in the analysis view the spacer is on the very right.

I have no strong preferences for one of the options but they should be the same.

daschuer · 2023-04-29T08:57:52Z

The display if the total run time is already visible in the side bar. From that point of view showing only the selected runtime should be good.

github-actions · 2023-08-23T00:06:37Z

This PR is marked as stale because it has been open 90 days with no activity.

…otal_time

daschuer · 2024-04-23T06:26:58Z

Oh sorry, conflicts has been developed.

ninomp · 2024-04-23T11:46:36Z

@daschuer No worries. I am planning to get back and finish this soon.

DlgAutoDJ: Show total track time

f0b4f6d

github-actions bot added the library label Jul 6, 2022

ronso0 reviewed Jul 6, 2022

View reviewed changes

src/library/autodj/dlgautodj.cpp Outdated Show resolved Hide resolved

uklotzde reviewed Jul 6, 2022

View reviewed changes

daschuer reviewed Jul 8, 2022

View reviewed changes

daschuer requested changes Jul 8, 2022

View reviewed changes

Be-ing reviewed Jul 9, 2022

View reviewed changes

Introduce PlaylistStorage

a29f375

* Heavily inspired by CrateStorage * Move playlist summary computation from PlaylistFeature to it * PlaylistFeature no longer contains any raw SQL

github-actions bot added the build label Dec 2, 2022

ninomp added 7 commits December 2, 2022 22:20

Fix build on macOS 11

7f94c69

Use PlaylistStorage to retrieve summary of AutoDJ playlist

f059b36

DlgAutoDJ: Rename updateInfo() to updateTotalInfo()

865a429

DlgAutoDJ: Rename labelInfo to labelTotalInfo

4a110a9

PlaylistStorage: Introduce readAutoDJPlaylistSummary()

4555fe8

Show total AutoDJ track time in sidebar

8850afb

Resolve code formatting issue

7cc0230

daschuer requested changes Dec 13, 2022

View reviewed changes

ninomp added 5 commits December 21, 2022 22:00

Show AutoDJ track count in sidebar also

1798e32

Sidebar: Cache AutoDJ feature title

9fadb20

Replace QString with QStringLiteral where possible

fe7b602

DlgAutoDJ: Add null checks for accessing PlaylistDAO and PlaylistStorage

5f588b0

Remove unnecessary ASCII Art from playliststorage.h

6861456

ronso0 removed the build label Jan 17, 2023

daschuer changed the base branch from main to 2.4 January 30, 2023 13:31

daschuer reviewed Jan 30, 2023

View reviewed changes

ninomp added 2 commits February 18, 2023 00:50

Fix regression caused by fe7b602

86c9577

Merge commit '7c70ceed718471bb5b8bebd216b661f7af7819f4' into autodj_t…

ef6a67c

…otal_time

github-actions bot added the build label Feb 19, 2023

Merge commit '302b40eefe6948a7080d1ee87bedafdf48624f69' into autodj_t…

ea0e42d

…otal_time

ninomp force-pushed the autodj_total_time branch from 55485d3 to ea0e42d Compare February 20, 2023 01:49

ninomp added 2 commits February 20, 2023 02:50

AutoDJ: Indicate that total track length is (over)estimated

18dd4cb

PlaylistFeature: Address MSVC warning

85aedee

'=': conversion from 'size_t' to 'int', possible loss of data

daschuer requested changes Apr 29, 2023

View reviewed changes

ronso0 mentioned this pull request Jul 20, 2023

Show total length of remaining AutoDJ tracks #7340

Open

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Aug 23, 2023

ninomp added 6 commits November 19, 2023 00:38

Merge commit '271903bcb43826e0d9f55f0b38c1b4768b5f5999' into autodj_t…

310c57d

…otal_time

Merge commit '468ee176d0cc434ebcfa80e9aaab969f91497333' into autodj_t…

bae8706

…otal_time

Merge commit 'a09236f12a7d7a0f6e45d91101c96f3640ba5f68' into autodj_t…

767e6d6

…otal_time

Merge commit 'e66e177158fd4e2beb3edc4149d6ff7b79675983' into autodj_t…

9f901aa

…otal_time

Fix build

4c0ccae

Address code style issue detected by pre-commit

e6a954a

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Dec 24, 2023

AutoDJ: Left align selection/total time

dea1a7c

github-actions bot added the skins label Dec 28, 2023

ronso0 mentioned this pull request Apr 18, 2024

Playlists: improve table update after deleting (purging) track files #13127

Merged

daschuer marked this pull request as draft April 23, 2024 06:27

ronso0 mentioned this pull request Apr 28, 2024

Refactor: Extract the CountsDuration table logic into PlaylistStatsDAO #13177

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AutoDJ: Show total track time #4846

AutoDJ: Show total track time #4846

ninomp commented Jul 6, 2022

ronso0 commented Jul 6, 2022

uklotzde Jul 6, 2022 •

edited

Loading

daschuer Jul 7, 2022

uklotzde Jul 7, 2022

daschuer Jul 8, 2022

daschuer Jul 8, 2022

daschuer left a comment

Be-ing Jul 9, 2022

ronso0 Jul 9, 2022

Be-ing Jul 9, 2022

Be-ing Dec 13, 2022

ronso0 commented Nov 22, 2022

ninomp commented Nov 23, 2022

ninomp commented Dec 10, 2022

daschuer commented Dec 13, 2022

daschuer left a comment

daschuer Dec 13, 2022

daschuer Dec 13, 2022

daschuer Dec 13, 2022

daschuer Dec 13, 2022

daschuer Jan 30, 2023

ninomp Apr 28, 2023

daschuer Apr 29, 2023

daschuer commented Jan 30, 2023

daschuer commented Jan 30, 2023

daschuer Jan 30, 2023

ninomp Apr 28, 2023

daschuer left a comment

daschuer Apr 29, 2023

daschuer commented Apr 29, 2023

github-actions bot commented Aug 23, 2023

daschuer commented Apr 23, 2024

ninomp commented Apr 23, 2024

	label.append(QString(" (%1)").arg(summary.getTrackCount()));
	label.append(QStringLiteral(" (%1)").arg(summary.getTrackCount()));

AutoDJ: Show total track time #4846

Are you sure you want to change the base?

AutoDJ: Show total track time #4846

Conversation

ninomp commented Jul 6, 2022

ronso0 commented Jul 6, 2022

uklotzde Jul 6, 2022 • edited Loading

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

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

ronso0 commented Nov 22, 2022

ninomp commented Nov 23, 2022

ninomp commented Dec 10, 2022

daschuer commented Dec 13, 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

daschuer commented Jan 30, 2023

daschuer commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Apr 29, 2023

github-actions bot commented Aug 23, 2023

daschuer commented Apr 23, 2024

ninomp commented Apr 23, 2024

uklotzde Jul 6, 2022 •

edited

Loading