-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Improve diffing of styles containing "visibility": "visible" #8005
Conversation
All callsites that use the visibility property check it as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -210,7 +209,7 @@ class StyleLayer extends Evented { | |||
'paint': this._transitionablePaint && this._transitionablePaint.serialize() | |||
}; | |||
|
|||
if (this.visibility === 'none') { | |||
if (this.visibility) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I’ve fixed this in a subsequent commit. Or I misunderstand. There’s a test that ensures it can be set to “visible”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I was viewing a single commit instead of all the changes.
Thanks @asheemmamoowala! I think you need to press the big green button 😄. |
When I execute this code:
I expect
setStyle
to do a diff-based update which reduces to a no-op.However what actually happens is that it produces a
['setLayoutProperty', 'visibility', null]
command because we storevisibility: 'visible'
the same way asvisibility: undefined
and always serialize into the latter.map.getStyle().layers[0].layout.visibility
isundefined
.It is expensive to run a large number of
['setLayoutProperty', 'visibility', null]
commands.Other properties do not behave like this. Visibility is a special case.
This PR correctly stores and serializes the state of
visibility
into eithervisible
orundefined
.This may
fix
#7459This may
fix
#7648Launch Checklist