Inter-window messages sent to a view via `ViewId.update_state()` are broken about floem HOT 11 CLOSED

I've created a PR with a fix, and solving some general issues around window ids - it allows views to get their window id if they are realized on screen, extends WindowId with methods (in a separate trait) that let things like the window's screen bounds and monitor bounds for the monitor it is on be queried - following the same pattern used for ViewId already.

I am wondering a bit if there shouldn't be a sort of ScreenCoordinateSpace type you can get for a View, which has the necessary information to, say, compute window locations. For example, updating the demo code I linked in the first comment above, computing a proper location for a popup window relative to a View is more complex than would be really delightful - we have to reach into the WindowId once to get the bounds-on-screen, and again to get the monitor bounds (in order to reposition the target point so the popup cannot extend beyond the monitor bounds):

    fn compute_screen_location(&self, local_point: Option<Point>) -> Option<Point> {
        if let Some(bds) = self
            .id()
            .window_id()
            .and_then(|wid| wid.bounds_of_content_on_screen())
        {
            let mut start = Point::new(bds.x0, bds.y0);

            start.x += self.window_origin.x;
            start.y += self.window_origin.y;

            if let Some(lp) = local_point {
                start.x += lp.x;
                start.y += lp.y;
            }

            if let Some(wid) = self.id().window_id() {
                if let Some(mbds) = wid.monitor_bounds() {
                    let expected_size = square_size();
                    if mbds.x1 < start.x + expected_size {
                        start.x -= (start.x + expected_size) - mbds.x1;
                    }
                    if mbds.y1 < start.y + expected_size {
                        start.y -= (start.y + expected_size) - mbds.y1;
                    }
                }
            }

            Some(start)
        } else {
            local_point
        }
    }

Plus, this still involves fishing around in the Layout to find the position of the component within it's parent (that's just done when computing the layout, so the code doesn't appear here).

Thoughts?

from floem.

Some notes from digging into this a little bit, to see what possible solutions could be:

• The entry path to process any messages at all for a view is ApplicationHandle.handle_window_event(WindowEvent::RedrawRequested) -> WindowHandle.render_frame() -> WindowHandle.process_update_no_paint -> WindowHandle.process_update_messages()
• So, there is literally no path to reach process_update_messages() currently except via an OS-level paint event
  • I suspect creating any path to call that method that does not do it by generating such an event could easily violate assumptions floem makes internally about threading, or platform requirements that paints happen on a specific thread
  • In a sense, the root problem here is that ViewId.request_repaint() does nothing if the view is in an inactive window - it won't be processed until something else - native input event processing from winit triggers a repaint for its own reasons.
    • That, I think, is the bug here: a repaint request should not be deferred until ... something else requests a repaint. In a don't-call-us-we'll-call-you framework like this, where all work is driven by repaint requests, repaint requests themselves have to be special and privileged.

Now, how to fix it minimally?

It seems like forcing a repaint event through winit - i.e. tell the OS the window needs a repaint - is the only safe and sane way to do it - it ensures platform restrictions about when and how repainting code runs is honored. So, something like

