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
Suggested labels
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.
| void destroyMenuItem(void* nsMenuItem) { | ||
| MenuItem *menuItem = (MenuItem *)nsMenuItem; | ||
| [menuItem release]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use dispatch_async for thread-safe cleanup.
While the implementation correctly releases the NSMenuItem, it should use dispatch_async to ensure thread safety, consistent with other methods in the file.
Apply this diff to make the cleanup thread-safe:
void destroyMenuItem(void* nsMenuItem) {
- MenuItem *menuItem = (MenuItem *)nsMenuItem;
- [menuItem release];
+ dispatch_async(dispatch_get_main_queue(), ^{
+ MenuItem *menuItem = (MenuItem *)nsMenuItem;
+ [menuItem release];
+ });
}📝 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.
| void destroyMenuItem(void* nsMenuItem) { | |
| MenuItem *menuItem = (MenuItem *)nsMenuItem; | |
| [menuItem release]; | |
| } | |
| void destroyMenuItem(void* nsMenuItem) { | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| MenuItem *menuItem = (MenuItem *)nsMenuItem; | |
| [menuItem release]; | |
| }); | |
| } |
There was a problem hiding this comment.
Give me a good reason to run a memory release async
There was a problem hiding this comment.
@leaanthony You're right - there's no good reason to run memory release asynchronously. The current synchronous implementation is correct because:
- Memory should be released immediately to prevent memory leaks
- Unlike UI operations, memory release doesn't require main thread coordination
- Adding async dispatch would unnecessarily delay cleanup with no benefits
✏️ Learnings added
Learnt from: leaanthony
PR: wailsapp/wails#4031
File: v3/pkg/application/menuitem_darwin.go:299-302
Timestamp: 2025-01-25T04:10:53.851Z
Learning: Memory release operations in Objective-C/Cocoa should be performed synchronously, not asynchronously via dispatch_async, as they don't require main thread coordination and should complete immediately to prevent memory leaks.
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.
There was a problem hiding this comment.
@leaanthony Thank you for confirming!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
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. |
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