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.21k stars 280 forks source link

Can't seek if insert-segment is enabled #539

Open jason30704 opened 2 months ago

jason30704 commented 2 months ago

Hello, WaveformDragMode currently only has 'scroll' and 'insert-segment'. But this will make zoomview unable to seek. Is it possible for ZoomView to retain the ability to seek even when scrolling is disabled?

Such as providing "insert-point" and "seek-without-scroll" mode.

chrisn commented 2 months ago

Currently, to seek in the zoomview you can either click in the waveform or drag the playhead, so this should still work if scrolling is disabled. Insert segment mode does prevent seek, I think it might be confusing if seek is also allowed.

jason30704 commented 2 months ago

But if I want to prevent zoomview scroll now, can't I only set Insert segment mode? This means that if you want zoomview to be unable to scroll, it will only be in Insert segment mode. Therefore, it will not be able to seek. Or maybe I misunderstood some part. thank you.

chrisn commented 2 months ago

Sorry, I think that's right, there currently isn't a way to only disable scrolling.

chrisn commented 2 months ago

I'm not sure I understand what you want to be able to do. Do you want an option to disable scrolling, but also not insert segments?

jason30704 commented 2 months ago

Yes, I hope that zoomview can disable scrolling and still retain the ability to seek when clicking. But currently using insert-segments, I will not be able to seek by clicking.

chrisn commented 2 months ago

Thanks, so we could add an option and method to enable and disable scrolling. But we'll need to think about how this would interact with the existing autoScroll option and enableAutoScroll(boolean, options) method.

jason30704 commented 2 months ago

Intuitively, I feel that if scrolling is disabled, autoScroll should also be disabled. Unless you go through the settings again. Is there any conflict between these two settings so that you need to consider specifically?

chrisn commented 2 months ago

I agree, disabling scroll would also disable auto-scroll. I'm thinking about the API we might provide. The simplest is to add a new enableScroll(boolean) method, and no config option, so the default is scroll enabled.

I thought about combining the auto-scroll controls into a single enableScroll(boolean, options) but this is probably more confusing:

view.enableScroll(false); // Disables all scrolling
view.enableScroll(true); // Enables scroll and auto-scroll
view.enableScroll(true, { auto: false }); // Enables scroll but not auto-scroll
                                          // same as existing view.enableAutoScroll(false)
view.enableScroll(true, { offset: pixels }); // Enables scroll and auto-scroll, sets auto-scroll offset
jason30704 commented 2 months ago

Based on my current experience, I agree that separate settings may be more suitable for current project usage. Also, I think add 'insert-point' to WaveformDragMode is a good idea.