New ContextMenu API + example #4013
Conversation
|
Warning Rate limit exceeded@leaanthony has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces a comprehensive overhaul of the context menu system in Wails v3. The changes involve creating a new Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying wails with
|
| Latest commit: |
940e6cb
|
| Status: | ⚡️ Build in progress... |
New Menu guide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
v3/examples/contextmenus/main.go (1)
Line range hint
27-36: Consider cross-platform window options.The window options are Mac-specific. Consider providing platform-agnostic examples or documenting platform-specific behavior.
docs/src/content/docs/guides/menus.mdx (2)
59-138: Consider adding platform-specific behavior notes.The documentation of menu item properties and states is comprehensive. Consider adding notes about platform-specific behavior differences, particularly for properties like tooltips and accelerators.
300-491: Consider adding best practices for context menu cleanup.While the context menu documentation is comprehensive, consider adding a section about best practices for cleaning up context menus when components are unmounted or destroyed, particularly in single-page applications where components may be frequently mounted and unmounted.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~340-~340: Did you mean “precedent”? (As in ‘set an example’)
Context: ...e most specific (closest) setting takes precedence 3. Theautovalue can be used to rese...(PRECEDENT_PRECEDENCE_CONFUSION)
[style] ~418-~418: Consider shortening or rephrasing this to strengthen your wording.
Context: ...nd #### Context Menu Management After making changes to a context menu, call theUpdate()met...(MAKE_CHANGES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/src/content/docs/changelog.mdx(2 hunks)docs/src/content/docs/guides/menus.mdx(1 hunks)v3/examples/contextmenus/main.go(2 hunks)v3/pkg/application/context.go(1 hunks)v3/pkg/application/menuitem.go(1 hunks)v3/pkg/application/messageprocessor_contextmenu.go(1 hunks)v3/pkg/application/roles.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- v3/pkg/application/roles.go
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/guides/menus.mdx
[grammar] ~34-~34: This phrase is duplicated. You should probably use “Radio Groups” only once.
Context: ... // true = initially checked ``` #### Radio Groups Radio groups allow users to select one option from a...
(PHRASE_REPETITION)
[duplication] ~43-~43: Possible typo: you repeated a word.
Context: ...u.AddRadio("Option 3", false) ``` #### Separators Separators are horizontal lines that help organise...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~50-~50: Possible typo: you repeated a word.
Context: ...s: go menu.AddSeparator() #### Submenus Submenus are nested menus that appear when hover...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~229-~229: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...at the top of the screen (macOS). ### Application Menu Behaviour When you set an applica...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~340-~340: Did you mean “precedent”? (As in ‘set an example’)
Context: ...e most specific (closest) setting takes precedence 3. The auto value can be used to rese...
(PRECEDENT_PRECEDENCE_CONFUSION)
[style] ~418-~418: Consider shortening or rephrasing this to strengthen your wording.
Context: ...nd #### Context Menu Management After making changes to a context menu, call the Update() met...
(MAKE_CHANGES)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.22)
- GitHub Check: Run Go Tests (windows-latest, 1.22)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
v3/pkg/application/messageprocessor_contextmenu.go (1)
11-11: Breaking change: Data field type constraint.The change from
anytostringfor theDatafield improves type safety but may break existing implementations that use non-string data types.Let's check for existing usages that might be affected:
v3/pkg/application/menuitem.go (1)
181-181: Breaking change: Role enum value renamed.The change from 'Minimize' to 'Minimise' maintains consistency with British English spelling but may break existing code.
Let's check for existing usages:
docs/src/content/docs/guides/menus.mdx (2)
1-58: LGTM! Clear and comprehensive documentation of menu creation and basic items.The documentation effectively covers menu creation and different types of menu items with clear examples.
🧰 Tools
🪛 LanguageTool
[grammar] ~34-~34: This phrase is duplicated. You should probably use “Radio Groups” only once.
Context: ... // true = initially checked ``` #### Radio Groups Radio groups allow users to select one option from a...(PHRASE_REPETITION)
[duplication] ~43-~43: Possible typo: you repeated a word.
Context: ...u.AddRadio("Option 3", false) ``` #### Separators Separators are horizontal lines that help organise...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~50-~50: Possible typo: you repeated a word.
Context: ...s:go menu.AddSeparator()#### Submenus Submenus are nested menus that appear when hover...(ENGLISH_WORD_REPEAT_RULE)
139-223: LGTM! Comprehensive coverage of event handling and menu roles.The documentation effectively explains event handling and provides a clear overview of available menu roles with platform-specific notes.
docs/src/content/docs/changelog.mdx (1)
33-33: LGTM! Clear and well-formatted changelog entries.The changelog entries follow the Keep a Changelog format and accurately document the changes:
- Addition of the new menu guide
- Change in ContextMenuData return type from
anytostringAlso applies to: 43-43
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v3/examples/window-menu/main.go (1)
29-33: Consider renaming "MenuBar" submenu to "View".The "MenuBar" submenu name is unconventional. Consider moving the menu bar visibility controls under a more standard "View" menu, which is a common practice in desktop applications.
-editMenu := menu.AddSubmenu("MenuBar") -editMenu.Add("Hide MenuBar").OnClick(func(ctx *application.Context) { +viewMenu := menu.AddSubmenu("View") +viewMenu.Add("Hide Menu Bar").OnClick(func(ctx *application.Context) {v3/examples/window-menu/README.md (1)
3-3: Fix markdown formatting.Remove spaces inside the emphasis markers.
-*** Windows Only *** +***Windows Only***🧰 Tools
🪛 Markdownlint (0.37.0)
3-3: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
3-3: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/src/content/docs/changelog.mdx(2 hunks)v3/examples/window-menu/README.md(1 hunks)v3/examples/window-menu/assets/about.html(1 hunks)v3/examples/window-menu/assets/index.html(1 hunks)v3/examples/window-menu/assets/style.css(1 hunks)v3/examples/window-menu/main.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- v3/examples/window-menu/assets/index.html
- v3/examples/window-menu/assets/about.html
- v3/examples/window-menu/assets/style.css
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/content/docs/changelog.mdx
🧰 Additional context used
🪛 Markdownlint (0.37.0)
v3/examples/window-menu/README.md
3-3: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
3-3: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
v3/examples/window-menu/main.go (3)
3-12: LGTM! Clean imports and proper asset embedding.The imports are appropriate and the embed directive is correctly used to bundle assets.
13-20: LGTM! Clean application initialization.The application setup follows best practices with proper configuration of name, description, and asset handling.
60-64: LGTM! Appropriate error handling for an example.The error handling is simple but sufficient for demonstration purposes.
v3/examples/window-menu/main.go
Outdated
| KeyBindings: map[string]func(window *application.WebviewWindow){ | ||
| "F1": func(window *application.WebviewWindow) { | ||
| window.ToggleMenuBar() | ||
| }, | ||
| "F2": func(window *application.WebviewWindow) { | ||
| window.ShowMenuBar() | ||
| }, | ||
| "F3": func(window *application.WebviewWindow) { | ||
| window.HideMenuBar() | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Resolve documentation-code mismatch for key bindings.
The README mentions F10 for menu bar toggling, but the code implements F1, F2, and F3 keys. This inconsistency should be resolved either by:
- Updating the README to reflect the actual F1, F2, F3 key bindings, or
- Updating the code to use F10 as documented
v3/examples/window-menu/README.md
Outdated
| The example creates a window with a default menu and binds the F10 key to toggle the menu bar's visibility. The menu bar will hide when F10 is pressed and show when F10 is released. | ||
|
|
There was a problem hiding this comment.
Update key binding documentation.
The documentation incorrectly states that F10 is used for toggling the menu bar. The actual implementation uses:
- F1 to toggle the menu bar
- F2 to show the menu bar
- F3 to hide the menu bar
There was a problem hiding this comment.
Make a committable suggestion
There was a problem hiding this comment.
@leaanthony Here's the corrected documentation:
# Window Menu Example
*** Windows Only ***
This example demonstrates how to create a window with a menu bar that can be toggled using the window.ToggleMenuBar() method.
## Features
- Default menu bar with File, Edit, and Help menus
- F1 key to toggle menu bar visibility
- F2 key to show menu bar
- F3 key to hide menu bar
- Simple HTML interface with instructions
## Running the Example
```bash
cd v3/examples/window-menu
go run .How it Works
The example creates a window with a default menu and binds function keys for menu bar control:
- F1 toggles the menu bar visibility
- F2 shows the menu bar
- F3 hides the menu bar
<!-- This is an auto-generated reply by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v3/examples/window-menu/README.md (1)
22-22:⚠️ Potential issueFix inconsistent key binding documentation.
The documentation mentions both F1 and F10 for toggling the menu bar. Based on the previous review discussion, F10 is incorrect:
-The example creates a window with a default menu and binds the F1 key to toggle the menu bar's visibility. The menu bar will hide when F10 is pressed and show when F10 is released. +The example creates a window with a default menu and binds function keys for menu bar control. The menu bar visibility can be toggled with F1, shown with F2, and hidden with F3.
🧹 Nitpick comments (1)
v3/examples/window-menu/README.md (1)
3-3: Fix markdown formatting.Remove spaces inside emphasis markers:
-*** Windows Only *** +***Windows Only***🧰 Tools
🪛 Markdownlint (0.37.0)
3-3: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
3-3: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/examples/window-menu/README.md(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
v3/examples/window-menu/README.md
3-3: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
3-3: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
v3/examples/window-menu/README.md (2)
24-24:⚠️ Potential issueUpdate platform-specific note.
The note about F10 should be updated to reflect the correct key bindings:
-Note: The menu bar toggling functionality only works on Windows. On other platforms, the F10 key binding will have no effect. +Note: The menu bar toggling functionality only works on Windows. On other platforms, the function key bindings (F1, F2, F3) will have no effect.Likely invalid or redundant comment.
9-11:⚠️ Potential issueComplete the features list.
Based on the previous review discussion, the features list is missing F2 and F3 key bindings:
- Default menu bar with File, Edit, and Help menus - F1 key to toggle menu bar visibility +- F2 key to show menu bar +- F3 key to hide menu bar - Simple HTML interface with instructionsLikely invalid or redundant comment.
This PR introduces a simplified API for context menu creation and handling.
Example:
Fixes: #4012
Summary by CodeRabbit
New Features
Changes
ContextMenuDatato return a string type.Documentation
Bug Fixes