bbc/peaks.js

segments.click doesn't trigger if insert-segment is enabled.

SeanBannister opened this issue · 8 comments

If you enable zoomview.setWaveformDragMode('insert-segment'); then peaksInstance.on('segments.click' refuses to trigger so you can't get a callback when a user clicks on a segment.

My use case is needing to know when a user clicks on an existing segment they created so I can mark it as selected.

Here's a demo of the bug, please scroll past the boilerplate code to line 152.

With insert-segment mode, any click will result in creating a new segment (whether inside our outside an existing segment). I'd expect these new segments to become selected on creation, which you do through the segments.add event.

To click to select existing segments, you'd have to switch insert-segment mode off.

This isn't to say there isn't an issue with segments.click events. Some notes below on which events are currently generated in different scenarios (not saying these are correct, just what currently happens).

A click event happens after a mousedown+mouseup. What do you expect to happen if the mousedown is within a segment but the mouseup is outside the segment? This could easily happen in insert-segment mode depending on where the user drags. Currently this does not result in a segments.click event, and it's not clear to me that it should.

Insert segment mode

mousedown within segment, mouseup within segment:

segments.mouseenter: {segment: Segment, evt: MouseEvent}
segments.mousedown: {segment: Segment, evt: MouseEvent}
segments.add: {segments: Array(1), insert: true}
segments.dragstart: {segment: Segment, marker: true, startMarker: false, evt: undefined} // evt is undefined
segments.dragged: {segment: Segment, marker: true, startMarker: false, evt: MouseEvent}
segments.dragend: {segment: Segment, marker: true, startMarker: false, evt: MouseEvent}
segments.insert: {segment: Segment}
// no segments.mouseup event
// no segments.click event
// no zoomview.click event

mousedown within segment, mouseup outside segment:

segments.mouseenter: {segment: Segment, evt: MouseEvent}
segments.mousedown: {segment: Segment, evt: MouseEvent}
segments.add: {segments: Array(1), insert: true}
segments.dragstart: {segment: Segment, marker: true, startMarker: false, evt: undefined}
segments.dragged: {segment: Segment, marker: true, startMarker: false, evt: MouseEvent}
segments.dragend: {segment: Segment, marker: true, startMarker: false, evt: MouseEvent}
segments.insert: {segment: Segment}
// no segments.mouseleave event
// no segments.click event (not expected)
// no zoomview.click event

mousedown outside segment, mouseup within segment

segments.add: {segments: Array(1), insert: true}
segments.dragstart: {segment: Segment, marker: true, startMarker: false, evt: undefined}
segments.dragged: {segment: Segment, marker: true, startMarker: false, evt: MouseEvent}
segments.dragend: {segment: Segment, marker: true, startMarker: false, evt: MouseEvent}
segments.insert: {segment: Segment}
segments.mouseenter: {segment: Segment, evt: MouseEvent} // emitted after mouseup, not when mouse enters the segment
// no segments.click (not expected)
// no zoomview.click event

Scroll mode

mousedown within segment, mouseup within segment:

segments.mouseenter: {segment: Segment, evt: MouseEvent}
segments.mousedown: {segment: Segment, evt: MouseEvent}
segments.mouseup: {segment: Segment, evt: MouseEvent}
segments.click: {segment: Segment, evt: MouseEvent, preventViewEvent: ƒ}
zoomview.click: {time: 150.16344671201813, evt: MouseEvent}
segments.mouseleave: {segment: Segment, evt: MouseEvent}

mousedown within segment, mouseup outside segment:

segments.mousedown: {segment: Segment, evt: MouseEvent}
segments.mouseleave: {segment: Segment, evt: MouseEvent}
zoomview.click: {time: 16.27718820861678, evt: MouseEvent}
// no segments.click event (not expected)

Thank you so much for your detailed response, that really helps me understand the inner workings. I wonder if the segments.click event should still be triggered for flexibility?

So my particular use case also has view.setSegmentDragMode('no-overlap') enabled so users never create a segment on or over an existing segment. view.setSegmentDragMode('no-overlap') doesn't currently prevent clicks on segments, if you click on an existing segment it'll allow you to create that segment.

I was hoping that the segments.click event would still be fired so I could prevent the default action of creating a segment (not exactly sure how to do this, maybe just delete the segment quickly). And then I could instead change the colour of the segment to show it as selected. And the user can perform actions on it such as pressing the delete key to delete it.

I'm just porting from wavesurfer and this is their default behaviour so just trying to replicate what my users are already doing, here is their example showing this.

Thanks, I think I understand what you want to do. Should Peaks.js automatically prevent creating a new segment if you click an existing segment in no-overlap mode? At the moment, no-overlap only affects dragging of existing segments, not creating new ones.

Yes, I think from a usability perspective Peaks.js should prevent creating a new segment if you click an existing segment with no-overlap enabled. That would certainly meet my use case.

And ideally still trigger segments.click so I can detect when a segment is clicked.

This should be working now, please try out v3.2.3 and let me know what you think. Thanks!

Thank you so much for this, greatly appreciate that you could take your time to sort this for me.

I was just doing a quick bit of QA and I did notice one side effect which doesn't actually affect my use case but might catch someone else out, with no-overlap enabled clicking a segment still triggers segments.insert even though no segment is inserted.

I forgot to add code to reset the state on mousedown, so it would work the first time but not after that. I've just published another update.