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.

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

Library: add overview column #14140

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 8, 2025

continuation of #13638
This is now based on @ninomp's initial POC commit 80d8b5f
from https://github.com/ninomp/mixxx/commits/overviewsinlibrary-rebased/
(incl. the fix from #14150 )
I addressed @Swiftb0y's review and a lot evolved anyway.

image

  • the library shows a pseudo-mono view (hardcoded in OverviewCache::prepareOverview), though with a bit of refactoring this could be made optional
  • overviews are rendered as soon as track analysis finished (loading tracks not required)
  • overviews (decks and library) are updated when the library row height, overview type or the Normalize/visual gain options are changed, and the cache is cleared
  • the signal colors can be set in the <Library> node exactly like in the waveform templates (<Visual> widget)
  • WOverView and OverviewDelegate use the same renderer for all types which I extracted from WOverview to src/waveform/renderers/waveformoverviewrenderer
    (I adopted the RGB simplifications made by @Nino MP, and simplified the others, too)
  • there's an OverviewCache which holds the images sized for the overview column

Ideas / Nice to have

  • show analyzer progress / partial painting like in decks
  • add mono-mixdown / stereo option
  • click Overview column header to sort by 'has overview'
  • cleanup / optimize draw functions in WaveformOverviewRenderer
  • some sort of analysis progress indicator in the library? (though I think partial rendering would be distracting)
  • implement preview interface: click to select, click position to preview
  • show cue markers?

@ronso0
Copy link
Member Author

ronso0 commented Jan 8, 2025

Draft until CI is happy.
I'll rebase and force-push, so please don't start reviewing yet.

@ronso0 ronso0 force-pushed the lib-overview-column-ro branch 2 times, most recently from ec7c78c to b0038fb Compare January 8, 2025 13:48
@ronso0 ronso0 force-pushed the lib-overview-column-ro branch from b0038fb to fba0ecc Compare January 9, 2025 02:00
@ronso0 ronso0 marked this pull request as ready for review January 9, 2025 02:00
@ronso0
Copy link
Member Author

ronso0 commented Jan 9, 2025

I took a second look and polished it a bit.
Ready for review!

@ronso0 ronso0 force-pushed the lib-overview-column-ro branch from 17243c3 to d644456 Compare January 9, 2025 13:39
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Left some comments, not tested yet.

// Set waveforms on track AFTER they'been written to disk in order to have
// a consistency when OverviewCache asks AnalysisDAO for a waveform summary.
if (m_waveform) {
tio->setWaveform(m_waveform);
Copy link
Member

Choose a reason for hiding this comment

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

You may chang it to pTrack->...

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, done.

@@ -353,6 +351,15 @@ void AnalyzerWaveform::storeResults(TrackPointer tio) {
m_waveform,
m_waveformSummary);

// Set waveforms on track AFTER they'been written to disk in order to have
// a consistency when OverviewCache asks AnalysisDAO for a waveform summary.
if (m_waveform) {
Copy link
Member

Choose a reason for hiding this comment

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

The old code removes the waveform from the track in case of nullptr. It hink that shall remain.

if (m_waveform) {
tio->setWaveform(m_waveform);
}
if (m_waveformSummary) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, write the waveform uncoditionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

okay. I didn't consider that the analyzer may a produce null waveforms.
done.

@@ -372,6 +373,12 @@ void CoreServices::initialize(QApplication* pApp) {
m_pPlayerManager.get(),
m_pRecordingManager.get());

OverviewCache* pOverviewCache = OverviewCache::createInstance(pConfig, m_pDbConnectionPool);
connect(&(m_pLibrary->trackCollectionManager()->internalCollection()->getTrackDAO()),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
connect(&(m_pLibrary->trackCollectionManager()->internalCollection()->getTrackDAO()),
connect(&(m_pTrackCollectionManager->internalCollection()->getTrackDAO()),

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

&TrackAnalysisScheduler::trackProgress,
this,
&AnalysisFeature::trackProgress,
Qt::DirectConnection);
Copy link
Member

Choose a reason for hiding this comment

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

We should not use a direct connection here, this may cause headaches later. See my other comment.

connect(m_pLibrary,
&Library::onTrackAnalyzerProgress,
overviewWidget,
&WOverview::onTrackAnalyzerProgress);
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 have probably the a queued connection. Without the Qt::DirectConnections before, we would have the queued connection at the place wher it enteres a main thread object and here we would have automatically a direct connection.
It think this is the way to go.

Copy link
Member Author

@ronso0 ronso0 Jan 9, 2025

Choose a reason for hiding this comment

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

Thanks for the hint!
The connection are added in #14150, and since that (probably) will be merged before this PR I'll fix it there and it'll be no issue here when I squash after final LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants