Feature: menu widget #877

toaster · 2020-04-17T09:52:09Z

Description:

This PR contains the first part of the menu change: internal/widget/menu.
This widget replaces the box that is used for menus up to now.

Checklist:

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

This adds some padding at the top and the bottom of a menu. It also moves the dismiss action from the MenuItem widget into the new Menu widget.

This is required for menus (with sub-menus) which will render their shadow on their own.

Thus sub-menus will have shadows, too. Also menus outside of pop-ups will have shadows.

andydotxyz · 2020-04-17T17:35:52Z

Hmm, thanks for all of this, it's really coming together. I see that the reason for making popup shadow optional is to allow the menu to draw it's own shadow - this seems a little strange.
Can you expand on why this is needed?

andydotxyz · 2020-04-17T17:39:43Z

widget/popup_test.go

+			require.GreaterOrEqual(t, len(r.Objects()), expectedObjectCount)
+
+			bgIdx := 0
+			if tt.castShadow {


This doesn't test the no-shadow state does it?

Not explicitly, good catch!
If the pop-up would cast a shadow with hideShadow == true, the following assertion regarding the background would fail. I’ll add an explicity assertion to clarify the intention.

toaster · 2020-04-18T08:12:25Z

Hmm, thanks for all of this, it's really coming together. I see that the reason for making popup shadow optional is to allow the menu to draw it's own shadow - this seems a little strange.
Can you expand on why this is needed?

Because menus will be allowed to have submenus in the next PR (tests just finished). These have to draw another shadow.

andydotxyz · 2020-04-18T08:21:31Z

I don’t follow sorry - won’t submenus look like top level menus? A list of items with a shadow around it...

toaster · 2020-04-18T09:23:24Z

Exactly, and who should draw the shadow?

stuartmscott · 2020-04-21T14:03:09Z

internal/widget/shadow.go

@@ -27,6 +27,7 @@ const (
 	ButtonLevel           ElevationLevel = 2
 	PopUpLevel            ElevationLevel = 8
 	SubmergedContentLevel ElevationLevel = 8
+	MenuLevel             ElevationLevel = 4


nit: move up 2 lines

… which has another shadow level.

…lay” PopUpMenu Thus the changes to PopUp can be reverted. This implicitly fixes fyne-io#892.

…rash

andydotxyz · 2020-04-24T10:04:36Z

I'm having difficulty reading this PR now sorry. The modifications to PopUp coming and going seem to be interleaved with the commits that get to this finished menu.
Is it possible to somehow pick the required commits onto a clean branch?

My other question is around the internal Overlay widget - it is only used by the new PopUpMenu - my normal rule of thumb is to not create an abstraction until there are multiple uses of it, so is the additional code really needed at this time?

toaster · 2020-04-24T16:13:32Z

My other question is around the internal Overlay widget - it is only used by the new PopUpMenu - my normal rule of thumb is to not create an abstraction until there are multiple uses of it, so is the additional code really needed at this time?

I don’t think this is an abstraction. As I already mentioned in #707 a widget should not be an overlay and another widget at the same time. The PopUpMenu can be moved and resized while an overlay can’t really (it should be canvas’ size at 0,0).
However, there will be another usage of the Overlay by the main menu widget, soon.

toaster · 2020-04-24T16:17:22Z

I'm having difficulty reading this PR now sorry. The modifications to PopUp coming and going seem to be interleaved with the commits that get to this finished menu.
Is it possible to somehow pick the required commits onto a clean branch?

Technically: yes.
Should I do it: I don’t think so.
It’s not that I’m too lazy (I am, of course), but the revert of the PopUp changes includes NotPadded which is already on the develop branch. In the end we would save like two small commits after investing a couple of hours.

andydotxyz · 2020-04-24T19:57:23Z

widget should not be an overlay and another widget at the same time

Hmm, I see what you mean. And yet we publicly expose the AddOverlay but hide the Overlay widget.
I think this may be a naming issue. To avoid confusing developers could something less generic and more meaningful help? Like OverlayWrapper, or OverlayContainer?
I would like to avoid people thinking they need to use this and can't, if they want an overlay. The reality is that any widget could be one, even though (as you say) it shouldn't be both.

Alternatively we could make a much more sweeping change that requires Overlays are in fact Overlays...

toaster · 2020-04-25T13:39:49Z

I think using a different name is the best way.
WDYT about OverlayBackground or OverlayPresenter?

andydotxyz · 2020-04-25T14:41:17Z

WDYT about OverlayBackground or OverlayPresenter?

Presenter is a bit of a trigger word as it's used for defined purposes in MVP https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93presenter.

OverlayBackground sounds servicable, I just proposed Container as we have used that elsewhere. It is both a background and a container I guess. Perhaps background sounds alittle more passive than the object is?

toaster · 2020-04-25T21:57:00Z

It’s fyne for me if you prefer Container over Background. It’s an internal widget for now and we are free to change the name if a better one occurs :).

andydotxyz · 2020-04-25T22:15:49Z

Good by me, though I think Stuart had an open review too?

toaster · 2020-04-26T06:18:49Z

Now everything should be in @stuartmscott @andydotxyz

toaster added 3 commits April 17, 2020 11:40

introduce internal Menu widget (instead of Box) as widget for menus

d47fd1b

This adds some padding at the top and the bottom of a menu. It also moves the dismiss action from the MenuItem widget into the new Menu widget.

popup's shadow is now optional

635385c

This is required for menus (with sub-menus) which will render their shadow on their own.

menu has shadow

a5068c1

Thus sub-menus will have shadows, too. Also menus outside of pop-ups will have shadows.

toaster requested a review from andydotxyz April 17, 2020 09:52

andydotxyz reviewed Apr 17, 2020

View reviewed changes

pop-up test: add explicit assertion for rendering without shadow

09d71c7

toaster mentioned this pull request Apr 20, 2020

Infrastructure/image based tests #882

Merged

3 tasks

stuartmscott reviewed Apr 21, 2020

View reviewed changes

toaster added 8 commits April 24, 2020 09:06

Merge develop into feature/menu_widget

11b2ce6

revert changes to NewPopupMenu*

37f6c5d

adjust master images due to revert of pop-up menu changes

cdbeea2

add ShowPopUpMenuAtPosition to replace NewPopUpMenuAtPosition

6405bdc

adjust master images due to switch from old to new pop-up menu

c0c3dd9

… which has another shadow level.

replace the “Menu in a PopUp” pop-up menu by the new “Menu in an Over…

fd0c460

…lay” PopUpMenu Thus the changes to PopUp can be reverted. This implicitly fixes fyne-io#892.

remove the customisations made to PopUp (hideShadow, NotPadded)

da63b1c

Having a current app without a driver (for some microseconds) might c…

e330080

…rash

rename Overlay to OverlayContainer

4621427

andydotxyz previously approved these changes Apr 25, 2020

View reviewed changes

sort shadow level constants

06faef4

toaster dismissed andydotxyz’s stale review via 06faef4 April 25, 2020 22:37

andydotxyz approved these changes Apr 26, 2020

View reviewed changes

toaster merged commit 32f93b6 into fyne-io:develop Apr 26, 2020

toaster mentioned this pull request Apr 26, 2020

Entry looses focus on pop-up menu dismiss #892

Closed

toaster deleted the feature/menu_widget branch April 26, 2020 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: menu widget #877

Feature: menu widget #877

toaster commented Apr 17, 2020

andydotxyz commented Apr 17, 2020

andydotxyz Apr 17, 2020

toaster Apr 18, 2020 •

edited

toaster commented Apr 18, 2020

andydotxyz commented Apr 18, 2020

toaster commented Apr 18, 2020

stuartmscott Apr 21, 2020

andydotxyz commented Apr 24, 2020

toaster commented Apr 24, 2020

toaster commented Apr 24, 2020

andydotxyz commented Apr 24, 2020

toaster commented Apr 25, 2020

andydotxyz commented Apr 25, 2020

toaster commented Apr 25, 2020

andydotxyz commented Apr 25, 2020

toaster commented Apr 26, 2020

Feature: menu widget #877

Feature: menu widget #877

Conversation

toaster commented Apr 17, 2020

Description:

Checklist:

andydotxyz commented Apr 17, 2020

andydotxyz Apr 17, 2020

Choose a reason for hiding this comment

toaster Apr 18, 2020 • edited

Choose a reason for hiding this comment

toaster commented Apr 18, 2020

andydotxyz commented Apr 18, 2020

toaster commented Apr 18, 2020

stuartmscott Apr 21, 2020

Choose a reason for hiding this comment

andydotxyz commented Apr 24, 2020

toaster commented Apr 24, 2020

toaster commented Apr 24, 2020

andydotxyz commented Apr 24, 2020

toaster commented Apr 25, 2020

andydotxyz commented Apr 25, 2020

toaster commented Apr 25, 2020

andydotxyz commented Apr 25, 2020

toaster commented Apr 26, 2020

toaster Apr 18, 2020 •

edited