Add command for clearing waveform to track's context menu by ninomp · Pull Request #1197 · mixxxdj/mixxx

ninomp · 2017-02-27T22:17:18Z

This PR is aiming to fix https://bugs.launchpad.net/mixxx/+bug/1014449

This is a continuation of #841.

daschuer

Thank you for coming back to this.
Your changes are looking reasonable.
Is the original issue from https://github.com/mixxxdj/mixxx/pull/841gone by the new strategy?

Did you consider the situation if the analysis is running during clear?

daschuer · 2017-02-28T07:24:53Z

+    } else {
+        // Null waveform pointer means waveform was cleared.
+        if (m_pWaveformSourceImage) {
+            delete m_pWaveformSourceImage;


The ownership of m_pWaveformSourceImage is unclear. This is not your fault, but we should fix it here, because this PR builds on top of this situation.
I think the best solution is to move m_pWaveformSourceImage into the inherited classes and put it into a std::unique_ptr
this WOverview should have a abstract virtual void invalidateImage() = 0, that is then implemented in the Inherited classes to actually clear the Image.

I have decided to take a (slightly) different approach with this and make this QImage statically allocated (like m_waveformImageScaled is). I hope you don't mind. 😃

daschuer · 2017-02-28T07:31:32Z

+        if (!pTrack) {
+            continue;
+        }
+        analysisDao.deleteAnalysesForTrack(pTrack->getId());


Will this delete the all analysis data?
I think we should rethink the use-cases for "ClearWaveform" and name this feature accordingly.
IMHO clearing all analysis makes sense if the use-case is, that the track samples have changed.
In this case all position related data is invalid.
What will happen with cue points?

Uhm Well, by looking at the AnalysisDao code, AnalysisDao is used just for storing waveforms, so this indeed is only clearing track's waveforms.

daschuer · 2017-02-28T07:34:53Z

+
+void WWaveformViewer::slotWaveformUpdated() {
+    if (m_waveformWidget) {
+        m_waveformWidget->setTrack(m_waveformWidget->getTrackInfo());


What dos this line? Can we replace it by a member function with a nice name?

daschuer · 2017-02-28T07:37:38Z

+
+    if (pNewTrack) {
+        connect(pNewTrack.get(), SIGNAL(waveformUpdated()),
+                this, SLOT(slotWaveformUpdated()));


the track was just cleared above. Can we move this to slotTrackLoaded, to keep symmetry?

Be-ing · 2017-03-05T18:30:03Z

The track context menu is already enormous. Let's not add anything else to it. Perhaps this should go in the track Properties dialog somewhere.

ronso0 · 2017-03-05T22:37:11Z

The track context menu is already enormous. Let's not add anything else to it. Perhaps this should go in the track Properties dialog somewhere.

I see what you mean, but here's a use case where it would be great to have 'Clear Waveform' button handy, maybe below 'Reset Replay Gain':
I have a large library where most of the tracks were analyzed by mixxx <=2.0, so I stumble across displaced waveforms quite often. It would be easy to throw those tracks into a temporary crate, then mark all & click "Clear Waveform" instead of going to each tracks Properties.

To clean up the track context menu, all AutoDJ entries could go to a submenu.
'Reset Play Count' could go to track Properties, I'd say.

Be-ing · 2017-03-06T04:08:37Z

I understand your point about the usefulness of having this in the track context menu. I think the context menu could be cleaned up by moving what is in each divider into a submenu.

…erSignal

Be-ing · 2017-03-06T14:48:53Z

        m_waveformWidget = nullptr;
    }

-    void slotWaveformUpdated();


Was there an issue with keeping this here? This needs to work for all waveform renderers, not just GLSL.

The thing is that only GLSL renderer was suffering from the issue of not updating the waveform (that is the issue from #841 I couldn't solve then).

Oh, okay then. Please add a comment explaining that.

Done in 399d6c9.

ninomp · 2017-03-06T15:29:02Z

@daschuer Yes, I have managed to solve the issue from #841. The problem was that WOverview was keeping the old waveform rendered in its m_pWaveformSourceImage and GLSLWaveformRendererSignal was keeping the old waveform in texture.

I tested the scenario of clearing the waveform while analyzer is running. Mixxx does not crash and waveform remains cleared after the analysis is over. I believe this behavior is acceptable. What do you think?

@Be-ing @ronso0 I agree that track context menu is crowded, but I am unsure about which actions to move to sub-menu or track properties dialog.

ronso0 · 2017-03-06T15:43:41Z

I agree that track context menu is crowded, but I am unsure about which actions to move to sub-menu or track properties dialog.

As I said, I'd vote to keep "Clear Waveforms" in context menu for now. A general clean-up could be discussed in another PR, I suppose there are diverse opinions on what's important and what's not ;)

Be-ing · 2017-03-06T16:36:41Z

After this is merged I'll work on cleaning up the context menu and bringing it up when right clicking on WTrackProperty (title and artist text).

…ated() signal

ninomp · 2017-03-10T19:41:26Z

I have addressed all the review comments, so I believe this is done.

ninomp · 2017-03-20T12:13:39Z

@daschuer Since there has been no activity here in the last 10 days, what is left to do to finish this PR?

Be-ing · 2017-03-29T15:47:20Z

Ping @daschuer. Ready for merge?

daschuer · 2017-03-29T21:45:21Z

I hope I find time for his soon.

daschuer · 2017-03-30T17:29:22Z

LGTM, and works good as well :-) Thank you.

rryan · 2017-04-17T02:57:49Z

+    // When the track's waveform has been changed (or cleared), it is necessary
+    // to update (or delete) the texture containing the waveform which was
+    // uploaded to GPU. Otherwise, previous waveform will be shown.
+    connect(pTrack.get(), SIGNAL(waveformUpdated()),


What about disconnecting the signal? I think this means clearing a track also reloads the texture for all decks it was loaded in (even if a new track has replaced it).

esbrandt · 2017-06-30T10:38:26Z

@ninomp
Does this work with latest master?
The Clear waveform just clears the waveform if the track is playing in a deck. Nothing is written to the database, even on shutdown.
I´d expect this feature to delete the cached waveform file from the disk, and to clear the database entry for the waveform.

Am i mistaken how this feature is supposed to work?
Thanks for clarification

ninomp · 2017-06-30T12:45:58Z

HI @esbrandt. Yes, that is how this is supposed to work. I have just checked and I can confirm that this is not working like it is supposed to. I will try to fix it. Thank you for warning.

Be-ing · 2017-06-30T13:13:21Z

I presume that is a regression caused by the recent work on the database connection handling and analyzers:
#1276, #1282, #1285, #1289

esbrandt · 2017-06-30T19:11:15Z

Thanks for the update, looking forward to the fix.

Add command for clearing waveform to track's context menu

1c354e3

daschuer requested changes Feb 28, 2017

View reviewed changes

Move slotWaveformUpdated() from WWaveformViewer to GLSLWaveformRender…

162a899

…erSignal

Be-ing reviewed Mar 6, 2017

View reviewed changes

Be-ing mentioned this pull request Mar 6, 2017

WTrackProperty mouse click actions #1209

Closed

ninomp added 3 commits March 8, 2017 22:53

Use statically allocated QImage in WOverview

511b31f

Fix a few memory leaks

3228d77

Explain why GLSLWaveformRendererSignal needs to listen to waveformUpd…

399d6c9

…ated() signal

daschuer approved these changes Mar 30, 2017

View reviewed changes

daschuer merged commit e6112fa into mixxxdj:master Mar 30, 2017

rryan reviewed Apr 17, 2017

View reviewed changes

ninomp mentioned this pull request Jul 1, 2017

Fix clear waveform #1299

Merged

esbrandt mentioned this pull request Apr 15, 2018

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

ninomp deleted the clear_waveform branch April 24, 2019 16:55

mixxxbot mentioned this pull request Aug 22, 2022

waveform does not update if music files are modified #7398

Open

Uh oh!

Conversation

ninomp commented Feb 27, 2017

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Mar 5, 2017

Uh oh!

ronso0 commented Mar 5, 2017

Uh oh!

Be-ing commented Mar 6, 2017

Uh oh!

Be-ing Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ninomp commented Mar 6, 2017

Uh oh!

ronso0 commented Mar 6, 2017

Uh oh!

Be-ing commented Mar 6, 2017

Uh oh!

ninomp commented Mar 10, 2017

Uh oh!

ninomp commented Mar 20, 2017

Uh oh!

Be-ing commented Mar 29, 2017

Uh oh!

daschuer commented Mar 29, 2017

Uh oh!

daschuer commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbrandt commented Jun 30, 2017

Uh oh!

ninomp commented Jun 30, 2017

Uh oh!

Be-ing commented Jun 30, 2017

Uh oh!

esbrandt commented Jun 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Be-ing Mar 6, 2017 •

edited

Loading