Skip to content

Improve window setting#94

Merged
hecrj merged 8 commits into
iced-rs:masterfrom
hatoo:improve-setting
Dec 3, 2019
Merged

Improve window setting#94
hecrj merged 8 commits into
iced-rs:masterfrom
hatoo:improve-setting

Conversation

@hatoo
Copy link
Copy Markdown
Contributor

@hatoo hatoo commented Nov 30, 2019

Hi, I'm interested in running iced in vst-rs environment.

For this, there are 2 things to do.

  1. (Windows only) Initialize a Window with the given parent window and with other settings.
  2. Run event loop discontinuously. The current run method will not return until close. This behavior is not undesirable.

This PR handles 1.
To create a window in VST, we must initialize winit like below.

let wb = winit::window::WindowBuilder::new()
            .with_title("A fantastic window!")
            .with_decorations(false)
            .with_resizable(false)
            .with_parent_window(parent)
            .with_inner_size(LogicalSize {
                width: 400.0,
                height: 300.0,
            });

To do the same thing in iced, we need support setting options with_decorations and with_parent_window.
This is what is the PR does.

For with_decorations, it's just straightforward to implement because it is supported in all platforms in winit.
For with_parent_window it's not straightforward since with_parent_window is Windows-specific API.
So I introduced a simple platform-specific module to implement.

Copy link
Copy Markdown
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing!

I think we can leave the API of the iced crate completely unaffected by this, at least for now.

For the second case you mention, the use case in #50 is definitely related. We should try to come up with a solution for both. I believe there should be a way to split iced_winit functionality for advanced needs on native platforms, while keeping iced completely unaffected.

I'd also like to remind you (and others looking to contribute) that I'd love to know about your use cases before we settle on a specific design! As the contributing guide states, we have a Zulip server and you can also find me on Discord.

Comment thread src/settings/mod.rs Outdated
#[path = "not_windows.rs"]
pub mod platform;

pub use platform::PlatformSpecific;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would only expose PlatformSpecific settings in iced_winit for the time being.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.
I made iced's platform-specific settings use iced_winit's PlatformSettings.

@hecrj hecrj added the feature New feature or request label Nov 30, 2019
@hecrj hecrj added this to the 0.1.0 milestone Nov 30, 2019
Make Window.platform_specific use iced_winit::settings::PlatformSpecific
@hatoo hatoo requested a review from hecrj December 1, 2019 06:10
Copy link
Copy Markdown
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the feedback! 🎉

I think there is still a detail left to fix, then we can merge this!

Comment thread Cargo.toml Outdated
iced_web = { version = "0.1.0", path = "web" }

[target.'cfg(target_os = "windows")'.dependencies.winapi]
version = "0.3.6"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not be necessary now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I forgot to remove it.
I've removed now.

Comment thread src/settings.rs Outdated
pub decorations: bool,

/// Platform specific Setting.
pub platform_specific: iced_winit::settings::PlatformSpecific,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to simply not expose this for now and make advanced users use iced_winit directly instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense!
I've removed extended settings from iced's setting.

Comment thread src/settings.rs Outdated
size: settings.window.size,
resizable: settings.window.resizable,
decorations: settings.window.decorations,
platform_specific: settings.window.platform_specific,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'll simply use iced_winit::settings::PlatformSpecific::default() here.

@hatoo hatoo requested a review from hecrj December 2, 2019 13:47
Copy link
Copy Markdown
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my feedback!

I have changed a couple of details. Let me know if it's okay and we can merge this!

@hatoo
Copy link
Copy Markdown
Contributor Author

hatoo commented Dec 3, 2019

Thanks for your work!
It's OK!
Please merge this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants