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

Is it impossible to disable "dragging" of createSegmentMarker ? #483

Closed coleea closed 1 year ago

coleea commented 1 year ago

version : 3.0.0-beta.8

I want to disable "segment dragging" and "segment marker dragging" when player is playing. the reason is simple. I found that if I drag a segment or drag a segment marker during segment.update() is called, the segment is duplicated

https://user-images.githubusercontent.com/7471674/230399744-67d747f7-ab5c-45e0-b4f6-9061e28be430.mp4

I think this is bug. So I want to avoid it.

I can disable "segment dragging" via view.enableSegmentDragging(). it is pretty simple.

but unfortunately, "disabling segment marker dragging" seems impossible. according to customizing.md, createSegmentMarker option is like this

draggable : This value is always true for segment marker handles.

So my question is that "Is there a way to disable this?"

or is there a way to prevent duplicating segment under the above-mentioned conditions ?

thank you.

chrisn commented 1 year ago

Thank you for reporting this, it's really helpful,

Calling segment.update() causes Peaks.js to remove the segment and add at back to the view, so this may be the cause of the issue.

I tried changing the segments.dragged event handler in one of the demo pages, to call segment.update() but this stops the drag from working. So I'm not sure exactly what causes the duplicate segment.

I'd prefer to make it so that segments can be dragged while playing, so I will continue to investigate...

If needed, I think we could change enableSegmentDragging() to also prevent dragging the markers.

chrisn commented 1 year ago

I think a solution to this would be to avoid removing and re-creating the segment shapes in SegmentsLayer._onSegmentsUpdate. But this would mean changing the custom marker API to allow the markers to be updated without re-creating them.

coleea commented 1 year ago

thank you for answer. I read SegmentsLayer._onSegmentsUpdate. Like you said, it's like a code to create after remove.

but It seems that konva.js supports updating objects directly instead of removing and rewriting. For example, rect.x (100) can move the x-coordinate, and text.text ('Hello, Konva.js!') can modify the text.

I think this way would be more efficient in terms of speed and preventing bugs that I experienced. But to do this, update logic should branch depending on which property update.

I'd like to hear your opinion

chrisn commented 1 year ago

I am currently working towards an update-based approach, but this involves some significant changes to the code, so may take some time to complete.

coleea commented 1 year ago

thank you for answer. I think this is a good choice for shfting update-based approach . And I look forward to seeing it when the code is committed to github.

chrisn commented 1 year ago

Thanks! I'll post an update here when I have something ready for you to test.

chrisn commented 1 year ago

There is a branch here with my current work in progress: https://github.com/bbc/peaks.js/tree/marker-updates. This isn't finished yet, as there are some further changes to think about.

You should now find that calling segment.update() from inside a segments.dragged handler works as expected (although changing the startTime or endTime attributes while dragging may do strange things).

The main change to the API is that custom point and segment markers now have an update method, which is called when any point or segment attribute has changed. This could replace the existing timeUpdate() method, but for now I have not removed timeUpdate().

When you create a point or segment, both the init() and update() methods are called. Calling update() seems unnecessary, so I may remove that.

I am also considering whether update() should receive an object that contains the changed values, because currently your custom marker code would have to look at the associated point or segment object (which is passed to createPointMarker or createSegmentMarker) to find out which attribute changed. But I only want to add this if it's actually needed.

Because it's not ready yet, I don't want to release this as a new beta version. So if you'd like to test it, you'll need to clone the repo and build locally, then use npm link to add to your project.

I appreciate your comments and feedback!

coleea commented 1 year ago

yeah. I'll check this soon

coleea commented 1 year ago

i'm struggling with npm link error (or bug). I'll check as soon as this problem is resolved

coleea commented 1 year ago

I checked. let me know some points

(point 1)

Segment.prototype.update = function(options) {
  if (objectHasProperty(options, 'id')) {
    throw new Error('peaks.segments.update(): id cannot be updated');
  }

 // ....
};

in your new source code, if (objectHasProperty(options, 'id')) <- this code prevent duplicating segment ?

(point 2) currently segment marker cannot be dragged. I want to check this is bug or not bug. if not bug, what should I do for moving segment marker (in my case, which action will be required in custom segment marker class ? )

chrisn commented 1 year ago

Thank you. To answer your questions:

  1. Yes, this code is there to prevent duplicate segments. This isn't a new feature, though, I just rewrote the code that handles updating segment attributes
  2. This should still work, the only thing you need to do is set editable to true when you create a segment. I tested it using the demo pages (including the custom segment marker demo). Do you see any errors in the console?
coleea commented 1 year ago

I already set {editable : true} in every segments. but dragging segment marker does not work that can be done in version 3.0.0-beta.8. my code has not been changed at all.

also I cannot found any error message in console. I want to know wether you can drag segment marker or not. If possible, It is good to share code with me

chrisn commented 1 year ago

Please try using the dist/peaks.esm.js (ES module build) or dist/peaks.ext.js (UMD build) file, not dist/peaks.js. Custom markers don't work with dist/peaks.js.

chrisn commented 1 year ago

Sorry, using npm link doesn't work (my mistake). This article explains the problem.

The way I do it is to use npm install peaks.js to add to my project, then copy the updated files:

cp ../peaks.js/peaks.js.d.ts node_modules/peaks.js/
cp ../peaks.js/dist/*.js node_modules/peaks.js/dist/
cp ../peaks.js/dist/*.map node_modules/peaks.js/dist/
coleea commented 1 year ago

can you upload this as a another package name ? ( eg : peaks.js-test )

chrisn commented 1 year ago

I'm not sure why that isn't working for you. I added some more detail to the FAQ. I think this code is stable enough for a new beta, so I have released v3.0.0-beta.9.

coleea commented 1 year ago

yeah. I tested 3.0.0-beta.9 and dragging segment marker works fine. and also checked segment-duplicating issue is solved. well done. thank you

chrisn commented 1 year ago

Thanks! I'll close this issue, but please re-open it if you have any more problems.

coleea commented 1 year ago

yes i will 😀