[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
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.
| NSPanel *panel = (NSPanel *) window; | ||
|
|
There was a problem hiding this comment.
Type casting may lead to unexpected behavior
In the C function panelSetFloating, the casting of nsPanel could lead to issues:
NSWindow *window = ((WebviewWindow*)nsPanel).w;
NSPanel *panel = (NSPanel *) window;Make sure that nsPanel is correctly cast from void* to WebviewWindow* and that accessing the w member is valid. Incorrect casting can lead to undefined behavior.
| 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.
| void windowSetTitle(void* window, char* title) { | ||
| NSString* nsTitle = [NSString stringWithUTF8String:title]; | ||
| [((WebviewWindow*)window).w setTitle:nsTitle]; | ||
| free(title); | ||
| } |
There was a problem hiding this comment.
Avoid freeing title to prevent potential crashes.
The function frees the title parameter after setting the window title. If title was not dynamically allocated using malloc or a similar function, freeing it can lead to undefined behavior or crashes. Ensure that memory management is handled correctly. If title doesn't need to be freed here, remove the free(title); statement.
Apply this diff to fix the issue:
void windowSetTitle(void* window, char* title) {
NSString* nsTitle = [NSString stringWithUTF8String:title];
[((WebviewWindow*)window).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* window, char* title) { | |
| NSString* nsTitle = [NSString stringWithUTF8String:title]; | |
| [((WebviewWindow*)window).w setTitle:nsTitle]; | |
| free(title); | |
| } | |
| void windowSetTitle(void* window, char* title) { | |
| NSString* nsTitle = [NSString stringWithUTF8String:title]; | |
| [((WebviewWindow*)window).w setTitle:nsTitle]; | |
| } |
| void navigationLoadURL(void* window, char* url) { | ||
| // Load URL on main thread | ||
| NSURL* nsURL = [NSURL URLWithString:[NSString stringWithUTF8String:url]]; | ||
| NSURLRequest* request = [NSURLRequest requestWithURL:nsURL]; | ||
| WebviewWindow *webviewWindow = (WebviewWindow*)window; | ||
| [webviewWindow.webView loadRequest:request]; | ||
| free(url); | ||
| } |
There was a problem hiding this comment.
Avoid freeing url to prevent potential crashes.
The function frees the url parameter after loading the request in the web view. If url was not dynamically allocated using malloc or a similar function, freeing it can lead to undefined behavior or crashes. Ensure that memory management is handled correctly. If url doesn't need to be freed here, remove the free(url); statement.
Apply this diff to fix the issue:
void navigationLoadURL(void* window, char* url) {
// Load URL on main thread
NSURL* nsURL = [NSURL URLWithString:[NSString stringWithUTF8String:url]];
NSURLRequest* request = [NSURLRequest requestWithURL:nsURL];
WebviewWindow *webviewWindow = (WebviewWindow*)window;
[webviewWindow.webView loadRequest:request];
- free(url);
}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 navigationLoadURL(void* window, char* url) { | |
| // Load URL on main thread | |
| NSURL* nsURL = [NSURL URLWithString:[NSString stringWithUTF8String:url]]; | |
| NSURLRequest* request = [NSURLRequest requestWithURL:nsURL]; | |
| WebviewWindow *webviewWindow = (WebviewWindow*)window; | |
| [webviewWindow.webView loadRequest:request]; | |
| free(url); | |
| } | |
| void navigationLoadURL(void* window, char* url) { | |
| // Load URL on main thread | |
| NSURL* nsURL = [NSURL URLWithString:[NSString stringWithUTF8String:url]]; | |
| NSURLRequest* request = [NSURLRequest requestWithURL:nsURL]; | |
| WebviewWindow *webviewWindow = (WebviewWindow*)window; | |
| [webviewWindow.webView loadRequest:request]; | |
| } |
|
@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 🙏 |
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