Skip to content

Image should not take bc.max() when only width bounded. #1184

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

Closed
JAicewizard opened this issue Sep 5, 2020 · 4 comments · Fixed by #1189
Closed

Image should not take bc.max() when only width bounded. #1184

JAicewizard opened this issue Sep 5, 2020 · 4 comments · Fixed by #1189
Labels
layout An issue with widget layout widget concerns a particular widget

Comments

@JAicewizard
Copy link
Contributor

Currently the Image widget takes bc.max
This means what when you want to have images in a scroll-able horizontal list, the height of the images is +inf .
This means that the images are not displayed at all.
When the image is width bounded the image should take into consideration the width/height of the image itself and calculate the correct height from that.

The same should probably be done to height constrained image widgets.

@JAicewizard
Copy link
Contributor Author

I could probably implement this, its not that hard.
If this is accepted I will start working on it.

@cmyr cmyr added layout An issue with widget layout widget concerns a particular widget labels Sep 6, 2020
@cmyr
Copy link
Member

cmyr commented Sep 6, 2020

Without looking at it hard I 100% believe you're correct, bc.max() is rarely the correct constraint.

@JAicewizard
Copy link
Contributor Author

I redid the calculations for the size, I tried making a test for this but the problem is that I need to set the size to +inf.
But the harness makes a creates an bitmap with the given size. Since we cannot make a bitmap with infinite size this obviously fails.
A possible solution would be to only create the bitmap after the layout has been done.

I will test this in an actual application, if that works I will create a PR.

I tried looking into the harness code, but it seems to be tightly integrated with the root window, so maybe that is left for someone who knows more about this.

@JAicewizard
Copy link
Contributor Author

I managed to get the test working using just scroll as the root widget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
layout An issue with widget layout widget concerns a particular widget
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants