Cue context menu for scrolling waveform #2783

hacksdump · 2020-05-12T22:34:49Z

Adds context menu to hot cues on the scrolling waveform just like WOverview. Right-click on the label to invoke the context menu.

TODOs:

Disallow opening context menu when a track is playing.
Open context menu when right-clicking in the vicinity of marker line.
Highlight cue mark on hover.

uklotzde · 2020-05-12T22:45:57Z

~~Please rebase and retarget on master. The commit history contains unrelated commits.~~

@Be-ing Should this really be added to 2.3?

hacksdump · 2020-05-12T23:00:12Z

Should I rebase to mixxxdj:2.3 or master?

uklotzde · 2020-05-12T23:53:55Z

@hacksdump Not sure. I just saw the unrelated commits and then later noticed that @Be-ing retargeted the PR to 2.3 which might have caused them.

Since this is a new "feature" master would be more appropriate imo.

daschuer · 2020-05-13T00:05:19Z

Yes, new feature=master. I have changed the target branch.

Be-ing · 2020-05-13T00:07:06Z

This was already planned to go to 2.3 before cutting the 2.3 branch.

daschuer · 2020-05-13T00:19:25Z

We need to get used to the new dead line based release strategy.
"Only one last feature" was the source of the delay of 2.3.

2.2 was release 11 Jan 2019. If we would have stopped to add features to 2.3, it would have been release in Juli 2019. Now we would have had released 2.4, a 2.5 beta and we where considering shiny new features for 2.6.

Be-ing · 2020-05-13T01:39:47Z

I think we should have a discussion about why 2.3 took so long. But this PR is not the place for that.

uklotzde · 2020-05-13T07:36:23Z

Try to rebase onto 2.3. If this works without conflicts we could decide after the review if it is safe to merge into 2.3.

src/waveform/renderers/waveformwidgetrenderer.cpp

src/waveform/renderers/waveformwidgetrenderer.h

src/waveform/waveformmarklabel.h

src/widget/wcuemenupopup.h

src/widget/wwaveformviewer.cpp

src/waveform/renderers/waveformrendermark.cpp

src/waveform/renderers/waveformwidgetrenderer.h

src/waveform/renderers/waveformwidgetrenderer.cpp

src/widget/wwaveformviewer.cpp

Be-ing · 2020-05-15T13:27:43Z

Good work! The color change when hovering the mark is a nice detail that makes this feature easier to discover. There are a few minor ways to make the code a little easier to understand. We can use the same general approach to add right click menus for beats, downbeats, phrases, and section markers. Hopefully we can reuse some of this code too.

src/widget/wwaveformviewer.cpp

uklotzde · 2020-05-15T14:01:13Z

Code is almost ready, only some minor comments. This feature would be a nice addition for 2.3 to complete the cue improvements and extensions. Let's keep 2.3 as the target.

uklotzde · 2020-05-15T14:03:07Z

wwaveformviewer.h seems to be missing an #include directive? The SCons builds fail.

hacksdump · 2020-05-15T16:09:20Z

We can use the same general approach to add right click menus for beats, downbeats, phrases, and section markers. Hopefully we can reuse some of this code too.

This functionality to find if a point lies in/near a cue marker, beat, downbeat, phrase marker will be quite common. Can this be abstracted to a new parent class?
For this PR, I was thinking of moving this functionality to WaveformMark so that it can be commonly used by WOverview and WaveformWidgetRenderer. Suggestions?

Be-ing · 2020-05-15T16:09:45Z

For this PR, I was thinking of moving this functionality to WaveformMark so that it can be commonly used by WOverview and WaveformWidgetRenderer. Suggestions?

Seems like a good idea to me.

src/waveform/renderers/waveformwidgetrenderer.cpp

src/widget/woverview.cpp

uklotzde · 2020-05-15T20:49:07Z

i.e. git rebase --onto 2.3 8b9cfed18cec1ad57c880a056a75bc1bde3969c4^

Holzhaus · 2020-05-15T20:51:16Z

Or <hash>~1 which also works on windows.

hacksdump · 2020-05-15T20:58:12Z

Thanks. I just deleted other commits. Would have been easier to just rebase to my first commit. Fixed the scons build as well. Let's wait for CI builds.

uklotzde · 2020-05-15T21:24:34Z

Or <hash>~1 which also works on windows.

Of course, both ^ and ~1 are equivalent.

uklotzde · 2020-05-15T21:33:32Z

Works, nice feature 👍

Be-ing · 2020-05-15T21:38:57Z

There is a merge conflict since #2663 was merged. Fix that and also please use the new utility function introduced in that PR for showing CueMenuPopup in the scrolling waveform.

uklotzde · 2020-05-15T21:40:10Z

I have merged #2663. A minor merge conflict needs to be resolved and we could use mixxx::widgethelper::mapPopupToScreen() for this new popup menu.

uklotzde · 2020-05-15T21:41:03Z

Race condition while commenting on this PR ;)

hacksdump · 2020-05-15T22:22:57Z

we could use mixxx::widgethelper::mapPopupToScreen() for this new popup menu.

Writing this explicitly for every child of QMenu seems cumbersome. Every menu needs to be within mixxx boundaries.
I was thinking something along the lines of creating a new QMenu derivative which redefines the popup function and then using this class instead of QMenu to create our menus.

For this PR, I will just change WWaveformViewer in similar manner to WOverview.

Be-ing · 2020-05-15T22:57:16Z

src/widget/wwaveformviewer.cpp

-                m_pCueMenuPopup->popup(event->globalPos());
+                QPoint cueMenuTopLeft = mixxx::widgethelper::mapPopupToScreen(
+                        windowHandle()->screen()->size(),
+                        event->globalPos(),
+                        m_pCueMenuPopup->size());
+                m_pCueMenuPopup->popup(cueMenuTopLeft);


You don't need to bother going back and changing this commit, but for the future please make merge commits the bare minimum required to resolve conflicts and get it to build and run, then add new changes in following commits.

Got that. Shouldn't have combined it into the merge resolution commit. Will keep that in mind.

uklotzde · 2020-05-16T11:59:09Z

Pre-commit warnings are unrelated. LGTM.

Do we agree to merge this into 2.3?

uklotzde · 2020-05-16T19:37:42Z

Great. Thank you for contributing these UX improvements, Harshit! Working on this PR was a pleasure and also a constructive collaboration among everyone involved.

Be-ing changed the base branch from master to 2.3 May 12, 2020 22:38

Be-ing added this to the 2.3.0 milestone May 12, 2020

daschuer changed the base branch from 2.3 to master May 13, 2020 00:04

uklotzde reviewed May 13, 2020

View reviewed changes

hacksdump marked this pull request as ready for review May 13, 2020 23:20

hacksdump changed the title ~~[WIP] Cue context menu for scrolling waveform~~ Cue context menu for scrolling waveform May 14, 2020

Be-ing reviewed May 15, 2020

View reviewed changes

src/waveform/renderers/waveformrendermark.cpp Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/waveform/renderers/waveformrendermark.cpp Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/waveform/renderers/waveformwidgetrenderer.h Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/waveform/renderers/waveformwidgetrenderer.h Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/waveform/renderers/waveformwidgetrenderer.cpp Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/widget/wwaveformviewer.cpp Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/widget/wwaveformviewer.cpp Outdated Show resolved Hide resolved

uklotzde reviewed May 15, 2020

View reviewed changes

src/widget/wwaveformviewer.cpp Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/waveform/renderers/waveformwidgetrenderer.cpp Outdated Show resolved Hide resolved

Be-ing reviewed May 15, 2020

View reviewed changes

src/widget/woverview.cpp Show resolved Hide resolved

