First cut at Widget trait #26

raphlinus · 2022-12-12T17:17:00Z

This is an attempt to define the Widget trait. It compiles and runs but is a bit incomplete:

The HotChanged propagation has not been ported.
It is missing accessibility methods.
No doubt some cleanup is still needed.

This is work toward #7

This is an attempt to define the Widget trait. It compiles and runs but is a bit incomplete: * The HotChanged propagation has not been ported. * It is missing accessibility methods. * No doubt some cleanup is still needed.

jeffparsons · 2022-12-12T21:23:39Z

src/widget.rs

+    /// the provided [`UpdateCtx`] to schedule calls to [`paint`] and [`layout`]
+    /// as required.
+    ///
+    /// This method may go around.


"May go around"? Is this saying that the method may go away in future, like the original comment suggested, or something else?

I meant "may go away" and typo'ed it. That said, my current thinking is to retain it, and I am also considering plumbing the AccessKit node update mechanism through the method. That will require some more careful design.

I was expecting the accessibility stuff to be handled more like painting with a separate ChangeFlag, phase and trait method. I have not yet looked at AccessKit's API however.

Using update as a kind of environment change event could be a good idea.

jeffparsons · 2022-12-12T21:26:39Z

src/widget.rs

+/// the direction in which they grow as their number of children increases.
+/// Has some methods for manipulating geometry with respect to the axis.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Axis {


Are these axes directional? I.e. does Vertical imply an origin at the top (or bottom) of the visual space, or is that direction independent of this?

Imberflur · 2022-12-13T18:00:31Z

src/widget/box_constraints.rs

+    pub fn shrink_max_height_to(&self, dim: f64) -> Self {
+        let mut max = self.max();
+        max.height = f64::max(dim, self.min().height);
+        BoxConstraints::new(self.min(), max)


Should there also be something like f64::min(dim, max.height) in here, to keep from increasing the max?

This (along with Axis) is copied without change from Druid. Not that there's no chance of flaws there, but it's not something I'm focusing on at the moment.

Turn on the AccessKit dependency and also update to more recent glazier and Vello versions.

This is a first cut at AccessKit integration. The intent is to get the signatures correct in the `Widget` trait, and demonstrate basic functionality (clicking a button), but there is much to be done before it can be considered complete.

At present, leaf widgets do need to check that the target id matches. Possibly we will relax that if we get precise child tracking, which would guarantee that an event won't be propagated unless the subtree contains the target.

xarvic

There are some things we could change but i think it works the way it is now. Therefore i am in favor of merging it as soon as possible and create followup PRs with small changes instead of making this PR perfect.

xarvic · 2023-01-21T12:04:19Z

src/widget.rs

+    /// This does not mean the value returned by layout() would be the same.
+    ///
+    /// This method **must** return a finite value.
+    fn compute_max_intrinsic(


I think we should either add a compute_max_intrinsic method to Pod or remove this one.

I agree, and I also think that should happen in a followup PR. The scope of this one is focused on the Widget trait itself, and it's known that there's a bunch more wiring, especially in Pod, that needs to be done.

xarvic · 2023-01-21T12:09:47Z

src/widget/core.rs

    /// The minimum intrinsic size of the widget.
    pub(crate) min_size: Size,
    /// The maximum intrinsic size of the widget.
    pub(crate) max_size: Size,


The values of min_size and max_size are never changed. Are they remains of the exploration of SwiftUI layout?

I will remove them, thanks for the heads-up.

xarvic · 2023-01-21T12:11:23Z

src/widget/contexts.rs

@@ -146,40 +155,51 @@ impl<'a, 'b> LayoutCx<'a, 'b> {
        }
    }

-    pub fn add_event(&mut self, event: Event) {
+    pub fn add_event(&mut self, event: Message) {


We should probably rename this and similar methods.

Will do. However, looking at this now, I'm concerned I've introduced a layering violation; Message is a Xilem type, and that might make it harder to integrate other reactive layers. I'll file an issue on this.

Remove some vestiges of old layout system. Make rename of View-level event to "message" more consistent.

raphlinus · 2023-01-21T19:36:04Z

Thanks for the review! Apparently the way we've got the repo set up, it needs another approval after a new commit, which honestly feels a bit restrictive. I'll self-approve and merge if there's no activity soon.

DJMcNab

I've not reviewed; signing off since @xarvic 's review isn't sufficient for GitHub's approval

richard-uk1 · 2023-01-22T14:58:35Z

I had a look at the settings, and it looks like you can push new commits and the approval will remain. Has someone changed the settings recently, perhaps?

DJMcNab · 2023-01-22T15:05:11Z

We worked it out on Zulip

First cut at Widget trait

1cf74a6

This is an attempt to define the Widget trait. It compiles and runs but is a bit incomplete: * The HotChanged propagation has not been ported. * It is missing accessibility methods. * No doubt some cleanup is still needed.

jeffparsons reviewed Dec 12, 2022

View reviewed changes

Imberflur reviewed Dec 13, 2022

View reviewed changes

raphlinus added 3 commits December 14, 2022 16:12

Merge branch 'main' into widget

28332c2

Enable AccessKit and update dependencies

4b187f0

Turn on the AccessKit dependency and also update to more recent glazier and Vello versions.

Add basic accessibility

4801a88

This is a first cut at AccessKit integration. The intent is to get the signatures correct in the `Widget` trait, and demonstrate basic functionality (clicking a button), but there is much to be done before it can be considered complete.

raphlinus mentioned this pull request Jan 17, 2023

Enable AccessKit and update dependencies #36

Merged

raphlinus marked this pull request as ready for review January 18, 2023 04:33

raphlinus added 2 commits January 18, 2023 11:56

Merge branch 'main' into widget

6a29320

Guard AccessKit action better

a9b1aab

At present, leaf widgets do need to check that the target id matches. Possibly we will relax that if we get precise child tracking, which would guarantee that an event won't be propagated unless the subtree contains the target.

xarvic approved these changes Jan 21, 2023

View reviewed changes

Address review feedback

37cb085

Remove some vestiges of old layout system. Make rename of View-level event to "message" more consistent.

raphlinus mentioned this pull request Jan 21, 2023

View-level Message type in Widget #39

Closed

DJMcNab approved these changes Jan 21, 2023

View reviewed changes

raphlinus merged commit d84812a into main Jan 21, 2023

raphlinus deleted the widget branch January 21, 2023 20:36

raphlinus mentioned this pull request Jan 21, 2023

Design new Widget trait #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First cut at Widget trait #26

First cut at Widget trait #26

raphlinus commented Dec 12, 2022

jeffparsons Dec 12, 2022

raphlinus Dec 12, 2022

Zoxc Dec 16, 2022

jeffparsons Dec 12, 2022

Imberflur Dec 13, 2022

raphlinus Dec 13, 2022

xarvic left a comment

xarvic Jan 21, 2023

raphlinus Jan 21, 2023

xarvic Jan 21, 2023

raphlinus Jan 21, 2023

xarvic Jan 21, 2023

raphlinus Jan 21, 2023

raphlinus commented Jan 21, 2023

DJMcNab left a comment

richard-uk1 commented Jan 22, 2023

DJMcNab commented Jan 22, 2023

First cut at Widget trait #26

First cut at Widget trait #26

Conversation

raphlinus commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xarvic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus commented Jan 21, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

richard-uk1 commented Jan 22, 2023

DJMcNab commented Jan 22, 2023