WTrackMenu: Add menu entry for (re)analyzing tracks #4806

fwcd · 2022-06-16T16:20:05Z

In an effort to make analysis more convenient, this branch adds a new submenu to the WTrackMenu for analyzing tracks (previously the user had to drag-n-drop the tracks to the analyzer queue in the sidebar):

Feel free to comment and make suggestions, this is my first actual code contribution to Mixxx so I am not super familiar with the codebase yet. :)

ronso0 · 2022-06-16T19:21:30Z

Thank you for working on this feature!

I think it makes sense to squash the two commits, why have a commit with a noop menu action.

The reanalyze action automatically clears the beatgrid and adds the selected tracks to the analysis queue.

fwcd · 2022-06-16T20:07:37Z

Done!

uklotzde

Thanks for starting to on work on this feature!

Adding a new top-level entry to the already crowded context menu would not be my first choice. Ideas how to restructure the menu are welcome. I don't have any.

src/widget/wtrackmenu.h

uklotzde · 2022-06-16T20:45:50Z

src/widget/wtrackmenu.cpp

@@ -2140,6 +2168,12 @@ bool WTrackMenu::featureIsEnabled(Feature flag) const {
                        TrackModel::Capability::RemoveCrate);
    case Feature::Metadata:
        return m_pTrackModel->hasCapabilities(TrackModel::Capability::EditMetadata);
+    case Feature::Reanalyze:
+        // TODO: Do we actually need the EditMetadata capability here?


When analyzing tracks their metadata needs to be updated for storing the results. Clearing the beat grid beforehand doesn't really matter imho.

TrackModel::Capability::Analyze should already imply TrackModel::Capability::EditMetadata. These capability flags are inconsistent and overlapping in their meaning. Everyone just extended it to keep it working. Unrelated, has to be resolved separately.

Clearing the beat grid beforehand doesn't really matter imho.

Yes, it does. Otherwise the beats where not reanalyzed inmy tests.

uklotzde · 2022-06-16T21:00:05Z

src/widget/wtrackmenu.cpp

+void WTrackMenu::addToAnalysis() {
+    const TrackIdList trackIds = getTrackIds();
+    if (trackIds.empty()) {
+        qWarning() << "No tracks selected for analysis";


We should probably use qInfo() for all these kind of log messages. I am aware that you simply copied it from existing code.

ronso0 · 2022-06-16T21:14:21Z

the action automatically clears bpm/beatgrid and adds the selected tracks to the analysis queue. The idea behind including the 'clear beatgrid' action is that (at least in my experience) this is a much more common operation than simply analyzing tracks that have no grid yet.

I think there are different use cases for (re-)analyzing which require different reset actions:

new tracks and don't like dragging to Analysis? simply analyze
edited the beat grid and messed it up? clear beats
track file was edited? clear the waveform, too
?

Could the different use cases be served with dialog incl. checkboxes for required reset actions? Even with "Remeber choices for this session" checkbox?

Ideas how to restructure the menu are welcome. I don't have any.

I agree Analyze is not really "urgent" enough to be a top-level action.
What about renaming "Reset" > "Reset / Analyze" and have all actions there?

Could anything go wrong when trying to reset beats? Beats could be locked for some of many selected tracks.. then re-analysis would be pointless (the result irritating/unexpected) without UI feedback.
Check isAnyTrackBpmLocked().
Probably better to process tracks (clearX, queue to be analyzed) one by one, like for (trackId : trackIds) {resetBeats; addToAnalysis;}, like in ClearAllPerformanceMetadataTrackPointerOperation?
Or unlock all tracks before clearing the beats?

I'm aware my proposals require quite some work to test / implement 😬

fwcd · 2022-06-16T21:20:38Z

While I agree that the menu is pretty crowded, burying analysis in a submenu IMO negates the convenience that this patch is intended to bring to a certain degree.

I'd actually argue that Analysis would make sense as a new top-level menu, given that analysis is a pretty prominent action a user takes on his tracks (and rather underrepresented in the UI currently). Additionally, there are several 'flavors' of analysis that a user might want to perform on-the-fly, that could fit into a top-level analysis menu:

'Just' analyzing
Resetting beats, then analyzing
Analyzing with constant bpm (overriding the setting)
Analyzing with variable bpm (also overriding the setting)

The latter two points would be out-of-scope for this PR, but planning ahead would make sense IMO.

ronso0 · 2022-06-16T22:21:37Z

'Just' analyzing
Resetting beats, then analyzing
Analyzing with constant bpm (overriding the setting)
Analyzing with variable bpm (also overriding the setting)

The first two already justify having an Analyze menu (not Analyze action + Analyze menu) instead of the dialog + checkbox I proposed.

daschuer · 2022-06-17T06:58:13Z

Is re-analyzing because of messed up beat grid the most common use case?

If not I can think of manual reset the desired properties first. On the other hand all other features are pretty much read only and can be re-analyzed without a risk.

The use case of reanalyze after editing the track sound itself is special, because it is probably desired to move the waveform along with cue points and beat-grid. This is not a core topic for this PR.

So for now I vote for this solution:

Analyze (missing properties)
Re-Analyze Const BPM
Re-Analyze Variable BPM

The next step would be to verify the offset, before Re-Analyze a new beat-grid, else the waveform and cue point will not match the nevw beatgrid.

Later we may add:

Re-Analyze Offset/Edits

fwcd · 2022-06-17T11:30:22Z

I would like to keep per-track constant/variable analysis out of this PR, since that seems to be quite a bit more complex to implement. Also I wouldn't do anything super fancy either in this PR, to keep the scope manageable, perhaps just an Analysis menu with two entries

Analyze
Reanalyze (with current settings)

And everything else could be determined in the future?

daschuer · 2022-06-17T12:51:32Z

That sounds reasonable

In this case it would be:

Analyze
Re-Analyze BPM

...for consistency, since it doesn't include the EditMetadata capability either.

fwcd · 2022-06-17T23:47:24Z

I have updated the menu entry to be a submenu containing both the 'simple' analysis action and one that clears the beats beforehand:

daschuer · 2022-06-19T22:26:14Z

This not just adds the tracks to the queue, it also starts the analyze, which is the main part of the use case.
How will this look like after the next step that allows to select const/non const?

What about this:

Analyze
** Missing properties
** BPM

Than we can change it later to:

BPM assume const
BPM variable

fwcd · 2022-06-19T22:33:24Z

I think calling it 'BPM' would be a bit misleading since it would analyze all other (missing) properties too. The mental model behind 'Add to Queue' is that the analyzer is already running (similar to dragging tracks into the 'analyzer' in the sidebar which to me is a queueing operation). Since that might not match the technical side of things, I agree that we probably want a better name.

daschuer · 2022-06-19T23:17:18Z

Maybe my proposed alternative is not the best. How will your proposal look like if we add the const / non const thing?

Your proposal would be translated like this:

Analysieren > Zur Warteschlange hinzufügen
Analysieren > bpm zurücksetzen und zur Warteschlange hinzufügen

IMHO the queue thing sounds like exposing an implementation detail to me.

I can also think of moving the BPM part to the Adjust BPM menu

Analyze
Adjust BPM -> Re-Analyze

fwcd · 2022-06-20T11:50:45Z

Hm, I agree that the German translation is quite verbose, but would still prefer to keep analysis-related things in a single menu. What do you think about:

Analyze (top-level menu)
- Analyze
- Reanalyze

adding

Reanalyze (constant BPM)
Reanalyze (variable BPM)

or similar in the future?

By the way, this got me thinking whether resetting the key (only on "Reanalyze") might be useful too (in case it was manually changed)?

daschuer · 2022-06-20T12:48:18Z

That works for me as well.

I would keep the key out of this game for now.
It might interesting once we have changing key support. Can be added later.

daschuer

LGTM, Thank you.
@uklotzde @ronso0: merge?

uklotzde

@daschuer Didn't check again, just merge. What I have seen so far from @fwcd all looked solid ;)

daschuer · 2022-06-23T06:54:35Z

Tank you.

github-actions bot added library ui labels Jun 16, 2022

ronso0 mentioned this pull request Jun 16, 2022

explain different ways to Analyze tracks mixxxdj/manual#488

Open

Add Reanalyze action to WTrackMenu

9f14d5c

The reanalyze action automatically clears the beatgrid and adds the selected tracks to the analysis queue.

fwcd force-pushed the wtrackmenu-reanalyze branch from 631d1b8 to 9f14d5c Compare June 16, 2022 20:07

uklotzde reviewed Jun 16, 2022

View reviewed changes

fwcd added 4 commits June 18, 2022 01:27

Rename feature to Analyze

d666ea3

Make analysis a submenu of WTrackMenu

642692f

Add Analyze capability to PlaylistTableModel

232f86f

Remove Analyze capability from BrowseTableModel

ce81e74

...for consistency, since it doesn't include the EditMetadata capability either.

Update menu action names to 'Analyze' and 'Reanalyze'

897296e

daschuer approved these changes Jun 21, 2022

View reviewed changes

uklotzde approved these changes Jun 21, 2022

View reviewed changes

daschuer merged commit 30ca720 into mixxxdj:main Jun 23, 2022

ronso0 added this to the 2.4.0 milestone Aug 5, 2022

fwcd deleted the wtrackmenu-reanalyze branch September 29, 2022 12:17

fwcd mentioned this pull request Sep 29, 2022

Analysis: Add support for overriding variable/constant BPM on a per-track basis #10931

Merged

ronso0 mentioned this pull request Jul 10, 2023

Analyse selected songs from right click menu #6551

Closed

ronso0 mentioned this pull request Aug 13, 2023

Feature request: Make track (re-)analyzing (BPM/Beatgrid) more handy #10522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WTrackMenu: Add menu entry for (re)analyzing tracks #4806

WTrackMenu: Add menu entry for (re)analyzing tracks #4806

fwcd commented Jun 16, 2022 •

edited

Loading

ronso0 commented Jun 16, 2022

fwcd commented Jun 16, 2022

uklotzde left a comment

uklotzde Jun 16, 2022

ronso0 Jun 16, 2022

uklotzde Jun 16, 2022

ronso0 commented Jun 16, 2022 •

edited

Loading

fwcd commented Jun 16, 2022

ronso0 commented Jun 16, 2022

daschuer commented Jun 17, 2022

fwcd commented Jun 17, 2022

daschuer commented Jun 17, 2022

fwcd commented Jun 17, 2022

daschuer commented Jun 19, 2022

fwcd commented Jun 19, 2022

daschuer commented Jun 19, 2022

fwcd commented Jun 20, 2022 •

edited

Loading

daschuer commented Jun 20, 2022

daschuer left a comment

uklotzde left a comment

daschuer commented Jun 23, 2022

WTrackMenu: Add menu entry for (re)analyzing tracks #4806

WTrackMenu: Add menu entry for (re)analyzing tracks #4806

Conversation

fwcd commented Jun 16, 2022 • edited Loading

ronso0 commented Jun 16, 2022

fwcd commented Jun 16, 2022

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde Jun 16, 2022

Choose a reason for hiding this comment

ronso0 Jun 16, 2022

Choose a reason for hiding this comment

uklotzde Jun 16, 2022

Choose a reason for hiding this comment

ronso0 commented Jun 16, 2022 • edited Loading

fwcd commented Jun 16, 2022

ronso0 commented Jun 16, 2022

daschuer commented Jun 17, 2022

fwcd commented Jun 17, 2022

daschuer commented Jun 17, 2022

fwcd commented Jun 17, 2022

daschuer commented Jun 19, 2022

fwcd commented Jun 19, 2022

daschuer commented Jun 19, 2022

fwcd commented Jun 20, 2022 • edited Loading

daschuer commented Jun 20, 2022

daschuer left a comment

Choose a reason for hiding this comment

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented Jun 23, 2022

fwcd commented Jun 16, 2022 •

edited

Loading

ronso0 commented Jun 16, 2022 •

edited

Loading

fwcd commented Jun 20, 2022 •

edited

Loading