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

Creation of unintentional, buggy regions #3642

Closed
ManimMadhav opened this issue Apr 8, 2024 · 21 comments · Fixed by #3654
Closed

Creation of unintentional, buggy regions #3642

ManimMadhav opened this issue Apr 8, 2024 · 21 comments · Fixed by #3654
Labels

Comments

@ManimMadhav
Copy link

Bug description:

Hi all, I have encountered a problem while working with this library which I believe might be a bug. While creating regions, there seems to be an issue with creation of regions that can be called rogue. I am attaching a video of how the unintended behaviour occurs. You can view the ID of the region that is being clicked on in the console on the right side.

bug-wavesurfer.mp4

You can see how the regions break into two while creating them. I have used a region-click event to check the region.id of the regions so created. When a region breaks into two, the first half of the region does not seem to have an ID which makes removal of that region impossible unless I restart the program (by refreshing the webpage).

You can see how I click on Clear All Regions at the end but somehow, those rogue regions do not seem to go away. Even when I click on those regions to view their ID, nothing appears in the console.

Environment:

  • Browser: Chrome
  • Version: v7.7.9, imported from: import WaveSurfer from 'https://unpkg.com/wavesurfer.js@7/dist/wavesurfer.esm.js'

Minimal code snippet:

Regions get created by dragging:

wsRegions.enableDragSelection({
    color: 'rgba(0,120,0,0.3)',
});

Region click code:

wsRegions.on('region-clicked', (region) => {
    console.log(region.id);
});

Clearing all regions code:

function clearAllMaps(){
    wsRegions.clearRegions();
}

Expected result:

Regions shouldn't be breaking into two for intended use.

Obtained result:

Region created split into two.

PS: Do let me know if I can make any clarifications, it's my first time opening an issue on GitHub. Thank you!

@ManimMadhav ManimMadhav added the bug label Apr 8, 2024
@katspaugh
Copy link
Owner

I wasn't able to reproduce this in Chrome (mac/touchpad) in this example: https://wavesurfer.xyz/examples/?regions.js

Is there any additional code that you added that might affect it? Can you make a reproducible example on codepen?

@ManimMadhav
Copy link
Author

hey @katspaugh. I have around 800 lines of code that I have used to create an entire application using the library. Could it be that I am trying to overwork the regions plugin? This issue is not reproducible all the time btw. Not sure why this is occuring, because I've started to face this issue only a few days ago and I have not made any changes to the code. Please let me know what I can do to further clarify the issue?

@katspaugh
Copy link
Owner

A few days ago there was a refactoring of the draggable function, so it may well be a regression. The problem is I don't know how to reproduce it.

Could you try going back to v7.7.6 and seeing if the bug goes away?

@ManimMadhav
Copy link
Author

i just tried all versions of 7.7.x, and the bug is replicable in every version.

@katspaugh
Copy link
Owner

katspaugh commented Apr 8, 2024

Is your app available at some URL?

@ManimMadhav
Copy link
Author

noo i have not deployed it yet :/

@VaysseRobin
Copy link

VaysseRobin commented Apr 9, 2024

I was able to reproduce the bug on https://wavesurfer.xyz/examples/?regions.js when I try to create several regions quickly (I've just removed the code that generates the predefined regions).

It seems that when clicking on the "ghost" region, it plays the region which was previously selected.

I'm on Ubuntu using Chrome with a mouse.

2024-04-09.09-47-52.mp4

@katspaugh
Copy link
Owner

@VaysseRobin @ManimMadhav can you reproduce it with this example? https://wavesurfer.xyz/examples/?064352f48990781f8e5328ed18f4301b (wavesurfer version prior to the draggable refactoring)

@ManimMadhav
Copy link
Author

No @katspaugh , I can't reproduce the error here in the link you have provided.
However, I can reproduce the error in the link @VaysseRobin shared earlier.
image

@VaysseRobin
Copy link

I can't reproduce it either in this older version.

@ManimMadhav
Copy link
Author

I can reproduce this error in my code though, while using version 7.7.6.

@leyiang
Copy link
Contributor

leyiang commented Apr 13, 2024

I have the same problem. (Ubuntu + chrome v123 + mouse)
I removed package-lock.json, node_modules, and set "wavesurfer.js": "7.7.6".
Run npm i, and the problem's gone.

@katspaugh
Copy link
Owner

Thanks for confirming @leyiang, I’ll revert the commit that caused this.

@katspaugh
Copy link
Owner

Could you guys try on this branch? https://revert-draggable.wavesurfer-js.pages.dev/#regions.js

@leyiang
Copy link
Contributor

leyiang commented Apr 13, 2024

Could you guys try on this branch? https://revert-draggable.wavesurfer-js.pages.dev/#regions.js

I tried it, and there were no issues. (at least for me)

@katspaugh
Copy link
Owner

I've published v7.7.11 with a fix.

@ManimMadhav
Copy link
Author

uhh when will this version be available on unpkg.com?

@katspaugh
Copy link
Owner

Unpkg seems to have been having issues lately, you migth want to switch to some other CDN (there are some suggestions in #3650).

@ManimMadhav
Copy link
Author

ManimMadhav commented Apr 15, 2024

have they not yet been updated with v7.7.11?
all of them still show the latest version as v7.7.10.

@katspaugh
Copy link
Owner

My bad, the package wasn’t published properly. I’ve just published 7.7.11 for real.

@ManimMadhav
Copy link
Author

thank you so much for this speedy fix @katspaugh!
it works great now :)

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

Successfully merging a pull request may close this issue.

4 participants