Tweak documentation for readability (#787)

Apply many small tweaks I found while going over the documentation.
Rename some variables.
Add asserts to `BoxConstraints::constrain_aspect_ratio`.
Flesh out the WidgetState documentation.
Introduce the concept of "zombie flags".

Author: PoignardAzur

masonry/src/action.rs

@@ -5,7 +5,8 @@ use std::any::Any;
 
 use crate::event::PointerButton;
 
-// TODO - Refactor - See issue https://github.com/linebender/xilem/issues/335
+// TODO - Replace actions with an associated type on the Widget trait
+// See https://github.com/linebender/xilem/issues/664
 
 // TODO - TextCursor changed, ImeChanged, EnterKey, MouseEnter
 #[non_exhaustive]

masonry/src/box_constraints.rs

@@ -184,7 +184,17 @@ impl BoxConstraints {
     ///
     /// Use this function when maintaining an aspect ratio is more important than minimizing the
     /// distance between input and output size width and height.
+    ///
+    /// ## Panics
+    ///
+    /// Panics if `aspect_ratio` or `width` are NaN, infinite or negative.
+    #[track_caller]
     pub fn constrain_aspect_ratio(&self, aspect_ratio: f64, width: f64) -> Size {
+        assert!(aspect_ratio.is_finite());
+        assert!(width.is_finite());
+        assert!(aspect_ratio >= 0.0);
+        assert!(width >= 0.0);
+
         // Minimizing/maximizing based on aspect ratio seems complicated, but in reality everything
         // is linear, so the amount of work to do is low.
         let ideal_size = Size {
@@ -201,8 +211,6 @@ impl BoxConstraints {
             return ideal_size;
         }
 
-        // Then we check if any `Size`s with our desired aspect ratio are inside the constraints.
-        // TODO this currently outputs garbage when things are < 0 - See https://github.com/linebender/xilem/issues/377
         let min_w_min_h = self.min.height / self.min.width;
         let max_w_min_h = self.min.height / self.max.width;
         let min_w_max_h = self.max.height / self.min.width;

masonry/src/contexts.rs

@@ -3,8 +3,6 @@
 
 //! The context types that are passed into various widget methods.
 
-use std::time::Duration;
-
 use accesskit::TreeUpdate;
 use dpi::LogicalPosition;
 use parley::{FontContext, LayoutContext};
@@ -574,7 +572,7 @@ impl_context_method!(MutateCtx<'_>, EventCtx<'_>, UpdateCtx<'_>, {
     pub fn children_changed(&mut self) {
         trace!("children_changed");
         self.widget_state.children_changed = true;
-        self.widget_state.update_focus_chain = true;
+        self.widget_state.needs_update_focus_chain = true;
         self.request_layout();
     }
 
@@ -710,14 +708,6 @@ impl_context_method!(
                 .push_back(RenderRootSignal::ShowWindowMenu(position));
         }
 
-        /// Request a timer event.
-        ///
-        /// The return value is a token, which can be used to associate the
-        /// request with the event.
-        pub fn request_timer(&mut self, _deadline: Duration) -> TimerToken {
-            todo!("request_timer");
-        }
-
         /// Mark child widget as stashed.
         ///
         /// If `stashed` is true, the child will not be painted or listed in the accessibility tree.
@@ -738,9 +728,6 @@ impl_context_method!(
     }
 );
 
-// FIXME - Remove
-pub struct TimerToken;
-
 impl EventCtx<'_> {
     // TODO - clearly document all semantics of pointer capture when they've been decided on
     // TODO - Figure out cases where widget should be notified of pointer capture

masonry/src/debug_logger.rs

@@ -204,7 +204,7 @@ impl DebugLogger {
             StateTree::new("has_focus", w_state.has_focus),
             StateTree::new("request_anim", w_state.request_anim),
             StateTree::new("children_changed", w_state.children_changed),
-            StateTree::new("update_focus_chain", w_state.update_focus_chain),
+            StateTree::new("update_focus_chain", w_state.needs_update_focus_chain),
         ]
         .into();
         state

masonry/src/doc/01_creating_app.md

@@ -118,8 +118,8 @@ When handling `ButtonPressed`:
 - `ctx.render_root()` returns a reference to the `RenderRoot`, which owns the widget tree and all the associated visual state.
 - `RenderRoot::edit_root_widget()` takes a closure; that closure takes a `WidgetMut<Box<dyn Widget>>` which we call `root`. Once the closure returns, `RenderRoot` runs some passes to update the app's internal states.
 - `root.downcast::<...>()` returns a `WidgetMut<RootWidget<...>>`.
-- `RootWidget::child_mut()` returns a `WidgetMut<Portal<...>>` for the `Portal`.
-- `Portal::child_mut()` returns a `WidgetMut<Flex>` for the `Flex`.
+- `RootWidget::child_mut()` returns a `WidgetMut<Portal<...>>`.
+- `Portal::child_mut()` returns a `WidgetMut<Flex>`.
 
 A [`WidgetMut`] is a smart reference type which lets us modify the widget tree.
 It's set up to automatically propagate update flags and update internal state when dropped.

masonry/src/doc/03_implementing_container_widget.md

@@ -254,11 +254,11 @@ Doesn't `VerticalStack::paint` need to call `paint` on its children, for instanc
 
 It doesn't.
 
-In Masonry, most passes are automatically propagated to children, without container widgets having to implement code iterating over their children.
+In Masonry, most passes are automatically propagated to children, and so container widgets do not need to *and cannot* call the pass methods on their children.
 
 So for instance, if `VerticalStack::children_ids()` returns a list of three children, the paint pass will automatically call `paint` on all three children after `VerticalStack::paint()`.
 
-So various methods in container widgets should only implement the logic that is specific to the container itself.
+Pass methods in container widgets should only implement the logic that is specific to the container itself.
 For instance, a container widget with a background color should implement `paint` to draw the background.
 
 [`Widget`]: crate::Widget

masonry/src/doc/05_pass_system.md

@@ -12,15 +12,15 @@
 
 </div>
 
-Masonry has a set of **passes**, which are computations run over a subset of the widget tree during every frame.
+Masonry's internal architecture is based on a set of **passes**, which are computations run over a subset of the widget tree during every frame.
 
 Passes can be split into roughly three categories:
 
-- **Event passes:** triggered by user interaction.
-- **Rewrite passes:** run after every event pass, may run multiple times until all invalidation flags are cleared.
-- **Render passes:** run just before rendering a new frame.
+- **Event passes** are triggered by user interaction.
+- **Rewrite passes** run after every event pass, may run multiple times until all invalidation flags are cleared.
+- **Render passes** run just before rendering a new frame.
 
-Note: unless otherwise specified, all passes run over widgets in depth-first preorder.
+Note that unless otherwise specified, all passes run over widgets in depth-first preorder, where child order is determined by their position in the `children_ids()` array.
 
 
 ## Event passes
@@ -68,7 +68,7 @@ To address these invalidations, Masonry runs a set of **rewrite passes** over th
 The layout and compose passes have methods with matching names in the Widget trait.
 The update_xxx passes call the widgets' update method.
 
-By default, each of these passes completes immediately, unless pass-dependent invalidation flags are set.
+By default, each of these passes completes without doing any work, unless pass-dependent invalidation flags are set.
 Each pass can generally request work for later passes; for instance, the mutate pass can invalidate the layout of a widget, in which case the layout pass will run on that widget and its children and parents.
 
 Passes may also request work for *previous* passes, in which case all rewrite passes are run again in sequence.
@@ -106,6 +106,13 @@ It is called when new widgets are added to the tree, or existing widgets are rem
 
 It will call the `register_children()` widget method on container widgets whose children changed, then the `update()` method with the `WidgetAdded` event on new widgets.
 
+<!-- TODO - document update disabled --- -->
+<!-- TODO - document update stashed --- -->
+<!-- TODO - document update focus chain --- -->
+<!-- TODO - document update focus --- (document iteration order) -->
+<!-- TODO - document update scroll --- -->
+<!-- TODO - document update pointer --- (document iteration order) -->
+
 ### Layout pass
 
 The layout pass runs bidirectionally, passing constraints from the top down and getting back sizes and other layout info from the bottom up.
@@ -126,8 +133,6 @@ Because the compose pass is more limited than layout, it's easier to recompute i
 
 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.
-
 
 ## Render passes
 
@@ -144,17 +149,16 @@ These nodes together form the accessibility tree.
 Methods for these passes should be written under the assumption that they can be skipped or called multiple times for arbitrary reasons.
 Therefore, their ability to affect the widget tree is limited.
 
-Masonry automatically calls these methods for all widgets in the tree in depth-first preorder.
 
 ## External mutation
 
 Code with mutable access to the `RenderRoot`, like the Xilem app runner, can get mutable access to the root widget and all its children through the `edit_root_widget()` method, which takes a callback and passes it a `WidgetMut` to the root widget.
 
-This is in effect a MUTATE pass which only processes one callback.
+This is in effect a "mutate" pass which only processes one callback.
 
 External mutation is how Xilem applies any changes to the widget tree produced by its reactive step.
 
-Calling the `edit_root_widget()` method, or any similar direct-mutation method, triggers the entire set of rewrite passes.
+`RenderRoot::edit_root_widget()` triggers the entire set of rewrite passes before returning.
 
 
 ## Pass context types
@@ -164,7 +168,7 @@ Some notes about pass context types:
 - Render 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, `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.
 - [`MutateCtx`], [`EventCtx`] and [`UpdateCtx`] can let you add and remove children.
 - [`RegisterCtx`] can't do anything except register children.
 - [`QueryCtx`] provides read-only information about the widget.

masonry/src/event.rs

@@ -181,7 +181,6 @@ impl From<PointerButton> for PointerButtons {
 // TODO - How can RenderRoot express "I started a drag-and-drop op"?
 // TODO - Touchpad, Touch, AxisMotion
 // TODO - How to handle CursorEntered?
-// Note to self: Events like "pointerenter", "pointerleave" are handled differently at the Widget level. But that's weird because WidgetPod can distribute them. Need to think about this again.
 #[derive(Debug, Clone)]
 pub enum PointerEvent {
     PointerDown(PointerButton, PointerState),
@@ -313,21 +312,7 @@ pub enum Update {
 }
 
 impl PointerEvent {
-    pub fn new_pointer_leave() -> Self {
-        // TODO - The fact we're creating so many dummy values might be
-        // a sign we should refactor that struct
-        let pointer_state = PointerState {
-            physical_position: Default::default(),
-            position: Default::default(),
-            buttons: Default::default(),
-            mods: Default::default(),
-            count: 0,
-            focus: false,
-            force: None,
-        };
-        Self::PointerLeave(pointer_state)
-    }
-
+    /// Returns the [`PointerState`] of the event.
     pub fn pointer_state(&self) -> &PointerState {
         match self {
             Self::PointerDown(_, state)
@@ -343,13 +328,17 @@ impl PointerEvent {
         }
     }
 
+    /// Returns the position of the pointer event, except for [`PointerEvent::PointerLeave`] and [`PointerEvent::HoverFileCancel`].
     pub fn position(&self) -> Option<LogicalPosition<f64>> {
         match self {
             Self::PointerLeave(_) | Self::HoverFileCancel(_) => None,
             _ => Some(self.pointer_state().position),
         }
     }
 
+    /// Short name, for debug logging.
+    ///
+    /// Returns the enum variant name.
     pub fn short_name(&self) -> &'static str {
         match self {
             Self::PointerDown(_, _) => "PointerDown",
@@ -365,6 +354,10 @@ impl PointerEvent {
         }
     }
 
+    /// Returns true if the event is likely to occur every frame.
+    ///
+    /// Developers should avoid logging during high-density events to avoid
+    /// cluttering the console.
     pub fn is_high_density(&self) -> bool {
         match self {
             Self::PointerDown(_, _) => false,
@@ -379,23 +372,47 @@ impl PointerEvent {
             Self::Pinch(_, _) => true,
         }
     }
+
+    /// Create a [`PointerEvent::PointerLeave`] event with dummy values.
+    ///
+    /// This is used internally to create synthetic `PointerLeave` events when pointer
+    /// capture is lost.
+    pub fn new_pointer_leave() -> Self {
+        // TODO - The fact we're creating so many dummy values might be
+        // a sign we should refactor that struct
+        let pointer_state = PointerState {
+            physical_position: Default::default(),
+            position: Default::default(),
+            buttons: Default::default(),
+            mods: Default::default(),
+            count: 0,
+            focus: false,
+            force: None,
+        };
+        Self::PointerLeave(pointer_state)
+    }
 }
 
 impl TextEvent {
+    /// Short name, for debug logging.
     pub fn short_name(&self) -> &'static str {
         match self {
-            Self::KeyboardKey(KeyEvent { repeat: true, .. }, _) => "KeyboardKey (repeat)",
+            Self::KeyboardKey(KeyEvent { repeat: true, .. }, _) => "KeyboardKey(repeat)",
             Self::KeyboardKey(_, _) => "KeyboardKey",
             Self::Ime(Ime::Disabled) => "Ime::Disabled",
             Self::Ime(Ime::Enabled) => "Ime::Enabled",
             Self::Ime(Ime::Commit(_)) => "Ime::Commit",
             Self::Ime(Ime::Preedit(s, _)) if s.is_empty() => "Ime::Preedit(\"\")",
-            Self::Ime(Ime::Preedit(_, _)) => "Ime::Preedit",
+            Self::Ime(Ime::Preedit(_, _)) => "Ime::Preedit(\"...\")",
             Self::ModifierChange(_) => "ModifierChange",
             Self::FocusChange(_) => "FocusChange",
         }
     }
 
+    /// Returns true if the event is likely to occur every frame.
+    ///
+    /// Developers should avoid logging during high-density events to avoid
+    /// cluttering the console.
     pub fn is_high_density(&self) -> bool {
         match self {
             Self::KeyboardKey(_, _) => false,
@@ -408,6 +425,9 @@ impl TextEvent {
 }
 
 impl AccessEvent {
+    /// Short name, for debug logging.
+    ///
+    /// Returns the enum variant name.
     pub fn short_name(&self) -> &'static str {
         match self.action {
             accesskit::Action::Click => "Click",
@@ -442,15 +462,6 @@ impl AccessEvent {
 
 impl PointerState {
     pub fn empty() -> Self {
-        #[cfg(FALSE)]
-        #[allow(unsafe_code)]
-        // SAFETY: Uuuuh, unclear. Winit says the dummy id should only be used in
-        // tests and should never be passed to winit. In principle, we're never
-        // passing this id to winit, but it's still visible to custom widgets which
-        // might do so if they tried really hard.
-        // It would be a lot better if winit could just make this constructor safe.
-        let device_id = unsafe { DeviceId::dummy() };
-
         Self {
             physical_position: PhysicalPosition::new(0.0, 0.0),
             position: LogicalPosition::new(0.0, 0.0),
@@ -466,7 +477,7 @@ impl PointerState {
 impl Update {
     /// Short name, for debug logging.
     ///
-    /// Essentially returns the enum variant name.
+    /// Returns the enum variant name.
     pub fn short_name(&self) -> &str {
         match self {
             Self::WidgetAdded => "WidgetAdded",

masonry/src/passes/accessibility.rs

@@ -102,7 +102,7 @@ fn build_access_node(widget: &mut dyn Widget, ctx: &mut AccessCtx) -> Node {
             .collect::<Vec<NodeId>>(),
     );
 
-    // Note - These WidgetState flags can be modified by other passes.
+    // Note - The values returned by these methods can be modified by other passes.
     // When that happens, the other pass should set flags to request an accessibility pass.
     if ctx.is_disabled() {
         node.set_disabled();

masonry/src/passes/compose.rs

@@ -74,7 +74,7 @@ fn compose_widget(
 pub(crate) fn run_compose_pass(root: &mut RenderRoot) {
     let _span = info_span!("compose").entered();
 
-    // If widgets are moved, pointer-related info may be stale.
+    // If widgets have moved, pointer-related info may be stale.
     // For instance, the "hovered" widget may have moved and no longer be under the pointer.
     if root.root_state().needs_compose {
         root.global_state.needs_pointer_pass = true;

masonry/src/passes/event.rs

@@ -11,7 +11,7 @@ use crate::render_root::RenderRoot;
 use crate::{AccessEvent, EventCtx, Handled, PointerEvent, TextEvent, Widget, WidgetId};
 
 // --- MARK: HELPERS ---
-fn get_target_widget(
+fn get_pointer_target(
     root: &RenderRoot,
     pointer_pos: Option<LogicalPosition<f64>>,
 ) -> Option<WidgetId> {
@@ -89,8 +89,8 @@ pub(crate) fn run_on_pointer_event_pass(root: &mut RenderRoot, event: &PointerEv
 
     if event.is_high_density() {
         // We still want to record that this pass occurred in the debug file log.
-        // However, we choose not record any other tracing for this event,
-        // as that would have a lot of noise.
+        // However, we choose not to record any other tracing for this event,
+        // as that would create a lot of noise.
         trace!("Running ON_POINTER_EVENT pass with {}", event.short_name());
     } else {
         debug!("Running ON_POINTER_EVENT pass with {}", event.short_name());
@@ -101,7 +101,7 @@ pub(crate) fn run_on_pointer_event_pass(root: &mut RenderRoot, event: &PointerEv
         root.last_mouse_pos = event.position();
     }
 
-    let target_widget_id = get_target_widget(root, event.position());
+    let target_widget_id = get_pointer_target(root, event.position());
 
     let handled = run_event_pass(
         root,
@@ -135,11 +135,6 @@ pub(crate) fn run_on_pointer_event_pass(root: &mut RenderRoot, event: &PointerEv
     handled
 }
 
-// TODO https://github.com/linebender/xilem/issues/376 - Some implicit invariants:
-// - If a Widget gets a keyboard event or an ImeStateChange, then
-// focus is on it, its child or its parent.
-// - If a Widget has focus, then none of its parents is hidden
-
 // --- MARK: TEXT EVENT ---
 pub(crate) fn run_on_text_event_pass(root: &mut RenderRoot, event: &TextEvent) -> Handled {
     if matches!(event, TextEvent::FocusChange(false)) {