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

Generalize bbox handling in UI views (DOM and canvas) #13863

Merged
merged 8 commits into from
May 6, 2024

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented May 5, 2024

Previously different UI views handled this in their own ways. Now bbox is handled at DOMView level. This in turn allows to simplify View.serializable_state(). In combination with using View.children(), it allows to remove almost all custom implementations (similar to changes in PR #13801). Having a common bbox also allows to uniformy handle positioning of DOM elements associated with renderers. Previously only axes were positioned on a trial basis. In this PR all renderers with a valid bbox are positioned. I also removed some redundant calls to update_geometry() and compute_geometry(), which were revealed by other changes in this PR. Most file changes in this PR are trivial updates to *.blf files.

Example of legend's associated DOM element:

image

Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

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

Minimal code changes look good, for my curiousity, what kinds of situations could cause these bboxes to be different from each other?

  GlyphRenderer bbox=[45, 11, 334, 119]
    Scatter bbox=[45, 11, 334, 119]

@mattpap
Copy link
Contributor Author

mattpap commented May 6, 2024

(...) what kinds of situations could cause these bboxes to be different from each other?

None, GlyphRenderer reports its primary glyph's bbox. However, for the time being I don't include secondary glyphs here, which after PR #13554 allow to have unrelated coordinates and thus bbox from the primary glyph. I could probably skip reporting bbox for glyph renderer, but that would require extra logic.

@mattpap mattpap merged commit f5e07a3 into branch-3.5 May 6, 2024
25 checks passed
@mattpap mattpap deleted the mattpap/common_bbox branch May 6, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants