Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AutoDJ: Show total track time #4846

Draft
wants to merge 26 commits into
base: 2.4
Choose a base branch
from
Draft

AutoDJ: Show total track time #4846

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f0b4f6d
DlgAutoDJ: Show total track time
ninomp Jul 5, 2022
a29f375
Introduce PlaylistStorage
ninomp Jul 10, 2022
7f94c69
Fix build on macOS 11
ninomp Dec 2, 2022
f059b36
Use PlaylistStorage to retrieve summary of AutoDJ playlist
ninomp Dec 2, 2022
865a429
DlgAutoDJ: Rename updateInfo() to updateTotalInfo()
ninomp Dec 2, 2022
4a110a9
DlgAutoDJ: Rename labelInfo to labelTotalInfo
ninomp Dec 2, 2022
4555fe8
PlaylistStorage: Introduce readAutoDJPlaylistSummary()
ninomp Dec 3, 2022
8850afb
Show total AutoDJ track time in sidebar
ninomp Dec 4, 2022
7cc0230
Resolve code formatting issue
ninomp Dec 10, 2022
1798e32
Show AutoDJ track count in sidebar also
ninomp Dec 21, 2022
9fadb20
Sidebar: Cache AutoDJ feature title
ninomp Dec 21, 2022
fe7b602
Replace QString with QStringLiteral where possible
ninomp Dec 21, 2022
5f588b0
DlgAutoDJ: Add null checks for accessing PlaylistDAO and PlaylistStorage
ninomp Jan 16, 2023
6861456
Remove unnecessary ASCII Art from playliststorage.h
ninomp Jan 16, 2023
86c9577
Fix regression caused by fe7b602e653ae177ed11c9f6f2ba24d23680584b
ninomp Feb 17, 2023
ef6a67c
Merge commit '7c70ceed718471bb5b8bebd216b661f7af7819f4' into autodj_t…
ninomp Feb 18, 2023
ea0e42d
Merge commit '302b40eefe6948a7080d1ee87bedafdf48624f69' into autodj_t…
ninomp Feb 19, 2023
18dd4cb
AutoDJ: Indicate that total track length is (over)estimated
ninomp Feb 19, 2023
85aedee
PlaylistFeature: Address MSVC warning
ninomp Apr 28, 2023
310c57d
Merge commit '271903bcb43826e0d9f55f0b38c1b4768b5f5999' into autodj_t…
ninomp Nov 18, 2023
bae8706
Merge commit '468ee176d0cc434ebcfa80e9aaab969f91497333' into autodj_t…
ninomp Nov 18, 2023
767e6d6
Merge commit 'a09236f12a7d7a0f6e45d91101c96f3640ba5f68' into autodj_t…
ninomp Nov 18, 2023
9f901aa
Merge commit 'e66e177158fd4e2beb3edc4149d6ff7b79675983' into autodj_t…
ninomp Nov 19, 2023
4c0ccae
Fix build
ninomp Dec 20, 2023
e6a954a
Address code style issue detected by pre-commit
ninomp Dec 22, 2023
dea1a7c
AutoDJ: Left align selection/total time
ninomp Dec 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/library/autodj/dlgautodj.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ DlgAutoDJ::DlgAutoDJ(WLibrary* parent,
this,
&DlgAutoDJ::updateSelectionInfo);

connect(&pLibrary->trackCollectionManager()
->internalCollection()
->getPlaylistDAO(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another place of a "->" row.

&PlaylistDAO::tracksChanged,
this,
&DlgAutoDJ::updateInfo);

connect(pLibrary,
&Library::setTrackTableFont,
m_pTrackTableView,
Expand Down Expand Up @@ -212,6 +219,7 @@ DlgAutoDJ::DlgAutoDJ(WLibrary* parent,
autoDJStateChanged(m_pAutoDJProcessor->getState());

updateSelectionInfo();
updateInfo();
}

DlgAutoDJ::~DlgAutoDJ() {
Expand Down Expand Up @@ -362,7 +370,7 @@ void DlgAutoDJ::updateSelectionInfo() {

if (!indices.isEmpty()) {
label.append(mixxx::DurationBase::formatTime(duration));
label.append(QString(" (%1)").arg(indices.size()));
label.append(QString(" (%1) / ").arg(indices.size()));
ronso0 marked this conversation as resolved.
Show resolved Hide resolved
labelSelectionInfo->setToolTip(tr("Displays the duration and number of selected tracks."));
labelSelectionInfo->setText(label);
labelSelectionInfo->setEnabled(true);
Expand All @@ -372,6 +380,22 @@ void DlgAutoDJ::updateSelectionInfo() {
}
}

void DlgAutoDJ::updateInfo() {
ronso0 marked this conversation as resolved.
Show resolved Hide resolved
double duration = 0.0;

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

@uklotzde uklotzde Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

duration += pTrack->getDuration();
}

QString label;
label.append(mixxx::DurationBase::formatTime(duration));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

label.append(QString(" (%1)").arg(rows));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


labelInfo->setText(label);
}

bool DlgAutoDJ::hasFocus() const {
return m_pTrackTableView->hasFocus();
}
Expand Down
1 change: 1 addition & 0 deletions src/library/autodj/dlgautodj.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class DlgAutoDJ : public QWidget, public Ui::DlgAutoDJ, public LibraryView {
void transitionSliderChanged(int value);
void autoDJStateChanged(AutoDJProcessor::AutoDJState state);
void updateSelectionInfo();
void updateInfo();
void slotTransitionModeChanged(int comboboxIndex);
void slotRepeatPlaylistChanged(int checkedState);

Expand Down
7 changes: 7 additions & 0 deletions src/library/autodj/dlgautodj.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,13 @@
</property>
</widget>
</item>
<item>
<widget class="QLabel" name="labelInfo">
<property name="text">
<string/>
</property>
</widget>
</item>
</layout>
</widget>
</item>
Expand Down