Fix/consistentbasewidget #2264

andydotxyz · 2021-05-26T22:25:33Z

Description:

Move our internal Base widget (that could not be extended) to be consistent with the public BaseWidget.
Now we have just 1 API and the ability to extend both.
It is still 2 different classes due to the cycle issues. One cannot be an alias of the other due to certain unexported APIs.
This will be further tidied before 2.1 by hopefully replacing the ExtendBaseWidget idea - but for now it should unblock some keyboard operation items.

Checklist:

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

This needs to be improved through design updates, but for now this is clearer. It means we can extend internal widgets and not be confused which API is used

toaster · 2021-05-28T05:35:03Z

internal/widget/base.go

+	size     fyne.Size
+	position fyne.Position
+	hidden   bool


FMI: why did you remove the alphabetical order?

A mistake, sorry

toaster · 2021-05-28T05:38:00Z

internal/widget/scroller.go

-	RefreshWidget(s)
+func (s *Scroll) Resize(sz fyne.Size) {
+	s.Base.Resize(sz)
+	s.Refresh()


Shouldn’t we keep the refreshes limited to actual changes of the size (size != s.size)?

Yes, good catch thanks.

toaster · 2021-05-28T05:41:45Z

internal/driver/glfw/menu_bar.go

@@ -199,6 +160,7 @@ func (r *menuBarRenderer) Refresh() {
 	r.Layout(r.b.Size())
 	r.background.FillColor = theme.ButtonColor()
 	r.background.Refresh()
+	r.ShadowingRenderer.RefreshShadow()


FYI: I guess, this is not really related to the base widget change and therefore should have been in a separate commit, right?

I had assumed it was something that I broke in the refactoring. If it was already broken then yes it should be a separate commit.
I don't think it's worth unpicking and force pushing a better version, but I will try to keep an eye out in future commits.
If you disagree say so and I'll rework the history

toaster · 2021-05-28T05:45:39Z

widget/tree.go

@@ -643,6 +643,12 @@ func (n *treeNode) Tapped(*fyne.PointEvent) {
 	n.tree.Select(n.uid)
 }

+func (n *treeNode) Resize(s fyne.Size) {
+	n.BaseWidget.Resize(s)
+	// TODO find out if the indentation changed. If not, and size is the same, then we can skip this as normal


TBH, I think that a caller of Resize should never rely on the refresh. If the indentation gets changed at some point and depends on the refresh of a Resize call with the same size, this is wrong. Instead the change of the indentation should trigger a refresh.

It's not relying on a Refresh - it's the Layout that we care about.
Normally we can ignore Resize if the size has not changed - but in the case of Tree it may be the same size but need to have the content moved when it's reused (because increased indent does not change size).
Maybe there is a better solution, but I cannot think of one right now.

I still don’t fully understand why a Resize of a tree node has to handle indentation change. AFAICS an indentation change is implemented by the node’s update method which should then do the appropriate things.
Also, AFAIK, the BaseWidget's Resize doesn’t do anything if the size didn’t change (apart from heating the world).

Quite right, I managed to track down the root cause. The leaf and branch widgets were having the treeNode widget inside accessed directly, which was part of the issue. The rest was that the partialRefresh called when depth changed was not updating the node layout.

With this in place it should be fixed much more satisfactorily.

toaster

A couple of questions/suggestions as normal comments (not needed for my approval).
And one objection to the menu bar refresh fix.

toaster · 2021-05-28T06:01:43Z

internal/driver/glfw/menu_bar_item.go

+
+	if i.Parent.active {
+		i.Parent.canvas.Focus(i)
+	}
+	i.Refresh()


There is no test for this, therefore I don’t know which bug you’re trying to fix with it.
But I think this is partly wrong. The parent’s toggle already focuses the item on activation and unfocuses it on deactivation.
So, if the menu is active after the toggle, the item should already be focused and the added focus code does nothing.
If the additional Refresh fixes some bug, we should try to add a test case for it and also move it to MenuBar’s toggle(). Currently, the item gets refreshed before it is being focused. Maybe that’s to early?

Apologies it was not a bug fix, I had misread the code in the refactor and you are right it should not have been needed. What has changed is sequence of refresh request events I think, so potentially extra refresh calls were previously made that have been reduced, so some areas are now not refreshing enough. I tracked down that activateChild does not actually refresh the button - only the bar. The bar's refresh does not cascade to the list of items it would seem. I have put in place what feels like a better fix in activateChild, what do you think?

andydotxyz

Thanks for the notes, I hope these comments and the commit help :)

andydotxyz · 2021-06-01T13:02:18Z

internal/driver/glfw/menu_bar.go

@@ -199,6 +160,7 @@ func (r *menuBarRenderer) Refresh() {
 	r.Layout(r.b.Size())
 	r.background.FillColor = theme.ButtonColor()
 	r.background.Refresh()
+	r.ShadowingRenderer.RefreshShadow()


I had assumed it was something that I broke in the refactoring. If it was already broken then yes it should be a separate commit.
I don't think it's worth unpicking and force pushing a better version, but I will try to keep an eye out in future commits.
If you disagree say so and I'll rework the history

andydotxyz · 2021-06-01T13:11:06Z

internal/driver/glfw/menu_bar_item.go

+
+	if i.Parent.active {
+		i.Parent.canvas.Focus(i)
+	}
+	i.Refresh()


Apologies it was not a bug fix, I had misread the code in the refactor and you are right it should not have been needed. What has changed is sequence of refresh request events I think, so potentially extra refresh calls were previously made that have been reduced, so some areas are now not refreshing enough. I tracked down that activateChild does not actually refresh the button - only the bar. The bar's refresh does not cascade to the list of items it would seem. I have put in place what feels like a better fix in activateChild, what do you think?

andydotxyz · 2021-06-01T13:12:07Z

internal/widget/base.go

+	size     fyne.Size
+	position fyne.Position
+	hidden   bool


A mistake, sorry

andydotxyz · 2021-06-01T13:15:35Z

internal/widget/scroller.go

-	RefreshWidget(s)
+func (s *Scroll) Resize(sz fyne.Size) {
+	s.Base.Resize(sz)
+	s.Refresh()


Yes, good catch thanks.

andydotxyz · 2021-06-01T13:25:02Z

widget/tree.go

@@ -643,6 +643,12 @@ func (n *treeNode) Tapped(*fyne.PointEvent) {
 	n.tree.Select(n.uid)
 }

+func (n *treeNode) Resize(s fyne.Size) {
+	n.BaseWidget.Resize(s)
+	// TODO find out if the indentation changed. If not, and size is the same, then we can skip this as normal


It's not relying on a Refresh - it's the Layout that we care about.
Normally we can ignore Resize if the size has not changed - but in the case of Tree it may be the same size but need to have the content moved when it's reused (because increased indent does not change size).
Maybe there is a better solution, but I cannot think of one right now.

Jacalz

This looks good and sensible to me, though I would have preferred to get rid of .ExtendBaseWidget() entirely ;)

andydotxyz · 2021-06-03T13:14:03Z

This looks good and sensible to me, though I would have preferred to get rid of .ExtendBaseWidget() entirely ;)

Yes, that is the plan. But it will be easier to get rid of from a consistent place :).
Plus I don't yet know how we will do it, but I do know that this was blocking.

toaster · 2021-06-03T13:14:12Z

This looks good and sensible to me, though I would have preferred to get rid of .ExtendBaseWidget() entirely ;)

Yeah, that’s something @andydotxyz and me discussed already. Sadly, it’s not trivial.

andydotxyz added 2 commits May 26, 2021 23:08

Consistent API with internal and external base widget

fb09c5d

This needs to be improved through design updates, but for now this is clearer. It means we can extend internal widgets and not be confused which API is used

Fix refreshing of menu bar on tap

5189ea9

andydotxyz requested a review from toaster May 26, 2021 22:25

Add missing doc for staticcheck

7aaba22

toaster reviewed May 28, 2021

View reviewed changes

toaster requested changes May 28, 2021

View reviewed changes

Tidy up following code review

c3ed736

andydotxyz commented Jun 1, 2021

View reviewed changes

Correct fix for some nodes not correcting indent

11a6979

andydotxyz requested a review from toaster June 3, 2021 11:34

Jacalz approved these changes Jun 3, 2021

View reviewed changes

toaster approved these changes Jun 3, 2021

View reviewed changes

andydotxyz merged commit 72d5607 into fyne-io:develop Jun 3, 2021

andydotxyz deleted the fix/consistentbasewidget branch June 3, 2021 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/consistentbasewidget #2264

Fix/consistentbasewidget #2264

andydotxyz commented May 26, 2021

toaster May 28, 2021

andydotxyz Jun 1, 2021

toaster May 28, 2021

andydotxyz Jun 1, 2021

toaster May 28, 2021

andydotxyz Jun 1, 2021

toaster May 28, 2021

andydotxyz Jun 1, 2021

toaster Jun 1, 2021

andydotxyz Jun 3, 2021

toaster left a comment

toaster May 28, 2021

andydotxyz Jun 1, 2021 •

edited

Loading

andydotxyz left a comment

andydotxyz Jun 1, 2021

andydotxyz Jun 1, 2021 •

edited

Loading

andydotxyz Jun 1, 2021

andydotxyz Jun 1, 2021

andydotxyz Jun 1, 2021

Jacalz left a comment

andydotxyz commented Jun 3, 2021

toaster commented Jun 3, 2021

Fix/consistentbasewidget #2264

Fix/consistentbasewidget #2264

Conversation

andydotxyz commented May 26, 2021

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz commented Jun 3, 2021

toaster commented Jun 3, 2021

andydotxyz Jun 1, 2021 •

edited

Loading

andydotxyz Jun 1, 2021 •

edited

Loading