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.

Sign up for GitHub

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

Jump to bottom

Fix focus handling in multi-window applications #489

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix focus handling in multi-window applications #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57adef1
Expose `View::default_compute_layout`
timboudreau Jun 3, 2024
1f05c20
Fix focus handling in multi-window applications
timboudreau Jun 5, 2024
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
18 changes: 15 additions & 3 deletions src/id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand All @@ -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);
Expand Down Expand Up @@ -155,11 +158,20 @@ impl ViewId {
pub(crate) fn root(&self) -> Option<ViewId> {
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
})
}

Expand Down
69 changes: 69 additions & 0 deletions src/update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
}
}
22 changes: 11 additions & 11 deletions src/view_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ViewId> {
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)
Comment on lines +60 to +66

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to leave it as-is too. Though I think if you have enough view ids to exhaust the stack, you might have some UI usability problems far worse than hitting that limit :-)

}
parent
}
}
63 changes: 59 additions & 4 deletions src/window_handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

the app_state focus will taken from it's option before a pointer down even is propagated. Once event propagation is done, if another view has set itself to have focus because of the pointer down event the focus_changed function will notify the old was_focused that it's focus has been lost and the new app state focus that it has gained focus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I found (I added dumping the stack to the focus method it calls if the value was None) is that, when someViewId.request_focus() was called with an existing focus target, focus goes old_id -> None -> new_id.

I didn't check if events were fired for focus going to None, since event propagation wasn't working at that point. As long as we're not doubling up event notifications, it's probably fine - just peculiar.

cx.app_state.focus_changed(was_focused, cx.app_state.focus);
}

Expand Down Expand Up @@ -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));
}
}
});
Expand All @@ -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));
}
}
});
Expand Down Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

Other events are from winit, so they would be dispatched by the event method, while Event::FocusGained and Event::FocusLost is kind of an "internal" event.

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

unconditional_view_event probably needs to called in focus_changed.

E.g. when the focus got changed due to pointer click.

}
}
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) => {
Expand Down
16 changes: 15 additions & 1 deletion src/window_tracking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,17 @@ impl WindowMapping {
}

fn window_id_for_root(&self, id: &ViewId) -> Option<WindowId> {
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<ViewId> {
Expand All @@ -94,6 +104,10 @@ pub fn with_window_id_and_window<F: FnOnce(&WindowId, &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<F: FnMut(&mut WindowMapping)>(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() {
Expand Down