bbc / peaks.js

JavaScript UI component for interacting with audio waveforms
https://waveform.prototyping.bbc.co.uk
GNU Lesser General Public License v3.0
3.16k stars 275 forks source link

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

Open SeanBannister opened 4 months ago

SeanBannister commented 4 months ago

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.

chrisn commented 4 months ago

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)
SeanBannister commented 4 months ago

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.

SeanBannister commented 4 months ago

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.

chrisn commented 4 months ago

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.

SeanBannister commented 4 months ago

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.

chrisn commented 4 months ago

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

SeanBannister commented 4 months ago

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.

chrisn commented 4 months ago

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.