-
-
Notifications
You must be signed in to change notification settings - Fork 635
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
feat: move selection emptiness check to predicate #7155
Conversation
c5c2fc8
to
28d6d8a
Compare
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.
Looks good except for one issue with an example.
Does it fix #4371?
@@ -298,7 +298,7 @@ | |||
"update": { | |||
"fill": [ | |||
{ | |||
"test": "!(length(data(\"pts_store\"))) || (!(vlSelectionTest(\"pts_store\", datum)))", | |||
"test": "!(!length(data(\"pts_store\")) || vlSelectionTest(\"pts_store\", datum))", |
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.
This example is now different from before. Is that intended?
The docs say "Notice, all marks are initially colored grey. This is because empty selections are treated as containing all data values." so I think either the example or the docs need to change.
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.
Thanks, good catch! I was concerned about this as well given what the docs said. But, as far as I can tell, this was a bug in the previous way we were constructing selection predicates and the docs are incorrectly and I fixed it in the previous commit: 28d6d8a#diff-a4cbaf0ac9270d18fd49ffa34980748d840aa87e9983abdf94b9f1088edea0ca. Explanation below.
From the docs:
a conditional value definition sets the
rect
marks to grey when they do not lie within the selection, and a regular field mapping is used otherwise.
So, in both our prior and this PR's approach, by default, when a selection is empty all data values are considered to lie within the selection. Thus, based on the above description of the spec, the initial state of the visualization begins with an empty selection, all values thus lie within the selection, and the "selection": "pts"
predicate should return true. This is then inverted "not": {"selection": "pts"}
and should become false for all data values, and the marks should be assigned else
branch of the condition.
The sort-of double negation definitely confused me for a bit (and is likely why the docs were written the way they were to start with) so please double check my work and make sure I'm not making a mistake :)
Yes, fixes #4371. |
f7853a4
to
bcdb5b8
Compare
Addresses #4371 — emptiness checks on the predicate allow us to get rid of the hacky
length(data("brush_store"))
hacks of yore. This PR, however, moves the emptiness check to the selection predicate rather than allowing emptiness to be tested with the predicate in addition to being defined on the selection as before (#3038).Although this approach can introduce some occasional duplication, I think it's preferable to have only one way of doing things and because keeping the selection-level emptiness definition would yield a somewhat confusing set of
"empty"
property values ("all" | "none"
at the selection level, and then booleans at the predicate level).Adding
empty
to the predicate level necessitates one other breaking change: selection predicates can only be composed alongside other predicate types (i.e., within acondition.test
block orfilter
) rather than being composable within theselection
property itself (i.e., we could previously do"selection": {"and": ["foo", "bar"]}
).