Fix #828: tab container update handling #998
Conversation
This prepares the move of the tests to the widget_test package which is necessary to use image based tests. The tests that have a misleading name due to this change will be adjusted in a later commit.
The refactoring of the tab container tests to reside in the widget_test package and to use image based tests if necessary (no access to renderer) revealed fyne-io#828. The refactoring of the tab container update handling fixed the issues.
| return false | ||
| } | ||
| for i, item := range r.container.Items { | ||
| if item.Content != r.objects[i] { |
There was a problem hiding this comment.
What about if the tab text or icon changes?
There was a problem hiding this comment.
Okay, I’m not sure if this really is a use case but it can be done, so I’ll add a test for this.
| } | ||
| var buttons, objects []fyne.CanvasObject | ||
| for i, item := range r.container.Items { | ||
| button := r.buildButton(item, iconPos) |
There was a problem hiding this comment.
Is it not worth re-using the existing buttons, if there are enough?
There was a problem hiding this comment.
I don’t think so.
- A tab container usually doesn’t have more than a couple of tabs.
- Modifying the tab list probably doesn’t happen that often.
- Checking if an item is reusable introduces a complexity with limited use (see 1. and 2.)
@stuartmscott you didn’t reviewed all commits at once, didn’t you? |
|
@stuartmscott @andydotxyz I merged by hand and that dismissed the approvals, sorry. |
|
Notable changes due to the merge:
|
Description:
This PR refactors the tab container widget’s update handling and fixes issues when adding/removing items to an existing tab container.
It also removes the tab container widget’s dependency from the renderer cache.
As a side-effect it introduces
TabContainer.SetItemswhich might be used to replace the tab items completely.Fixes #828
Checklist:
Where applicable: