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

widget: Move some minsize calculations to the renderer #4682

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Feb 27, 2024

Description:

Part of #4681 ripped out into a separate PR.
Moving calculations of the MinSize into the renderer allows us to clean up some of the code
because BaseWidget.MinSize() will create the renderer if it does not exists and as such do any setup necessary.

Checklist:

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

@coveralls
Copy link

coveralls commented Feb 27, 2024

Coverage Status

coverage: 64.792% (+0.009%) from 64.783%
when pulling 36c10da on Jacalz:minsize-into-renderer
into 6b7246b on fyne-io: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.

For Separator and Activity we don't need a renderer to know our minsize - those shortcuts were added as optimisations... so should they really be removed?

@Jacalz
Copy link
Member Author

Jacalz commented Feb 27, 2024

Hmm. Good point. The idea was that if we move all our MinSize logic to the renderer, we can add caching to the base widget more easily and we have better separation of the code paths by keeping size logic in one place. What do you think?

@Jacalz
Copy link
Member Author

Jacalz commented Feb 27, 2024

There is also less code duplication. Updating the sizing only has to happen in one place and there is no risk of them getting out of sync. If getting the MinSize form the renderer is slow then maybe we ought to optimise that code?

@andydotxyz
Copy link
Member

If getting the MinSize form the renderer is slow then maybe we ought to optimise that code?

It's not slow per-se - but it requires creating a renderer, and when we are calculating how to display something that is not visible its potential overhead. Maybe not an issue in the bigger picture though?

@Jacalz
Copy link
Member Author

Jacalz commented Mar 4, 2024

Hmm. I personally think that the less code duplication and separation of concerns is a bigger win. If we can get the minsize cache working it should be even faster going forward so hard coding renderer details into the widget itself should not be necessary.

@andydotxyz andydotxyz merged commit a92d6e0 into fyne-io:develop Apr 17, 2024
12 checks passed
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.

3 participants