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

Pass Specification #7

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Pass Specification #7

wants to merge 8 commits into from

Conversation

PoignardAzur
Copy link
Collaborator

@PoignardAzur PoignardAzur commented Jul 15, 2024

Formally define the semantics of the passes that Masonry runs in its event loop.

RENDERED

@PoignardAzur PoignardAzur marked this pull request as draft July 15, 2024 12:57
@DJMcNab DJMcNab changed the title Add "pass spec" RFC Pass Specification Jul 15, 2024
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I think this does significantly clarify the architecture.

The area I think needs a lot more clarification is the exact role distribution between layout and compose. I nebulously understand it, but didn't find it very clear in the text here


The **MUTATE** pass runs a list of callbacks with mutable access to the widget tree.
These callbacks can be queued with the `mutate_later()` method of various context types.

Copy link
Member

Choose a reason for hiding this comment

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

Does this pass also run the driver, with any action from the event handler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now the assumption is that the RenderRoot isn't aware of the driver.

So the template for any interaction is "the platform runs a RenderRoot method, which triggers a bunch of passes, which produce a list of signals, and the platform interprets these signals how it sees fit, potentially running the driver and/or re-running the passes".

rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
rfcs/0007-pass-spec.md Show resolved Hide resolved
rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
#### Compose pass

The **compose** pass runs top-down and assigns transforms to children.
Transform-only layout changes (e.g. scrolling) should request compose instead of requesting layout.
Copy link
Member

Choose a reason for hiding this comment

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

Would compose have the possibility of stashing child widgets, e.g. if they scroll off-screen.
Would that require a mutate pass to be ran?

Copy link
Collaborator Author

@PoignardAzur PoignardAzur Jul 15, 2024

Choose a reason for hiding this comment

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

Stashing in the sense it's currently used in the codebase still isn't implemented or documented.

Short answer is "compose could trigger a stash but it probably shouldn't".

Would that require a mutate pass to be ran?

No, because stashing doesn't change the tree structure. A stashed widget still has the same parent, a parent with stashed children still lists them as children, etc.

rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel McNab <[email protected]>
@PoignardAzur
Copy link
Collaborator Author

The area I think needs a lot more clarification is the exact role distribution between layout and compose. I nebulously understand it, but didn't find it very clear in the text here

I added a paragraph to try and explain it.

Comment on lines +98 to +108
#### Compose pass

The **compose** pass runs top-down and assigns transforms to children.
Transform-only layout changes (e.g. scrolling) should request compose instead of requesting layout.

Compose is meant to be a cheaper way to position widgets than layout.
Because the compose pass is more limited than layout, it's easier to recompute in many situations.

For instance, if a widget in a list changes size, its siblings and parents must be re-laid out to account for the change; whereas changing a given widget's transform only affects its children.

Masonry automatically calls the `compose` methods of all widgets in the tree, in depth-first preorder, where child order is determined by their position in the `children_ids()` array.

Choose a reason for hiding this comment

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

I actually hope, we can design layout in a way that this is not really necessary (e.g. by dirty tracking via the flags and only recalculating the path that is affected by this). It would be really nice if we could (unlike in the web) make layout changes really interactive, e.g. data-driven animations etc. (which I think would play nice with Xilem) But I'm no expert with layouting algorithms, so I'm not really sure...

But I guess this is mostly meant to be a more general pass than ParentWindowOrigin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incremental layout is the goal, and there are ways to implement it, but it's not trivial. Some layout change must update sibling widgets.

For instance, if you have a widget defined as "be as wide as my widest widest child", if one of the children gets much wider, then all its siblings will have more available space and need to be re-laid out.

One of the things I'd like to implement is flags that deliberately rules out those cases, eg a flag a widget can set to say "my layout is independent from the layout of my children". Or "my height is only dependent on available width, no available height".

In any case, that's beyond this PR.

- Display passes should be pure and can be skipped occasionally, therefore their context types (`PaintCtx` and `AccessCtx`) can't set invalidation flags or send signals.
- The `layout` and `compose` passes lay out all widgets, which are transiently invalid during the passes, therefore `LayoutCtx`and `ComposeCtx` cannot access the size and position of the `self` widget.
They can access the layout of children if they have already been laid out.
- For the same reason reason, `LayoutCtx`and `ComposeCtx` cannot create a `WidgetRef` reference to a child.

Choose a reason for hiding this comment

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

