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

fix layout of image when either width or height is bounded, but not the other #1189

Merged
merged 23 commits into from
Oct 1, 2020

Conversation

JAicewizard
Copy link
Contributor

This PR fixes #1184 with a more considerate layout function never taking +infinite in size.

Copy link
Collaborator

@ForLoveOfCats ForLoveOfCats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not qualified to review the unit test but the logic seems sound 👍

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
druid/src/widget/image.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
druid/src/widget/image.rs Outdated Show resolved Hide resolved
@ForLoveOfCats
Copy link
Collaborator

cc @cmyr I think you're the master of the rendering test system, can you take a look at the unit test here?

@luleyleo luleyleo added the S-waiting-on-author waits for changes from the submitter label Sep 10, 2020
@ForLoveOfCats ForLoveOfCats added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Sep 12, 2020
Copy link
Collaborator

@jneem jneem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the pixel interpolation and scaling has already been tested, probably it would be enough to just test the image's layout rect? You can look at the layout tests for an example.

druid/src/widget/image.rs Outdated Show resolved Hide resolved
druid/src/widget/image.rs Outdated Show resolved Hide resolved
druid/src/widget/image.rs Outdated Show resolved Hide resolved
druid/src/widget/image.rs Outdated Show resolved Hide resolved
@jneem jneem added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Sep 14, 2020
@JAicewizard
Copy link
Contributor Author

I made the tests test just the layout, it works well. I tested that the old implementation would indeed fail.

@JAicewizard
Copy link
Contributor Author

@jneem can you remove the waiting-on-author label? I think I addressed all the issues raised.

@jneem
Copy link
Collaborator

jneem commented Sep 28, 2020

Right, sorry for letting this sit. There's a conflict in the CHANGELOG now, but otherwise it looks good to go.

@jneem jneem removed the S-waiting-on-author waits for changes from the submitter label Sep 28, 2020
@jneem jneem dismissed ForLoveOfCats’s stale review September 28, 2020 14:36

I think the changes were made, right?

Copy link
Collaborator

@ForLoveOfCats ForLoveOfCats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeppers! My nits have been addressed :)

@JAicewizard
Copy link
Contributor Author

This took a bit longer than expected, I fixed some bugs with the renaming and behavior changes around the scrollbar and image buffer.
The mechanisms are the same, and I guess we have now already seen the tests actually work in practice :)
github seems to have broken again so I cant tag anyone.
Would be nice to get this merged to be done with it.

CHANGELOG.md Outdated Show resolved Hide resolved
@jneem
Copy link
Collaborator

jneem commented Sep 30, 2020

Thanks for your patience on this! I just see a merge conflict on the changelog, but otherwise I'm ready to merge.

@JAicewizard
Copy link
Contributor Author

oops! I should find a way to make vscode not auto-format .MD files, or highlight merge conflicts in vim.
I'm off to bed but I think its safe to say this wont trip one of the tests (fingers crossed).

It was very nice to work on this, I might try to fix more small things once I have time.

@ForLoveOfCats
Copy link
Collaborator

At some point the number for the PR in the changelog link became incorrect. Later tonight I'm going to take the liberty of fixing that then merge this. This message mostly serves as a note to any other collaborators that this change needs to be made real quick before merging

Copy link
Collaborator

@ForLoveOfCats ForLoveOfCats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well nevermind lol, the allow edits box was unchecked. Very very minor change here and then we can merge I promise

CHANGELOG.md Outdated Show resolved Hide resolved
@jneem jneem merged commit b38e873 into linebender:master Oct 1, 2020
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.

Image should not take bc.max() when only width bounded.
4 participants