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

Fix bugs #1

Merged
merged 5 commits into from
Jul 10, 2019
Merged

Fix bugs #1

merged 5 commits into from
Jul 10, 2019

Conversation

modos189
Copy link

Library is not compiled because of a typo. Fixed, plus added extra check to prevent errors.

@johnd0e
Copy link

johnd0e commented Apr 29, 2019

@Spaction
153525e introduced regression in event handling, open Issues section to discuss.

@johnd0e
Copy link

johnd0e commented May 1, 2019

Also see IITC-CE#10

@johnd0e
Copy link

johnd0e commented May 3, 2019

size = b.getSize(),
m = L.Browser.retina ? 2 : 1;
L.DomUtil.setPosition(container, b.min);
},

size is already defined.
size and m assigned a value but never used.


var iconOptions = marker.options.icon.options;

iconOptions is assigned a value but never used.



self is not needed here as it is only used in the same scope (with the same this).

@johnd0e
Copy link

johnd0e commented May 3, 2019

removeLayers: function (layers) {
var self = this;
layers.forEach(function (e) {
self.removeMarker(e, false);
});
if (redraw && redraw === true)
self.redraw();
},

redraw is not defined.

`this._markers` is undefined in `_searchPoints`, called from event listeners.
Now we check markers existence in the beginning of handlers.

Note 1:
Could be fixed in some another place:
 e.g. check `_markers` in `_searchPoints`
 ...or initialize (empty) `_markers` on CanvasMarkerLayer init

Note 2:
`!this._map` removed from condition in `_onMouseMove:` 'cause
it's never should be true as we detach listeners on remove from map
@johnd0e
Copy link

johnd0e commented May 11, 2019

Concerning last fix (ab64e18): in several places of code there are excessively verbose variant:

            if (self._map)
                isDisplaying = self._map.getBounds().contains(latlng);
            else
                isDisplaying = false;

Similar excessive patterns present in many places, e.g.

            if (redraw && redraw === true)
            if (isDisplaying === true && redraw === true) {

Was protected in marker add time, but not on zoom/pan

Sample:
```
VM2992:16252 Uncaught DOMException: Failed to execute 'drawImage' on 'CanvasRenderingContext2D': The HTMLImageElement provided is in the 'broken' state.
    at NewClass._drawImage (<anonymous>:16252:23)
    at NewClass._drawMarker (<anonymous>:16239:22)
    at <anonymous>:16200:22
    at Array.forEach (<anonymous>)
    at NewClass._draw (<anonymous>:16179:16)
    at NewClass._redraw (<anonymous>:16077:18)
    at NewClass.fire (<anonymous>:1867:11)
    at NewClass._move (<anonymous>:5506:9)
    at NewClass._onZoomTransitionEnd (<anonymous>:5963:8)
```
@johnd0e
Copy link

johnd0e commented May 12, 2019

fix exception if image is not loaded (yet) / bad url / etc

I suppose there is one more fix needed: invalidate delayed draw queue (self._imageLookup[iconUrl][2]) on pan/zoom.
Small refactoring is needed to do it more effectively (separate queue from img cache).

@Spaction Spaction merged commit 15bb90b into Spaction:master Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants