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

Document RenderRoot fields #824

Merged
merged 2 commits into from
Jan 14, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 56 additions & 10 deletions masonry/src/render_root.rs
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the new gaps between fields. I would rather that related fields (size_policy, size, scale_factor for example) were grouped without newlines.

Original file line number Diff line number Diff line change
Expand Up @@ -51,45 +51,92 @@ const INVALID_IME_AREA: Rect = Rect::new(f64::NAN, f64::NAN, f64::NAN, f64::NAN)
///
/// This is also the type that owns the widget tree.
pub struct RenderRoot {
/// Root of the widget tree.
pub(crate) root: WidgetPod<Box<dyn Widget>>,

/// Whether the window size should be determined by the content or the user.
pub(crate) size_policy: WindowSizePolicy,

/// Current size of the window.
pub(crate) size: PhysicalSize<u32>,
// TODO - Currently this is always 1.0
// kurbo coordinates are assumed to be in logical pixels

/// DPI scale factor.
///
/// Kurbo coordinates are assumed to be in logical pixels
pub(crate) scale_factor: f64,

/// Is `Some` if the most recently displayed frame was an animation frame.
pub(crate) last_anim: Option<Instant>,

/// Last mouse position. Updated by `on_pointer_event` pass, used by other passes.
pub(crate) last_mouse_pos: Option<LogicalPosition<f64>>,
pub(crate) cursor_icon: CursorIcon,

/// State passed to context types.
pub(crate) global_state: RenderRootState,
// TODO - Add "access_tree_active" to detect when you don't need to update the

/// Whether the next accessibility pass should rebuild the entire access tree.
///
/// TODO - Add `access_tree_active` to detect when you don't need to update the
// access tree
pub(crate) rebuild_access_tree: bool,

/// The widget tree; stores widgets and their states.
pub(crate) widget_arena: WidgetArena,
}

// TODO - Document these fields.
/// State shared between passes.
pub(crate) struct RenderRootState {
/// Queue of signals to be processed by the event loop.
pub(crate) signal_queue: VecDeque<RenderRootSignal>,

/// Currently focused widget.
pub(crate) focused_widget: Option<WidgetId>,

/// List of ancestors of the currently focused widget.
pub(crate) focused_path: Vec<WidgetId>,

/// Widget that will be focused once the `update_focus` pass is run.
pub(crate) next_focused_widget: Option<WidgetId>,

/// Most recently clicked widget.
///
/// This is used to pick the focused widget on Tab events.
pub(crate) most_recently_clicked_widget: Option<WidgetId>,

/// Widgets that have requested to be scrolled into view.
pub(crate) scroll_request_targets: Vec<(WidgetId, Rect)>,

/// List of ancestors of the currently hovered widget.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here would be a good reason to explain why we store this (i.e. so that hover state can be properly tracked after a reparenting)

pub(crate) hovered_path: Vec<WidgetId>,

/// Widget that currently has pointer capture.
pub(crate) pointer_capture_target: Option<WidgetId>,

/// Current cursor icon.
pub(crate) cursor_icon: CursorIcon,

/// Cache for Parley font data.
pub(crate) font_context: FontContext,

/// Cache for Parley text layout data.
pub(crate) text_layout_context: LayoutContext<BrushIndex>,

/// List of callbacks that will run in the next `mutate` pass.
pub(crate) mutate_callbacks: Vec<MutateCallback>,

/// Whether an IME session is active.
pub(crate) is_ime_active: bool,
/// The IME area last sent to the platform.
///
/// This allows only sending the area to the platform when the area has changed.

/// The area in which text is being edited.
Comment on lines -86 to +130
Copy link
Member

Choose a reason for hiding this comment

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

I think this is worse than what was there before? How does this improve clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overall I'm trying to document "what does this field represent" more than "when is it updated".

Copy link
Member

Choose a reason for hiding this comment

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

I'd claim that this new doc comment is actively worse than not having a comment at all. The name of the field is clearer for what it is for...

I do agree that we should have documented this workaround more clearly, but I'm not sure I agree that gutting the existing documentation is the right way to force that.

pub(crate) last_sent_ime_area: Rect,

/// Scene cache for the widget tree.
pub(crate) scenes: HashMap<WidgetId, Scene>,

/// Whether data set in the pointer pass has been invalidated.
pub(crate) needs_pointer_pass: bool,

/// Pass tracing configuration, used to skip tracing to limit overhead.
pub(crate) trace: PassTracing,
}

Expand Down Expand Up @@ -193,7 +240,6 @@ impl RenderRoot {
scale_factor,
last_anim: None,
last_mouse_pos: None,
cursor_icon: CursorIcon::Default,
global_state: RenderRootState {
signal_queue: VecDeque::new(),
focused_widget: None,
Expand Down Expand Up @@ -404,7 +450,7 @@ impl RenderRoot {

/// Get the current icon that the mouse should display.
pub fn cursor_icon(&self) -> CursorIcon {
self.cursor_icon
self.global_state.cursor_icon
}

// --- MARK: ACCESS WIDGETS---
Expand Down
Loading