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

Feature: submenus #905

Merged
merged 18 commits into from
Apr 29, 2020
Merged

Feature: submenus #905

merged 18 commits into from
Apr 29, 2020

Conversation

toaster
Copy link
Member

@toaster toaster commented Apr 26, 2020

Description:

This PR introduces the ability to create menus with submenus.

It also includes:

  • a bugfix for menu position and size handling for edge cases.
  • a modification to SelectEntry to let the pop-up size match the entry size if it changes (like for Select)

Fixes #395

Checklist:

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

Where applicable:

  • Public APIs match existing style.

@toaster
Copy link
Member Author

toaster commented Apr 26, 2020

artifact detected … will clean-up

@toaster toaster closed this Apr 26, 2020
@toaster
Copy link
Member Author

toaster commented Apr 26, 2020

Okay, I think I will remove it by extra commit :).

@toaster toaster reopened this Apr 26, 2020
@andydotxyz
Copy link
Member

I'm not sure I follow the need for widget.Label{Text: "secondary-tap me for context menu"} in advanced. It's already available on entry. I see that this has a submenu - but that is now in the menu anyway so I don't think it adds anything.

@toaster
Copy link
Member Author

toaster commented Apr 26, 2020

I'm not sure I follow the need for widget.Label{Text: "secondary-tap me for context menu"} in advanced. It's already available on entry. I see that this has a submenu - but that is now in the menu anyway so I don't think it adds anything.

You can check the placement on the far right which also shows that submenus are placed left on insufficient space to the right :).
And on MacOS there is no main menu bar by default.

internal/driver/glfw/menu_darwin.go Outdated Show resolved Hide resolved
internal/driver/glfw/menu_darwin.go Outdated Show resolved Hide resolved
@andydotxyz
Copy link
Member

shows that submenus are placed left on insufficient space to the right :)

Fair enough. Can you find a place to put it that does not take up so much space?

@toaster
Copy link
Member Author

toaster commented Apr 26, 2020

Fair enough. Can you find a place to put it that does not take up so much space?

I hoped, you would suggest one :). I’ll have a look.

@andydotxyz
Copy link
Member

I hoped, you would suggest one :). I’ll have a look.

Maybe buttons -> Actions and it's added to the bottom of that panel?

This fixes also a bug which miscalculated the callback item IDs.
The native menu still will trigger an event which should be handled by
an appropriate callback.
callbacks = append(callbacks, func() { w.queueEvent(action) })
nextItemID++
}
action := item.Action // catch action value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure here, the action is being captured on menu creation so it cannot change - should instead the item be captured so the latest value for Action is used in the callback? I only noticed it as the nil check is on a value that doesn't change.

@andydotxyz
Copy link
Member

Just 1 inline comment if you could check it out.

I also realised something (from a previous commit I think): macOS apps use "Preferences..." instead of "Settings" in the label - should we be rewriting it, should we move to Preferences or should we check for both?

@andydotxyz andydotxyz merged commit 8b361ec into fyne-io:develop Apr 29, 2020
@toaster toaster deleted the feature/submenus branch April 30, 2020 05:06
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.

None yet

2 participants