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

Fixing semantics of test canvas resize #5554

Merged
merged 4 commits into from
Feb 28, 2025
Merged

Conversation

andydotxyz
Copy link
Member

Fixes #5548

Checklist:

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

@@ -766,6 +766,9 @@ func (r *treeContentRenderer) Objects() []fyne.CanvasObject {

func (r *treeContentRenderer) Refresh() {
r.refreshForID(r.treeContent.nextRefreshID)
for _, s := range r.separators {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this needed? I think this will lead to RefreshItem, and maybe scrolling, refreshing all separators always, which shouldn't be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without it the resize after changing theme was picking up bad separators.

It seemed like a logical change because the Refresh of the content was being called and the items were being refreshed - so shouldn't the separators?

Copy link
Member Author

Choose a reason for hiding this comment

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

(You can see that List has essentially the same - when the content Refresh() is called we loop through the separators and refresh them)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - may be something that can be optimized later since it should only be needed for a full Refresh, and not RefreshItem, etc which will still go through this path. But as you point out it's the same as List for now

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'm not too worried as a separator refresh is so minimal :)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should open an issue to make sure we don't forget to do that optimization in the future

@andydotxyz
Copy link
Member Author

This change uncovered a bug in both tree and list refreshing (theme change then resize) which I think I have fixed appropriately...

@coveralls
Copy link

coveralls commented Feb 25, 2025

Coverage Status

coverage: 62.374% (+0.008%) from 62.366%
when pulling bbd8a96 on andydotxyz:fix/5548
into ea99f29 on fyne-io:develop.

@andydotxyz andydotxyz merged commit 8828e52 into fyne-io:develop Feb 28, 2025
11 checks passed
@andydotxyz andydotxyz deleted the fix/5548 branch February 28, 2025 19:15
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.

4 participants