hacksdump added 13 commits May 16, 2020 02:17

waveformwidgetrenderer: remove std::optional wrapping

26b812a

wcuemenupopup: override closeEvent as protected

3643057

remove redundant inlines

5f5f7bf

remove redundant menu open state variable

ad10431

wwaveformviewer: highlight hovered mark

99aa12e

wwaveformviewer: disable cue menu when playing

58c88a6

use functor syntax for connect calls

e1f1329

waveformrendermark: cleaner rectangle initialisation

2918486

waveformmark: add function to detect point in mark

68ae87f

rename variables and change comments

2538208

wwaveformviewer: limit cue menu to hotcues

dd617cf

wwaveformviewer: fix scons build

afe1282

convert to const methods

c8e7506

hacksdump force-pushed the scrolling-waveform-cue-context-menu branch from 22b1915 to c8e7506 Compare May 15, 2020 20:55

resolve merge conflict

3deebd9

Be-ing reviewed May 15, 2020

View reviewed changes

uklotzde merged commit a440c5a into mixxxdj:2.3 May 16, 2020

mixxxbot mentioned this pull request Aug 23, 2022

add hotcue menu to scrolling waveform #9801

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cue context menu for scrolling waveform #2783

Cue context menu for scrolling waveform #2783

hacksdump commented May 12, 2020 •

edited

Loading

uklotzde commented May 12, 2020 •

edited

Loading

hacksdump commented May 12, 2020

uklotzde commented May 12, 2020

daschuer commented May 13, 2020

Be-ing commented May 13, 2020 •

edited

Loading

daschuer commented May 13, 2020

Be-ing commented May 13, 2020

uklotzde commented May 13, 2020

Be-ing commented May 15, 2020 •

edited

Loading

uklotzde commented May 15, 2020

uklotzde commented May 15, 2020

hacksdump commented May 15, 2020

Be-ing commented May 15, 2020

uklotzde commented May 15, 2020

Holzhaus commented May 15, 2020

hacksdump commented May 15, 2020

uklotzde commented May 15, 2020

uklotzde commented May 15, 2020

Be-ing commented May 15, 2020

uklotzde commented May 15, 2020

uklotzde commented May 15, 2020

hacksdump commented May 15, 2020

Be-ing May 15, 2020

hacksdump May 15, 2020

uklotzde commented May 16, 2020

uklotzde commented May 16, 2020

Cue context menu for scrolling waveform #2783

Cue context menu for scrolling waveform #2783

Conversation

hacksdump commented May 12, 2020 • edited Loading

uklotzde commented May 12, 2020 • edited Loading

hacksdump commented May 12, 2020

uklotzde commented May 12, 2020

daschuer commented May 13, 2020

Be-ing commented May 13, 2020 • edited Loading

daschuer commented May 13, 2020

Be-ing commented May 13, 2020

uklotzde commented May 13, 2020

Be-ing commented May 15, 2020 • edited Loading

uklotzde commented May 15, 2020

uklotzde commented May 15, 2020

hacksdump commented May 15, 2020

Be-ing commented May 15, 2020

uklotzde commented May 15, 2020

Holzhaus commented May 15, 2020

hacksdump commented May 15, 2020

uklotzde commented May 15, 2020

uklotzde commented May 15, 2020

Be-ing commented May 15, 2020

uklotzde commented May 15, 2020

uklotzde commented May 15, 2020

hacksdump commented May 15, 2020

Be-ing May 15, 2020

Choose a reason for hiding this comment

hacksdump May 15, 2020

Choose a reason for hiding this comment

uklotzde commented May 16, 2020

uklotzde commented May 16, 2020

hacksdump commented May 12, 2020 •

edited

Loading

uklotzde commented May 12, 2020 •

edited

Loading

Be-ing commented May 13, 2020 •

edited

Loading

Be-ing commented May 15, 2020 •

edited

Loading