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

Bugs/default canvas scale #3673

Merged
merged 6 commits into from Feb 27, 2023
Merged

Bugs/default canvas scale #3673

merged 6 commits into from Feb 27, 2023

Conversation

Bluebugs
Copy link
Contributor

Description:

This default image.MinSize() to be calculated with a SCALE=1 when using it with software.Render().

Fixes #2285

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@Bluebugs Bluebugs changed the base branch from master to develop February 21, 2023 22:39
@coveralls
Copy link

coveralls commented Feb 21, 2023

Coverage Status

Coverage: 61.623% (+0.0005%) from 61.623% when pulling f0ead08 on bugs/default-canvas-scale into ce3e287 on develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks for the catch, a couple of questions inline


c := container.NewMax(image, bg)

test.AssertImageMatches(t, "image_size.png", Render(c, theme.LightTheme()))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using this deprecated API why not use the test theme? Or default theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did just follow the pattern of the other tests in this file. I don't really mind switching to a different API, but should we do the same for the rest of the file to keep things consistent?

canvas/image.go Outdated
@@ -285,6 +285,24 @@ func (i *Image) calculateMinSize() error {
return nil
}

func (img *Image) scaleSizeForCanvas(width, height int) (fyne.Size, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this be a good candidate for the new scale package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, good idea!

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

This isn't returning a coordinate, it is returning a size. Wouldn't "ToFyneSize" fit better with the naming you set up before?

@Bluebugs
Copy link
Contributor Author

This isn't returning a coordinate, it is returning a size. Wouldn't "ToFyneSize" fit better with the naming you set up before?

Oh, that's a good name. Updated.

andydotxyz
andydotxyz previously approved these changes Feb 24, 2023
@Bluebugs Bluebugs merged commit 4776b46 into develop Feb 27, 2023
@Bluebugs Bluebugs deleted the bugs/default-canvas-scale branch February 27, 2023 21:43
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.

crash-when-original-fill-used-fresh
3 participants