-
Notifications
You must be signed in to change notification settings - Fork 12
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 with_alpha_factor
to BrushRef
/Brush
#40
Conversation
Still using the old format - will follow-up with new standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, like seeing the #[must_use]
attributes.
/// Returns the brush with the alpha component multiplied by the specified | ||
/// factor. | ||
#[must_use] | ||
pub fn with_alpha_factor(&self, alpha: f32) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one should take self
by value and perform the modifications in place. This lets the user decide if they want to clone the full brush.
/// Returns the brush with the alpha component multiplied by the specified | ||
/// factor. | ||
#[must_use] | ||
pub fn with_alpha_factor(&self, alpha: f32) -> Brush { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make the change suggested above, this one might not be needed. I think it’s probably fine to call to_owned().with_alpha_factor()
on a BrushRef
.
Looks like I was late to the party. I was curious why it wouldn’t let me click approve in the review. |
Follow-up from #40
Fixes #27
The key feature is adding
alpha
toImage
. There were also a couple of other papercuts which I've tweaked