• Repaint requested on id
• Find the root view id for the window
• Find the window id from the root
• Is it the active window?
  • Yes? Put the message in the queue and done, as it happens now
  • No? Do whatever dance is necessary with winit to send an OS-level repaint event to the parent window (I haven't looked at how to do that yet - going to do that now)

I see a few other things that seem questionable in the core update-messages plumbing: The use of thread-locals for the message queues is probably because you needed something mutable not because you needed something local to a specific thread - thought experiement: If I spawn a thread and push 100k state update messages to a view, then park it unitil the end of time, will those messages ever be processed? No. Will they ever be dropped? No. I think the data structure you want is a treiber stack (doesn't have to be my implementation that I just linked, but it's the right algorithm - a single-ended stack using atomics - UPDATE_MESSAGES is single-ended stack, and you just need a way to push and pull in a thread-safe way that doesn't run into mutability / borrow issues - there might even be benefits to the queue not needing a heap-allocated Vec per id). Anyway, that's for later examination - I'm mentioning it here because it's pretty intimately tied to code this bug touches (and so I or someone remembers to look at it).

from floem.

More notes: Thus far, digging through the code, I have not found any way for a View to get a reference to the WindowId that contains it (or for how to get a reference to the ApplicationHandle that owns the Window and can see windows and views). The relations (not surprising in Rust) are entirely one-way.

At the application-level, you get a window id when you create a window, so the user application knows the window ID being used - and the view can find its root view in the window inside the floem crate - it can associate a view with a window id, and, if there was a way to call a force_native_paint(WindowId) function, trigger that paint - but that would be a dreadful API, since the application author would have to pass window ids down through every view that might possibly need to force a repaint, know they need to force one, and imperatively do so when needed - and the whole point here is that request_repaint() should work as advertised and simply cause a repaint no matter what.

Altering ViewId lets me detect when the repaint request is happening on a view in an inactive window accurately - that solves half the problem:

    fn is_in_active_window(&self) -> bool {
        if let Some(root) = self.root() {
            return crate::window_handle::get_current_view() == root;
        }
        false
    }

    fn schedule_native_paint_event(&self) {
        println!("Need native paint for {:?} root {:?}", self, self.root());
    }

    pub fn request_paint(&self) {
        let active = self.is_in_active_window();
        self.add_update_message(UpdateMessage::RequestPaint);
        if !active {
            self.schedule_native_paint_event();
        }
    }

but what's needed to implement schedule_native_paint_event() to actually do something requires:

• A way to get the WindowId for a view that is not in the active window
• A way to get the WindowHandle for that id
• Some way that does not go through the message queues which will not be processing messages to tell the window handle to take the native winit window handle and post whatever message the OS requires to send a native repaint event to that window

It's lovely how de-coupled Views are from windows - until you have a situation like this where there is no alternative to calls flowing up, not down the tree of lovely decoupled things.

from floem.

Best I can think of is some similar registry of root-view:window-id and window-id:app-handle-id (app-handle-id is not a thing at present) and a background thread that sits quietly waiting on a channel for messages saying force a repaint to this native window id and dispatches them - which could probably do some sort of OnceCell lazy init thing so applications that don't ever post to that queue don't incur any overhead?

Am I missing something here?

from floem.

One other note: There is also the issue of knowing the window's location on screen when instantiating other windows - which also is likely to need to be addressed by a view accessing its containing window (though I'll note on Mac OS you can only query window size on the main thread, so some caching is needed) - I'm not sure the plumbing has this information initially either.

from floem.

I'm thinking of create_window took Into it would make it easy to provide convenience types to compute a screen position relative to a view (or whatever), and at long as they implement that too, it would be transparent and source-compatible for old code.

from floem.

Updated the patch to add a ScreenLayout struct, obtainable from a ViewId, which contains a snapshot of the window bounds and other information necessary to convert from view locations to screen locations and back (it takes several calls to Window that return Result to obtain all of the necessary information to do these conversions, so it makes more sense, to me, to take care of that and offer the user something which is "batteries included").

from floem.

I'm thinking maybe we should move process_update() to ApplicationHandle. What happened here is a event in one Window triggered updates in both windows. So the ApplicationHandle should process all update messages in CENTRAL_UPDATE_MESSAGES after dispatching the event.

from floem.

Makes sense.

Seems like we might want to do something about AppState - it contains some things that are logically global and some things that are local to a window.

from floem.

@dzhou121 - Take a look at this pr - it fixes the problem with very minimal changes.

I think in the bigger picture, you are right about process_update() belonging to AppHandle not WindowHandle, but looking at the code I asked myself if the code moves, does it change what windows get to process events? and the answer was that no matter what, we need to iterate all the windows and give them a chance to process updates.

I do think the code in ApplicationHandle and WindowHandle needs some untangling of what really belongs to windows and what belongs to the application, but we may want to solve this one thing now and boil the ocean later :-)

I would keep the other PR for this issue open and use it as a base for some improved window-management features, since it implements a few already.

from floem.

Fixed in 6fe51db.

from floem.