Conversation
cmyr
left a comment
There was a problem hiding this comment.
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 |
cmyr
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
@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?? |
cmyr
left a comment
There was a problem hiding this comment.
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 <colin@cmyr.net>
Co-authored-by: Colin Rofls <colin@cmyr.net>
7245cec to
fd6bb38
Compare
|
Thanks! |
This changes the image example to showcase FillStrat and Interpolation mode.