Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for setting window icon #285

Merged
merged 2 commits into from
Jul 1, 2020

Conversation

frapa
Copy link
Contributor

@frapa frapa commented Apr 11, 2020

This PR is just to start the discussion around this topic, the code is currently a hack, the naming is not final and I just added it somewhere, not even in the right place. But since this is my first contribution to the library and my experience with Rust is not extensive, I would like if somebody more experienced can have a look an tell me if I'm on the right track.

This adds a new property from Settings::window::icon and an Icon struct which can be converted to winit::window::Icon.

It also adds code to display this icon in Application::run.

Due to the fact that the Icon struct cannot be copied, I had to remove the Copy trait from all Settings,
both in iced and iced_winit. This is an API breaking change.

Currently, I'm able to set an icon with the code:
image

Thanks to winit it gets correctly displayed also on the taskbar:
image

And of course in the window switcher (alt+tab).

Fixes #433.

@hecrj hecrj added the feature New feature or request label Apr 13, 2020
@hecrj hecrj added this to the 0.2.0 milestone Apr 13, 2020
Copy link
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.

Looks like a good start!

We should probably add this to iced_winit only for the time being. We can figure out how to reconcile it with iced_web later.

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Icon {
///
rgba: Box<[u8]>,
Copy link
Member

Choose a reason for hiding this comment

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

I would use a Vec<u8> here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@frapa frapa force-pushed the add_support_for_window_icons branch from c21d298 to b9fdfb8 Compare June 29, 2020 19:35
@frapa
Copy link
Contributor Author

frapa commented Jun 29, 2020

@hecrj Do you mean that no code should be added to the main iced package? As far as I can see, the web package is very experimental, but I could try implementing the favicon there, too. I don't really get the build process, tough...

This adds a new property from Settings::window::iconand a Icon struct which can be converted to winit::window::Icon.

It also adds code to display this icon in Application::run. Due to the fact that the Icon struct is non copyable, I also had to remove the Copy trait from all Settings, both in `iced` and `iced_winit`.
@hecrj hecrj force-pushed the add_support_for_window_icons branch from b9fdfb8 to 9a037a2 Compare July 1, 2020 03:38
@hecrj hecrj changed the title WIP: Add support for setting window icon Add support for setting window icon Jul 1, 2020
Copy link
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.

I have moved the Icon type to the iced crate to properly hide the iced_winit dependency and avoid breaking the Wasm target.

Additionally, I have changed Icon::from_rgba to return a proper error instead. We avoid panicking in a From trait this way.

I believe this is ready to be merged. Let me know what you think!

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.

Window icon
2 participants