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

Added convenience functions for window::icon::Icon #1174

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

daladim
Copy link
Contributor

@daladim daladim commented Dec 28, 2021

See #1170

Since the image dependency was already an (optional) dependency of iced, why not provide these convenience functions that save a few lines of code in applications that need an icon

@13r0ck
Copy link
Member

13r0ck commented Dec 29, 2021

Thank you for the PR!

Could you make it so that your convenience function is only available if the iced image feature is enabled?

The image crate is quite large, and iced tries to give users the option for simplicity, but tries to be as light as it can be by default. This is the reason that image isn't already included by default.

@daladim
Copy link
Contributor Author

daladim commented Dec 29, 2021

Well, I had already gated this behind an image_rs feature I had just added, for the very reasons you described.
I've changed this PR so that this optional feature is now tied to the already-existing image feature.

Note that this pulls all the default features (= supported file formats) of image, there's no format-by-format opt-in as some other sub-crates of iced do.

I'm afraid I cannot remove the image_rs feature and keep only the image feature, since image is both the name of a crate and a feature. To do so, we would need:

@daladim
Copy link
Contributor Author

daladim commented Dec 29, 2021

Also, I'm not sure how docs.rs generates its doc. Will these functions be shown in https://docs.rs/iced/latest/iced/?

iced's Cargo.toml already contains

[package.metadata.docs.rs]
rustdoc-args = ["--cfg", "docsrs"]
features = ["image", "svg", "canvas"]

...but I'm not sure how this will render the functions that are gated behind the image_rs feature instead of the image feature

@daladim
Copy link
Contributor Author

daladim commented Jan 19, 2022

I did not manage to build the doc locally though the docs.rs project to test the image_rs-gated functions are visible, but since this feature is implied by the image one, it should be fine.

I think this MR is ready to be merged. What's your opinion on this?
Thanks

@hecrj hecrj force-pushed the master branch 2 times, most recently from e6ab610 to 8b0f2e6 Compare January 19, 2022 15:04
@daladim
Copy link
Contributor Author

daladim commented Mar 1, 2022

I'm finally having a green CI on this PR.

Is this PR OK for you? Or do you need anything else from me before merging it?

Thanks

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 great! Thanks 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants