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

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

Open
SeanBannister opened this issue Feb 18, 2024 · 8 comments
Open

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

SeanBannister opened this issue Feb 18, 2024 · 8 comments

Comments

@SeanBannister
Copy link

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
Copy link
Member

chrisn commented Feb 18, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

chrisn commented Feb 19, 2024

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
Copy link
Author

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 added a commit that referenced this issue Feb 19, 2024
This change prevents inserting new segments if the user
clicks on an existing segment, where the segment drag mode
is 'no-overlap' or 'compress'.

See #524
chrisn added a commit that referenced this issue Feb 20, 2024
@chrisn
Copy link
Member

chrisn commented Feb 20, 2024

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

@SeanBannister
Copy link
Author

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 added a commit that referenced this issue Feb 20, 2024
@chrisn
Copy link
Member

chrisn commented Feb 20, 2024

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.

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

No branches or pull requests

2 participants