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

do not clear _currentSnapshot when hiding the layer #1212

Merged
merged 3 commits into from
Jul 17, 2020
Merged

do not clear _currentSnapshot when hiding the layer #1212

merged 3 commits into from
Jul 17, 2020

Conversation

gavinr
Copy link
Contributor

@gavinr gavinr commented Jul 9, 2020

The behavior in #1207 is happening because the first time the layer is hidden due to the maxZoom, this._currentSnapshot is emptied, so the next time it tries to hide the layers, this._currentSnapshot is empty so it does not remove anything.

The bug is fixed if you remove that line (this PR), but I'm not quite convinced this is the correct fix because I'm guessing there's some side-effect that requires that line, and I'm just not understanding it - @patrickarlt @jgravois please let me know if you have any insights into why this line (this._currentSnapshot = [];) was originally included.

bug fixed

@jgravois
Copy link
Contributor

jgravois commented Jul 9, 2020

🤷 nobody has touched that code in 5 years.

Screen Shot 2020-07-09 at 3 25 32 PM

off the top of my head it doesn't seem problematic to preserve the cache instead of destroying it in the block.

the only other thing i'd suggest testing would be applying a new filter both while the layer is at a visible zoom level and when its not. i have a very vague recollection of folks complaining about a stale cache in a scenario like that.

second param should be an object, not a string.
@gavinr
Copy link
Contributor Author

gavinr commented Jul 10, 2020

Thanks @jgravois - you were correct there was an issue when zooming in, calling setWhere, then zooming out/in.

Seems like in _requestFeatures function second parameter should be an object (correct here), but in three other places the code was passing a string to _requestFeatures(), which was causing issues. Fixed that in 87b4d14. (affects setWhere(), setTimeRange(), and refresh())

(note this might address the issues from #665)

@gavinr gavinr requested a review from jwasilgeo July 10, 2020 21:54
@patrickarlt
Copy link
Contributor

@gavinr this looks like a good change. Lets merge this.

@gavinr
Copy link
Contributor Author

gavinr commented Jul 13, 2020

thanks Patrick. @jwasilgeo found one small issue - i'm going to try to fix that quickly before merge.

@gavinr gavinr self-assigned this Jul 13, 2020
@jwasilgeo
Copy link
Contributor

@gavinr, I'd like to also add the Labeling Features demo page to our list of manual tests. The odd behavior described in #1193 was fixed as a byproduct of v2.4.0 and I'd like to close it out if it still works as expected with the changes in this PR.

Copy link
Contributor

@jwasilgeo jwasilgeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great. Thanks for diving into this one and fixing it, @gavinr.

@gavinr gavinr merged commit e54473d into master Jul 17, 2020
@gavinr gavinr deleted the fix-1207 branch July 17, 2020 20:15
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
do not clear _currentSnapshot when hiding the layer
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
do not clear _currentSnapshot when hiding the layer
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.

5 participants