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

Only Generate ShadowGroups When Needed #1748

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

corranwebster
Copy link
Contributor

This is work towards #1740 - it doesn't fix the issue, but it attempts to minimize the number of ShadowGroup instances being created by just using the Group directly when it is OK to do so.

A ShadowGroup is needed in situations where the content of the Group as instantiated in the UI is not the same as the content of the Group as defined by the user. There are 4 ways this can happen:

  • there is an element of the Group which has a defined_when that evaluates to False (and so must be hidden)
  • the Group has one or more Include elements
  • there is an element which needs to be replaced by a ShadowGroup for the above 2 reasons
  • when doing a "normal" layout which mixes Item and Group elements, runs of Item elements are wrapped in ShadowGroup instances so that the layout algorithm only deals with all-item or all-group layout cases

Other than the last, these are not common; and the last example does not "propagate" because it is computed on an as-needed basis during UI creation, so it is somewhat contained.

This is a behavioural change, however, as using the Group directly means that any trait changes on the Group affect all views and UIs which are using that groups (the ShadowGroup class instead prototype the traits, so changes only go in one direction).

Checklist

  • Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

Note: opening this as a draft, as not sure whether this is 100% OK, given the behaviour change (or if the behaviour change is OK, whether we can push it further).

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.

1 participant