Lib overview column #79
Conversation
ronso0
force-pushed
the
lib-overview-column-ro
branch
from
45028bd
to
23fb83e
Compare
ronso0
force-pushed
the
lib-overview-column-ro
branch
3 times, most recently
from
e74b62a
to
9394e6f
Compare
ronso0
force-pushed
the
lib-overview-column-ro
branch
2 times, most recently
from
c372752
to
6469090
Compare
There was a problem hiding this comment.
I left a few minor comments.
Regarding mono overview waveforms, I personally prefer to see the same stereo waveform in both WOverview and OverviewDelegate, but I understand your desire. Maybe we should make this an option in the preferences? However, I think such an option should affect WOverview too, so we would have consistent UI.
src/library/dao/trackdao.cpp
Outdated
| &Track::waveformSummaryUpdated, | ||
| this, | ||
| [trackId]() { | ||
| OverviewCache::instance()->onTrackSummaryChanged(trackId); |
There was a problem hiding this comment.
I'm concerned this makes library tightly coupled with UI.
There was a problem hiding this comment.
Well, I understand you're only concerned about the place where the connection to OverviewCache is established?
This is just a shortcut, I moved it to CoreServices.
src/library/overviewcache.cpp
Outdated
| // Create pixmap, GUI thread only | ||
| QPixmap pixmap = QPixmap::fromImage(res.image); | ||
| if (!pixmap.isNull() && !res.resizedToSize.isEmpty()) { | ||
| // we have to be sure that res.cover.hash is unique |
There was a problem hiding this comment.
This comment needs to be updated (it is still mentioning 'cover').
CMakeLists.txt
Outdated
| src/waveform/renderers/glwaveformrenderbackground.cpp | ||
| src/waveform/renderers/glvsynctestrenderer.cpp | ||
| src/waveform/renderers/waveformmark.cpp | ||
| src/waveform/renderers/waveformmarkrange.cpp | ||
| src/waveform/renderers/waveformmarkset.cpp | ||
| src/waveform/overviews/waveformoverviewrenderer.cpp |
There was a problem hiding this comment.
I believe this line needs to be just below the overviewtype.cpp to be in alphabetical order.
ronso0
force-pushed
the
lib-overview-column-ro
branch
from
6469090
to
0939a05
Compare
ronso0
force-pushed
the
lib-overview-column-ro
branch
from
0939a05
to
a747b51
Compare
This catches all additions as before but in a simpler way. Also catches updates and removals (when the track waveform has been cleared).
…act what's needed
ronso0
force-pushed
the
lib-overview-column-ro
branch
from
a747b51
to
251df7e
Compare
|
smoke testing, it works great, fast, looks BEAUTIFUL
|
src/library/overviewcache.cpp
Outdated
| // TODO Lock to prevent interferences when this is called repeatedly when | ||
| // Normalize or VisualGainAll value are changed in quick succession? | ||
| // TODO use QMultihash::key_value_iterator to collect keys and values? |
There was a problem hiding this comment.
do you want to do this checking / cleanup in this PR?
There was a problem hiding this comment.
I'm not sure how to proceed and if that's the best place to react on normalize/visual gain changes.
A locker here would basically ignore the last change notification.
Maybe we should listen to changes in OverviewDelegate and trigger a tracks view update. OverviewCache then reads the normalize/visual gain values, compares with the stored values and, if necessary, clears cached images and re-requests them one by one.
There was a problem hiding this comment.
I reworked this slot, please check if that makes sense.
There was a problem hiding this comment.
I regret that I have been promoted past the point of expertise in a lot of areas of the Mixxx codebase 😅 . This is a credit to y'all and the work that you've done to make the codebase better, but it means I'm not very good at reviewing the details any more.
But, traditionally, if a signal can be pinged many times, the signals themselves should be stuffed into a queue of some sort that is processed on a ticker (could be 50ms, this is low priority) and can be deduped.
so:
- onNormalizeOrVisualGainChanged() -> spam calls into queue (necessary to lock queue, or use threadsafe queue)
onticker() { lock queue; dedupe queue without popping; pop 1 item and process it; unlock queue}
You have to protect against queue overload (first-run with 10000 tracks being processed), so I would say only process things that are visible.
There was a problem hiding this comment.
Haha...
Thanks for the advice!
This was rather about identical "changed" signals, either from the gain spinbox or the normalize checkbox in the preferences.
So, yeah, a queue/wait/discard mechanism like here mixxxdj@abfcc99 would suffice probably.
ronso0
force-pushed
the
lib-overview-column-ro
branch
from
3a5e791
to
12a8da2
Compare
WaveformOverviewRenderer, get rid of static colors forOverviewDelegateReplayGain?incl. instant update when Normalize or Visual Gain settings are changed in preferencesTODO