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

Minor refactor of refresh code in base widgets #5383

Closed
wants to merge 1 commit into from

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Jan 9, 2025

Description:

Removing some code duplication for how methods were refreshing the widget. I also reordered unexported methods to be in the correct order. This does move .Hide() and .Show() away from doing a canvas.Refresh() which I think is both more correct and a performance win, right?

Checklist:

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

@coveralls
Copy link

Coverage Status

coverage: 59.353% (+0.04%) from 59.316%
when pulling 7114445 on Jacalz:minor-base-widget-cleanup
into e6225b5 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.

This confuses Widget.Refresh() and canvas.Refresh(Widget).
This means that when hiding a BaseWidget it will now refresh the tree of widgets which it did not do before, as it just told the driver that those items should be removed from cache.

@Jacalz Jacalz requested a review from andydotxyz January 11, 2025 21:00
@Jacalz
Copy link
Member Author

Jacalz commented Jan 11, 2025

So it should only refresh on show if I understand you correctly? Or not refresh at all for show/hide?

@Jacalz
Copy link
Member Author

Jacalz commented Jan 11, 2025

I'm really confused about the difference between canvas.Refresh() for a widget and refresh on that widget directly.

@andydotxyz
Copy link
Member

Calling Widget.Refresh cascades into the widget renderer and probably all of its children to redraw each canvas object.
canvas.Refresh(obj) just tells the driver that the item should be purged from the cache - so if it is then hidden no further code will be executed until it is shown again.

@Jacalz
Copy link
Member Author

Jacalz commented Jan 13, 2025

Ah, alright. I'll close this and do the refactoring in some future work after #5388 has been merged

@Jacalz Jacalz closed this Jan 13, 2025
@Jacalz Jacalz deleted the minor-base-widget-cleanup branch January 13, 2025 13:16
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