Suggested change
- For the same reason reason, `LayoutCtx`and `ComposeCtx` cannot create a `WidgetRef` reference to a child.
- For the same reason, `LayoutCtx`and `ComposeCtx` cannot create a `WidgetRef` reference to a child.

Comment on lines 261 to 262
Instead, widgets should be created and added to the widget tree as a single atomic operation.
To keep the WidgetPod logic simple and avoid too many corner cases, this is only allowed inside the MUTATE pass.

Choose a reason for hiding this comment

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

I wonder how that will look like in practice?
I.e. with different constructors for different kind of widgets.
so is something like this still valid (more pseudocode, this is wrong of course):

let button = Button::new(..);
// later e.g. for a flex
flex_widget.insert_flex_child_pod(button);

Or how should that roughly look like?
Is the constructor for a widget just a data container, and gets properly constructed when adding that child to whatever widget it contains?

(But the general idea sounds good)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we're still trying to figure that out.

See this discussion on zulip.

Instead, widgets should be created and added to the widget tree as a single atomic operation.
To keep the WidgetPod logic simple and avoid too many corner cases, this is only allowed inside the MUTATE pass.

`MutateCtx` should have an `add_child` and a `remove_child` method.

Choose a reason for hiding this comment

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

I wonder how moving a child from A -> B can be modeled (as mentioned in the explicit-identity zulip topic), so that ideally properties where it makes sense can be animated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, there's a reason "Reparenting children" is in future possibilities.

FWIW, I favor something like the View Transition API.

rfcs/0007-pass-spec.md Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Aug 13, 2024
This is a first step in implementing the "Pass Specification" RFC:
linebender/rfcs#7

Create a `passes` module.
Create event passes.
Create the update_pointer pass.

Remove `WidgetPod::update_hot_state` method.
Move mouse-cursor-handling code to update_pointer pass.
Implement pointer capture.
Refactor the TreeArena code.

---------

Co-authored-by: Daniel McNab <[email protected]>
Comment on lines +71 to +72
To avoid infinite loops in those cases, the number of reruns has a static limit.
If passes are still requested past that limit, they're delayed to a later frame.

Choose a reason for hiding this comment

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

I guess that's relevant for frame-pacing, would it make sense to instead use a max duration limit (possibly relevant for layout too)?
(infinite-loop detection would be even better I guess, but that's out of scope for now I guess)

Copy link
Member

Choose a reason for hiding this comment

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

The reason for this design is that the only infinite loops would be pathological cases, and we expect any reasonable scene to only require 2 or maybe 3 cycles. That is, it's a bug in your app if you hit this fallback, so it's not a problem to have it present.

github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Aug 14, 2024
This is part of the "Pass Specification" RFC:
linebender/rfcs#7

Rename WidgetCtx to MutateCtx.
Add a mutate pass.
Add a `mutate_later` context method to trigger that pass.
Refactor `edit_root_widget` to use a version of that pass.
Add a separate constructor for the synthetic WidgetState created in
RenderRoot.
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Aug 14, 2024
This is part of the "Pass Specification" RFC:
linebender/rfcs#7

Rename WidgetCtx to MutateCtx.
Add a mutate pass.
Add a `mutate_later` context method to trigger that pass.
Refactor `edit_root_widget` to use a version of that pass.
Add a separate constructor for the synthetic WidgetState created in
RenderRoot.

---------

Co-authored-by: Philipp Mildenberger <[email protected]>
Co-authored-by: Daniel McNab <[email protected]>
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Sep 9, 2024
This is part of the Pass Specification RFC:
linebender/rfcs#7

Note: This PRs comes with a lot of new TODO items. Addressing most of
these items is difficult without major refactors, because Portal code
deals with accessing values across multiple widgets, which is still hard
to do elegantly.
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Sep 12, 2024
This is part of the Pass Specification RFC:
linebender/rfcs#7

---------

Co-authored-by: Daniel McNab <[email protected]>
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Sep 12, 2024
Make Textbox Widget tab-focusable.

This is part of the Pass Specification RFC:
linebender/rfcs#7
@PoignardAzur PoignardAzur marked this pull request as ready for review October 6, 2024 13:23
@PoignardAzur
Copy link
Collaborator Author

This RFC is now in its quasi-final state. I'd like to flesh out the "prior art" part, but finding documentation on GUI frameworks' internals is still really hard.

Other than that, I think the current text of the RFC reflects how we've implemented things, and future work outside of its scope won't need to refer to this document.

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.

3 participants