Fix focus handling in multi-window applications #489
Conversation
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.
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.
|
Ignore the fact that Expose View::default_compute_layout appears to be one of the commits here - I did this on my branch containing that commit, but the commit that applies here reverts it. |
|
Noted in comments, but will mention this here: Retaining messages for unrecognized view roots has the potential to leak You know more about the common use-cases for Floem than I do - is this likely to be a problem? If so, the simplest gc-like solution would just be to wrap messages in a struct that keeps a counter, increment the counter each time the same message is seen in the queue but not processed, and if it crosses some magic threshold number, don't put it back. Any other solution would involve diffing collections and could get expensive, while if a message hangs out in the queue unprocessed for 100 events (or whatever number seems right), that's not going to have a big impact. I think probably only implement a solution if we find out that it's actually a problem - fiddling with |
|
Just in case you're wondering what on earth I'm doing that I'm running into these sorts of issues, here's a screen shot of a prototype of the DSP plugin UI: (Actually plumbing this thing, given the need to interoperate with pointers handed to me over FFI as if they were |
I think is a regression after the ECS pr |
| // 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) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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 :-)
| @@ -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? | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Not familiar with that PR. Is there a less invasive (or at least less clunky) fix, then? |
| // 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? |
There was a problem hiding this comment.
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.
| cx = UpdateCx { | ||
| app_state: ec.app_state, | ||
| }; | ||
|
|
||
| cx.app_state.focus_changed(old, cx.app_state.focus); |
There was a problem hiding this comment.
unconditional_view_event probably needs to called in focus_changed.
E.g. when the focus got changed due to pointer click.
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. Focus handling did not work at all in the popup window.
It turned out to be three separate issues:
window_handle.process_central_messages()would discard any messages forViews with a different root than its own, so they would never be processed, so focus requests would be discarded if the main window queried the queue firstViewIdattempted 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, and it would always returnNonefromwindow_id()which relies on the (in this case bogus) root to map it to a window - so a call torequest_focus()on a view too early would leave it with garbage as its root id, and no window handle would ever recognize and process it with the wrong rootFocusGainedandFocusLostevents caused by a call toViewId.request_focus()would be delivered to listeners, but aViewwould never receive them viaevent_before_children()orevent_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 methodsThe fix for 3. I am the least sure about:
EventCxfor anUpdateCxto callunconditional_view_event()and then recreating theUpdateCx)ViewId.apply_event()might be a cleaner place to implement it (though it would mean a bunch of calling back and forth betweenWindowHandleandViewIdwhich might be less than ideal)event_*_children()returnsEventPropagation::Stop- as long is this is about focus events, I think it is probably harmful to be able to block propagation of focus events at all - 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
printlnschool of debugging,UpdateMessagenow implementsDebug- needed it to prove that retained messages really were eventually processed, and it will probably be useful in the future.