-
Notifications
You must be signed in to change notification settings - Fork 104
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
Draft for a simple container view #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really glad to see this. Most of the changes I agree with, but I hesitate at just passing the Pod
into the view methods. That means views need to do their own downcasting, which would be good to avoid.
I haven't dug deep into why this change was made, but it doesn't feel necessary, the previous prototype was done without it. For now, I'd request taking another look at whether it can be implemented without that change. I can do a deeper look if it seems important, but wanted to raise this early.
Ok, I've read through the patch, and think I understand what's going on. This seems like a continuation of whether we can get rid of the The underlying problem is: how do you set the pod flags in response to mutations of the widget tree? There are two twists: one is that flags must be propagated up the tree to the root so they're visible to a top-down traversal, and the other is that the exact flags set depend on the specifics of the mutation - paint, accessibility, layout for sure, maybe other things like focus. This had a working if perhaps clunky solution in the prototype developed in the xilem_tokio branch. The That changed in #6, which started moving that logic out of One choice is to revert the change in #6 and go back to the way it was done in xilem_tokio. But I think it's possible to move forward, keeping the flavor of both. Basically, If we do that, here's what I think should happen with Other than this concern, the patch makes a lot of sense to me. I'd be happy to review it when it's ready. Thanks for pushing this forward! |
That makes sense. I will do that. But just as a side note. I think there was a discussion about making the view trait generic over the type of element (Native, Web, etc...) constructed. In that case we probably would need to use Pod anyway instead of a concrete element. One way to make this possible without having to downcast the the element, would be to have a |
I think changes to the |
I moved the call to Pod::mark into the |
What @raphlinus already wrote about accessibility looks right to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gone through this carefully, but it generally looks like a good direction after a skim. Some thoughts.
As mentioned, I'd like to establish a policy of keeping accessibility current, rather than letting it lag. If you want more detailed guidance on that, let me know, though I hope there are breadcrumbs from the previous work and the prototype druid integration.
Another thing that's incomplete is the children tracking, including generation of WidgetAdded lifecycle events. This was discussed at office hours, and there was consensus that would make sense to do as a separate PR, to keep focus on that functionality. I'd be fine with the Vec<Id>
solution proposed in the Zulip thread, but I'm also very open to other approaches as long as they solve the fundamental problem of tracking the parent-child structure well enough to at least build the focus chains etc. Not coincidentally, this structure tracking is also similar to what's needed to generate the accessibility updates, as add/remove notifications will be required exactly when the container updates its accessibility node with a new for the children
field.
Thanks for your work pushing this forward!
I changed the way accessibility works and added updated the implementation of Since many other changes are blocked on this PR, i tried to keep it minimal:
The points where i am a bit unsure are the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels close to landing, thanks for the work on it.
Here are my thoughts, but I'm open to discussion.
I think the "removed" flag from rebuild should be removed, and we shouldn't add an additional lifetime invariant that we traverse rebuild on widget removal. Perhaps I'm just not yet understanding why it's needed, in which case I'm willing to be convinced.
Based on my understanding, I think the best plan for the end state is context methods (probably on update context) to be called by the parent notifying the framework on child add and removal. The framework maintains a tracking tree (map child -> parent and parent -> list of children). One thought I had is that if the framework maintains the tracking tree, then a single method could be called to generate the AccessKit children vec, as opposed to having to do another traversal.
(Another subtlety is whether this needs to be a list of children or a set of children. For just lifetime tracking, the latter suffices, but for AccessKit and probably also focus chain building, the order matters. In that case, we probably need additional context methods for the parent to reorder, or perhaps better in that case to have a single call that updates the entire list)
For this PR, I think the best strategy is to hew as closely as possible to existing Druid, which I believe means not doing anything in particular for removal. Changes to tracking should be moved to a separate PR focused on that topic, and ideally worked out in an issue before diving too deep into implementation (although having a prototype can definitely be useful for discussion).
src/widget/linear_layout.rs
Outdated
child.accessibility(cx); | ||
} | ||
|
||
let mut node = accesskit::Node::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always building a node for the container, and I think that can and should be tightened up, to only build the node when needed. That's on layout change and when the children have changed, I believe, but it's possible I'm missing something.
However, that can be considered a future optimization, as it's not wrong now, just less efficient than it might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this by reintroducing the HAS_ACCESSIBILITY
flag.
To allow for ChangeFlags
which don't get propagated up the tree i added a return value to Pod::mark
which contains the flags the parent needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this again. Making ChangeFlags
a wrapper type around PodFlags
probably counts as a Layer violation...
Then we have probably have to stick to the old method of of using the same bit masks for both types. It works but it feels brittle.
I only see one problem with the accessibility implementation: the role of a layout container should always be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick skim and left a few inline comments/questions.
One running theme is the lack of comments on pub
items. This matches a lot of the surrounding code, but we need to stop this from spreading further. Druid has the documentation it has thanks to the policy of requiring all pub
items to be documented. I think at a bare minimum this policy should be repeated here. I say minimum, because even with Druid having all pub
items documented some people still complain about insufficient docs.
I know that some of the lacking items in this PR were just refactors, but I think you're in a great position of knowledge as you have been immersed in the code in question. Even more so for brand new additions.
The docs don't need to be comprehensive (although always appreciated!), especially as things are subject to change. However we don't really know in advance what is going to change next month and what we're stuck with for years. So even for "temporary" solutions we should have some short docs.
Thanks for all your effort on this!
@@ -103,7 +104,6 @@ pub struct WakeQueue(Arc<Mutex<Vec<IdPath>>>); | |||
|
|||
impl<T: Send + 'static, V: View<T> + 'static> App<T, V> | |||
where | |||
V::Element: Widget + 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for removing the constraint here and elsewhere?
The code seems to still compile and run when I add these back, so I'm wondering what the motivation is. Is it because the View
trait already specifies the constraint as type Element: Widget;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happened during a refactor, but if it still compiles we should leave out unnecessary constrains.
src/app.rs
Outdated
if let Some(element) = self.root_pod.as_mut().and_then(|pod| pod.downcast_mut()) { | ||
if let Some(element) = self.root_pod.as_mut() { | ||
let mut state = response.state.unwrap(); | ||
let changes = response.view.rebuild( | ||
&mut self.cx, | ||
response.prev.as_ref().unwrap(), | ||
self.id.as_mut().unwrap(), | ||
&mut state, | ||
element, | ||
element.downcast_mut().unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation here, does this change any behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically there are 3 cases:
self.root_pod.as_mut()
is Noneself.root_pod.as_mut()
is Some andelement.downcast_mut()
is Noneself.root_pod.as_mut()
is Some andelement.downcast_mut()
is Some
Before, they are mapped to two arms: 3 maps to the true case, 1 & 2 maps to the false case.
Now, 3 is mapped to the true case, 1 is mapped to the false case, and 2 is mapped to a panic.
If we know out of band that 2 is impossible then there should be no change of behavior.
... that's my analysis anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That matches my thinking. I also did some minor digging and it seems element.downcast_mut()
can only be None
when the type assertion fails. However both the build
and rebuild
paths use self.response_chan
which has a generic constraint on View
. So my initial reading is that case#2 is impossible right now.
However I'm not super familiar with the Xilem type wrangling yet, hence my question. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch :)
This was not intentional, but i think it is better this way. If the Widget type changes this is a bug which should be handled by the framework, instead of silently discarding everything and starting from scratch. That would make bugs much harder to detect.
Sorry for the late response on this, but the current accessibility implementation in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's now a merge conflict due to #43 having landed, but it should be straightforward to fix. Hopefully just a case of a rebase on main
and changing LinearLayout::accessibility
to meet the new expectations. I have added a comment with some code I think should be correct, but I didn't try compiling.
I added much of the missing documentation. Most of it i just copied from druid and changed the parts which are no longer true. I think the PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a pass through this and didn't find any serious issues. That said, I haven't been able to fully immerse myself in the logic, though I was hoping to. I do appreciate the added comments, I find them very helpful. Thanks!
src/widget/button.rs
Outdated
@@ -123,6 +123,7 @@ impl Widget for Button { | |||
} | |||
|
|||
fn paint(&mut self, cx: &mut PaintCx, builder: &mut SceneBuilder) { | |||
println!("paint button with text {}", self.label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to not have such debug printing turned on by default. A more principled solution is to log this with info!
, which is available because we have tracing
as a dep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! Using tracing makes much more sense but i never used it. I probably should look into it
src/widget/core.rs
Outdated
let mut builder = SceneBuilder::for_fragment(&mut self.fragment); | ||
self.widget.paint(&mut inner_cx, &mut builder); | ||
|
||
println!("try paint!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment wrt the debug logging here.
This is an attempt to implement a simple container view. I tried to keep it as close as possible to the original xilem_tokio branch. I also added the ViewSequence trait again.
There are a few thing i changed: