Raruto / leaflet-rotate

Leaflet plugin that allows to add rotation functionality to map tiles
GNU General Public License v3.0
77 stars 21 forks source link

fix: remove duplicated event listeners for `L.Marker` when `draggable: true` #34

Closed yuri-karelics closed 1 year ago

yuri-karelics commented 1 year ago

Current logic with patching marker drag is to replace 'native' listeners with 'patched' listeners. But class extend goes before 'dragging.disable()', so for disable already new handlers exists in class, so old listeners remains. As a result we have two listeners: image

Fix: 'dragging.disable()' should run before extend. image

@Raruto could you please check, merge and release patch version, thanks

yuri-karelics commented 1 year ago

Hi @Raruto , ok, I can do it (in addition to screenshots describing the problem). One question: are your tests configured to run on windows machine or...? I am asking because I am not familiar with 'uvu' tool and I got the following error running 'npm run test'

image
yuri-karelics commented 1 year ago

ok, with 'uvu examples' it works and I believe it will be hard to convince you fix is needed, because one test fails with my fix :) but as you can see from screenshots problem exists ('native' listeners are not removed, so double logic is executed with unexpected results). I'll try to find what is wrong with test and how to change it.

yuri-karelics commented 1 year ago

...now it is green either with my fix or not... I'll add some additional test, but I think problem requires your personal attention

yuri-karelics commented 1 year ago

@Raruto done, one more test added, you can see it fails without small fix from this PR. 'pan inside marker' test is a bit unstable (but it is not because of my changes), it sometimes fails

Raruto commented 1 year ago

are your tests configured to run on windows machine?

Yes, i only tested on windows.

Probably to make it cross-compatible we should add some escape characters around that regex:

https://github.com/Raruto/leaflet-rotate/blob/e082bf38b1fbb5be6cf0ec07af9e57552e70aa67/package.json#L10

eg: uvu . '^(examples)?(src)?\\.*\\.spec\\.js$' (👉 https://github.com/lukeed/uvu/issues/13)

It would be a 👍, if you can work it out.

'pan inside marker' test is a bit unstable

Don't worry, as you can see from the comment at top it refers to an issue that has been open for a long time:

https://github.com/Raruto/leaflet-rotate/blob/e082bf38b1fbb5be6cf0ec07af9e57552e70aa67/examples/leaflet-rotate.spec.js#L56-L59

yuri-karelics commented 1 year ago

@Raruto on mac it works like this "test": "uvu . '^(examples)?(src)?.*.spec.js$'", - so, no escape characters needed at all. Could you check if it works on Windows this way?

yuri-karelics commented 1 year ago

and maybe it can be simplified to 'uvu examples'? Because looks like current regex if effectively equals to '.*.spec.js', because it targets all files with 'spec.js' ending in all folders. I would say slash is missing after (examples)?(src)? if you want to stick only to these folders

yuri-karelics commented 1 year ago

@Raruto one more adjustment "test": "uvu . '^(examples)?(src)?/.*\\.spec\\.js$'", - escape characters needed to specify '.spec.js' ending exactly (not any character on the . place). And slash added. Give me know if it works on Windows, I'll push the change

Raruto commented 1 year ago

@yuri-karelics single quotes seem to be problematic on windows (node scripts).

Rewriting it as follows seems to work correctly for me:

"test": "uvu . \"(examples|src)\\\\.*\\.spec\\.js$\""

NB the leading ^ character has been removed

Let me kwnow, Raruto

yuri-karelics commented 1 year ago

@Raruto yes, it works, made a commit, but with couple of adjustments. It is regex on file path (if I got it right), so now logic is: path should contain 'examples' or 'src'; then '/' to highlight it is a folder name, not part of filename; then any combination of characters (.*), dot is special symbol for any character here so not excaped; then exactly '.spec.js', dot is the dot here so it is escaped If you agree, maybe it is time to merge

Raruto commented 1 year ago

Maybe we're almost there, the following version should also take into account the different directory separator (slash direction):

"test": "uvu . \"(examples|src)[\\\\\/].*\\.spec\\.js$\""

NB [\\\\\/] what a lotta of escapes for just two characters.. 🫠

yuri-karelics commented 1 year ago

yep, it works on my side too

Raruto commented 1 year ago

Patch released in v0.2.8

yuri-karelics commented 1 year ago

actually, this issue https://github.com/Raruto/leaflet-rotate/issues/28 can be related to this fix. At least I started my investigation when faced with kind of the same problem - coordinates jump on marker drag (because of redundant 'native' drag listener). Can be double checked on released version and probably closed