From 57adef1fd4e91736732d9b4313e5502f43ee3fc3 Mon Sep 17 00:00:00 2001 From: Tim Boudreau Date: Mon, 3 Jun 2024 15:54:37 -0400 Subject: [PATCH 1/2] Expose `View::default_compute_layout` While this method *is* implementable in custom component by copy-paste programming, it would be preferable for custom components to be able to keep up with any changes in the default implementation by calling it directly instead. --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 4f66f132..fc46d702 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -195,6 +195,6 @@ pub use peniko; pub use peniko::kurbo; pub use screen_layout::ScreenLayout; pub use taffy; -pub use view::{recursively_layout_view, AnyView, IntoView, View}; +pub use view::{default_compute_layout, recursively_layout_view, AnyView, IntoView, View}; pub use window::{close_window, new_window}; pub use window_id::{Urgency, WindowIdExt}; From 1f05c206dcf4c71f5fd4bc6c5dd10b98b9814145 Mon Sep 17 00:00:00 2001 From: Tim Boudreau Date: Wed, 5 Jun 2024 15:35:40 -0400 Subject: [PATCH 2/2] Fix focus handling in multi-window applications This took some time to debug - starting with an application that opens a popup window, wherein the user should be able to tab between items and press a key to select and close the popup. It turned out to be **three** separate issues: 1. `window_handle.process_central_messages()` would discard any messages for `View`s with a different root than its own, so they would never be processed. 2. `ViewId` attempted to cache the root view id, but would just cache the deepest parent found - so if you do something that generates an update message during construction of a tree of views, the cached value would not actually be the root view for the (not yet existing) window 3. Not quite as strictly a bug, but I can't imagine why it would be by design: `FocusGained` and `FocusLost` events caused by a call to `ViewId.request_focus()` *would* be delivered to listeners, but a `View` would never receive them via `event_before_children()` or `event_after_children()` - so this one had a workaround, but the workaround - a view attaching listeners to itself - would be much less readable than a straightforward implementation of one of the event handler methods The fix for 3. I am the least sure about: * It is a bit ugly (swapping an `EventCx` for an `UpdateCx` to call `unconditional_view_event()` and then recreating the `UpdateCx`) * It is not clear if there are *other* kinds of events which have the same problem of non-delivery to the view itself * `ViewId.apply_event()` might be a cleaner place to implement it (though it would mean a bunch of calling back and forth between `WindowHandle` and `ViewId` which seems less than ideal) * I do *not* stop propagation of events if `event_*_children()` returns `EventPropagation::Stop` - as long is this is about focus events, I think it is probably harmful to be able to block propagation of focus events - window focus events more so, since it's unlikely that one view in a tree knows for sure that none of its siblings need to know about the window losing focus - but the same logic applies more weakly to plain view focus events - it's a recipe for hard-to-diagnose focus bugs. Since I'm a proud member of the 1990's `println` school of debugging, `UpdateMessage` now implements `Debug` - needed it to prove that retained messages really were eventually processed, and it will probably be useful in the future. --- src/id.rs | 18 +++++++++-- src/lib.rs | 2 +- src/update.rs | 69 ++++++++++++++++++++++++++++++++++++++++++ src/view_storage.rs | 22 +++++++------- src/window_handle.rs | 63 +++++++++++++++++++++++++++++++++++--- src/window_tracking.rs | 16 +++++++++- 6 files changed, 170 insertions(+), 20 deletions(-) diff --git a/src/id.rs b/src/id.rs index becc3c7c..3f1c2432 100644 --- a/src/id.rs +++ b/src/id.rs @@ -22,7 +22,7 @@ use crate::{ view::{IntoView, View}, view_state::{ChangeFlags, StackOffset, ViewState}, view_storage::VIEW_STORAGE, - window_tracking::window_id_for_root, + window_tracking::{is_known_root, window_id_for_root}, ScreenLayout, }; @@ -37,6 +37,9 @@ impl ViewId { pub fn remove(&self) { VIEW_STORAGE.with_borrow_mut(|s| { + // Remove the cached root, in the (unlikely) case that this view is + // re-added to a different window + s.root.remove(*self); if let Some(Some(parent)) = s.parent.get(*self) { if let Some(children) = s.children.get_mut(*parent) { children.retain(|c| c != self); @@ -155,11 +158,20 @@ impl ViewId { pub(crate) fn root(&self) -> Option { VIEW_STORAGE.with_borrow_mut(|s| { if let Some(root) = s.root.get(*self) { + // The cached value will be cleared on remove() above return *root; } let root_view_id = s.root_view_id(*self); - s.root.insert(*self, root_view_id); - root_view_id + // root_view_id() always returns SOMETHING. If the view is not yet added + // to a window, it can be itself or its nearest ancestor, which means we + // will store garbage permanently. + if let Some(root) = root_view_id { + if is_known_root(&root) { + s.root.insert(*self, root_view_id); + return Some(root); + } + } + None }) } diff --git a/src/lib.rs b/src/lib.rs index fc46d702..4f66f132 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -195,6 +195,6 @@ pub use peniko; pub use peniko::kurbo; pub use screen_layout::ScreenLayout; pub use taffy; -pub use view::{default_compute_layout, recursively_layout_view, AnyView, IntoView, View}; +pub use view::{recursively_layout_view, AnyView, IntoView, View}; pub use window::{close_window, new_window}; pub use window_id::{Urgency, WindowIdExt}; diff --git a/src/update.rs b/src/update.rs index 941be91a..bd690ddb 100644 --- a/src/update.rs +++ b/src/update.rs @@ -96,3 +96,72 @@ pub(crate) enum UpdateMessage { }, WindowVisible(bool), } + +impl std::fmt::Debug for UpdateMessage { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + UpdateMessage::Focus(id) => f.write_fmt(format_args!("Focus({:?})", id)), + UpdateMessage::ClearFocus(id) => f.write_fmt(format_args!("ClearFocus({:?})", id)), + UpdateMessage::Active(id) => f.write_fmt(format_args!("Active({:?})", id)), + UpdateMessage::ClearActive(id) => f.write_fmt(format_args!("ClearActive({:?})", id)), + UpdateMessage::WindowScale(scale) => { + f.write_fmt(format_args!("WindowScale({})", scale)) + } + UpdateMessage::Disabled { id, is_disabled } => { + f.write_fmt(format_args!("Disabled({:?}:{})", id, is_disabled)) + } + UpdateMessage::RequestPaint => f.write_str("RequestPaint"), + UpdateMessage::State { id, state: _ } => { + f.write_fmt(format_args!("State({:?}:???)", id)) + } + UpdateMessage::KeyboardNavigable { id } => { + f.write_fmt(format_args!("KeyboardNavigable({:?})", id)) + } + UpdateMessage::Draggable { id } => f.write_fmt(format_args!("Draggable({:?})", id)), + UpdateMessage::ToggleWindowMaximized => f.write_str("ToggleWindowMaximized"), + UpdateMessage::SetWindowMaximized(maximized) => { + f.write_fmt(format_args!("SetWindowMaximized({})", maximized)) + } + UpdateMessage::MinimizeWindow => f.write_str("MinimizeWindow"), + UpdateMessage::DragWindow => f.write_str("DragWindow"), + UpdateMessage::DragResizeWindow(direction) => { + f.write_fmt(format_args!("DragResizeWindow({:?})", direction)) + } + UpdateMessage::SetWindowDelta(delta) => { + f.write_fmt(format_args!("SetWindowDelta({}, {})", delta.x, delta.y)) + } + UpdateMessage::Animation { id, animation: _ } => { + f.write_fmt(format_args!("Animation({:?})", id)) + } + UpdateMessage::ShowContextMenu { menu: _, pos } => { + f.write_fmt(format_args!("ShowContextMenu({:?})", pos)) + } + UpdateMessage::WindowMenu { menu: _ } => f.write_str("WindowMenu"), + UpdateMessage::SetWindowTitle { title } => { + f.write_fmt(format_args!("SetWindowTitle({:?})", title)) + } + UpdateMessage::AddOverlay { + id, + position, + view: _, + } => f.write_fmt(format_args!("AddOverlay({:?} : {:?})", id, position)), + UpdateMessage::RemoveOverlay { id } => { + f.write_fmt(format_args!("RemoveOverlay({:?})", id)) + } + UpdateMessage::Inspect => f.write_str("Inspect"), + UpdateMessage::ScrollTo { id, rect } => { + f.write_fmt(format_args!("ScrollTo({:?}:{:?})", id, rect)) + } + UpdateMessage::FocusWindow => f.write_str("FocusWindow"), + UpdateMessage::SetImeAllowed { allowed } => { + f.write_fmt(format_args!("SetImeAllowed({})", allowed)) + } + UpdateMessage::SetImeCursorArea { position, size } => { + f.write_fmt(format_args!("SetImeCursorArea({:?}, {:?})", position, size)) + } + UpdateMessage::WindowVisible(visible) => { + f.write_fmt(format_args!("WindowVisible({})", visible)) + } + } + } +} diff --git a/src/view_storage.rs b/src/view_storage.rs index b6ed7b3c..3a0b67ba 100644 --- a/src/view_storage.rs +++ b/src/view_storage.rs @@ -52,18 +52,18 @@ impl ViewStorage { } } + /// Returns the deepest view ID encountered traversing parents. It does *not* guarantee + /// that it is a real window root; any caller should perform the same test + /// of `window_tracking::is_known_root()` that `ViewId.root()` does before + /// assuming the returned value is really a window root. pub(crate) fn root_view_id(&self, id: ViewId) -> Option { - let mut parent = self.parent.get(id).cloned().flatten(); - while let Some(parent_id) = parent { - match self.parent.get(parent_id).cloned().flatten() { - Some(id) => { - parent = Some(id); - } - None => { - return parent; - } - } + // Rewritten from the original looping version - unless we anticipate view ids + // deeper than the possible stack depth, this should be more efficient and + // require less cloning. + if let Some(p) = self.parent.get(id).unwrap_or(&None) { + self.root_view_id(*p) + } else { + Some(id) } - parent } } diff --git a/src/window_handle.rs b/src/window_handle.rs index 1b7c3c3b..aa24498b 100644 --- a/src/window_handle.rs +++ b/src/window_handle.rs @@ -310,6 +310,9 @@ impl WindowHandle { } } if was_focused != cx.app_state.focus { + // What is this for? If you call ViewId.request_focus(), this + // causes focus to be set to your view, then briefly set to + // None here and then back. Why? cx.app_state.focus_changed(was_focused, cx.app_state.focus); } @@ -701,11 +704,28 @@ impl WindowHandle { CENTRAL_UPDATE_MESSAGES.with_borrow_mut(|central_msgs| { if !central_msgs.is_empty() { UPDATE_MESSAGES.with_borrow_mut(|msgs| { - let central_msgs = std::mem::take(&mut *central_msgs); - for (id, msg) in central_msgs { + // We need to retain any messages which are for a view that either belongs + // to a different window, or which does not yet have a root + let removed_central_msgs = + std::mem::replace(central_msgs, Vec::with_capacity(central_msgs.len())); + for (id, msg) in removed_central_msgs { if let Some(root) = id.root() { let msgs = msgs.entry(root).or_default(); msgs.push(msg); + } else { + // Messages that are not for our root get put back - they may + // belong to another window, or may be construction-time messages + // for a View that does not yet have a window but will momentarily. + // + // Note that if there is a plethora of events for ids which were created + // but never assigned to any view, they will probably pile up in here, + // and if that becomes a real problem, we may want a garbage collection + // mechanism, or give every message a max-touch-count and discard it + // if it survives too many iterations through here. Unclear if there + // are real-world app development patterns where that could actually be + // an issue. Since any such mechanism would have some overhead, there + // should be a proven need before building one. + central_msgs.push((id, msg)); } } }); @@ -716,11 +736,17 @@ impl WindowHandle { if !central_msgs.borrow().is_empty() { DEFERRED_UPDATE_MESSAGES.with(|msgs| { let mut msgs = msgs.borrow_mut(); - let central_msgs = std::mem::take(&mut *central_msgs.borrow_mut()); - for (id, msg) in central_msgs { + let removed_central_msgs = std::mem::replace( + &mut *central_msgs.borrow_mut(), + Vec::with_capacity(msgs.len()), + ); + let unprocessed = &mut *central_msgs.borrow_mut(); + for (id, msg) in removed_central_msgs { if let Some(root) = id.root() { let msgs = msgs.entry(root).or_default(); msgs.push((id, msg)); + } else { + unprocessed.push((id, msg)); } } }); @@ -748,11 +774,40 @@ impl WindowHandle { if cx.app_state.focus != Some(id) { let old = cx.app_state.focus; cx.app_state.focus = Some(id); + + // Fix: Focus events were never dispatched to views' own + // event_before_children / event_after_children. + // + // This is a bit messy; could it be done in ViewId.apply_event() + // instead? Are there other cases of dropped events? + let mut ec = EventCx { + app_state: cx.app_state, + }; + + if let Some(old) = old.as_ref() { + ec.unconditional_view_event(*old, Event::FocusLost, true); + } + ec.unconditional_view_event(id, Event::FocusGained, true); + + cx = UpdateCx { + app_state: ec.app_state, + }; + cx.app_state.focus_changed(old, cx.app_state.focus); } } UpdateMessage::ClearFocus(id) => { + let old = cx.app_state.focus; cx.app_state.clear_focus(); + if let Some(old) = old { + let mut ec = EventCx { + app_state: cx.app_state, + }; + ec.unconditional_view_event(old, Event::FocusLost, true); + cx = UpdateCx { + app_state: ec.app_state, + }; + } cx.app_state.focus_changed(Some(id), None); } UpdateMessage::Active(id) => { diff --git a/src/window_tracking.rs b/src/window_tracking.rs index 6ef01607..94e77913 100644 --- a/src/window_tracking.rs +++ b/src/window_tracking.rs @@ -72,7 +72,17 @@ impl WindowMapping { } fn window_id_for_root(&self, id: &ViewId) -> Option { - self.window_id_for_root_view_id.get(id).copied() + let result = self.window_id_for_root_view_id.get(id).copied(); + // We are called by ViewId.window_id(), which should no longer ever return a + // ViewId from root() that is not actually a root - so if we get here with a + // window id that has gained a parent since it was determined to be a root, + // something is very wrong. + debug_assert!( + id.parent().is_none(), + "Not a root view id: {:?} - check the logic in ViewId.window_id().", + id + ); + result } fn root_view_id_for(&self, window_id: &WindowId) -> Option { @@ -94,6 +104,10 @@ pub fn with_window_id_and_window T, T>( .unwrap_or(None) } +pub fn is_known_root(id: &ViewId) -> bool { + with_window_map(|map| map.window_id_for_root_view_id.contains_key(id)).unwrap_or(false) +} + fn with_window_map_mut(mut f: F) -> bool { let map = WINDOW_FOR_WINDOW_AND_ROOT_IDS.get_or_init(|| RwLock::new(Default::default())); if let Ok(mut map) = map.write() {