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

add hotcue text labels, position, and right click menu to overview waveform #2238

Merged
merged 103 commits into from
Oct 22, 2019

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Aug 13, 2019

Text labels are rendered so they do not overlap with other labels. If the text would be too wide, it is elided. However, the can hover the mouse cursor over a label to show the whole label, temporarily hiding any following labels that would be drawn over it. Implementing this turned out to be somewhat complicated.

Without hovering:
image

With hovering, notice the labels to the right are temporarily hidden:
image
image

@Be-ing Be-ing changed the title render hotcue text labels on overview waveform [WIP] render hotcue text labels on overview waveform Aug 13, 2019
This prevents labels from overlapping without enforcing an arbitrary
limit on the width of the label. The user can hover the mouse cursor
over an elided label to show its full text. In this case, subsequent
labels are temporarily hidden until the mouse cursor is moved away.
@Be-ing Be-ing changed the title [WIP] render hotcue text labels on overview waveform render hotcue text labels on overview waveform Aug 13, 2019
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.

Thank you for this useful addition. I have left some comments.

src/waveform/renderers/waveformmarkproperties.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
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.

Here some more test results:

I have an refresh issue when editing the label with the track properties dialog. Some are instantly shown and some not.

I think the elide can be improved. The cue number should be always shown. How about this
1: 11111111111111111 2
1: 1111111111… 2
1: 1… 2
1 2
12

The hover area of a hovered label overlaps the hover area of the following marks. This way I cannot hover them any more. How about to not grow the hover area, or just define a sensitive area around the cue bar.

A cue near the end should be painted right aligned to not be cut bay the end of the waveforms.

src/waveform/renderers/waveformmarkproperties.h Outdated Show resolved Hide resolved
src/waveform/renderers/waveformrendermark.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
I do not understand why these were separate classes in the first place.
This is much easier to use and discover than finding the track in the
library, right clicking it, opening the track Properties window, and
going to the Cuepoints tab.
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2019

Woohoo! 🙌 I have added a right click menu for hotcues on the overview waveform that lets you set their label and color. This is way easier to use and discover than finding the track in the library, right clicking it, opening the Properties window, and going to the Cuepoints tab. I'll actually start using hotcues now. I think we can remove the Cuepoints tab from the Properties window now, what do you think?

Also, the overview waveform shows the time of a hotcue when hovering it. There is more work to do here to prevent that from getting drawn over the intro & outro duration text.
Screenshot from 2019-08-15 15-24-49
Screenshot from 2019-08-15 15-25-08

@Be-ing Be-ing changed the title render hotcue text labels on overview waveform add hotcue text labels, duration, and right click menu to overview waveform Aug 15, 2019
@Be-ing Be-ing changed the title add hotcue text labels, duration, and right click menu to overview waveform add hotcue text labels, position, and right click menu to overview waveform Aug 15, 2019
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 16, 2019

There is more work to do here to prevent that from getting drawn over the intro & outro duration text.

Done. Also cue labels and this position text get drawn over the playhead.
image
image

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 15, 2019

I tested decreasing the font size for the preview decks and I don't think that's a great solution. In LateNight I had to decrease the font size to 7, which is not easy to see. I'll add the option to not show the times on cue hover. I don't think that information is very helpful in the context of the preview deck.

@ronso0
Copy link
Member

ronso0 commented Oct 15, 2019

When I mentioned the mini decks I only thought of the text labels. The cue times and duration of certain ranges might be too much.

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 15, 2019

I increased the height of the overview waveforms in mini decks in Deere and LateNight so there is room for all the labels. In Tango it already worked. I attempted to fix Shade, but I got lost moving the bottom border of the mini deck. I will not put any more effort into maintaining that hacky old skin.

@daschuer
Copy link
Member

Increasing the height of the Shade mini deck is not necessary. We can just remove the hoovering. Can you also take care of the samplers?

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 15, 2019

I would prefer to not have different functionality in different skins. I will turn off the times on cue hover for the samplers.

@ronso0
Copy link
Member

ronso0 commented Oct 15, 2019

I think we can leave Shade mini decks as they are: the cue numbers are so tiny that the mouse pointer covers it up anyway, so no big deal if it's hidden by other means on hover..

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 15, 2019

Letting the labels disappear on mouse hover is IMO a bug and not acceptable for release.

@ronso0
Copy link
Member

ronso0 commented Oct 15, 2019

I'm building this branch right now.
Maybe I can help with Shade, and I'll try to fix the conflicts with the recent LateNight update.

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 22, 2019

@daschuer ready for merge?

@daschuer
Copy link
Member

Yes. Thank you very much for this good feature enabling full Cue power.
LGTM :-)

@daschuer daschuer merged commit 362cb42 into mixxxdj:master Oct 22, 2019
@Be-ing Be-ing deleted the hotcue_labels branch October 22, 2019 18:42
@ywwg
Copy link
Member

ywwg commented Oct 31, 2019

As described in https://bugs.launchpad.net/mixxx/+bug/1850644, this change removes the left click-and-drag behavior of the overview, which is used quickly scan around the track (especially to rewind to the beginning). Looking at the code I see no reason we can't restore that functionality, although it's not as simple as reverting the removed m_bDrag references.

@ronso0
Copy link
Member

ronso0 commented Nov 18, 2019

@Be-ing Maybe this was discussed here earlier and I simply don't find it, or it's an obvious feature request that would make the new hotcue features complete and even more awesome:
add play_from_cue to cue markers in the overview
https://bugs.launchpad.net/mixxx/+bug/1853043

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

Successfully merging this pull request may close these issues.

7 participants