Refactor PopUp size and position handling #699
Conversation
This makes the widget.PopUp independent from internal.cache (see fyne-io#696). This also removes a dependency of internal/driver/glfw from widget. And it fixes the broken shadow size for select's drop-down.
widget/popup_test.go
Outdated
| @@ -48,6 +50,7 @@ func TestPopUp_Move(t *testing.T) { | |||
|
|
|||
| pos := fyne.NewPos(10, 10) | |||
| pop.Move(pos) | |||
| cache.Renderer(pop).Layout(pop.Size()) | |||
There was a problem hiding this comment.
This concerns me. This seems to be required because the Move function no longer lays out content to accommodate hitting the edge of the screen.
And so any code that calls PopUp.Move() may not work any more.
There was a problem hiding this comment.
The Move method of a widget should not lay out anything. That's the responsibility of the renderer. That's why the adjustments are now made there.
There was a problem hiding this comment.
In that case if the move could cause a need to layout then it should trigger it - that way the tests (and all other code) don't need this line added...
There was a problem hiding this comment.
There is no “all other code” and the tests are not normal code. I don't think that a Move should run Layout immediately. This is for several reasons:
- I consider
Layoutas quite expensive - The
Moveof an element X is not important for the layout of X but for its parent p(X). AlsoMoveis normally called from parent's renderer'sLayoutbecause the parent decides where the element has to be placed.. - The
PopUpis special because it mixes up two things: a container (an overlay with canvas size and 0,0 pos) and a content (a freely sizeable and positionable element). Inconsistent size/position handling for overlays #707 describes a proposal to reduce or even eliminate the effects of this special behaviour. - Position adjustment due to outer constraints (window size) is clearly renderer responsibility. That's why the test that tests this adjustment has to ensure that the renderer is called. If Inconsistent size/position handling for overlays #707 was addressed, the container and its content would be separated and the position won't be sent to container's
Movebut to a specific container function likeMoveContentor alike. This would also make the test more clear because it would callMoveContentwhich then might trigger a layout. But honestly I don't like the idea of the latter because it adds internal dependencies to the widget. This could be avoided by performingRefreshand callLayoutfrom the renderer'sRefreshif needed.
After writing all this I think I might add the Refresh mentioned in point 4 to PopUp. The Layout line then would vanish from the test again. Stay tuned :).
P.S.: This Refresh would take its proper place if #707 is being addressed.
There was a problem hiding this comment.
Yes I think you're right. And this should get better now that #707 is landed.
Thanks
| @@ -83,6 +83,7 @@ func TestSelect_Tapped_Constrained(t *testing.T) { | |||
| canvas := fyne.CurrentApp().Driver().CanvasForObject(combo) | |||
| canvas.(test.WindowlessCanvas).Resize(fyne.NewSize(100, 100)) | |||
|
|
|||
| combo.Resize(combo.MinSize()) | |||
There was a problem hiding this comment.
The addition of this line is problematic, it seems to reflect a change in behaviour.
In fyne_demo go to Graphics, then tap the icon name combo...
You will see that it's sizing up to full size instead of staying the correct width.
There was a problem hiding this comment.
This line is not the cause of the drop.down size bug. I reported it as #706.
This line is here because the test set-up is uncommon. The combo is not set as canvas' content and therefore never laid out and therefore does have size 0x0.
There was a problem hiding this comment.
Oh yes, thanks. in that case is the canvas resize really helping? If the component is not inside the canvas then the parent size may have no impact?
There was a problem hiding this comment.
I don't resize the canvas but the combo :).
There was a problem hiding this comment.
I was meaning the resize call above, which is a canvas resize (I think it was perhaps put in accidentally?)
|
much nicer now, thanks |
Description:
The
PopUpsize and position handling is a difficult thing.I tried to improve the implementation and to reduce the visibility of internal structure.
This refactoring also fixes the bug which my previous bugfix of
Select's drop-down size adjustment fixed. That's why I reverted the previous fix.The refactoring reduces unwanted dependencies and also fixes the
Selectdrop-down size on mobile.Checklist:
[ ] Tests included.