Fix hasIconPotentially check #905
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hasIconPotentially logic tries to filter out symbols that never need to be
drawn. For this, we tried to evaluate icon-image expression with
zoom/state independent context and if that is successful and an empty
string we know it will never have an icon.
This was flawed. First, there was a logic error. Should have been
hasIconPotentially = (iconImage == nullptr || !iconImage->empty());
instead of
hasIconPotentially = (iconImage != nullptr && !iconImage->empty());
Also, the zoom-indpendent evaluation does not work as was assumed. The
assumption was that accessing state or zoom in an expression would
always result in std::monostate value to indicate evaluation failure.
However that's not consistently the case. For example, "step"
expressions will return the first value in such a case.
Finally, it did not correctly handle result values other than strings,
breaking it e.g. for "concat" expressions.
Fixed by relying on (more conservative) usedKeys.
Fixes "Filter symbols with neither text nor icon early" (e0e2e54)
matzf
force-pushed
the
bugfix/has-icon-potentially
branch
from
e8d453b to
d7b5b2b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
hasIconPotentially logic tries to filter out symbols that never need to be drawn. For this, we tried to evaluate icon-image expression with zoom/state independent context and if that is successful and an empty string we know it will never have an icon.
This was flawed. First, there was a logic error. Should have been
instead of
Also, the zoom-indpendent evaluation does not work as was assumed. The assumption was that accessing state or zoom in an expression would always result in std::monostate value to indicate evaluation failure. However that's not consistently the case. For example, "step" expressions will return the first value in such a case.
Finally, it did not correctly handle result values other than strings, breaking it e.g. for "concat" expressions.
Fixed by relying on (more conservative) usedKeys.
Fixes "Filter symbols with neither text nor icon early" (e0e2e54)