Fix #463 - broken inter-window update messages #464
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timboudreau
force-pushed
the
window_ids_clean
branch
3 times, most recently
from
cba5e85
to
35ceb9c
Compare
This addresses a couple of issues: * Messages (including repaints) not getting processed when delivered a `View` in a window which is inactive * Ability to retrieve the window id of a view without the application having to pass that information down to whatever code needs it and addresses a related issue - obtaining a `WindowId` for a realized `View` and its bounds on screen, in order to convert view-relative locations into screen locations for purposes such as positioning windows relative to a `View`. This patch adds a tracking data structure, and a convenience data structure which contains sufficient information to convert view-relative positions to screen-positions and back. * A static mapping of `root-id:WindowId`:`Arc<Window>` which is populated on window creation and cleared on destruction, which makes it possible to access `Window.request_redraw()` to force a redraw. It is guarded by a read-write lock (but written to only during window creation and destruction). * `ScreenLayout` contains a snapshot of the window id, the window's content and framed bounds, the view's origin within the window and the monitor scaling. So, a ViewId that wants its window-id looks up its root view's id, and looks up the WindowId associated with that root. We extend `WindowId` with methods to get the window bounds on screen with and without decorations and similar - these methods could be added to `ViewId` instead, but it seems more intuitive that you ask a window id for the window's bounds the same way you ask a view id for the view's bounds - that seems like what users will expect. Given the usage patterns (the mapping will only be locked for writing when a window is being created or destroyed, and will not be read-locked except in the case of forcing a repaint or looking up window coordinates), it is not touched in the critical path of rendering - unless the application and destroying many windows at the same time on different threads, I would not expect the read-write lock to be contended at all in practice (famous last words, I know). A thread-local for the mapping, as is done in `update.rs` might work (or look like it works), but we would be assuming no platform floem will ever support uses different event threads for different windows' event loops. I know of two that do - BeOS and Java AWT (with its stack of event threads to allow modal dialogs that work when their parent is disabled or blocked, which was a source of "interesting" deadlocks). If there was some path that I missed that would let a `ViewId` reach down to the `WindowHandle` that owns it without doing this sort of tracking, I would love to know about it - `PaintCx` has this information, but it is the only thing that does, and the `ViewId` has no access to it.
timboudreau
force-pushed
the
window_ids_clean
branch
from
35ceb9c
to
6ab4d1f
Compare
dzhou121
reviewed
May 30, 2024
| self.add_update_message(UpdateMessage::RequestPaint); | ||
| if !active { | ||
| self.force_repaint(); |
There was a problem hiding this comment.
do we still need this given that we've now processing all update messages, and if there's a repaint needed, the repaint request would be sent for that window?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a couple of issues:
and addresses a related issue (related to dealing with WindowIds - the solution falls out of the solution for the above):
This patch adds atracking data structure:
root-id:WindowId:Arc<Window>which is populated on window creation and cleared on destruction, which makes it possible to accessWindow.request_redraw()to force a redraw. It is guarded by a read-write lock (but written to only during window creation and destruction).So, a ViewId that wants its window-id looks up its root view's id, and looks up the WindowId associated with that root.
We extend
WindowIdwith methods to get the window bounds on screen with and without decorations and similar - these methods could be added toViewIdinstead, but it seems more intuitive that you ask a window id for the window's bounds the same way you ask a view id for the view's bounds - that seems like what users will expect.Given the usage patterns (the mapping will only be locked for writing when a window is being created or destroyed, and will not be read-locked except in the case of forcing a repaint or looking up window coordinates), it is not touched in the critical path of rendering - unless the application and destroying many windows at the same time on different threads, I would not expect the read-write lock to be contended at all in practice (famous last words, I know).
A thread-local for the mapping, as is done in
update.rsmight work (or look like it works), but we would be assuming no platform floem will ever support uses different event threads for different windows' event loops. I know of two that do - BeOS and Java AWT (with its stack of event threads to allow modal dialogs that work when their parent is disabled or blocked, which was a source of "interesting" deadlocks).If there was some path that I missed that would let a
ViewIdreach down to theWindowHandlethat owns it without doing this sort of tracking, I would love to know about it -PaintCxhas this information, but it is the only thing that does, and theViewIdhas no access to it.