Split Application::run to expose EventLoop and Window.

[Matthias-Fauconneau]

I'm not sure what iced platform abstraction plan is, since winit does it already (including web).
This splits Application::run in new+run to expose EventLoop and Window.
I don't think wrapping platform details makes much sense, so, for iced_winit, I chose to directly expose winit::event_loop::EventLoop and winit::window::Window for now.
If this was stable, that would mean the other iced platform(s) would need to be wrapped to match the exposed winit interface.
On the other hand, that seems a good solution to rely on since the exposed winit interface should be exactly the interface we actually want.

So for now, it is up to the user to be careful in which interface are used if all iced platform are to be supported.
A feature could be used to choose between restricted (wrapped only) and direct (no cross iced platform compatibility guarantees).

Besides EventLoop and Window, iced_winit currently also exposes winit::dpi.
This is currently not needed directly in the iced inteface but allows the user to avoid explicitly directly use winit::dpi, which makes it possible for another iced platform to be source compatible with the user code.

Intended usage :
winit::EventLoop needs to be created on the main thread on some platforms while we might want to Application::run in another thread.
Instead of using Application::new, compatible EventLoop and Window can be provided directly by the user to Application::run.
For example, create a window (Window::new), maybe get display information (current_monitor().size, hidpi_factor) and configure (set_inner_size).

let event_loop = iced::EventLoop::new();
let window = iced::Window::new(&event_loop).unwrap();
let size = window.current_monitor().size();
       window.set_inner_size(iced::dpi::LogicalSize::from_physical(iced::dpi::PhysicalSize{width: size.width/2., height: size.height/2.}, window.hidpi_factor()));
use iced::Application;
UserApplication::new().run(event_loop, window);

https://github.com/Matthias-Fauconneau/notify-server/blob/master/src/main.rs

P.S: I recently started using Rust, so if this all makes no sense at all, please help me out :D

[hecrj]

Thanks for the use case report!

I think we should tackle the use case directly here, instead of exposing internals.

The idea I had in mind was to expose a Settings struct in iced, allowing you to pass the initial window dimensions and additional configuration (like renderer-specific parameters) to Application::run. The window dimensions, in particular, would be ignored by iced_web for the most part.

Is there any reason why this may not work for your use case?

[hecrj]

Oh, I see. You are trying to take control of the event loop.

I don't think this is something that we should allow. There are many guarantees that can only be held when the application is in charge (smooth resizing, state changes, relayouting, etc.).

