Fix Path.setStyle bug (replacing #6671)

[NielsHolt]

This is a new PR replacing #6671

Fix issue #6662 and #6664

[mourner]

This looks good, but I agree it would nice to have a unit test to make sure we don't regress in future.

[NielsHolt]

Hi @mourner and @IvanSanchez
Do you know when you expect this PR to be included in a patch?
Kind regards
Niels

[naedx]

Hi @mourner and @IvanSanchez
Do you know when you expect this PR to be included in a patch?
Kind regards
Niels

Hi guys, are there any updates on merging this?

[NielsHolt]

Hi @mourner and @IvanSanchez
I see that this PR was not included in 1.7.1
Do you know when you expect this PR to be included in a patch?
It fixes a bug that was introduced in version 1.5 so I would urge you to please include it in the next patch.

Kind regards
Niels

[johnd0e]

@NielsHolt

1. Resolve conflicts
2. Add unit test

[fodor0205]

The error only occurs with the following order:

1. Polyline added to the map
2. setStyle() called with weight

The description in #6662 does not seem to be correct, because no error occurs if setStyle is called before addTo().

[johnd0e]

The description in #6662 does not seem to be correct, because no error occurs if setStyle is called before addTo().

But it is not about description only, as there is code in steps to reproduce section:

var polyline = L.polyline([[45.51, -122.68],[37.77, -122.43],[34.04, -118.2]]);
polyline.setStyle({color: 'red', weight: 4});
polyline.addTo(map);

And I do not see any problem even if I call setStyle after adding to map.

[johnd0e]

So I am able to see that only #6664 issue is relevant, and there described slightly different case:

• polygon/polyline is empty
• setStyle is called after feature added to map.

Ok, then I have more questions:

1. As test is not specific to Polyline, shouldn't we place to place into PathSpec.js?
2. Doesn't other shapes suffer from the issue?
3. Isn't there some performance loss on replacing this._updateBounds with _project call?
   I'm afraid that here we are fixing only empty polygon/line - very particular and unlikely case - at the cost of overall performance of every L.Path's descendant.

[fodor0205]

As test is not specific to Polyline, shouldn't we place to place into PathSpec.js?
Doesn't other shapes suffer from the issue?

5 vector overlays extend Path:

• Polyline and Polygon are affected:

  They have unit test in my branch referenced below, which perfectly trigger the bug.

• Rectangle is not affected:

  Rectangle has no option to initialize with empty values, so this offending code-path can never be reached. (an Error is throw during initialization with empty values)

• CircleMarker and Circle are not affected:

  The reference to undefined variable _this._rawPxBounds is located in Polyline's _updateBounds() function, not inside any classes reached on this chain.

A possible fix without touching Path.js:
master...fodor0205:master

Isn't there some performance loss on replacing this._updateBounds with _project call? I'm afraid that here we are fixing only empty polygon/line - very particular and unlikely case - at the cost of overall performance of every L.Path's descendant.

The alternative fix linked above only changes the offending code-path, making less impact in other use cases.

@johnd0e @NielsHolt Could you please review this alternative fix?

Edit: added info about Polygon, Rectangle and Circle.

[johnd0e]

The alternative fix linked above only changes the offending code-path, making less impact in other use cases.

I still doubt that it makes sense to project empty polyline.
How about simpler fix:

		if (!this._rawPxBounds) { return; }

[fodor0205]

This solution makes more sense @johnd0e, I have updated my branch, waiting for @NielsHolt to pull it into his branch, so it will be available in this PR.

[NielsHolt]

Hi
Thanks! The fix from @fodor0205 seems to fix the issue.
I have merged the PR into my master, but I am not sure what to do next except wait for the leaflet guys to include the PR?

[fodor0205]

@johnd0e do you think this is ready to be merged?

[johnd0e]

Waiting for FCOO#4

[fodor0205]

What's the next step? @johnd0e

[johnd0e]

It is ready for merge, so waiting @mourner or @IvanSanchez.

[contibru]

Is there any due date for this issue?