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

change Image example #1447

Merged
merged 14 commits into from
Jan 13, 2021
Merged

change Image example #1447

merged 14 commits into from
Jan 13, 2021

Conversation

JAicewizard
Copy link
Contributor

This changes the image example to showcase FillStrat and Interpolation mode.

Copy link
Member

@cmyr cmyr left a 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?

druid/examples/image.rs Outdated Show resolved Hide resolved
druid/examples/image.rs Show resolved Hide resolved
druid/examples/image.rs Outdated Show resolved Hide resolved
@cmyr cmyr added the S-waiting-on-author waits for changes from the submitter label Dec 13, 2020
@JAicewizard
Copy link
Contributor Author

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 cmyr added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jan 4, 2021
Copy link
Member

@cmyr cmyr left a 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 Show resolved Hide resolved
druid/examples/image.rs Outdated Show resolved Hide resolved
druid/examples/image.rs Outdated Show resolved Hide resolved
InterpolationModeData(mode)
}
}
impl druid::Data for InterpolationModeData {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

@JAicewizard JAicewizard Jan 13, 2021

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.

Copy link
Member

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.

druid/examples/image.rs Outdated Show resolved Hide resolved
druid/examples/image.rs Outdated Show resolved Hide resolved
@cmyr cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jan 4, 2021
@JAicewizard
Copy link
Contributor Author

had to rebase off master but forgot to pull this first, there was a commit I hadn't pulled apparently

@JAicewizard
Copy link
Contributor Author

the failing ones are getrandom not being compatible with wasm(?)

@cmyr
Copy link
Member

cmyr commented Jan 12, 2021

that getrandom error is strange, since we don't see it on master and I don't think anything should have changed??

Copy link
Member

@cmyr cmyr left a 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!

druid/examples/image.rs Show resolved Hide resolved
@cmyr cmyr merged commit 0d97246 into linebender:master Jan 13, 2021
@cmyr
Copy link
Member

cmyr commented Jan 13, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author waits for changes from the submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants