api: convert `Window` to `dyn Window` #3857

kchibisov · 2024-08-12T16:14:05Z

--

The only issue right now is how to deal with maybe_queue_on_main and maybe_block_on_main. From what I can say the backends should do that themselves for the time being, since it's a temporary peace unless we remove the Send and Sync from the window.

I've only did the Wayland backend and examples/window.rs to show how it may look.

I'd assume @daxpedda will do for MonitorHandle (trait is not really needed since data is used) and after that it should be pretty much ready to split backends.

The only issue I see is that we can not use generics, since they prevent object safety.

The Drop impl we had should also be done for backends, since it's a backend specific behavior and clearly not needed on most of the backends. Though, I've kept it to simple not forget about it.

daxpedda · 2024-08-13T08:33:51Z

I was briefly considering if we should wrap Box<Window> into its own type so it "looks better", but I'm not sure to what end because we do actually want to expose Box to the user because of e.g. converting to an Arc. After we remove Send + Sync it might become more viable.

The Drop impl we had should also be done for backends, since it's a backend specific behavior and clearly not needed on most of the backends. Though, I've kept it to simple not forget about it.

AFAIK you can't implement Drop on trait objects anyway.

daxpedda

I know the PR isn't ready but I went ahead and implemented Web anyway just to see how everything would work out.
Its a shame about the loss of generics, but again could be resolved by adding a Window type wrapper in the future?

LGTM!

madsmtm

I'm not really in the loop, is the plan to keep the maybe_queue_on_main/maybe_wait_on_main and Send + Sync, or are we getting rid of that?

Because if we're getting rid of it, I'd prefer to do that first, to keep the platform-specific diff smaller (otherwise we'll end up with one commit moving them to web/appkit/uikit, and one commit removing them again, which is a lot of indentation changes).

daxpedda · 2024-08-13T20:27:39Z

I'm not really in the loop, is the plan to keep the maybe_queue_on_main/maybe_wait_on_main and Send + Sync, or are we getting rid of that?

The plan was to expose queue_on_main/wait_on_main on a new type called WindowHandle, which would be Send + Sync and remove Send + Sync on Window.
But the plan was also to do this later, for now Window will remain Send + Sync.

kchibisov · 2024-08-13T22:04:58Z

Because if we're getting rid of it, I'd prefer to do that first, to keep the platform-specific diff smaller (otherwise we'll end up with one commit moving them to web/appkit/uikit, and one commit removing them again, which is a lot of indentation changes).

I think you can keep the diff small, but I can not make the window !Send + !Sync at the current state of things. So, a temporary situation.

kchibisov · 2024-08-22T21:44:46Z

I have no idea whether what I did to AnyThread on windows still works as it was intended, @notgull , so would be nice if you can take a look.

kchibisov · 2024-08-22T21:45:04Z

The PR should be ready and every backend is implemented.

madsmtm

Still a little unsure about the API changes, but let's discuss it synchronously

madsmtm · 2024-08-23T14:01:24Z

examples/child_window.rs

                    let parent_window = self.windows.get(&self.parent_window_id.unwrap()).unwrap();
-                    let child_window = spawn_child_window(parent_window, event_loop);
+                    let child_window = spawn_child_window(parent_window.as_ref(), event_loop);


Nit: Use &parent_window to auto-deref the value instead (I think that works, at least)

Doesn't seem to work.

How about &*parent_window?

Nah, I guess you have to do exactly the same as we do with the ApplicationHandler and all the generic impls over there, I don't mind them, but I'll leave it to follow-up, since there's a chance that none of these as_ref will be needed once we figure out how we actually store the window for the end user.

madsmtm · 2024-08-23T14:04:01Z

src/window.rs

    /// Create a new [`WindowAttributes`] which allows modifying the window's attributes before
    /// creation.
-    #[inline]
-    pub fn default_attributes() -> WindowAttributes {
+    fn default_attributes() -> WindowAttributes


This should probably be:

impl dyn Window { pub fn default_attributes() -> ... } // Usage: <dyn Window>::default_attributes()

Or just removed entirely

I've updated.

madsmtm · 2024-08-23T14:40:35Z

src/window.rs

-
-        self.window.maybe_queue_on_main(move |w| w.set_outer_position(position))
-    }
+    fn set_outer_position(&self, position: Position);


Really quite unfortunate that we loose the ability to have impl Into<...> parameters.

I feel like there's ways that we could probably make it work, my initial attempt is here, I'll also link the explanation for erased-serde, there might be some useful information in there.

I think I'll leave it if it really become a problem, but nice to remember.

This should allow us to make future split of backends much easier. The `Box<dyn Window>` is a _temporary_ solution, which will be removed with the future updates when we decide on how the Window should be stored.

kchibisov requested review from daxpedda, madsmtm and notgull August 12, 2024 16:14

daxpedda reviewed Aug 13, 2024

View reviewed changes

madsmtm requested changes Aug 13, 2024

View reviewed changes

kchibisov force-pushed the kchibisov/dyn-window branch 6 times, most recently from d568280 to 5103e3b Compare August 22, 2024 21:28

kchibisov marked this pull request as ready for review August 22, 2024 21:44

kchibisov requested review from jackpot51 and MarijnS95 as code owners August 22, 2024 21:44

kchibisov requested a review from madsmtm August 23, 2024 11:18

kchibisov added the C - nominated Nominated for discussion in the next meeting label Aug 23, 2024

madsmtm approved these changes Aug 23, 2024

View reviewed changes

kchibisov force-pushed the kchibisov/dyn-window branch 2 times, most recently from 97f4c74 to 1644c06 Compare August 23, 2024 17:42

kchibisov removed the C - nominated Nominated for discussion in the next meeting label Aug 23, 2024

kchibisov force-pushed the kchibisov/dyn-window branch from 1644c06 to 6f3d1f8 Compare August 23, 2024 17:44

api: convert Window to dyn Window

c11e088

This should allow us to make future split of backends much easier. The `Box<dyn Window>` is a _temporary_ solution, which will be removed with the future updates when we decide on how the Window should be stored.

kchibisov force-pushed the kchibisov/dyn-window branch from 6f3d1f8 to c11e088 Compare August 23, 2024 17:53

kchibisov merged commit 241b7a8 into master Aug 23, 2024
58 checks passed

kchibisov deleted the kchibisov/dyn-window branch August 23, 2024 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: convert `Window` to `dyn Window` #3857

api: convert `Window` to `dyn Window` #3857

kchibisov commented Aug 12, 2024

daxpedda commented Aug 13, 2024

daxpedda left a comment

madsmtm left a comment

daxpedda commented Aug 13, 2024

kchibisov commented Aug 13, 2024

kchibisov commented Aug 22, 2024

kchibisov commented Aug 22, 2024

madsmtm left a comment

madsmtm Aug 23, 2024

kchibisov Aug 23, 2024

madsmtm Aug 23, 2024

kchibisov Aug 23, 2024

madsmtm Aug 23, 2024

kchibisov Aug 23, 2024

madsmtm Aug 23, 2024

kchibisov Aug 23, 2024

api: convert Window to dyn Window #3857

api: convert Window to dyn Window #3857

Conversation

kchibisov commented Aug 12, 2024

daxpedda commented Aug 13, 2024

daxpedda left a comment

Choose a reason for hiding this comment

madsmtm left a comment

Choose a reason for hiding this comment

daxpedda commented Aug 13, 2024

kchibisov commented Aug 13, 2024

kchibisov commented Aug 22, 2024

kchibisov commented Aug 22, 2024

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

api: convert `Window` to `dyn Window` #3857

api: convert `Window` to `dyn Window` #3857