[v3 feature] support for MacOS Panel #3763
Conversation
…pha.7)' into v3-alpha-feature/mac-panel
WalkthroughThe changes in this pull request involve a significant refactor of the window management system in the application, transitioning from the Changes
Possibly related issues
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Outside diff range and nitpick comments (17)
v3/pkg/application/webview_panel_linux.go (1)
3-5: Enhance the error message to provide more context.Consider updating the error message to provide more information about the current state of the Linux implementation and any potential workarounds or alternatives.
Here's a suggestion:
- panic("Linux Panel not implemented") + panic("WebviewPanel is currently not supported on Linux. This feature is specific to MacOS at the moment. Please use WebviewWindow for cross-platform compatibility or check the project roadmap for Linux support.")v3/pkg/application/webview_panel_windows.go (1)
4-4: Enhance the panic message to provide more context.Consider updating the panic message to provide more information about the expected behavior and guide the user on the supported platforms.
Apply this diff to enhance the panic message:
- panic("Windows Panel not implemented") + panic("WebviewPanel is not supported on Windows. It is currently only implemented for MacOS.")v3/pkg/application/webview_responder_darwin.h (1)
1-15: Consider adding documentation comments.While the code is well-structured and follows conventions, it would be beneficial to add documentation comments to describe the purpose and behavior of the
WebviewResponderinterface and its methods. This will improve the code's readability and maintainability.v3/pkg/application/webview_panel_darwin.go (3)
78-86: Enhance error logging inhandleKeyEventWhen logging the error for an unparsed accelerator, include the problematic
acceleratorStringto aid in debugging.Apply this diff to improve the error message:
if err != nil { - globalApplication.error("unable to parse accelerator: %s", err.Error()) + globalApplication.error("unable to parse accelerator '%s': %s", acceleratorString, err.Error()) return }
31-36: Add documentation formacosWebviewPanelstructThe
macosWebviewPanelstruct lacks comments explaining its purpose and usage. Adding documentation will improve code maintainability and assist other developers.
37-49: Document thenewPanelImplfunctionThe function
newPanelImplcould benefit from a comment describing its functionality, parameters, and return value.v3/pkg/application/webview_responder_darwin.m (1)
16-16: Address the TODO: Investigate whyctrl+lnever reacheskeyDownThe TODO comment indicates that the
ctrl+lkey combination is not reaching thekeyDown:method. This might be due to certain key events being intercepted before they reach your responder. Investigate and resolve this issue to ensure all intended key events are properly handled.Do you want me to assist in identifying the cause or suggest potential solutions?
v3/internal/commands/appimage_testfiles/main.go (2)
Line range hint
69-79: Potential issue with casting and comparing window interfaces.In the
ShouldClosefunction, you're castingwindow *application.WebviewWindowtoapplication.Windowfor use inlo.Containsandlo.Without:if !lo.Contains(hiddenWindows, application.Window(window)) { // ... } hiddenWindows = lo.Without(hiddenWindows, application.Window(window))Be cautious with this approach, as comparing interfaces that contain pointers may not yield the expected results due to interface value comparisons in Go. This could lead to logic errors in window management.
Consider one of the following solutions:
- Option 1: Change the
ShouldClosefunction to acceptwindow application.Windowif possible.- Option 2: Use a unique identifier for each window (e.g., assign an ID) and compare these identifiers instead.
232-234: Update dialog titles for consistency with the window type.The dialog titles still refer to "Current WebviewWindow" even though the code now uses
application.Window. For clarity and consistency, consider updating the titles to reflect the generic window type.Apply this diff to update the dialog titles:
-application.InfoDialog().SetTitle("Current WebviewWindow Size").SetMessage(...) +application.InfoDialog().SetTitle("Current Window Size").SetMessage(...) -application.InfoDialog().SetTitle("Current WebviewWindow Position").SetMessage(...) +application.InfoDialog().SetTitle("Current Window Position").SetMessage(...)Also applies to: 263-265, 282-284
v3/pkg/application/webview_window_darwin.go (3)
445-446: Avoid Multiple Conversions ofmacOptions.AppearanceAt line 445,
macOptions.Appearanceis converted to a string and then to a C string within the same line.For clarity and efficiency, store the intermediate string conversion result:
if macOptions.Appearance != "" { + appearanceStr := string(macOptions.Appearance) + cAppearance := C.CString(appearanceStr) C.windowSetAppearanceTypeByName(w.nsWindow, cAppearance) + C.free(unsafe.Pointer(cAppearance)) }
404-404: Remove or Clarify Commented-Out CodeLine 404 contains commented-out code:
//w.setZoom(options.Zoom)If this code is no longer needed, consider removing it. If it's retained for future use, add a comment explaining why it's commented out.
410-417: Empty Case in Switch StatementIn the switch statement for
macOptions.Backdrop, theMacBackdropNormalcase is empty.To improve code readability, add a comment indicating that no action is needed for this case.
case MacBackdropNormal: // No action needed }v3/examples/window/main.go (2)
200-200: Remove redundant type conversions toapplication.WindowThe variable
windowis already of typeapplication.Window, so casting it withapplication.Window(window)is unnecessary. Removing these redundant conversions will enhance code readability.Apply the following changes:
- if !lo.Contains(hiddenWindows, application.Window(window)) { + if !lo.Contains(hiddenWindows, window) {- hiddenWindows = lo.Without(hiddenWindows, application.Window(window)) + hiddenWindows = lo.Without(hiddenWindows, window)Also applies to: 210-210
Line range hint
42-42: Update log message to reflect new window typeThe log message still references
WebviewWindow, but the code now usesapplication.Window. To maintain consistency and avoid confusion, consider updating the log message.Apply the following change:
- println("Current WebviewWindow is nil") + println("Current window is nil")v3/pkg/application/application.go (1)
165-178: Include problematic key in error messages for better debuggingIn
processKeyBindingOptions, when logging an error due to an invalid key binding, it would be helpful to include the specific key that caused the error. This provides clearer context when debugging issues with key bindings.Apply this diff to enhance the error message:
if err != nil { - globalApplication.error("Invalid keybinding: %s", err.Error()) + globalApplication.error("Invalid keybinding for key '%s': %s", key, err.Error()) continue }v3/pkg/application/webview_window_bindings_darwin.m (2)
47-50: Consider making the corner radius configurable for frameless windows.The corner radius is hardcoded to
8.0whenframelessistrue. To provide flexibility and accommodate different design requirements, consider making the corner radius a configurable parameter.
774-775: Remove redundant call tomakeKeyWindowto streamline window focusing.In the
windowFocusfunction, both[nsWindow makeKeyAndOrderFront:nil];and[nsWindow makeKeyWindow];are called. SincemakeKeyAndOrderFront:already makes the window key and brings it to the front, the subsequent call tomakeKeyWindowmay be unnecessary.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (26)
- v3/examples/build/main.go (3 hunks)
- v3/examples/events-bug/main.go (1 hunks)
- v3/examples/keybindings/main.go (1 hunks)
- v3/examples/window/main.go (7 hunks)
- v3/internal/commands/appimage_testfiles/main.go (8 hunks)
- v3/pkg/application/application.go (12 hunks)
- v3/pkg/application/application_options.go (1 hunks)
- v3/pkg/application/menu_windows.go (1 hunks)
- v3/pkg/application/menuitem_roles.go (1 hunks)
- v3/pkg/application/popupmenu_windows.go (1 hunks)
- v3/pkg/application/screen_darwin.go (1 hunks)
- v3/pkg/application/webview_panel.go (1 hunks)
- v3/pkg/application/webview_panel_darwin.go (1 hunks)
- v3/pkg/application/webview_panel_linux.go (1 hunks)
- v3/pkg/application/webview_panel_options.go (1 hunks)
- v3/pkg/application/webview_panel_windows.go (1 hunks)
- v3/pkg/application/webview_responder_darwin.h (1 hunks)
- v3/pkg/application/webview_responder_darwin.m (1 hunks)
- v3/pkg/application/webview_window.go (1 hunks)
- v3/pkg/application/webview_window_bindings_darwin.h (1 hunks)
- v3/pkg/application/webview_window_bindings_darwin.m (1 hunks)
- v3/pkg/application/webview_window_darwin.go (2 hunks)
- v3/pkg/application/webview_window_darwin.h (2 hunks)
- v3/pkg/application/webview_window_darwin.m (2 hunks)
- v3/pkg/application/webview_window_darwin_dev.go (1 hunks)
- v3/pkg/application/window.go (2 hunks)
Additional comments not posted (53)
v3/pkg/application/webview_responder_darwin.h (3)
1-2: Header guards are properly defined.The header file uses the
#ifndefand#definedirectives to prevent multiple inclusions of the header file. The header guard macro is unique and follows the naming convention of the file.Also applies to: 15-15
4-4: Appropriate header inclusion.The code includes the
Cocoa/Cocoa.hheader, which is necessary for using Cocoa framework classes and types in the interface definition.
6-13:WebviewResponderinterface is well-defined.
- The interface extends the
NSResponderclass, which is appropriate for handling events in a Cocoa application.- It declares a property
wof typeNSWindow *to store a reference to the associated window.- The
initAttachToWindow:method is declared for initializing the responder and attaching it to a window.- The
keyDown:method is overridden to handle key down events.- The
keyStringFromEvent:andspecialKeyStringFromEvent:methods are utility methods for converting key events to strings.- The method names follow the Objective-C naming conventions and use appropriate parameter types.
v3/pkg/application/webview_window_darwin.h (5)
1-2: LGTM!The header guard change aligns with the file name and follows a consistent naming convention.
6-7: LGTM!The inclusion of the new header file
webview_responder_darwin.his necessary for theWebviewResponderclass.
33-33: LGTM!The header guard closing statement has been updated to match the new header guard name, which is consistent with the change at the beginning of the file.
14-15: Verify the usage of the new initialization methods.The addition of the
initAsWindow:styleMask:backing:defer:andinitAsPanel:styleMask:backing:defer:methods provides a clearer way to initializeWebviewWindowas either a window or a panel. However, the removal of the previousinitWithContentRect:styleMask:backing:defer:method is a breaking change.Please ensure that all occurrences of the previous initialization method have been updated to use the new methods and that the new methods are being used correctly throughout the codebase.
Run the following script to verify the usage of the initialization methods:
#!/bin/bash # Description: Verify the usage of the new initialization methods. # Test 1: Search for occurrences of the previous initialization method. # Expect: No occurrences found. rg --type objective-c $'initWithContentRect:styleMask:backing:defer:' # Test 2: Search for occurrences of the new `initAsWindow` method. # Expect: Only valid usages found. rg --type objective-c $'initAsWindow:styleMask:backing:defer:' # Test 3: Search for occurrences of the new `initAsPanel` method. # Expect: Only valid usages found. rg --type objective-c $'initAsPanel:styleMask:backing:defer:'
8-12: Verify the impact of the class inheritance change and new properties.The
WebviewWindowclass now inherits fromNSObjectinstead ofNSWindow, which is a breaking change. Please ensure that this change has been properly handled throughout the codebase and does not introduce any compatibility issues.Additionally, verify that the addition of the
wandresponderproperties does not introduce any unintended side effects or memory management issues.Run the following script to verify the usage of
WebviewWindow:Verification successful
Verification complete: Class inheritance change implemented consistently, but 'responder' property not found.
The change from
WebviewWindowinheritingNSWindowto having anNSWindowpropertywhas been implemented consistently throughout the codebase. This architectural change appears to be properly handled, with no obvious issues or inconsistencies found.However, the
responderproperty mentioned in the original review comment was not found in the codebase. It's possible that this property was removed or renamed during the refactoring process.Key findings:
WebviewWindownow wraps anNSWindowinstance instead of subclassing it.- The
wproperty of typeNSWindowis widely used across the codebase.- No remaining direct inheritances from
NSWindowforWebviewWindowwere found.- Initialization methods for
WebviewWindoware present and consistent with the new structure.It's recommended to double-check the status of the
responderproperty, as it was not found in the current codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `WebviewWindow` after the class inheritance change. # Test 1: Search for occurrences of `WebviewWindow` being treated as an `NSWindow`. # Expect: No occurrences found. rg --type objective-c $'WebviewWindow \*\).*NSWindow' # Test 2: Search for occurrences of `WebviewWindow` being initialized without providing an `NSWindow`. # Expect: No occurrences found. rg --type objective-c $'WebviewWindow alloc\] init' # Test 3: Search for occurrences of accessing the `w` property of `WebviewWindow`. # Expect: Only valid accesses found. rg --type objective-c $'WebviewWindow.*\.w' # Test 4: Search for occurrences of accessing the `responder` property of `WebviewWindow`. # Expect: Only valid accesses found. rg --type objective-c $'WebviewWindow.*\.responder'Length of output: 591
Script:
#!/bin/bash # Description: Find Objective-C files and verify WebviewWindow usage # Find Objective-C file extensions echo "Objective-C file extensions:" fd -e h -e m -e mm --type f # Search for WebviewWindow in all files (ignoring case) echo -e "\nWebviewWindow usage:" rg -i "WebviewWindow" # Search for specific patterns related to WebviewWindow echo -e "\nWebviewWindow inheritance and property access:" rg -i "WebviewWindow.*NSWindow" rg -i "WebviewWindow.*\.w" rg -i "WebviewWindow.*\.responder"Length of output: 119023
Script:
#!/bin/bash # Description: Further verify WebviewWindow changes and search for 'responder' # Search for 'responder' in relation to WebviewWindow echo "Searching for 'responder' in relation to WebviewWindow:" rg -i "WebviewWindow.*responder" # Search for potential issues or inconsistencies echo -e "\nSearching for potential issues or inconsistencies:" rg -i "WebviewWindow.*NSWindow" -A 5 -B 5 # Check for any remaining direct inheritance from NSWindow echo -e "\nChecking for any remaining direct inheritance from NSWindow:" rg -i "WebviewWindow\s*:\s*NSWindow" # Search for WebviewWindow initialization echo -e "\nSearching for WebviewWindow initialization:" rg -i "WebviewWindow.*init"Length of output: 41816
v3/examples/events-bug/main.go (1)
16-17: Verify that theWindowtype provides all the necessary functionality.The change from
*application.WebviewWindowtoapplication.Windowallows for more flexibility in the key binding function, as it can now accept any type that satisfies theWindowinterface or extends theWindowbase type.However, please ensure that the
Windowtype provides all the necessary methods and properties for the intended use case within the key binding function. If anyWebviewWindow-specific functionality is required, you may need to update the function body accordingly.v3/pkg/application/webview_window_darwin_dev.go (2)
59-61: LGTM!The addition of the
openDevToolsmethod to themacosWebviewPanelstruct is a positive change that enhances its functionality and aligns its behavior with themacosWebviewWindowstruct. The implementation correctly passes thensPanelpointer to the C function.
63-65: LGTM!The addition of the
enableDevToolsmethod to themacosWebviewPanelstruct is a positive change that enhances its functionality and aligns its behavior with themacosWebviewWindowstruct. The implementation correctly passes thensPanelpointer to the C function.v3/pkg/application/webview_panel_options.go (3)
3-15: LGTM!The
WebviewPanelOptionsstruct is well-defined, with good reusability by embeddingWebviewWindowOptions, and useful customization options through theFloating,ShouldClose, andKeyBindingsfields.
17-19: LGTM!Providing default options through
WebviewPanelDefaultsis a good practice to ensure consistent behavior and reduce boilerplate. InitializingWebviewWindowOptionswithWebviewWindowDefaultsensures that the default window options are used as a starting point.
21-47: LGTM!The
processKeyBindingOptionsForPanelfunction is well-implemented, with good handling of both panel and window key bindings. Parsing the keys into accelerators ensures that the key bindings are valid and consistent. Wrapping the window key binding callbacks to convert theWebviewWindowto aWebviewPanelis a good way to reuse existing key bindings. Logging invalid key bindings and skipping them is a good way to handle errors gracefully.v3/pkg/application/window.go (3)
25-25: LGTM!The
Flashmethod is a valuable addition to theWindowinterface, providing a clear and concise way to enable or disable the flash effect within the window. The method signature and parameter naming are appropriate.
51-51: LGTM!The
Windowinterface, providing a straightforward way to handle printing functionality. Returning an error is a good practice to handle and propagate any failures during the printing operation. The method signature and error handling are appropriate.
58-58: LGTM!The
SetEnabledmethod is a valuable addition to theWindowinterface, providing a clear and concise way to enable or disable the window itself. The method signature and parameter naming are appropriate.v3/pkg/application/menu_windows.go (1)
76-78: LGTM! The change aligns with the PR objectives.The change in the parameter type of the anonymous function from
*WebviewWindowtoWindowbroadens the compatibility of the function with different window types. This change enables theprocessMenumethod to work with bothWebviewWindowand the newWebviewPanelcomponent, which is the desired behavior as per the PR objectives.The change is unlikely to affect the existing functionality of
WebviewWindowas it also implements theWindowinterface.v3/pkg/application/screen_darwin.go (1)
176-179: LGTM!The new
getScreenForPanelfunction is implemented correctly and follows the existing pattern for retrieving screens. It expands the capability to handle panels, complementing the existing functionality for windows and systrays.The function:
- Takes a pointer to a
macosWebviewPanelas an argument.- Calls the existing C function
getScreenForWindowwith thensPanelproperty to retrieve the corresponding screen.- Converts the result to a
Screentype using the existingcScreenToScreenfunction.The function signature and usage of the C function are consistent with the existing
getScreenForWindowfunction used formacosWebviewWindow.v3/pkg/application/popupmenu_windows.go (1)
126-126: Verify that all the code has been updated to pass a type that implements theWindowinterface.The change in the function signature from
*WebviewWindowtoWindowmakes the function more flexible as it can now work with any type that implements theWindowinterface. This is a good change.However, this is a breaking change. Please verify that all the code that was passing a
*WebviewWindowto this function has been updated to pass a type that implements theWindowinterface.Run the following script to verify the usage of the
addKeyBindingfunction:Verification successful
Verification successful:
addKeyBindingusage is consistent with the newWindowinterfaceThe review of
globalApplication.addKeyBindingusage across the codebase confirms that the change from*WebviewWindowtoWindowhas been consistently applied. All occurrences now usefunc(w Window) { ... }, which aligns with the new interface-based approach. This verification indicates that the breaking change has been properly addressed throughout the relevant parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `addKeyBinding` pass a type that implements the `Window` interface. # Test: Search for the function usage. Expect: No occurrences of `*WebviewWindow` being passed. rg --type go -A 5 $'globalApplication\.addKeyBinding'Length of output: 814
v3/pkg/application/menuitem_roles.go (1)
50-55: LGTM!The type switch enhances type safety by ensuring that the
cutmethod is invoked only on the appropriate types. It correctly handles the new*WebviewPaneltype introduced in the PR and exhaustively handles all possible types ofcurrentWindow, preventing potential runtime errors.v3/examples/build/main.go (12)
Line range hint
32-38: LGTM!The change in the
currentWindowfunction signature to accept a parameter of typefunc(window application.Window)instead offunc(window *application.WebviewWindow)is a good move towards using interfaces for better abstraction and flexibility.This suggests that the
application.Windowinterface has been introduced to provide a more generic way of interacting with windows, andapplication.WebviewWindowlikely implements this interface. The unchanged function body indicates that theapplication.Windowinterface likely provides the same methods asapplication.WebviewWindow.
121-124: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature.It also confirms that the
application.Windowinterface provides aSetSizemethod, which was previously available on*application.WebviewWindow.
127-130: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segment.It further confirms that the
application.Windowinterface provides aSetSizemethod, which was previously available on*application.WebviewWindow.
132-135: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It confirms that the
application.Windowinterface provides aSetMinSizemethod, which was previously available on*application.WebviewWindow.
137-141: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It confirms that the
application.Windowinterface providesSetMaximiseButtonStateandSetMaxSizemethods, which were previously available on*application.WebviewWindow.
143-147: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It confirms that the
application.Windowinterface provides aSizemethod, which was previously available on*application.WebviewWindow.
150-153: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It further confirms that the
application.Windowinterface provides aSetMinSizemethod, which was previously available on*application.WebviewWindow.
156-160: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It further confirms that the
application.Windowinterface providesSetMaxSizeandSetMaximiseButtonStatemethods, which were previously available on*application.WebviewWindow.
163-166: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It confirms that the
application.Windowinterface provides aSetRelativePositionmethod, which was previously available on*application.WebviewWindow.
168-171: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It further confirms that the
application.Windowinterface provides aSetRelativePositionmethod, which was previously available on*application.WebviewWindow.
174-178: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It confirms that the
application.Windowinterface provides aRelativePositionmethod, which was previously available on*application.WebviewWindow.
181-184: LGTM!The change in the function literal passed to
currentWindowto accept a parameter of typeapplication.Windowinstead of*application.WebviewWindowis consistent with the updatedcurrentWindowfunction signature and the previous code segments.It confirms that the
application.Windowinterface provides aCentermethod, which was previously available on*application.WebviewWindow.v3/pkg/application/webview_window.go (2)
260-260: LGTM!The function name change is consistent and the key bindings are correctly assigned to
result.keyBindings.
Line range hint
266-280: LGTM!The function has been renamed but the logic remains unchanged. The key bindings are correctly parsed and added to the result map.
v3/pkg/application/webview_panel_darwin.go (3)
4-6: Verify the minimum macOS version in CFLAGSThe
#cgo CFLAGSsets-mmacosx-version-min=10.13. Ensure that this minimum macOS version aligns with the features used in this implementation and the project's supported macOS versions.
45-47: Confirm the correctness ofexecJSinvocationThe call to
result.execJS(runtime.Core())may need verification. Ensure thatexecJSis intended to be called withruntime.Core()as an argument and that this aligns with its definition.
73-74: EnsuresetFloatingis called afternsPanelis initializedThe method
p.setFloating(options.Floating)is called immediately after setting up the panel. Confirm thatp.nsPanelis properly initialized before this call to avoid potential nil pointer dereferences.v3/pkg/application/webview_window_bindings_darwin.h (2)
93-96: Verify the correct parameter type forsetButtonStateThe function
setButtonStatetakes avoid *buttonparameter, whereas related functions likesetMinimiseButtonStateusevoid *window. Ensure thatsetButtonStateis intended to operate on a button object and that the parameter type is correct.Please confirm whether
setButtonStateshould be:-void setButtonState(void *button, int state); +void setButtonState(void *window, int state);Or if it should indeed operate on a separate button object.
107-107: Include guard correctly closedThe include guard
#endif // WEBVIEW_WINDOW_BINDINGS_DARWINis properly placed, ensuring the header is correctly encapsulated.v3/internal/commands/appimage_testfiles/main.go (2)
31-31: Verify the change from pointer to interface inhiddenWindows.The variable
hiddenWindowshas been updated from[]*application.WebviewWindowto[]application.Window. Ensure thatapplication.Windowcorrectly represents the windows you intend to store and that all methods and comparisons used with this slice function as expected.
33-36: Update tocurrentWindowfunction enhances flexibility.Changing the
currentWindowhelper function to acceptapplication.Windowinstead of*application.WebviewWindowallows for greater flexibility, especially ifapplication.Windowis an interface that multiple window types implement.v3/pkg/application/webview_window_darwin.go (1)
419-422: Ensure Proper Handling of Default Window LevelThe check for an empty
macOptions.WindowLevelassignsMacWindowLevelNormalif it's empty.Confirm that comparing
macOptions.WindowLevelto an empty string ("") is appropriate. IfWindowLevelis of a specific type or has default values, ensure this logic correctly handles all cases.v3/pkg/application/webview_window_darwin.m (1)
60-60: Verification Needed: Ensurewindow.wAccess Is ValidIn the
startDrag:method, you've changed the call to[window.w performWindowDragWithEvent:self.leftMouseEvent];. Verify thatwindow.wis properly initialized and thatwindowis indeed an instance ofWebviewWindowwith a validwproperty.Run the following script to ensure that all instances of
windowpassed tostartDrag:are of typeWebviewWindow:This script searches for calls to
startDrag:and provides context to help confirm that thewindowparameter is correctly typed.Verification successful
To gather more information and verify our concerns, let's run the following scripts:
Verification Successful:
window.wAccess Is ValidThe code review has been verified. The
windowparameter passed tostartDrag:is indeed an instance ofWebviewWindow, and it has a validwproperty. The changes made in thestartDrag:method are consistent with the rest of the codebase.Key findings:
- The
startDrag:method is implemented to accept aWebviewWindow*parameter.WebviewWindowinstances consistently have awproperty used throughout the code.- The call to
startDrag:inwebview_window_bindings_darwin.mpasses aWebviewWindow*object.No further action is required regarding this verification.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to startDrag: pass a WebviewWindow instance. # Search for all calls to startDrag: and check the object type. rg --type objc '\[.* startDrag:.*\]' -A 2Length of output: 245
Script:
#!/bin/bash # Search for webviewWindow declaration/initialization rg --type objc 'webviewWindow' v3/pkg/application/webview_window_bindings_darwin.m -C 2 # Look for WebviewWindow class implementation ast-grep --lang objc --pattern 'interface WebviewWindow : $_ { $$$ }' # Check windowDelegate implementation and startDrag: method rg --type objc 'windowDelegate' v3/pkg/application/webview_window_bindings_darwin.m -C 2 rg --type objc 'startDrag:' -C 2Length of output: 6785
v3/examples/window/main.go (1)
38-38: Verify type compatibility incurrentWindowfunctionThe
currentWindowfunction now accepts a parameter of typeapplication.Windowinstead of*application.WebviewWindow. Please ensure that all methods invoked onwindowwithin this function and its callbacks are part of theapplication.Windowinterface and that no pointer-specific functionality is required.To confirm, you can run the following script to check for any methods called on
windowthat might not be defined in theapplication.Windowinterface:This script will list all unique methods called on
window, allowing you to cross-reference them with theapplication.Windowinterface.Verification successful
Type compatibility in
currentWindowfunction verifiedThe change from
*application.WebviewWindowtoapplication.Windowin thecurrentWindowfunction is compatible with all operations performed on the window parameter. All method calls observed are consistent with what would be expected from a general window interface, and no pointer-specific functionality is used.Methods called on the window parameter include operations for size, position, state management, and various window-specific actions. These are all appropriate for an
application.Windowinterface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all methods called on `window` are defined in `application.Window` interface. # Search for method calls on `window` in `main.go`. rg --type go --no-heading -A 1 'func\(w application\.Window\)' main.go | grep '\.w\.' | awk '{print $2}' | sort | uniqLength of output: 168
Script:
#!/bin/bash # Find the correct location of main.go file_path=$(fd -t f main.go) if [ -z "$file_path" ]; then echo "main.go not found in the repository" exit 1 fi echo "File found at: $file_path" # Display the content of the file to verify it's the correct one echo "File content:" cat "$file_path" echo "Analyzing method calls on window parameter:" rg --type go --no-heading -A 1 'func\(w application\.Window\)' "$file_path" | grep '\.w\.' | awk '{print $2}' | sort | uniqLength of output: 26515
Script:
#!/bin/bash echo "File content:" cat v3/examples/window/main.go echo -e "\nAnalyzing method calls on window parameter:" rg --type go --no-heading -A 10 'func\(.*application\.Window\)' v3/examples/window/main.go | rg '\.(\w+)\(' echo -e "\nListing all method calls on 'w' parameter:" rg --type go --no-heading 'w\.\w+' v3/examples/window/main.go | awk '{print $2}' | sort | uniqLength of output: 26292
v3/pkg/application/application.go (5)
340-341: Thread-safe management ofkeyBindingsmap usingsync.RWMutexGood use of
RWMutexto manage concurrent access to thekeyBindingsmap. This ensures that key bindings are safely read and modified across multiple goroutines.
420-423: Initialization of key binding structuresThe initialization of
a.keyBindingsand other related fields in theinitfunction is appropriate and ensures that the application is correctly set up before use.
Line range hint
755-766: Correctly updatedCurrentWindowto returnWindowinterfaceThe
CurrentWindowmethod has been updated to return theWindowinterface instead of*WebviewWindow. This change aligns with the shift towards using the more generalWindowtype and enhances flexibility.
Line range hint
965-974: Ensure key binding callbacks are thread-safeThe
processKeyBindingmethod executes callbacks in separate goroutines to prevent blocking. Ensure that the callback functions provided inkeyBindingsare thread-safe, as they may be executed concurrently.Verify that all key binding callbacks are thread-safe by reviewing their implementations. If necessary, add synchronization mechanisms within the callbacks to handle shared resources safely.
985-988: Proper synchronization when adding and removing key bindingsThe use of write locks in
addKeyBindingensures thread-safe modification of thekeyBindingsmap. This is appropriate for preventing race conditions during updates.v3/pkg/application/webview_window_bindings_darwin.m (3)
14-17: Verify window style mask settings for frameless windows.When
framelessistrue, the style mask is set toNSWindowStyleMaskBorderless | NSWindowStyleMaskResizable. Ensure that this style mask includes all necessary window styles for frameless windows to function correctly and that it aligns with the desired window behavior.
628-629: Verify setting window size constraints to zero inwindowSetFullScreen.In the
windowSetFullScreenfunction,windowSetMaxSize(nsWindow, 0, 0);andwindowSetMinSize(nsWindow, 0, 0);set the maximum and minimum window sizes to zero. This might lead to unexpected behavior or restrict user interactions. Confirm that this is the intended effect when entering full-screen mode.
85-86: Ensure compatibility when settingfraudulentWebsiteWarningEnabled.The property
fraudulentWebsiteWarningEnabledis set within anif (@available(macOS 10.15, *))block. Verify that this property exists and functions as expected on all targeted macOS versions to prevent potential runtime issues on unsupported versions.
| p.setFloating(options.Floating) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unit tests are missing for macosWebviewPanel
No unit tests are provided for the new macosWebviewPanel implementation. Adding tests will help ensure that the panel behaves as expected and will prevent future regressions.
| void windowSetTitle(void* nsWindow, char* title) { | ||
| NSString* nsTitle = [NSString stringWithUTF8String:title]; | ||
| [((WebviewWindow*)nsWindow).w setTitle:nsTitle]; | ||
| free(title); |
There was a problem hiding this comment.
Review the use of free(title); to prevent memory management issues.
After setting the window title, the code calls free(title);. If title was not dynamically allocated using malloc or a similar function, freeing it can lead to undefined behavior or crashes. Verify the ownership and allocation of title. If it's not necessary to free title here, remove the free(title); line.
Apply this diff to fix the issue:
void windowSetTitle(void* nsWindow, char* title) {
NSString* nsTitle = [NSString stringWithUTF8String:title];
[((WebviewWindow*)nsWindow).w setTitle:nsTitle];
- free(title);
}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 windowSetTitle(void* nsWindow, char* title) { | |
| NSString* nsTitle = [NSString stringWithUTF8String:title]; | |
| [((WebviewWindow*)nsWindow).w setTitle:nsTitle]; | |
| free(title); | |
| void windowSetTitle(void* nsWindow, char* title) { | |
| NSString* nsTitle = [NSString stringWithUTF8String:title]; | |
| [((WebviewWindow*)nsWindow).w setTitle:nsTitle]; | |
| } |
| void windowSetAppearanceTypeByName(void* nsWindow, const char *appearanceName) { | ||
| // set window appearance type by name | ||
| // Convert appearance name to NSString | ||
| NSString* appearanceNameString = [NSString stringWithUTF8String:appearanceName]; | ||
| // Set appearance | ||
| [((WebviewWindow*)nsWindow).w setAppearance:[NSAppearance appearanceNamed:appearanceNameString]]; | ||
|
|
||
| free((void*)appearanceName); |
There was a problem hiding this comment.
Avoid freeing appearanceName to prevent potential crashes.
The function windowSetAppearanceTypeByName calls free((void*)appearanceName); after setting the window appearance. If appearanceName was not dynamically allocated, this can cause crashes. Ensure that memory management is handled correctly. If appearanceName doesn't need to be freed here, remove the free statement.
Apply this diff to fix the issue:
void windowSetAppearanceTypeByName(void* nsWindow, const char *appearanceName) {
// Convert appearance name to NSString
NSString* appearanceNameString = [NSString stringWithUTF8String:appearanceName];
// Set appearance
[((WebviewWindow*)nsWindow).w setAppearance:[NSAppearance appearanceNamed:appearanceNameString]];
- free((void*)appearanceName);
}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 windowSetAppearanceTypeByName(void* nsWindow, const char *appearanceName) { | |
| // set window appearance type by name | |
| // Convert appearance name to NSString | |
| NSString* appearanceNameString = [NSString stringWithUTF8String:appearanceName]; | |
| // Set appearance | |
| [((WebviewWindow*)nsWindow).w setAppearance:[NSAppearance appearanceNamed:appearanceNameString]]; | |
| free((void*)appearanceName); | |
| void windowSetAppearanceTypeByName(void* nsWindow, const char *appearanceName) { | |
| // set window appearance type by name | |
| // Convert appearance name to NSString | |
| NSString* appearanceNameString = [NSString stringWithUTF8String:appearanceName]; | |
| // Set appearance | |
| [((WebviewWindow*)nsWindow).w setAppearance:[NSAppearance appearanceNamed:appearanceNameString]]; | |
| } |
|
Thanks for taking the time to open this. The rabbit has picked up on a few points to address. Would it be possible to also add an example as that really helps testing |
At this repo (https://github.com/nixpare/wails-mac-panel) you will find the "hello world" of the project modified to use the |
|
Yeah I think some of the points raised are worth addressing 👍 |
|
I will fix all the problems i can, but on those related to memory safety of the objective-c bindings (inherited by |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- v3/pkg/application/application.go (12 hunks)
- v3/pkg/application/application_options.go (1 hunks)
- v3/pkg/application/webview_panel.go (1 hunks)
- v3/pkg/application/webview_panel_darwin.go (1 hunks)
- v3/pkg/application/webview_panel_options.go (1 hunks)
- v3/pkg/application/webview_window.go (2 hunks)
- v3/pkg/application/webview_window_bindings_darwin.h (1 hunks)
- v3/pkg/application/webview_window_bindings_darwin.m (1 hunks)
- v3/pkg/application/webview_window_darwin.go (4 hunks)
- v3/pkg/application/webview_window_darwin.m (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- v3/pkg/application/application.go
- v3/pkg/application/application_options.go
- v3/pkg/application/webview_panel.go
- v3/pkg/application/webview_panel_darwin.go
- v3/pkg/application/webview_panel_options.go
- v3/pkg/application/webview_window_darwin.go
- v3/pkg/application/webview_window_darwin.m
Additional context used
Learnings (1)
v3/pkg/application/webview_window.go (1)
Learnt from: nixpare PR: wailsapp/wails#3763 File: v3/examples/keybindings/main.go:16-17 Timestamp: 2024-09-20T23:34:29.841Z Learning: In the codebase, `application.Options.KeyBindings` uses the `application.Window` type, whereas `application.WebviewWindowOptions.KeyBindings` uses `*application.WebviewWindow`. This is intentional and acceptable.
Additional comments not posted (12)
v3/pkg/application/webview_window_bindings_darwin.m (11)
3-114: LGTM!The function correctly creates a new window or panel based on the provided parameters. The window delegate, content view, and webview are set up properly. The function also configures the window and webview based on the provided parameters.
116-165: LGTM!The function correctly retrieves the window delegate and style mask. It then checks the style mask against various window style constants and prints the corresponding style name. The logic is straightforward and there are no apparent issues.
168-175: LGTM!The function correctly retrieves the window delegate and sets the
invisibleTitleBarHeightproperty to the providedheightvalue. The logic is straightforward and there are no apparent issues.
177-181: LGTM!The function correctly sets the window's
opaqueproperty toNOand thebackgroundColortoclearColorto make the window transparent. The logic is straightforward and there are no apparent issues.
183-187: LGTM!The function correctly retrieves the window delegate and sets the
invisibleTitleBarHeightproperty to the providedheightvalue. The logic is straightforward and there are no apparent issues.
190-195: LGTM!The function correctly converts the provided
titlestring to anNSStringand sets it as the window's title. It also frees thetitlestring after setting the window title to avoid memory leaks. The logic is straightforward and there are no apparent issues.
197-204: LGTM!The function correctly calculates the content size based on the provided
widthandheightvalues. It then sets the window'scontentSizeandframeproperties to resize the window with animation. The logic is straightforward and there are no apparent issues.
206-210: LGTM!The function correctly sets the window's
levelproperty based on the value ofalwaysOnTopparameter. IfalwaysOnTopis true, it sets the window to be always on top by usingNSFloatingWindowLevel. IfalwaysOnTopis false, it sets the window to be a normal window by usingNSNormalWindowLevel. The logic is straightforward and there are no apparent issues.
212-219: LGTM!These small functions correctly set the window's
levelproperty to various predefined window level constants. Each function is straightforward and sets the window level to a specific constant such asNSNormalWindowLevel,NSFloatingWindowLevel,NSPopUpMenuWindowLevel,NSMainMenuWindowLevel,NSStatusWindowLevel,NSModalPanelWindowLevel,NSScreenSaverWindowLevel, orNSTornOffMenuWindowLevel. There are no apparent issues with these functions.
221-229: LGTM!The function correctly loads a URL in the window's webview. It converts the provided
urlstring to anNSURLobject and creates anNSURLRequestobject with the URL. It then loads the URL request in the window's webview using theloadRequestmethod. The function also frees theurlstring after loading the URL to avoid memory leaks. The logic is straightforward and there are no apparent issues.
231-242: LGTM!The function correctly sets whether the window is resizable or not. It retrieves the window's current style mask and modifies it based on the value of the
resizableparameter. Ifresizableis true, it adds theNSWindowStyleMaskResizableflag to the style mask. Ifresizableis false, it removes theNSWindowStyleMaskResizableflag from the style mask. It then sets the window's style mask to the updated style mask. The logic is straightforward and there are no apparent issues.v3/pkg/application/webview_window.go (1)
260-260: Ensure All References toprocessKeyBindingOptionsAre UpdatedThe function
processKeyBindingOptionshas been renamed towindowProcessKeyBindingOptions. Please ensure that all occurrences of the old function name have been updated throughout the codebase to prevent any undefined function errors.Run the following script to verify no references to the old function name remain:
Verification successful
To gather more information and clarify the situation, I'll run additional scripts to investigate the
windowProcessKeyBindingOptionsfunction and the context of otherprocessKeyBindingOptionsoccurrences.
Renaming of
processKeyBindingOptionsis Correct and IntentionalThe renaming of
processKeyBindingOptionstowindowProcessKeyBindingOptionsinv3/pkg/application/webview_window.gois correct and intentional. This change makes the function specific to WebviewWindow type.Other similarly named functions in the codebase serve different purposes:
processKeyBindingOptionsForPanelinwebview_panel_options.gofor WebviewPanel typeprocessKeyBindingOptionsinapplication.gofor general Window typeThese functions should not be renamed as they are intentionally different for type-specific key binding processing.
The original review comment's recommendation to update all occurrences was overly broad. The current implementation maintains clear separation of concerns for different window types.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any occurrences of `processKeyBindingOptions` in Go files. # Test: Find references to `processKeyBindingOptions`. Expect: No occurrences. rg --type go 'processKeyBindingOptions'Length of output: 687
Script:
#!/bin/bash # Search for windowProcessKeyBindingOptions echo "Searching for windowProcessKeyBindingOptions:" rg --type go 'windowProcessKeyBindingOptions' # Show context for processKeyBindingOptions occurrences echo -e "\nContext for processKeyBindingOptions occurrences:" rg --type go -C 5 'processKeyBindingOptions'Length of output: 3458
|
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- v3/pkg/application/application.go (12 hunks)
- v3/pkg/application/webview_panel.go (1 hunks)
- v3/pkg/application/webview_panel_darwin.go (1 hunks)
- v3/pkg/application/webview_panel_options.go (1 hunks)
- v3/pkg/application/webview_responder_darwin.h (1 hunks)
- v3/pkg/application/webview_responder_darwin.m (1 hunks)
- v3/pkg/application/webview_window.go (4 hunks)
- v3/pkg/application/webview_window_bindings_darwin.h (1 hunks)
- v3/pkg/application/webview_window_bindings_darwin.m (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- v3/pkg/application/application.go
- v3/pkg/application/webview_panel.go
- v3/pkg/application/webview_panel_darwin.go
- v3/pkg/application/webview_panel_options.go
- v3/pkg/application/webview_responder_darwin.h
- v3/pkg/application/webview_responder_darwin.m
- v3/pkg/application/webview_window_bindings_darwin.h
Additional comments not posted (12)
v3/pkg/application/webview_window_bindings_darwin.m (10)
3-114: LGTM!The
createWindowfunction correctly handles the creation of both window and panel types based on the providedWindowTypeparameter. It sets up the window style, delegate, content view, and embeds a properly configured WKWebView. The function also enables drag and drop if requested.
116-157: LGTM!The
printWindowStylefunction is a helpful utility for debugging purposes. It retrieves the window style mask and delegate, and then prints the corresponding string representation for each style mask flag. This function does not affect the functionality of the window.
161-167: LGTM!The
setInvisibleTitleBarHeightfunction correctly sets the invisible title bar height for the window by retrieving the window and its delegate, and then setting theinvisibleTitleBarHeightproperty of the delegate to the provided height value. This function is useful for customizing the appearance of the window's title bar.
170-173: LGTM!The
windowSetTransparentfunction correctly makes the window backdrop transparent by setting the window'sopaqueproperty toNOand its background color toclearColor. This function is useful for creating transparent windows.
189-195: LGTM!The
windowSetSizefunction correctly sets the size of the window by calculating the content size based on the provided width and height, and then setting the window's content size and frame using the calculated content size. The function also animates the window resizing, providing a smooth visual experience.
198-201: LGTM!The
windowSetAlwaysOnTopfunction correctly sets the window to be always on top by setting its level toNSFloatingWindowLevelifalwaysOnTopis true, orNSNormalWindowLevelotherwise. This function is useful for controlling the window's behavior in relation to other windows.
203-210: LGTM!The functions
setNormalWindowLevel,setFloatingWindowLevel,setPopUpMenuWindowLevel,setMainMenuWindowLevel,setStatusWindowLevel,setModalPanelWindowLevel,setScreenSaverWindowLevel, andsetTornOffMenuWindowLevelcorrectly set the window level to different predefined levels using thesetLevelmethod. These functions provide a convenient way to control the window's behavior and appearance in relation to other windows.
223-233: LGTM!The
windowSetResizablefunction correctly sets whether the window is resizable by retrieving the window's current style mask and adding or removing theNSWindowStyleMaskResizableflag based on theresizableparameter. The function then sets the updated style mask on the window, effectively controlling its resizability.
236-243: LGTM!The
windowSetMinSizefunction correctly sets the minimum size of the window by calculating the content size based on the provided width and height, and then setting the window's minimum content size using thesetContentMinSizemethod. The function also sets the window's minimum size using thesetMinSizemethod, ensuring that the window cannot be resized below the specified dimensions.
246-255: LGTM!The
windowSetMaxSizefunction correctly sets the maximum size of the window by initializing the maximum size toFLT_MAXfor both width and height, and updating the corresponding dimension if the provided width or height is greater than 0. The function then calculates the content size based on the maximum size and sets the window's maximum content size using thesetContentMaxSizemethod. It also sets the window's maximum size using thesetMaxSizemethod, ensuring that the window cannot be resized beyond the specified dimensions.v3/pkg/application/webview_window.go (2)
Line range hint
221-262: LGTM!The introduction of the
idvariable and its usage to setoptions.Nameand initialize theidfield is a good optimization that ensures consistency and reduces the chances of errors.The renaming of
processKeyBindingOptionstowindowProcessKeyBindingOptionsimproves clarity and follows a consistent naming convention. The function call withinNewWindowhas been updated correctly to reflect this change.
1234-1237: Condition update is correct.The change in the condition from
w.impl == nil && !w.isDestroyed()tow.impl == nil || w.isDestroyed()is correct and ensures that the function returns early without handling the key event if eitherw.implisnilor the window is destroyed.This prevents unexpected behavior that could have occurred with the previous condition, where the key event would be handled if the window was destroyed but
w.implwas notnil.
| void windowSetInvisibleTitleBar(void* window, unsigned int height) { | ||
| NSWindow* nsWindow = ((WebviewWindow*)window).w; | ||
| WebviewWindowDelegate* delegate = (WebviewWindowDelegate*)[nsWindow delegate]; | ||
| delegate.invisibleTitleBarHeight = height; | ||
| } |
There was a problem hiding this comment.
Remove the duplicate function.
The windowSetInvisibleTitleBar function is a duplicate of the setInvisibleTitleBarHeight function at line range 161-167. To maintain code clarity and avoid confusion, consider removing this duplicate function.
|
@leaanthony I think I have addressed every issue I could do alone, now the majority of the remaining points are all linked to code that I have neither added nor modified. I hope to have been helpful! |
| @@ -47,7 +47,12 @@ func NewCutMenuItem() *MenuItem { | |||
| SetAccelerator("CmdOrCtrl+x").OnClick(func(ctx *Context) { | |||
| currentWindow := globalApplication.CurrentWindow() | |||
| if currentWindow != nil { | |||
There was a problem hiding this comment.
We should add cut() to the Window interface. It can be done in a different PR
|
Regarding the |
|
Hi @nixpare - Sorry to ask this: could you please fix the current merge conflict with this PR? Thank you 🙏 |
|
@nixpare - do you still want this PR merged? |
leaanthony
added
the
awaiting feedback
Fixed task update.
empty name guard
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…alpha-feature/mac-panel # Conflicts: # v3/pkg/application/webview_window_darwin.go
…/mac-panel # Conflicts: # v3/go.mod # v3/go.sum
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
v3/pkg/application/webview_panel_options.go (3)
6-7: Fix grammatical error in commentThe comment for the Floating field has a grammatical error.
- // Floating will make the panel float above other application in every workspace. + // Floating will make the panel float above other applications in every workspace.
20-20: Add function documentationThe function lacks documentation explaining its purpose, parameters, and return value.
+// processKeyBindingOptionsForPanel merges window and panel-specific key bindings into a single map. +// Panel-specific bindings take precedence over window bindings when keys conflict. +// Parameters: +// - keyBindings: Panel-specific key bindings +// - windowKeyBindings: Base window key bindings +// Returns: A merged map of key bindings for the panel func processKeyBindingOptionsForPanel(keyBindings map[string]func(panel *WebviewPanel), windowKeyBindings map[string]func(panel *WebviewWindow)) map[string]func(panel *WebviewPanel) {
23-44: Refactor duplicate key binding processing logicThe key binding processing logic is duplicated between window and panel bindings. Consider extracting the common validation and logging into a helper function.
+func processBinding(key string, result map[string]func(panel *WebviewPanel), callback func(panel *WebviewPanel)) { + acc, err := parseAccelerator(key) + if err != nil { + globalApplication.error("Invalid keybinding: %s", err.Error()) + return + } + result[acc.String()] = callback + globalApplication.debug("Added Keybinding", "accelerator", acc.String()) +} + func processKeyBindingOptionsForPanel(keyBindings map[string]func(panel *WebviewPanel), windowKeyBindings map[string]func(panel *WebviewWindow)) map[string]func(panel *WebviewPanel) { result := make(map[string]func(panel *WebviewPanel)) for key, callback := range windowKeyBindings { - acc, err := parseAccelerator(key) - if err != nil { - globalApplication.error("Invalid keybinding: %s", err.Error()) - continue - } - result[acc.String()] = func(panel *WebviewPanel) { + processBinding(key, result, func(panel *WebviewPanel) { callback(panel.WebviewWindow) - } - globalApplication.debug("Added Keybinding", "accelerator", acc.String()) + }) } for key, callback := range keyBindings { - // Parse the key to an accelerator - acc, err := parseAccelerator(key) - if err != nil { - globalApplication.error("Invalid keybinding: %s", err.Error()) - continue - } - result[acc.String()] = callback - globalApplication.debug("Added Keybinding", "accelerator", acc.String()) + processBinding(key, result, callback) } return result }v3/pkg/application/webview_panel.go (2)
3-7: Add interface documentation.Please add documentation for the
webviewPanelImplinterface describing its purpose, responsibilities, and the relationship withwebviewWindowImpl. This will help maintainers understand the interface contract.
9-16: Document thread safety guarantees.The struct contains a map for key bindings which could be accessed concurrently. While there are locks used in the methods, the struct documentation should explicitly state the thread safety guarantees.
Add documentation like:
+// WebviewPanel represents a panel window with additional floating capabilities. +// Thread safety: Access to keyBindings is protected by keyBindingsLock. type WebviewPanel struct {v3/pkg/application/application.go (3)
175-188: LGTM! Consider enhancing error handling.The key binding processing logic is well-structured and thread-safe. However, it might be beneficial to collect and report all invalid key bindings at once rather than logging them individually.
Consider this approach:
func processKeyBindingOptions(keyBindings map[string]func(window Window)) map[string]func(window Window) { result := make(map[string]func(window Window)) + var invalidBindings []string for key, callback := range keyBindings { acc, err := parseAccelerator(key) if err != nil { - globalApplication.error("Invalid keybinding: %s", err.Error()) + invalidBindings = append(invalidBindings, fmt.Sprintf("%s: %s", key, err.Error())) continue } result[acc.String()] = callback globalApplication.debug("Added Keybinding", "accelerator", acc.String()) } + if len(invalidBindings) > 0 { + globalApplication.error("Invalid keybindings found:\n%s", strings.Join(invalidBindings, "\n")) + } return result }
559-564: Consider adding documentation for panel behavior differences.While the implementation is solid, it would be beneficial to add documentation explaining:
- Platform-specific behavior differences for panels
- Usage guidelines for choosing between windows and panels
- Limitations on Windows and Linux platforms
Breaking changes detected in type assertions and method calls
The verification reveals multiple locations where the code explicitly relies on
*WebviewWindowtype assertions and direct method calls that could break with the interface change:
- Type assertions found in multiple files:
v3/pkg/application/dialogs.gov3/pkg/application/dialogs_linux.gov3/pkg/application/linux_purego.gov3/pkg/application/messageprocessor_dialog.gov3/pkg/application/linux_cgo.go- Direct method calls in:
v3/pkg/application/roles.go:SetURLmethodv3/pkg/application/messageprocessor_screens.go:GetScreenmethodv3/examples/menu/main.go:ResizableandSetResizablemethods🔗 Analysis chain
Line range hint
772-783: Verify impact of return type change.The change in
CurrentWindow()'s return type from*WebviewWindowtoWindowinterface is a breaking change that could affect existing code that expects a concreteWebviewWindowtype.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential type assertions or direct WebviewWindow method calls that might break # Search for type assertions rg -A 2 '\.\(\*WebviewWindow\)' # Search for direct method calls on CurrentWindow() rg -A 2 'CurrentWindow\(\)\.'Length of output: 3026
v3/pkg/application/linux_cgo.go (1)
713-714: Good memory management, but remove extra blank lineThe deferred cleanup is good practice, but there's an unnecessary blank line that should be removed for consistency.
func menuItemSetToolTip(widget pointer, tooltip string) { value := C.CString(tooltip) defer C.free(unsafe.Pointer(value)) - C.gtk_widget_set_tooltip_text(v3/pkg/application/webview_panel_darwin.go (1)
77-77: Improve error handling inhandleKeyEventWhile logging the error is helpful, consider returning the error or handling it appropriately to provide better feedback to the caller.
v3/pkg/application/webview_window_darwin.go (1)
492-495: Consistent Use ofdeferfor Memory ManagementIn the
setupmethod, the C stringcCSSis freed immediately after use instead of usingdefer. For consistency and to ensure the C string is freed even if an error occurs, consider usingdefer C.free(...)as in other parts of the code.Apply the following diff to use
defer:cCSS := C.CString(options.CSS) -C.windowInjectCSS(w.nsWindow, cCSS) -C.free(unsafe.Pointer(cCSS)) +defer C.free(unsafe.Pointer(cCSS)) C.windowInjectCSS(w.nsWindow, cCSS)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
v3/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
v3/examples/window/main.go(8 hunks)v3/go.mod(2 hunks)v3/internal/commands/build-assets.go(1 hunks)v3/internal/commands/tool_package_test.go(3 hunks)v3/pkg/application/application.go(12 hunks)v3/pkg/application/application_options.go(1 hunks)v3/pkg/application/keys_linux.go(1 hunks)v3/pkg/application/linux_cgo.go(11 hunks)v3/pkg/application/messageprocessor.go(1 hunks)v3/pkg/application/screen_darwin.go(1 hunks)v3/pkg/application/webview_panel.go(1 hunks)v3/pkg/application/webview_panel_darwin.go(1 hunks)v3/pkg/application/webview_panel_options.go(1 hunks)v3/pkg/application/webview_window.go(4 hunks)v3/pkg/application/webview_window_darwin.go(7 hunks)v3/pkg/application/window.go(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- v3/pkg/application/keys_linux.go
- v3/internal/commands/tool_package_test.go
- v3/pkg/application/messageprocessor.go
🚧 Files skipped from review as they are similar to previous changes (3)
- v3/pkg/application/screen_darwin.go
- v3/pkg/application/webview_window.go
- v3/examples/window/main.go
🧰 Additional context used
📓 Learnings (3)
v3/pkg/application/webview_panel_options.go (2)
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/examples/keybindings/main.go:16-17
Timestamp: 2024-11-12T12:40:39.204Z
Learning: In the codebase, `application.Options.KeyBindings` uses the `application.Window` type, whereas `application.WebviewWindowOptions.KeyBindings` uses `*application.WebviewWindow`. This is intentional and acceptable.
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/pkg/application/application_options.go:93-93
Timestamp: 2024-11-12T12:40:39.204Z
Learning: The `KeyBindings` property of `application.Option` uses the `application.Window` type, while `application.WebviewWindowOptions` still accepts `*application.WebviewWindow`, and this is intentional.
v3/pkg/application/webview_panel_darwin.go (2)
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/pkg/application/webview_panel_darwin.go:88-89
Timestamp: 2024-11-12T12:40:39.204Z
Learning: Safety checks for `p.nsPanel` are performed in the `SetFloating` method of `WebviewPanel`, following the `WebviewWindow` and `macosWebviewWindow` implementations and code style.
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/pkg/application/webview_panel_darwin.go:63-69
Timestamp: 2024-11-12T12:40:39.204Z
Learning: The function `panelNew` is defined in `webview_window_bindings_darwin.h` and is properly included in the code.
v3/pkg/application/application_options.go (2)
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/examples/keybindings/main.go:16-17
Timestamp: 2024-11-12T12:40:39.204Z
Learning: In the codebase, `application.Options.KeyBindings` uses the `application.Window` type, whereas `application.WebviewWindowOptions.KeyBindings` uses `*application.WebviewWindow`. This is intentional and acceptable.
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/pkg/application/application_options.go:93-93
Timestamp: 2024-11-12T12:40:39.204Z
Learning: The `KeyBindings` property of `application.Option` uses the `application.Window` type, while `application.WebviewWindowOptions` still accepts `*application.WebviewWindow`, and this is intentional.
🔇 Additional comments (30)
v3/pkg/application/webview_panel_options.go (2)
3-18: Well-designed struct with clear documentation
The WebviewPanelOptions struct is well-designed with:
- Clear separation of concerns through embedding
- Proper documentation explaining merge behavior
- Type-safe callback signatures
1-46: Consider platform-specific behavior handling
According to the PR description, WebviewPanel should only be fully functional on MacOS, with potential panics on Windows. Consider adding platform-specific checks or documentation to prevent misuse on unsupported platforms.
Let's check if there are any platform-specific guards in related files:
v3/pkg/application/window.go (3)
15-16: Clarify the relationship between Bounds and existing position/size methods
The new Bounds() and SetBounds() methods seem to overlap with existing Position(), Size(), and their variants. Please clarify:
- How does
Rectdiffer from using separate position and size? - When should developers use
Boundsvs the existing methods? - Is
Rectdefined in another file? If so, please document the import.
Consider:
- Adding documentation comments explaining the purpose and usage
- Including examples of when to use each method
- Providing migration guidelines for existing implementations
27-27: Document Flash behavior across platforms
The Flash method needs documentation about its platform-specific behavior, especially since:
- Windows panics with WebviewPanel
- Linux hasn't been tested
- It's primarily for MacOS Panel support
Consider:
- Adding documentation comments explaining platform-specific behavior
- Including graceful fallbacks for unsupported platforms
- Providing usage examples in the documentation
Line range hint 15-60: Breaking Changes: Provide migration guide
These interface additions constitute breaking changes that require all implementers to add new methods. Please:
- Document the breaking changes in CHANGELOG.md
- Provide a migration guide for existing implementations
- Consider adding default implementations where possible
Consider:
- Creating an abstract base implementation with reasonable defaults
- Providing a migration timeline
- Adding deprecation notices if any methods are being replaced
v3/pkg/application/webview_panel.go (1)
47-56: Add state transition validation.
The SetFloating method should validate if the state transition is allowed in the current panel lifecycle stage. For example, it might not be valid to change floating state while the panel is being destroyed.
Let's verify the current usage:
v3/go.mod (2)
36-36: Verify MacOS system calls support
The golang.org/x/sys package is now a direct dependency, which aligns with implementing MacOS panel support. However, let's verify that the version v0.27.0 includes all necessary MacOS system calls.
✅ Verification successful
Let me gather more information about the MacOS panel support by searching for panel-related code in the codebase.
Based on the search results, I can now provide a definitive response about the MacOS panel support and the golang.org/x/sys dependency.
The golang.org/x/sys v0.27.0 dependency is appropriate for MacOS panel support
The codebase shows comprehensive MacOS panel implementation that relies on system-level functionality:
- Native NSPanel integration in
v3/pkg/application/webview_panel_darwin.go - Window level management through
setWindowLevelfunctions - Panel-specific features like floating panels and window stacking
The golang.org/x/sys v0.27.0 package is correctly positioned as a direct dependency since it provides the necessary low-level system interfaces for:
- Window/panel management
- System-level event handling
- OS-specific functionality required by the MacOS panel implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check golang.org/x/sys for MacOS panel-related functionality
# Search for MacOS-specific panel/window management code
rg -i "darwin.*panel|darwin.*window" $(fd -t f . $(go env GOPATH)/pkg/mod/golang.org/x/[email protected])
Length of output: 22509
Script:
#!/bin/bash
# Search for panel-related code in the codebase
rg -i "panel|window.*level" --type go
Length of output: 21946
17-17: Clarify the addition of nfpm package
The addition of github.com/goreleaser/nfpm/v2 seems unrelated to the MacOS panel feature. This package is typically used for building Linux packages (deb, rpm). Could you clarify the reason for adding this dependency in this PR?
v3/internal/commands/build-assets.go (2)
130-133: LGTM! The formatting changes improve readability.
The consistent indentation in the WailsConfig struct's Info fields improves code readability while maintaining the same functionality.
130-133: Consider adding panel-specific configuration fields.
Given that this PR introduces MacOS Panel support, consider whether panel-specific configuration fields (e.g., panel behavior, default position) should be added to the WailsConfig struct.
Let's check if any panel-related configuration is defined elsewhere:
If no panel configuration exists yet, consider adding fields like:
defaultPanelPositionpanelBehaviorpanelSize
to support customization of the new panel functionality.
v3/pkg/application/application_options.go (1)
92-95: Well-documented interface change!
The documentation clearly explains the transition to the Window interface and provides good guidance on maintaining type-specific control through window options or type switching.
v3/pkg/application/application.go (3)
554-578: LGTM! Well-structured window management refactoring.
The extraction of common window initialization logic into addNewWindow is a great improvement that:
- Reduces code duplication
- Ensures consistent window lifecycle management
- Maintains thread safety
Line range hint 988-1008: LGTM! Thread-safe key binding implementation.
The key binding implementation is well-designed with:
- Proper use of RWMutex for concurrent access optimization
- Clean separation of concerns between processing and management
- Appropriate goroutine usage for callback execution
721-721: Update error messages to use generic Window type.
The error messages should use the generic "Window" term instead of specific window types to maintain consistency with the updated architecture.
Also applies to: 734-734, 757-757, 1026-1026
v3/pkg/application/linux_cgo.go (6)
485-485: Good memory management practice!
The deferred cleanup of the C string ensures proper memory management and prevents memory leaks.
665-665: Good memory management practice!
The deferred cleanup of the C string ensures proper memory management when setting menu item labels.
1192-1192: Good memory management practice!
The deferred cleanup of the C string ensures proper memory management when setting window titles.
1219-1219: Good memory management practice!
The deferred cleanup of the C string ensures proper memory management when setting window URLs.
1376-1376: Good memory management practice!
The deferred cleanup of the C string ensures proper memory management during window reloads.
1625-1631: Good memory management practice!
The deferred cleanup of C strings is properly placed immediately after allocation, ensuring proper memory management for file chooser dialogs.
v3/pkg/application/webview_panel_darwin.go (3)
31-85: Unit tests are missing for macosWebviewPanel
No unit tests are provided for the new macosWebviewPanel implementation. Adding tests will help ensure that the panel behaves as expected and will prevent future regressions.
51-53: Potential issue with event listener iteration
When iterating over p.parent.eventListeners, consider whether you should be iterating over the values or the keys. Currently, you're iterating over the keys:
for eventId := range p.parent.eventListeners {
p.on(eventId)
}If eventListeners is a map and you need both keys and values, consider using:
for eventId, listener := range p.parent.eventListeners {
// Use eventId and listener as needed
}83-85:
Consider adding a nil check for p.nsPanel before calling C.panelSetFloating
Accessing p.nsPanel without ensuring it's not nil may lead to runtime errors if setFloating is called before the panel is initialized. Adding a nil check can prevent a potential nil pointer dereference.
Apply this diff to add a nil check:
func (p *macosWebviewPanel) setFloating(floating bool) {
+ if p.nsPanel == nil {
+ return
+ }
C.panelSetFloating(p.nsPanel, C.bool(floating))
}⛔ Skipped due to learnings
Learnt from: nixpare
PR: wailsapp/wails#3763
File: v3/pkg/application/webview_panel_darwin.go:88-89
Timestamp: 2024-11-12T12:40:39.204Z
Learning: Safety checks for `p.nsPanel` are performed in the `SetFloating` method of `WebviewPanel`, following the `WebviewWindow` and `macosWebviewWindow` implementations and code style.
v3/pkg/application/webview_window_darwin.go (7)
11-12: Consistent Window Creation Function
The windowNew function has been updated to call createWindow with the specified parameters, ensuring consistent window creation. This change enhances maintainability.
246-248: Proper Memory Management of C Strings
C strings are now correctly allocated and freed using C.CString and defer C.free(...) in functions such as execJS, setURL, setTitle, setup, and setHTML. This addresses previous memory leak concerns noted in earlier reviews.
Also applies to: 253-255, 275-275, 454-457, 600-600
399-401: Modularization of Window Setup Logic
The creation and initialization of window properties have been refactored into a new setup method. This improves code organization, readability, and maintainability by encapsulating the setup logic.
Also applies to: 403-530
419-425: Verify Backdrop Configuration Effects
The switch statement for macOptions.Backdrop configures the window's backdrop settings. Ensure that the functions windowSetTransparent, webviewSetTransparent, and windowSetTranslucent are applied correctly and that the visual effects match the intended design on macOS.
Test the backdrop settings by running the application with different Backdrop options and observing the window's appearance to confirm the settings are applied as expected.
492-495: Potential Issues with Shadow and Always-On-Top Settings
When the window is hidden initially and then shown later, the code sets up a callback to adjust the shadow and always-on-top properties upon the WindowDidBecomeKey event. Ensure that this callback is correctly invoked and that the properties are set as intended, even if the window does not become key immediately.
Test this scenario by launching the application with the window hidden (options.Hidden is true) and verify that the shadow and always-on-top settings are correctly applied when the window is shown.
Also applies to: 503-507
515-526: Event Handling for Window Focus and Resize
Event listeners for window focus loss (WindowDidResignKey, WindowDidResignMain) and resize (WindowDidResize) have been added. Confirm that these events are correctly emitted and handled, and that any associated functionality (like UI updates) responds appropriately.
201-201: Confirm Correct Function Name Usage
The function windowIsFullScreen is called to check if the window is in fullscreen mode. Ensure that this function exists in the C bindings and that the naming is consistent across all uses.
Run the following script to check for the correct function name usage:
✅ Verification successful
Function windowIsFullScreen is correctly defined and used
The verification confirms that the function windowIsFullScreen is properly defined in the C bindings file (webview_window_bindings_darwin.h) with the exact same name and case sensitivity as it's being used in the Go code. The function signature matches the expected usage for checking window fullscreen state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'windowIsFullScreen' is consistently used.
# Test: Search for both 'windowIsFullScreen' and 'windowIsFullscreen' in the C binding files.
# Expect: Only 'windowIsFullScreen' should be present.
rg 'windowIsFullScreen|windowIsFullscreen' v3/pkg/application/webview_window_bindings_darwin.h
Length of output: 136
| RegisterContextMenu(name string, menu *Menu) | ||
| RelativePosition() (int, int) | ||
| Reload() | ||
| Resizable() bool | ||
| Restore() | ||
| Run() | ||
| SetEnabled(enabled bool) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now generate the final response:
Document SetEnabled impact and focus management
The SetEnabled method requires documentation as it has significant implications:
- It's implemented across multiple platforms (Darwin/macOS, Windows, Linux)
- It interacts with the window's focus management system
- It affects the window's interaction state
Key areas to document:
- The method directly controls the window's enabled state through platform-specific implementations
- It has a relationship with focus management (
Focus(),IsFocused()) - It should address high-contrast and accessibility modes supported by the application
🔗 Analysis chain
Document SetEnabled impact and accessibility considerations
The SetEnabled method needs documentation about:
- Impact on window interaction
- Relationship with focus management
- Accessibility implications
Consider:
- Adding documentation comments explaining the enabled/disabled states
- Including accessibility guidelines
- Providing examples of proper state management
Let me gather more context about the window implementation and any existing documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SetEnabled implementation and usage
rg -g '**/*window*.go' 'SetEnabled.*bool' -A 5
Length of output: 1221
Script:
#!/bin/bash
# Look for any existing documentation or comments about window enabling/disabling
rg -g '**/*window*.go' -B 5 'SetEnabled'
# Check for any accessibility-related code or comments
rg -g '**/*window*.go' -i 'accessibility|aria|a11y'
# Look for focus management related code
rg -g '**/*window*.go' 'focus|Focus'
Length of output: 7724
| @@ -47,12 +50,14 @@ type Window interface { | |||
| OnWindowEvent(eventType events.WindowEventType, callback func(event *WindowEvent)) func() | |||
| OpenContextMenu(data *ContextMenuData) | |||
| Position() (int, int) | |||
| Print() error | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the implementation across different platforms, I can now provide a final response:
Document platform-specific Print behavior and error cases
The Print() method implementation varies by platform:
- Windows/Linux: Invokes browser's
window.print()JavaScript function - macOS: Uses native
windowPrintCocoa API
Error cases:
- Returns nil if window is destroyed
- Delegates actual printing errors to the platform's native print dialog
🔗 Analysis chain
Document Print error cases and platform support
The Print() method returns an error but lacks documentation about:
- Possible error cases
- Platform-specific behavior
- Required window state for printing
Consider:
- Adding documentation comments explaining error conditions
- Including platform-specific limitations
- Providing examples of proper error handling
Let me check the implementation in more detail to understand the error handling and platform-specific behavior.
Let me check one more thing to see if there are any related tests or additional documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Print implementation and error handling
rg -g '**/*window*.go' 'Print\(\).*error' -A 5
Length of output: 718
Script:
#!/bin/bash
# Search for print implementation in platform-specific files
rg -g '**/*.go' 'print\(\).*error' -A 5
# Look for any documentation or comments about printing
rg -g '**/*.go' -B 3 'print\(\).*error'
Length of output: 2766
Script:
#!/bin/bash
# Look for test files and documentation
rg -g '**/*window*.go' -g '**/*print*.go' -g '**/README*' -i 'print.*window' -B 2 -A 2
# Check if there are any C header files for macOS implementation
fd -g '*.h' -x cat {} \;
Length of output: 38101
| func (p *WebviewPanel) processKeyBinding(acceleratorString string) bool { | ||
| // Check menu bindings | ||
| if p.menuBindings != nil { | ||
| p.menuBindingsLock.RLock() | ||
| defer p.menuBindingsLock.RUnlock() | ||
| if menuItem := p.menuBindings[acceleratorString]; menuItem != nil { | ||
| menuItem.handleClick() | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Check key bindings | ||
| if p.keyBindings != nil { | ||
| p.keyBindingsLock.RLock() | ||
| defer p.keyBindingsLock.RUnlock() | ||
| if callback := p.keyBindings[acceleratorString]; callback != nil { | ||
| // Execute callback | ||
| go callback(p) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return globalApplication.processKeyBinding(acceleratorString, p.WebviewWindow) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve goroutine management and error handling consistency.
The key event handling has several areas for improvement:
- Launching unbounded goroutines for callbacks could lead to resource exhaustion
- Inconsistent error handling between menu and key callbacks
- No timeout handling for callbacks
Consider using a worker pool for callbacks and consistent error handling:
func (p *WebviewPanel) processKeyBinding(acceleratorString string) bool {
// Check menu bindings
if p.menuBindings != nil {
p.menuBindingsLock.RLock()
defer p.menuBindingsLock.RUnlock()
if menuItem := p.menuBindings[acceleratorString]; menuItem != nil {
- menuItem.handleClick()
+ if err := menuItem.handleClick(); err != nil {
+ log.Printf("menu binding handler failed: %v", err)
+ }
return true
}
}
// Check key bindings
if p.keyBindings != nil {
p.keyBindingsLock.RLock()
defer p.keyBindingsLock.RUnlock()
if callback := p.keyBindings[acceleratorString]; callback != nil {
- // Execute callback
- go callback(p)
+ // Execute callback with timeout
+ select {
+ case p.callbackWorkerPool <- struct{}{}:
+ go func() {
+ defer func() { <-p.callbackWorkerPool }()
+ callback(p)
+ }()
+ case <-time.After(callbackTimeout):
+ log.Printf("callback worker pool full, dropping key event: %s", acceleratorString)
+ }
return true
}
}Committable suggestion skipped: line range outside the PR's diff.
| func (p *WebviewPanel) Run() { | ||
| if p.impl != nil { | ||
| return | ||
| } | ||
|
|
||
| p.impl = newPanelImpl(p) | ||
| p.WebviewWindow.impl = p.impl.getWebviewWindowImpl() | ||
|
|
||
| InvokeSync(p.impl.run) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and documentation.
The Run method needs documentation explaining when it should be called in the panel lifecycle. Additionally, consider handling errors from impl.run.
Consider:
+// Run initializes and starts the panel. It should be called after all panel
+// configurations are set. This method is idempotent - subsequent calls will
+// have no effect.
func (p *WebviewPanel) Run() {
if p.impl != nil {
return
}
p.impl = newPanelImpl(p)
p.WebviewWindow.impl = p.impl.getWebviewWindowImpl()
- InvokeSync(p.impl.run)
+ var err error
+ InvokeSync(func() {
+ err = p.impl.run()
+ })
+ if err != nil {
+ log.Printf("failed to run panel: %v", err)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| package application | ||
|
|
||
| type webviewPanelImpl interface { | ||
| webviewWindowImpl | ||
| getWebviewWindowImpl() webviewWindowImpl | ||
| setFloating(floating bool) | ||
| } | ||
|
|
||
| type WebviewPanel struct { | ||
| *WebviewWindow | ||
|
|
||
| options WebviewPanelOptions | ||
| impl webviewPanelImpl | ||
| // keyBindings holds the keybindings for the panel | ||
| keyBindings map[string]func(*WebviewPanel) | ||
| } | ||
|
|
||
| // NewPanel creates a new panel with the given options | ||
| func NewPanel(options WebviewPanelOptions) *WebviewPanel { | ||
| window := NewWindow(options.WebviewWindowOptions) | ||
| options.WebviewWindowOptions = window.options | ||
|
|
||
| result := &WebviewPanel{ | ||
| WebviewWindow: window, | ||
| options: options, | ||
| } | ||
|
|
||
| // Process keybindings specific to the WebviewPanel | ||
| if result.options.KeyBindings != nil || result.options.WebviewWindowOptions.KeyBindings != nil { | ||
| result.keyBindings = processKeyBindingOptionsForPanel(result.options.KeyBindings, result.options.WebviewWindowOptions.KeyBindings) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| func (p *WebviewPanel) Run() { | ||
| if p.impl != nil { | ||
| return | ||
| } | ||
|
|
||
| p.impl = newPanelImpl(p) | ||
| p.WebviewWindow.impl = p.impl.getWebviewWindowImpl() | ||
|
|
||
| InvokeSync(p.impl.run) | ||
| } | ||
|
|
||
| // SetFloating makes the panel float above other application in every workspace. | ||
| func (p *WebviewPanel) SetFloating(b bool) Window { | ||
| p.options.Floating = b | ||
| if p.impl != nil { | ||
| InvokeSync(func() { | ||
| p.impl.setFloating(b) | ||
| }) | ||
| } | ||
| return p | ||
| } | ||
|
|
||
| func (p *WebviewPanel) HandleKeyEvent(acceleratorString string) { | ||
| if p.impl == nil || p.isDestroyed() { | ||
| return | ||
| } | ||
| InvokeSync(func() { | ||
| p.impl.handleKeyEvent(acceleratorString) | ||
| }) | ||
| } | ||
|
|
||
| func (p *WebviewPanel) processKeyBinding(acceleratorString string) bool { | ||
| // Check menu bindings | ||
| if p.menuBindings != nil { | ||
| p.menuBindingsLock.RLock() | ||
| defer p.menuBindingsLock.RUnlock() | ||
| if menuItem := p.menuBindings[acceleratorString]; menuItem != nil { | ||
| menuItem.handleClick() | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| // Check key bindings | ||
| if p.keyBindings != nil { | ||
| p.keyBindingsLock.RLock() | ||
| defer p.keyBindingsLock.RUnlock() | ||
| if callback := p.keyBindings[acceleratorString]; callback != nil { | ||
| // Execute callback | ||
| go callback(p) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return globalApplication.processKeyBinding(acceleratorString, p.WebviewWindow) | ||
| } |
There was a problem hiding this comment.
Add platform-specific handling.
According to the PR objectives, this implementation is specifically for MacOS and currently panics on Windows. The code should include proper platform-specific handling to prevent panics and provide meaningful error messages on unsupported platforms.
Consider:
- Add build tags to restrict this file to MacOS
- Provide stub implementations for other platforms with appropriate error messages
- Add platform-specific documentation
Example:
+// +build darwin
+
package application
+
+// WebviewPanel is currently only supported on MacOS.
+// Attempting to use this on other platforms will result in a runtime error.Committable suggestion skipped: line range outside the PR's diff.
| // interface, so if you need more granular control over the window (e.g. [*WebviewWindow]), it's possible | ||
| // to declare the key binding directly in the specific window options or switch over | ||
| // the types that implement the [Window] interface | ||
| KeyBindings map[string]func(window Window) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Breaking change verified: KeyBindings type change requires updates
The change from *WebviewWindow to Window interface in KeyBindings field is a breaking change that affects existing implementations. Found several examples that need updates:
examples/systray/main.go: Usesmap[string]func(window *application.WebviewWindow)examples/keybindings/main.go: Two instances usingmap[string]func(window *application.WebviewWindow)examples/events-bug/main.go: Two instances usingmap[string]func(window *application.WebviewWindow)
These examples need to be updated to use the new Window interface type in their key binding function signatures.
🔗 Analysis chain
Breaking change: KeyBindings now accepts Window interface
The change from *WebviewWindow to Window interface makes the API more flexible and enables support for both WebviewWindow and WebviewPanel. This is a breaking change that requires updates to existing key binding implementations.
Let's verify the impact on existing key binding implementations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing key binding implementations that might need updates
# Search for key binding function declarations
echo "Checking key binding function declarations..."
rg --type go -A 2 "func.*\(.*WebviewWindow\)"
# Search for direct KeyBindings usage
echo "Checking KeyBindings field usage..."
rg --type go -A 2 "KeyBindings.*map\[string\]func.*WebviewWindow"
Length of output: 100532
| var debounceTimer *time.Timer | ||
| var isDebouncing bool = false | ||
|
|
There was a problem hiding this comment.
Consider per-window debounce implementation
The current implementation uses global variables for debouncing, which could lead to race conditions in multi-window scenarios. Consider moving the debounce timer and state into the window struct for better isolation and thread safety.
-var debounceTimer *time.Timer
-var isDebouncing bool = false
type linuxWebviewWindow struct {
// ... existing fields ...
+ debounceTimer *time.Timer
+ isDebouncing bool
}Committable suggestion skipped: line range outside the PR's diff.
|
@nixpare - I had a little dig in to this today as there's been a few other things that have been merged in and I found a number of issues which I'm wondering if they were part of a bad merge. The If you could possibly have a little look over and see anything that appears out of the ordinary, then hopefully we can resolve it and get it merged. I've added a new menu option in the window example but I think a dedicated window panel example would be pretty cool. |
Description
Implementation of
WebviewPanel, a new component which extends the functionalities ofWebviewWindowadding the capability of creating windows able to behave like panels / overlays. This PR implements the new component for MacOS.Proposed on Issue #3760.
Type of change
How Has This Been Tested?
Created an example program to test if both
WebviewWindowandWebviewPanelare working as expected (https://github.com/nixpare/wails-mac-panel).WebviewWindow, panics (as expected) only when trying to useWebviewPanelTest Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Release Notes
New Features
WebviewPanelstruct for managing webview panels with customizable options.Windowinterface with new methods for flashing, printing, and enabling/disabling windows.Improvements
application.Windowtype, improving flexibility and future extensibility.Bug Fixes