More control over menus #4031
More control over menus #4031
Conversation
WalkthroughThe pull request introduces several new methods to the Changes
Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
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
|
|
Thanks 🙏 Please could you add an entry to the changelog located at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/menu.go(2 hunks)
🔇 Additional comments (1)
v3/pkg/application/menu.go (1)
Line range hint
72-202: Add tests and update documentation for new menu methods.As requested in the PR comments, please:
- Add an entry to the changelog in
docs/src/content/docs/changelog.mdx- Update the menu documentation in
docs/src/content/docs/guides/menus.mdxAdditionally, consider adding test cases for:
- Clear method with nested submenus
- Append/Prepend with nil input
- Append/Prepend with empty menus
- Append/Prepend maintaining proper menu state
✅ Verification successful
Review comment is accurate - documentation and tests are missing
The verification confirms that:
- No tests exist for the new Clear, Append, and Prepend methods
- Documentation updates are missing in both:
- docs/src/content/docs/changelog.mdx
- docs/src/content/docs/guides/menus.mdx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests of the new methods rg -l "TestMenu.*(?:Clear|Append|Prepend)" --type goLength of output: 54
Script:
#!/bin/bash # Find all test files and menu-related tests echo "=== All test files ===" fd "_test\.go$" echo -e "\n=== Menu-related test content ===" rg "func Test.*Menu" --type go echo -e "\n=== Check documentation files ===" ls docs/src/content/docs/changelog.mdx docs/src/content/docs/guides/menus.mdx 2>/dev/null || echo "Documentation files not found"Length of output: 3727
Script:
#!/bin/bash # Check documentation content for mentions of new methods echo "=== Changelog content ===" rg -A 5 "Clear|Append|Prepend" docs/src/content/docs/changelog.mdx || echo "No mentions in changelog" echo -e "\n=== Menu documentation content ===" rg -A 5 "Clear|Append|Prepend" docs/src/content/docs/guides/menus.mdx || echo "No mentions in menu guide"Length of output: 406
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/src/content/docs/guides/menus.mdx (1)
59-76: LGTM! Consider adding error handling details.The "Combining menus" section is well-documented with clear examples. The code demonstrates both
AppendandPrependoperations effectively.Consider adding information about:
- Return values of these methods
- Error handling scenarios
- Thread safety considerations when modifying menus
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/changelog.mdx(1 hunks)docs/src/content/docs/guides/menus.mdx(1 hunks)
🔇 Additional comments (2)
docs/src/content/docs/guides/menus.mdx (1)
78-91: LGTM! The example effectively demonstrates menu clearing.The "Clearing a menu" section provides a clear explanation and practical example of using the
Clearmethod.docs/src/content/docs/changelog.mdx (1)
43-43: LGTM! Changelog entry is accurate and well-formatted.The entry correctly documents the addition of
Menu.ClearandMenu.Prependmethods, with proper attribution and PR reference.
|
Done! Added it to the changelog & have written in the docs on how to use the new functions. Might also write some tray docs soon but that's out of scope for this change. |
Add notes to documentation.
|
@FalcoG - I've added some docs and a |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/src/content/docs/guides/menus.mdx(1 hunks)v3/pkg/application/linux_cgo.go(1 hunks)v3/pkg/application/menu.go(2 hunks)v3/pkg/application/menuitem.go(2 hunks)v3/pkg/application/menuitem_darwin.go(2 hunks)v3/pkg/application/menuitem_linux.go(1 hunks)v3/pkg/application/menuitem_windows.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
v3/pkg/application/menu.go (1)
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
🔇 Additional comments (10)
v3/pkg/application/menuitem_windows.go (1)
99-101: LGTM! Clean implementation of menu item destruction.The implementation correctly uses the Windows API to remove the menu item using its handle and identifier.
v3/pkg/application/menuitem.go (1)
44-44: LGTM! Interface update for resource cleanup.The addition of destroy() to the menuItemImpl interface ensures consistent cleanup across all platform implementations.
v3/pkg/application/menuitem_linux.go (1)
24-30: LGTM! Thread-safe implementation with proper signal handling.The implementation correctly:
- Uses InvokeSync for thread safety
- Handles signal blocking/unblocking
- Follows the established pattern in the file
v3/pkg/application/menuitem_darwin.go (1)
352-354: LGTM! Simple and effective Go wrapper.The Go implementation correctly wraps the C function for cleanup.
v3/pkg/application/linux_cgo.go (1)
590-592: LGTM! Clean implementation of widget destruction.The function correctly uses
gtk_widget_destroyto clean up GTK widgets, which is essential for proper resource management.v3/pkg/application/menu.go (4)
72-75: LGTM! Simple and effective menu clearing.The implementation efficiently clears menu items by setting the slice to nil, allowing Go's garbage collector to handle cleanup.
77-82: LGTM! Thorough cleanup in Destroy method.The implementation properly calls Destroy() on each item before clearing the slice, ensuring complete resource cleanup.
201-204: LGTM! Maintains intended shared state.The implementation correctly maintains shared menu item references as intended by the design.
206-209: LGTM! Maintains intended shared state.The implementation correctly maintains shared menu item references as intended by the design.
docs/src/content/docs/guides/menus.mdx (1)
59-117: LGTM! Comprehensive and well-structured documentation.The documentation effectively:
- Explains the new menu manipulation features
- Provides clear code examples
- Includes important notes about state sharing and memory management
- Maintains consistency with the implementation
v3/pkg/application/menuitem.go
Outdated
| func (m *MenuItem) Destroy() { | ||
| // Clean up resources | ||
| if m.impl != nil { | ||
| m.impl.destroy() | ||
| } | ||
| if m.submenu != nil { | ||
| m.submenu.Destroy() | ||
| m.submenu = nil | ||
| } | ||
|
|
||
| if m.contextMenuData != nil { | ||
| m.contextMenuData = nil | ||
| } | ||
|
|
||
| if m.accelerator != nil { | ||
| m.accelerator = nil | ||
| } | ||
|
|
||
| m.callback = nil | ||
| m.radioGroupMembers = nil | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup of menuItemMap in Destroy().
The implementation thoroughly cleans up resources but should also remove the menu item from the global menuItemMap to prevent memory leaks.
Apply this diff to add the cleanup:
func (m *MenuItem) Destroy() {
+ menuItemMapLock.Lock()
+ delete(menuItemMap, m.id)
+ menuItemMapLock.Unlock()
+
// Clean up resources
if m.impl != nil {
m.impl.destroy()
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/pkg/application/menuitem.go (1)
436-460: Add documentation for the Destroy method.Please add documentation that explains:
- The purpose and cleanup process
- Whether it's safe to call multiple times
- Any side effects users should be aware of
Example documentation:
+// Destroy releases all resources associated with the MenuItem. +// This includes cleaning up platform-specific implementations, +// removing it from the global menuItemMap, and recursively +// destroying any submenu. It's safe to call this method +// multiple times on the same MenuItem. func (m *MenuItem) Destroy() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/menuitem.go(3 hunks)
🔇 Additional comments (2)
v3/pkg/application/menuitem.go (2)
36-40: LGTM! Thread-safe implementation of map cleanup.The function is well-implemented with proper synchronization using mutex lock/unlock and follows Go best practices.
436-460: LGTM! Thorough cleanup implementation.The implementation thoroughly handles resource cleanup by:
- Removing the item from the global map
- Cleaning up platform-specific resources
- Recursively destroying submenus
- Clearing all references to prevent memory leaks
Great addition! That's a convenient way to remove it completely. Also didn't know about the submenu memory thing, good catch. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/menu.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
v3/pkg/application/menu.go (1)
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
🔇 Additional comments (2)
v3/pkg/application/menu.go (2)
80-85: LGTM! Thorough cleanup implementation.The Destroy method properly cleans up resources by destroying each item and clearing the items slice.
72-78: 🛠️ Refactor suggestionConsider UI consistency in Clear method.
The Clear method should trigger a UI update after clearing items to ensure the menu's visual state remains consistent.
// Clear all menu items func (m *Menu) Clear() { for _, item := range m.items { removeMenuItemByID(item.id) } m.items = nil + m.Update() }Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v3/pkg/application/menu.go (3)
80-85: Enhance documentation for Destroy method.The implementation looks good, but consider adding documentation to clarify the purpose and when to use Destroy() vs Clear().
+// Destroy releases all resources associated with the menu and its items. +// Use this method when you want to completely clean up the menu and its resources. func (m *Menu) Destroy() {
204-211: Enhance documentation for Append method.The implementation correctly maintains shared state and includes proper checks. Consider improving the documentation to better describe the behavior.
-// Insert menu after an existing menu +// Append adds all items from the input menu to the end of this menu. +// The items maintain shared state with the input menu. func (m *Menu) Append(in *Menu) {
Line range hint
72-215: Consider adding method chaining for better fluent interface.The new methods enhance menu control as intended. Consider returning the menu instance from these methods to enable method chaining, which would align with the fluent interface pattern used elsewhere in the codebase.
Example implementation:
-func (m *Menu) Clear() { +func (m *Menu) Clear() *Menu { // ... existing implementation ... + return m } -func (m *Menu) Append(in *Menu) { +func (m *Menu) Append(in *Menu) *Menu { // ... existing implementation ... + return m }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/menu.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
v3/pkg/application/menu.go (1)
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
🔇 Additional comments (1)
v3/pkg/application/menu.go (1)
213-215: 🛠️ Refactor suggestionAdd safety check and UI update to Prepend method.
The Prepend method should include the same safety features as Append while maintaining shared state.
-// Insert menu before an existing menu +// Prepend adds all items from the input menu to the beginning of this menu. +// The items maintain shared state with the input menu. func (m *Menu) Prepend(in *Menu) { + if in == nil { + return + } m.items = append(in.items, m.items...) + m.Update() }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/src/content/docs/changelog.mdx(1 hunks)v3/pkg/application/menu.go(2 hunks)v3/pkg/application/systemtray_linux.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/content/docs/changelog.mdx
🧰 Additional context used
📓 Learnings (1)
v3/pkg/application/menu.go (1)
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.23)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (3)
v3/pkg/application/menu.go (3)
80-85: LGTM! Clean implementation of resource cleanup.The Destroy() method properly cleans up resources by destroying each menu item and clearing the items slice.
72-78: 🛠️ Refactor suggestionAdd UI update and submenu cleanup.
The Clear() method should:
- Clean up submenu resources
- Call Update() to ensure UI consistency
// Clear all menu items func (m *Menu) Clear() { for _, item := range m.items { + if item.submenu != nil { + item.submenu.Clear() + } removeMenuItemByID(item.id) } m.items = nil + m.Update() }Likely invalid or redundant comment.
212-215: 🛠️ Refactor suggestionAdd safety check, UI update, and improve comment.
The implementation should include nil check and UI update while maintaining shared state.
-// Prepend menu before an existing menu +// Prepend items from the input menu to the beginning of this menu func (m *Menu) Prepend(in *Menu) { + if in == nil { + return + } m.items = append(in.items, m.items...) + m.Update() }Likely invalid or redundant comment.
| func (s *systrayMenuItem) destroy() {} | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Empty destroy() method needs DBus cleanup implementation
The empty implementation is inconsistent with other platform implementations that properly clean up their resources. Based on the codebase patterns:
- Remove DBus menu item registration and clean up associated DBus resources
- Clear references to parent system tray and MenuItem
- Consider implementing signal blocking during cleanup, similar to
linuxMenuItem.destroy()
🔗 Analysis chain
Implement cleanup logic in the destroy() method.
The empty implementation might lead to resource leaks. Consider cleaning up the DBus resources and references held by the systrayMenuItem.
Let's check how other implementations handle cleanup:
Consider implementing the following cleanup tasks:
- Remove DBus menu item registration
- Clear references to parent system tray
- Clean up any DBus-specific resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other destroy method implementations
rg -A 5 "func.*destroy\(\)" --type go
Length of output: 6427
| // Append menu to an existing menu | ||
| func (m *Menu) Append(in *Menu) { | ||
| if in == nil { | ||
| return | ||
| } | ||
| m.items = append(m.items, in.items...) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add UI update after appending items.
The implementation correctly maintains shared state, but should call Update() to refresh the UI.
// Append menu to an existing menu
func (m *Menu) Append(in *Menu) {
if in == nil {
return
}
m.items = append(m.items, in.items...)
+ m.Update()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Append menu to an existing menu | |
| func (m *Menu) Append(in *Menu) { | |
| if in == nil { | |
| return | |
| } | |
| m.items = append(m.items, in.items...) | |
| } | |
| // Append menu to an existing menu | |
| func (m *Menu) Append(in *Menu) { | |
| if in == nil { | |
| return | |
| } | |
| m.items = append(m.items, in.items...) | |
| m.Update() | |
| } |
There was a problem hiding this comment.
Update is meant to be called manually
There was a problem hiding this comment.
@leaanthony Understood, I'll retract that suggestion. The manual Update() pattern gives users more control over UI refresh timing.
(｀ω´)
✏️ Learnings added
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menu.go:204-210
Timestamp: 2025-01-28T20:27:39.389Z
Learning: In the Wails menu system, Update() is meant to be called manually by users to refresh the UI. Methods like Append(), Clone() etc. should not automatically call Update() as this control is intentionally left to the user.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Description
Allows a menu to be cleared and in addition to Menu.Append, you can also now use Menu.Prepend to insert items before an existing menu.
My app has dynamic menu items that are at the top of a menu, which requires more control over the menu than previously possible. More detail and motivation is seen in the linked issue.
Fixes #4030
Type of change
Please select the option that is relevant.
How Has This Been Tested?
I've linked up wails (go work use ../../wails/v3) and tried the following: https://gist.github.com/FalcoG/b38517339969008195df7e51346db6be
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
wails doctor.application.NewServiceWithOptionsfor service initialization with extra configuration.Bug Fixes
RegisterContextMenu.Documentation