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.

Sign up for GitHub

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

Jump to bottom

Feature: menu shortcuts #2715

Merged
merged 22 commits into from
Jan 6, 2022
Merged

Feature: menu shortcuts #2715

merged 22 commits into from
Jan 6, 2022

Conversation

toaster
Copy link
Member

@toaster toaster commented Jan 2, 2022

Description:

This PR introduces basic shortcut support for menus.
The shortcuts still have to be registered explicitly.
Also, the shortcut is not deactivated if the menu item is deactivated.

Fixes #682

Checklist:

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

Where applicable:

  • Public APIs match existing style.
  • Any breaking changes have a deprecation path or have been discussed.

@toaster
typo
7ed5a8f
@toaster
move desktop.Modifier to fyne.KeyModifier
621adb0 
This deprecates `desktop.Modifier` and uses a type alias to map to
`fyne.KeyModifier`.
@toaster
new fyne.KeyboardShortcut interface
fe6fed2 
All keyboard shortcuts should implement these.
It will be used for matching a keyboard action to built-in shortcuts and
to display the keys when assigned to a menu.
@toaster
new constant fyne.KeyModifierShortcutDefault
f5d6f55 
This is either KeyModifierSuper (Darwin) or KeyModifierControl (other).
@toaster
built-in shortcuts implement KeyboardShortcut
b654b95
@toaster
remove logic from font embedder’s bundleFont
8cf35c6 
Thus, it can be used more flexible with the caller deciding upon the details.
@toaster
sort methods in theme/gen.go
8e2aafe 
main() is on top instead of in the middle.
@toaster
add symbol font generation to theme/gen.go
0ab1826 
This requires fontTools (https://github.com/fonttools/fonttools) to be installed.
The source for the symbol font is Inter-Regular which is stripped to the
few glyphs we need. The resulting font is about 8k bytes currently.
@toaster
bundle symbol font
c15d947
@toaster
theme delivers InterSymbols-Regular.ttf as DefaultSymbolFont()
05090cf
@toaster
[internal/painter] deliver symbol font for new Symbol style
50fd2c1
@toaster
add Shortcut property to fyne.MenuItem
4caa26c
@toaster
menu_darwin_test: separate action expectation from menu entry expecation
4873c0e 
This allows to extract the menu entry expectation into a reusable function.
@toaster
menu_darwin_test: extract assertNSMenuItem
c8b0708
@toaster
menu_darwin_test: remove artifact
e1df408
@toaster
MacOS (darwin) menu supports shortcuts
8e3f1f3
@toaster
MacOS (darwin) menu supports shortcuts with special keys
3a422d6
@toaster
menu desktop test: add items with shortcuts
9a98756 
This does not test anything, yet.
@toaster
menu desktop test: make layout image masters configurable
8a696a7 
This allows for additional test cases with different render outcome.
@toaster
sort menuItemRenderer methods
3e54248
@toaster
menuItem widget displays menu shortcuts
3e53fbe
@toaster
refactor menuItemRenderer.Refresh
3f39044 
Separate text from check-icon refresh.
The text refresh uses the helper method .refreshText()
which was introduced with shortcut support.
@andydotxyz
Copy link
Member

This looks amazing thank you :).
I was about to approve as the code looks clean and clear, however I just wondered:

  • the menu shortcut text is as clear as the menu text (and disabled appropriately) but should it be more subtle? (I think I have seen some places do that).
    Would having the shortcut hints be in the disabled colour too light? do we need a "caption" colour that would make sense or similar?

@toaster
Copy link
Member Author

toaster commented Jan 5, 2022 

* the menu shortcut text is as clear as the menu text (and disabled appropriately) but should it be more subtle? (I think I have seen some places do that).
  Would having the shortcut hints be in the disabled colour too light? do we need a "caption" colour that would make sense or similar?

Hm, I think this is a matter of personal taste, isn’t it? I can’t remember to ever have seen such an approach but I’m not against it. It’s probably worth to think over and create some mock-ups at least. Then we could decide if we want to do it.
I’ll open an issue for it, so we don’t forget about it and have a place for the discussion and the mock-ups.

@toaster toaster mentioned this pull request Jan 5, 2022
Closed
@andydotxyz
Copy link
Member

Hm, I think this is a matter of personal taste, isn’t it? I can’t remember to ever have seen such an approach but I’m not against it. It’s probably worth to think over and create some mock-ups at least. Then we could decide if we want to do it.

Strange, it must just be the GTK theme I have loaded :) thanks for opening a new ticket to discuss

andydotxyz
andydotxyz approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Excellent work, thanks!

@toaster toaster merged commit 9b134b2 into fyne-io:develop Jan 6, 2022
@toaster toaster deleted the feature/menu_shortcuts branch January 6, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andydotxyz andydotxyz andydotxyz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@toaster @andydotxyz