-
Notifications
You must be signed in to change notification settings - Fork 566
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
change Image example #1447
change Image example #1447
Conversation
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.
Cool, I think showing more off here makes sense. If we wanted to get fancy we could also do something here like we do in examples/flex.rs
, where we actually let the user choose the FillStrat
and InterpolationMode
from some radio buttons, and then rebuild the widget in response to that?
I reworked it to be more like the flex, that was a great idea. You can now play around with width/height interpolation mode and fillstrat dynamically |
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.
Cool, I think this is a great improvement.
Playing around with it: the default settings might be confusing, since changing many of the options doesn't actually change anything.
I think we can maybe make it a bit clearer? Here's what I would probably do:
- remove the 'debug layout' option, and just always paint the border of the image widget, which should make it easier to see what's going on.
- I would check 'fix width' and 'fix height' by default, and give 'height' and 'width' some non-zero starting values; maybe something like 200 & 320. (making them non-uniform will make it more obvious what's happening with the various fill strategies.
Other than that, the code looks good, just a couple of notes in-line.
Thanks!
druid/examples/image.rs
Outdated
InterpolationModeData(mode) | ||
} | ||
} | ||
impl druid::Data for InterpolationModeData { |
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.
we should be able to derive this, like:
#[derive(Clone, Data)]
struct InterpolationModeData(
#[data(same_fn="PartialEq::eq")]
InterpolationMode
);
I would also be okay with us adding a Data
impl to InterpolationMode
, I did this for the various types in examples/flex.rs
for this same reason.
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.
yes that would be great! I think it makes sense to have InterpolationMode
in data for some things, its not something we want to prevent people from putting in there.
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.
Did we make this change? If we did we can also probably get rid of this InterpolationModeData
type, and just use InterpolationMode
directly?
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.
InterpolationMode
does not have the data trait implemented for it, yet. It would be nice if that would be possible to do.
I can create a PR for it later?
EDIT: I can also put it in this PR, its not a big change and then I don't have to rebase.
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.
@JAicewizard yes feel free to add it to this PR. Also if merge in the latest master it should fix that problem with CI failing.
had to rebase off master but forgot to pull this first, there was a commit I hadn't pulled apparently |
the failing ones are getrandom not being compatible with wasm(?) |
that getrandom error is strange, since we don't see it on master and I don't think anything should have changed?? |
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.
Okay one fixup and one question about just using InterpolationMode
, but other than that (and assuming we can figure out why CI is failing) this looks good. Thanks!
Co-authored-by: Colin Rofls <[email protected]>
Co-authored-by: Colin Rofls <[email protected]>
7245cec
to
fd6bb38
Compare
Thanks! |
This changes the image example to showcase FillStrat and Interpolation mode.