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

Expose view and taffy node from ViewId #454

Merged
merged 3 commits into from
May 24, 2024
Merged

Conversation

timboudreau
Copy link
Contributor

Rationale: Solving two problems here:

  1. It is impossible to implement the same code as the default implementation of View::layout() outside of the floem crate, which limits what components in other crates can do to lay themselves out.

  2. Most of the components in floem::views actually implement layout by manipulating the taffy tree (e.g, in Label, self.id.taffy().borrow_mut().set_style(text_node, style);. It is possible to implement at least the things I am trying to do right now simply by having access to the existing taffy NodeId for a View and using ViewId::set_taffy_style, so rather that the more extreme approach of exposing the whole taffy tree, I opted simply to add a call to get the node id - otherwise, the only access to it is from within a layout method which (prior to this patch at least) was inaccessible unless you could afford to give up the default layout behavior.

I'm not entirely sure that just exposing the view() method is ideal here (though something is needed) - in my case I was debugging a panic from the borrow_mut() call in View::layout() which turned out to be that I was misusing the ViewId that gets passed in to the closure in add_overlay() as the id of the overlay component, which was already in use by the container for the overlay - so I wanted to reimplement the default behavior with logging to see exactly where things were going wrong. Attempting to use try_borrow_mut() instead to detect the source of the problem runs into lifetime problems between the erased lifetimes of RefMut<'_, Box<dyn View>> and &mut LayoutCx<'_>, which had to be resolved with some compiler trickery like this monstrosity (again, this is just debug code):

fn layout_if_posible<'l, V: View + ?Sized + 'l>(
    view_cell: &Rc<RefCell<Box<V>>>,
    cx: &mut LayoutCx<'_>,
) -> Option<NodeId> {
    if let Ok(view) = view_cell.try_borrow_mut() {
        return Some(layout_view(view, cx));
    }
    None
}

fn layout_view<'l, V: View + ?Sized + 'l>(
    mut v: RefMut<'l, Box<V>>,
    cx: &mut LayoutCx<'_>,
) -> NodeId {
    v.layout(cx)
}

So it might actually be better to expose a method on ViewId that takes a closure with a signature like with_view_id(&mut self, mut f : impl FnMut(&mut dyn View)) so users of the API are insulated from the erased lifetimes involved, and (perhaps?) use try_borrow_mut there and return a boolean (is there ever a legitimate case where there could be contention for the view, or was that only because I was abusing one id for two views?).

At any rate, this patch is likely to be harmless as-is.

Rationale: Solving two problems here:

1. It is impossible to implement the same code as the default implementation of `View::layout()` outside of the `floem` crate,
which limits what components in other crates can do to lay themselves out.

2. Most of the components in `floem::views` actually implement layout by manipulating the taffy tree (e.g, in `Label`,
`self.id.taffy().borrow_mut().set_style(text_node, style);`.  It is possible to implement at least the things I am trying
to do right now simply by having access to the existing taffy NodeId for a View and using `ViewId::set_taffy_style`, so rather
that the more extreme approach of exposing the whole taffy tree, I opted simply to add a call to get the node id - otherwise,
the only access to it is from within a `layout` method which (prior to this patch at least) was inaccessible unless you could
afford to give up the default layout behavior.

I'm not entirely sure that just exposing the `view()` method is ideal here (though *something* is needed) - in my case I was
debugging a panic from the `borrow_mut()` call in `View::layout()` which turned out to be that I was misusing the `ViewId`
that gets passed in to the closure in `add_overlay()` as the id of the overlay component, which was already in use by the
container for the overlay - so I wanted to reimplement the default behavior with logging to see exactly where things were
going wrong.  Attempting to use `try_borrow_mut()` instead to detect the source of the problem runs into lifetime problems
between the erased lifetimes of `RefMut<'_, Box<dyn View>>` and `&mut LayoutCx<'_>`, which had to be resolved with some
compiler trickery like this monstrosity (again, this is just debug code):

```rust
fn layout_if_posible<'l, V: View + ?Sized + 'l>(
    view_cell: &Rc<RefCell<Box<V>>>,
    cx: &mut LayoutCx<'_>,
) -> Option<NodeId> {
    if let Ok(view) = view_cell.try_borrow_mut() {
        return Some(layout_view(view, cx));
    }
    None
}

fn layout_view<'l, V: View + ?Sized + 'l>(
    mut v: RefMut<'l, Box<V>>,
    cx: &mut LayoutCx<'_>,
) -> NodeId {
    v.layout(cx)
}
```

So it might actually be better to expose a method on `ViewId` that takes a closure with a signature like `with_view_id(&mut self,
mut f : impl FnMut(&mut dyn View))` so users of the API are insulated from the erased lifetimes involved, and (perhaps?) use
`try_borrow_mut` there and return a boolean (is there *ever* a **legitimate** case where there could be contention for the view,
or was that only because I was abusing one id for two views?).

At any rate, this patch is likely to be harmless as-is.
@dzhou121
Copy link
Contributor

Adding taffy_node looks good to me.

I'm a bit nervous exposing View to user though. I still can't understand why you'd need to access View?

@timboudreau
Copy link
Contributor Author

To have the ability to reimplement the same functionality (or a portion of it) that View::layout does. Specifically, it gets the View instance so that it can call layout on it to recursively lay out its children.

If you have a reason to override layout in a View in another create, then you lose the ability to do that - at least in a straightforward way, or in a way that will return a NodeId.

I agree, exposing View is not ideal - it could be abused.

Is there another way to achieve the same thing?

I could imagine just moving the default implementation of layout() into a plain fn that takes an id and a layout context, and that would solve most problems (maybe someone will need to do the default layout on every child except one? I can't really imagine this, but it's not impossible, I suppose).

Since the problem I'm attempting to solve is to be able to override `layout()` in
a `View` and still be able to use the default functionality, the ability to simply
call that functionality as a standalone `pub fn` is probably enough to solve it
(TBD, but very likely).
@timboudreau
Copy link
Contributor Author

I just added a commit which avoids making ViewId.view() public, and instead moves the default implementation of View.layout() into a standalone fn which is callable by any view implementation in any crate.

That will probably solve the problem well enough.

@jrmoulton
Copy link
Collaborator

I'm running into these same issues now. Would be great to have this

src/view.rs Show resolved Hide resolved
@dzhou121 dzhou121 merged commit 58ef62f into lapce:main May 24, 2024
7 checks passed
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