add multi-track property editor / batch tag editor

[ronso0]

I have been wanting this desperatly for ages for unifying track comments in Mixxx because the alternative workflows are a PITA, e.g. copy/paste per track, or use a tag editor and reload tags in Mixxx.

Intended to fix #9023

This is a modded/stripped clone of DlgTrackInfo, just without DlgTagFetcher and the BPM tab.
It's is not polished yet and I'll push fixes, so please don't take a too close look at the code, yet ; )

• for editable fields
• Cover show, clear and set works (shown empty if covers differ, maybe we can show a special placeholder)
• Star rating works (is 0 if values differ)

Currently supported properties

Editable fields:

• title
• artist
• album title
• album artist
• genre
• grouping (?)
• comment
• track color (rainbow button for multiple track colors)
• musical key
• cover art
• star rating

Static labels:

• BPM: show range of loaded track BPM, or nothing if all are 0 / invalid
• bitrate: show common value or range
• duration: show common value or range
• directory: show common directory or (might be helpful to grab info like album title, catalog id etc.)
• "Open in File Browser" button is enabled if all tracks are in the same dir

Special fields, to be discussed:

• BPM: I only see few valid uses cases for setting the BPM of multiple tracks:
  • BPM are all horribly off, and scaling via track menu is not applicable for some reason
  • I sometimes explicitly set BPM to 1 (and lock) in order to 'flag' tracks as 'beatless' (athmo, fields recordings and alike) when I'm too lazy to add this to the comments.

Nice to have:

• placeholder for [multiple covers] ?
• reset button for each field? (quite some work, probably not worth it, there's the 'Reload from File' button)

[ronso0]

Wooh, that was easier than I expected when I took the first look a while back (actually half the time I spent on debugging stupid mistakes..)

This seems to work well for all fields, except cover reload. fixed
(and except key and BPM, I pushed those back since I don't consider them useful here anyway)

[fwcd]

Wow, very nice to see progress on this. Got to play around with the PR once I find some time.

[acolombier]

@ronso0 is this ready for review? I can see couple of TODO and commented code. Is there some clean-up to be expected before starting review/testing?

[ronso0]

Thanks for taking a look!

I did remove commit with (cover) trace logging and temp. stuff. There are small spots I may polish (turn DEBUG_ASSERT into VERIFY_OR_DEBUG_ASSERT) but that doesn't matter much. For further changes I'll push small commits and maybe squash them later on.

[ronso0]

Currently rating and color were stored immediately (not saved to tracks of course).
I'm about to change that so all properties are handled consistently, i.e. apply all changes to track records only when saving.

[ronso0]

Please let me know when someone starts reviewing.

[daschuer]

I was able to reproduce the DEBUG_ASSERT
Here is the fix: ronso0#73

[ywwg]

my notes are addressed, thank you!

[ronso0]

I was able to reproduce the DEBUG_ASSERT Here is the fix: ronso0#73

Thanks for your investigation!
I could reproduce with Ok (tried only Apply earlier).

Please correct me if I'm wrong but I think the actual issue is clear() which clears the QHash with loaded tracks and the dialog, even though it is now hidden, can still receive the CoverArtCache::coverFound signal.

I adopted it from DlgTrackInfo but actually we don't need it here at all. These are the relevant cases:

• load tracks from WTrackTableView while a DlgTrackInfo/~Multi is open (they're not modal):
  loaded via WTrackMenu (not literally but by its function), the object hold by the unique ptr m_pDlgTrackInfoMulti is deleted when a new object is assigned
• closed with X or Cancel: no issue, dialog is deleted by WTrackMenu
  https://github.com/mixxxdj/mixxx/pull/12548/files#diff-848ff12a6bbb8c39f28e2a6be33ca2bbbe31f53e6ba550b2f6df7b8bdfaecf1fR2427-R2434
• clicked Okay (with new cover): CoverArtCache::coverFound signal is received but who cares, same as above

[daschuer]

I am afraid we need clear(), to release the reference and in turn write the changes to the track file.
Since there can always be a stray CoverFound signal, we may just deal with it and remove the DEBUG_ASSERT()

[ywwg]

build warning:

AutoUic: /home/owen/src/github/mixxx/src/library/dlgtrackinfomulti.ui: Warning: The name 'horizontalLayout_5' (QHBoxLayout) is already in use, defaulting to 'horizontalLayout_51'.
/home/owen/src/github/mixxx/src/library/dlgtrackinfomulti.ui: Warning: The name 'horizontalSpacer_6' (QSpacerItem) is already in use, defaulting to 'horizontalSpacer_61'.

[ronso0]

I am afraid we need clear(), to release the reference ...

Okay, we can do that in the destructor.

... and in turn write the changes to the track file.

I don't quite understand?

[daschuer]

I think the dialog outlives longer, is only hidden after OK. We need to take care that m_pLoadedTrack becomes nullptr, this event will then trigger the write to the file itself (if this is the last instance)

Clear() in the destructor us redundant, because it is cleared anyway.

[daschuer]

Probably a straight forward solution is to just remove the assertion.

[ronso0]

I think the dialog outlives longer, is only hidden after OK.

Maybe you overlooked it, I explained when & how the dialog is deleted here #12548 (comment)

We need to take care that m_pLoadedTrack becomes nullptr, this event will then trigger the write to the file itself (if this is the last instance)

I don't understand. Tracks are updated in saveTracks() by replacing each TrackRecord. (there is no TrackPointer m_pLoadedTrack in DlgTrackInfoMulti).
I though your comment was about explicitly clearing the QHash<TrackId, TrackPointer> (which also happens if the parent dialog is deleted, no?).

I appended a fixup that transforms the DEBUG_ASSERT (which I couldn't trigger aymore with the current state btw).
I also added a demo commit just to VERIFY_OR_DEBUG_ASSERT(!m_pLoadedTracks.isEmpty()).

[daschuer]

Oh, you are right. Now everything works flawlessly.
Can you rebase the fixup and than we can merge.

[ronso0]

Alright.

I noticed some issues I need to fix, though:

• selecting an empty item in the key box does not clear the key (TrackRecord::updateGlobalKeyNormalizeText() rejects empty strings)
• dialog is still a QTabWidget, though removing the 'Summary' tab miraculously that creates some size policiy / strecth issues with some comboboxes (I love Qt...)

[ronso0]

Sooo, the key bug is fixed.

removing the 'Summary' tab miraculously that creates some size policiy / strecth issues with some comboboxes (I love Qt...)

puuuh, hours of head scratching and fiddling with various SizePolicy combos...
Think I got it right now, at least it somewhat looks like the single-track Track Info dialog.
The "Clear" item is now only an icon, the (X) which is the default Clear icon for the search bar.

I'd appreciate another round of review if someone has the time to do so, and get this in to 2.5-alpha finally.
Thank you!

[ywwg]

I will do some end-user testing and let you know :)

[daschuer]

Thank you. Lets merge and test it during beta.