Do you need to listen to background events and change your app state based on that? This is what event subscriptions (see #29) are meant to be used for! They are still not implemented, but I will get to it soon!

[Matthias-Fauconneau]

Thanks for the feedback. Since it was a breaking change anyway, I took the opportunity to see how other things would need to adapt to maintain a consistent design and those kind of propagated a cascade of edits 😆😅. The downsides of a custom predefined Settings interface : - This is exactly what winit::WindowBuilder does, so might as well use their interface. - in my use case, I want to know the display size before configuration. I only need the event loop myself before configuration to get the display size. If you don't need it, you can just forward the Platform(EventLoop,WindowBuilder) and not be exposed to any internals. Maybe rename EventLoop to something to make it clear that's what it's meant to be used for. I just went the existing winit interface because I do not see anything justifying a custom one. I understand iced_web is a nice to have, but my understanding was that native+winit+wgpu is the main target ? So I feel like having more limited support for iced_web should be fine. I could demonstrate how my changes affect it if you are interested. Implementing Settings to emulate a dummy WindowBuilder preserves multiple iced platform support while not cutting users access to full winit feature set. The run_return and later commits are only here because I don't want to maintain multiple forks (and pull request discussion threads :) while we search for a good solution. In my app both UI and the (DBus) notification server are good candidates to take control of the event loop. It happens to be that with (experimental) rust-notify notificationserver.start I need to return to keep processing messages, but also it makes sense for app (IPC) processing to be the main loop over UI. P.S: I also wanted to change the background color: Added renderer::Style, Application::style, forwarded by winit, used by wgpu::Renderer.

…

On Mon, Nov 11, 2019, 02:01 Héctor Ramón ***@***.***> wrote: Oh, I see. You are trying to take control of the event loop. I don't think this is something that we should allow. There are many guarantees that can only be held when the application is in charge (smooth resizing, state changes, relayouting, etc.). Do you need to listen to background events and change your app state based on that? This is what *event subscriptions* (see #29 <#29>) are meant to be used for! They are still not implemented, but I will get to it soon! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#50>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGGMFTAOVXNDNKEIUQXTP3QTCVIDANCNFSM4JLKDSSA> .

[hecrj]

Thank you for exploring this!

This is exactly what winit::WindowBuilder does, so might as well use their interface.

Exposing internal libraries directly is not recommended unless your library is meant to extend them. iced_winit exposing winit may make sense, but the fact that we are using iced_winit to power iced should be hidden from our users.

For instance, in the future we may want to offer our users the possibility of using SDL instead of winit. That change should not affect end-user code, the same way using iced_web over iced_winit does not affect it either.

If you don't need it, you can just forward the Platform(EventLoop,WindowBuilder) and not be exposed to any internals.

The API is still leaking the internals. When there is a breaking change, we cannot assume users have been using the library in a particular way.

In my app both UI and the (DBus) notification server are good candidates to
take control of the event loop. It happens to be that with (experimental) rust-notify notificationserver.start I need to return to keep processing messages, but also it makes sense for app (IPC) processing to be the main loop over UI.

As I said earlier, the only way to keep iced_winit guarantees is for iced to take control of the event loop.

A notification server or a heavy processing task can be run, while keeping guarantees, by using event subscriptions (#29) and async actions (#28), respectively.

---

Overall, I don't think this is a direction we should be headed in. There are many breaking changes here that don't line up with my current vision.

As an alternative proposal, I think you would have a much better time if you used iced_winit directly instead of iced, leveraging the conversion module to build your own custom event loop.

Then, we could review your use cases and needs, identify shortcomings, and design based on that. Let me know how that sounds!

[hecrj]

This is similar to what I had in mind to configure renderers. The plan, however, is to make each renderer define its own configuration!

[Matthias-Fauconneau]

Exposing internal libraries directly is not recommended unless your library is meant to extend them. iced_winit exposing winit may make sense, but the fact that we are using iced_winit to power iced should be hidden from our users.

I understand. I mean reusing winit::WindowBuilder interface not necessarily the implementation. Indeed for now, I can just fork iced/lib and rely on the switch for mod platform to be path=winit. For instance, in the future we may want to offer our users the possibility

of using SDL instead of winit. That change should not affect end-user code, the same way using iced_web over iced_winit does not affect it either.

The API is still leaking the internals. When there is a breaking change, we

cannot assume users have been using the library in a particular way.

Agreed, access to the specific interface would need to be explicitly enabled by the user, with the understanding that it compromises transparency. Only the common denominator interface compatible with any selected iced platform would be accessible by default. And this might indeed need wrappers for more fine-grained interface forwarding than pub use. This system does not necessarily need to be a feature of iced master but it would be nice to keep it in mind when implementing alternative backends to not unecessarily design incompatible wrapping interfaces which would prevent an API transparent cross compatibility. As I said earlier, the only way to keep iced_winit guarantees is for iced

to take control of the event loop. A notification server or a heavy processing task can be run, while keeping guarantees, by using event subscriptions (#29 <#29>) and async actions (#28 <#28>), respectively.

Symetrically, another component might need exclusive control of the event loop. Generally I feel this is an implementation detail as long as all components can cooperate both ways. The user can choose whose guarantees are more important or which architecture makes more sense. For example, in my case, the server guarantees are more important and also the UI lifetime is shorter, it is only active when there are notifications to be shown (if a display is active at all). Overall, I don't think this is a direction we should be taking. There are

many breaking changes here that don't line up with my current vision.

Of course, though I would like to keep the discussion alive to minimize fork maintenance. I don't know what channel is best for this. Maybe an issue on my fork ?

As an alternative proposal, I think you would have a much better time if you used iced_winit directly instead of iced, leveraging the conversion module to build your own custom event loop.

Well, in the end it seems that's what I'm doing.

Then, we can review your use cases and needs, identify shortcomings, and design based on that. Let me know how that sounds!

Sounds good. Let me know if there is anything I can pack nicely for a PR to reduce my fork maintenance exposure.

…

[Matthias-Fauconneau]

Can you explain what you had in mind? How would you configure each renderer ? Maybe you could write out some stub interfaces :)

…

On Mon, Nov 11, 2019 at 8:39 PM Héctor Ramón ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In native/src/renderer/windowed.rs <#50 (comment)>: > @@ -2,10 +2,24 @@ use crate::MouseCursor; use raw_window_handle::HasRawWindowHandle; +use crate::Color; +#[derive(Debug)] +pub struct Style { + pub window_background : Color /*= Color::WHITE*/, + pub text_color : Color /*= Color::BLACK*/, + pub text_size: f32 /*= 20.0*/, +} This is similar to what I had in mind to configure renderers. The plan, however, is to make each renderer define its own configuration! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#50>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGGMFXSQI25VZJVD7LPQK3QTGYF5ANCNFSM4JLKDSSA> .

[hecrj]

For example, in my case, the server guarantees are more important and also the UI lifetime is shorter, it is only active when there are notifications to be shown (if a display is active at all).

Could you describe your use case from a high-level POV? Why does the server need to control the GUI loop? As I understand, you want to run a server that opens a GUI application for a short amount of time (?).

Well, in the end it seems that's what I'm doing.

Not exactly. You should be able to satisfy your use case without changing any code on this repository and/or maintaining a fork. iced_winit offers a conversion module that can serve you to build your own event loop by using winit directly.

Of course, though I would like to keep the discussion alive to minimize fork maintenance. I don't know what channel is best for this.

It would be great if you could open a new topic in our Zulip server describing your use case! If you want to talk to me personally, you can also message me on Discord (lone_scientist#9554).

[Matthias-Fauconneau]

Just for context, this is only a toy app to learn Rust so it is not too serious. Yes, there is no UI shown most of the time, so right now I'm connecting in the callback and returning ASAP. Proper way would be to send a message that an UI thread. I think I'll do that next if I want to keep toying with this. I do not see any downside in allowing control of the loop though with enough warnings. Anyway the split is necessary for configuration (using 'event_loop' as handle to platform (display size) and WindowBuilder). Also I think it's better to split trait (iced_winit::)Application from its impl .run() and document() in separate files. Configuration without event_loop control would be compatible with my use case (with an UI thread). Most of the other changes are necessary but unrelated, they are probably easier to adapt and closer to your view. I can cherry pick and prepare PRs if you are interested. Though I'm a bit afraid of having multiple PRs pending while there is still churn on master.

…

On Thu, Nov 14, 2019, 06:12 Héctor Ramón ***@***.***> wrote: For example, in my case, the server guarantees are more important and also the UI lifetime is shorter, it is only active when there are notifications to be shown (if a display is active at all). Could you describe your use case from a high-level POV? Why does the server need to control the GUI loop? As I understand, you want to run a server that opens a GUI application for a short amount of time (?). Well, in the end it seems that's what I'm doing. Not exactly. You should be able to satisfy your use case without changing any code on this repository and/or maintaining a fork. iced_winit offers a conversion module that can serve you to build your own event loop by using winit directly. Of course, though I would like to keep the discussion alive to minimize fork maintenance. I don't know what channel is best for this. It would be great if you could open a new topic in our Zulip server <https://iced.zulipchat.com/> describing your use case! If you want to talk to me personally, you can also message me on Discord (lone_scientist#9554). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#50>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGGMFW2GSPKTYFK42WBMQTQTTM2LANCNFSM4JLKDSSA> .

[hecrj]

I think the new integration example can be adapted to satisfy this use case somewhat easily.

Therefore, I am closing this. Please, let me know if you have any questions or hit any issues.