katspaugh / wavesurfer.js

Audio waveform player
https://wavesurfer.xyz
BSD 3-Clause "New" or "Revised" License
8.76k stars 1.62k forks source link

Selection Canvas should not be visible if loopSelection is false #123

Closed lionelB closed 10 years ago

lionelB commented 10 years ago

Hello,

As a user, I should not be able to make a selection on the wave if I a don't plan to use it. By default, the option "loopSelection" is set to false so canvas selection should not be visible.

What do you think ?

katspaugh commented 10 years ago

If you ask me, loopSelection and useSelection should be two different options. By the way, why do you not want to have it always enabled? Do you use mouse drag for something else?

mwise commented 10 years ago

The loopSelection option/feature and the actual selection action are indeed separate features by design. It seemed so intuitive and useful to have drag-select that I didn't bother adding the 'useSelection' option that @katspaugh references. As it is, the getSelection() method will return the selection start and end position/percentage, which is useful when you are not using the loopSelection feature.

For me, this would be a non-fix, but I could easily add the useSelection option (although I would probably leave it as is).

lionelB commented 10 years ago

@mwise I don't say that drag-select is not useful generally, just that it's not useful for minimal use cases

@katspaugh What I'm trying to say is if wavesurfer is only here to handle audio playback and navigation, allow to do drag-select allows the user to guess that he can interact with that selection, that's why I think that drag-selection should not be supported or displayed for a minimal use case. Feature augmentation should be done through plugin, ihmo.

Hope my words are clearer

mwise commented 10 years ago

@lionelB right on, I understand where you're coming from. I could see how it could be confusing to have a selectable region w/o the corresponding end-user functionality to use the selection. I'll defer to @katspaugh on this. I would vote to keep it in Wavesurfer core as an option, since a) integration is pretty deep with respect to the position-calculation stuff and b) the selection as a plugin wouldn't have any configurable options in the current state of it. Also, selfishly, I feel like I'd want to use it in more cases than not and don't want to include/initialize a plugin each time :-P

lionelB commented 10 years ago

@mwise Glad to be understood. To be honest, I just review briefly the code related to that feature, so I imagine there were good reasons to put that in the core :)

katspaugh commented 10 years ago

I agree, that selection without looping (or at least stopping in the end of selection), or zooming, may seem confusing. However, I think it does belong to the core – like seek-on-click and markers.

Maybe, binding drag selection through an option (enableDragSelection, off by default) and enabling loopSelection by default, would be unobtrusive enough while not too verbose to turn on. Of course, programmatic selection should be available unconditionally.

I let @mwise make the final decision, as a person who understands the use case better than me and has a clearer picture of forthcoming zoom feature (which I believe is connected to selection).

mwise commented 10 years ago

Ok, I added an option for this and created a pull request (https://github.com/katspaugh/wavesurfer.js/pull/126) for it.

katspaugh commented 10 years ago

Mark, thank you. I agree both options should be on by default. Done.

lionelB commented 10 years ago

Thanks. You guys rock!

2014-02-21 8:25 GMT+01:00 katspaugh notifications@github.com:

Closed #123 https://github.com/katspaugh/wavesurfer.js/issues/123.

— Reply to this email directly or view it on GitHubhttps://github.com/katspaugh/wavesurfer.js/issues/123 .