-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v3 feature] support for MacOS Panel #3763
base: v3-alpha
Are you sure you want to change the base?
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
WebviewResponder
interface 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 inhandleKeyEvent
When logging the error for an unparsed accelerator, include the problematic
acceleratorString
to 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 formacosWebviewPanel
structThe
macosWebviewPanel
struct lacks comments explaining its purpose and usage. Adding documentation will improve code maintainability and assist other developers.
37-49
: Document thenewPanelImpl
functionThe function
newPanelImpl
could 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+l
never reacheskeyDown
The TODO comment indicates that the
ctrl+l
key 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
ShouldClose
function, you're castingwindow *application.WebviewWindow
toapplication.Window
for use inlo.Contains
andlo.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
ShouldClose
function to acceptwindow application.Window
if 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.Appearance
At line 445,
macOptions.Appearance
is 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
, theMacBackdropNormal
case 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.Window
The variable
window
is 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.0
whenframeless
istrue
. To provide flexibility and accommodate different design requirements, consider making the corner radius a configurable parameter.
774-775
: Remove redundant call tomakeKeyWindow
to streamline window focusing.In the
windowFocus
function, both[nsWindow makeKeyAndOrderFront:nil];
and[nsWindow makeKeyWindow];
are called. SincemakeKeyAndOrderFront:
already makes the window key and brings it to the front, the subsequent call tomakeKeyWindow
may 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
#ifndef
and#define
directives 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.h
header, which is necessary for using Cocoa framework classes and types in the interface definition.
6-13
:WebviewResponder
interface is well-defined.
- The interface extends the
NSResponder
class, which is appropriate for handling events in a Cocoa application.- It declares a property
w
of 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.h
is necessary for theWebviewResponder
class.
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 initializeWebviewWindow
as 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
WebviewWindow
class now inherits fromNSObject
instead 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
w
andresponder
properties 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
WebviewWindow
inheritingNSWindow
to having anNSWindow
propertyw
has been implemented consistently throughout the codebase. This architectural change appears to be properly handled, with no obvious issues or inconsistencies found.However, the
responder
property 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:
WebviewWindow
now wraps anNSWindow
instance instead of subclassing it.- The
w
property of typeNSWindow
is widely used across the codebase.- No remaining direct inheritances from
NSWindow
forWebviewWindow
were found.- Initialization methods for
WebviewWindow
are present and consistent with the new structure.It's recommended to double-check the status of the
responder
property, 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 theWindow
type provides all the necessary functionality.The change from
*application.WebviewWindow
toapplication.Window
allows for more flexibility in the key binding function, as it can now accept any type that satisfies theWindow
interface or extends theWindow
base type.However, please ensure that the
Window
type 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
openDevTools
method to themacosWebviewPanel
struct is a positive change that enhances its functionality and aligns its behavior with themacosWebviewWindow
struct. The implementation correctly passes thensPanel
pointer to the C function.
63-65
: LGTM!The addition of the
enableDevTools
method to themacosWebviewPanel
struct is a positive change that enhances its functionality and aligns its behavior with themacosWebviewWindow
struct. The implementation correctly passes thensPanel
pointer to the C function.v3/pkg/application/webview_panel_options.go (3)
3-15
: LGTM!The
WebviewPanelOptions
struct is well-defined, with good reusability by embeddingWebviewWindowOptions
, and useful customization options through theFloating
,ShouldClose
, andKeyBindings
fields.
17-19
: LGTM!Providing default options through
WebviewPanelDefaults
is a good practice to ensure consistent behavior and reduce boilerplate. InitializingWebviewWindowOptions
withWebviewWindowDefaults
ensures that the default window options are used as a starting point.
21-47
: LGTM!The
processKeyBindingOptionsForPanel
function 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 theWebviewWindow
to aWebviewPanel
is 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
Flash
method is a valuable addition to theWindow
interface, 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
Window
interface, 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
SetEnabled
method is a valuable addition to theWindow
interface, 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
*WebviewWindow
toWindow
broadens the compatibility of the function with different window types. This change enables theprocessMenu
method to work with bothWebviewWindow
and the newWebviewPanel
component, which is the desired behavior as per the PR objectives.The change is unlikely to affect the existing functionality of
WebviewWindow
as it also implements theWindow
interface.v3/pkg/application/screen_darwin.go (1)
176-179
: LGTM!The new
getScreenForPanel
function 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
macosWebviewPanel
as an argument.- Calls the existing C function
getScreenForWindow
with thensPanel
property to retrieve the corresponding screen.- Converts the result to a
Screen
type using the existingcScreenToScreen
function.The function signature and usage of the C function are consistent with the existing
getScreenForWindow
function 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 theWindow
interface.The change in the function signature from
*WebviewWindow
toWindow
makes the function more flexible as it can now work with any type that implements theWindow
interface. This is a good change.However, this is a breaking change. Please verify that all the code that was passing a
*WebviewWindow
to this function has been updated to pass a type that implements theWindow
interface.Run the following script to verify the usage of the
addKeyBinding
function:Verification successful
Verification successful:
addKeyBinding
usage is consistent with the newWindow
interfaceThe review of
globalApplication.addKeyBinding
usage across the codebase confirms that the change from*WebviewWindow
toWindow
has 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
cut
method is invoked only on the appropriate types. It correctly handles the new*WebviewPanel
type 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
currentWindow
function 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.Window
interface has been introduced to provide a more generic way of interacting with windows, andapplication.WebviewWindow
likely implements this interface. The unchanged function body indicates that theapplication.Window
interface likely provides the same methods asapplication.WebviewWindow
.
121-124
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature.It also confirms that the
application.Window
interface provides aSetSize
method, which was previously available on*application.WebviewWindow
.
127-130
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segment.It further confirms that the
application.Window
interface provides aSetSize
method, which was previously available on*application.WebviewWindow
.
132-135
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It confirms that the
application.Window
interface provides aSetMinSize
method, which was previously available on*application.WebviewWindow
.
137-141
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It confirms that the
application.Window
interface providesSetMaximiseButtonState
andSetMaxSize
methods, which were previously available on*application.WebviewWindow
.
143-147
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It confirms that the
application.Window
interface provides aSize
method, which was previously available on*application.WebviewWindow
.
150-153
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It further confirms that the
application.Window
interface provides aSetMinSize
method, which was previously available on*application.WebviewWindow
.
156-160
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It further confirms that the
application.Window
interface providesSetMaxSize
andSetMaximiseButtonState
methods, which were previously available on*application.WebviewWindow
.
163-166
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It confirms that the
application.Window
interface provides aSetRelativePosition
method, which was previously available on*application.WebviewWindow
.
168-171
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It further confirms that the
application.Window
interface provides aSetRelativePosition
method, which was previously available on*application.WebviewWindow
.
174-178
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It confirms that the
application.Window
interface provides aRelativePosition
method, which was previously available on*application.WebviewWindow
.
181-184
: LGTM!The change in the function literal passed to
currentWindow
to accept a parameter of typeapplication.Window
instead of*application.WebviewWindow
is consistent with the updatedcurrentWindow
function signature and the previous code segments.It confirms that the
application.Window
interface provides aCenter
method, 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 CFLAGS
sets-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 ofexecJS
invocationThe call to
result.execJS(runtime.Core())
may need verification. Ensure thatexecJS
is intended to be called withruntime.Core()
as an argument and that this aligns with its definition.
73-74
: EnsuresetFloating
is called afternsPanel
is initializedThe method
p.setFloating(options.Floating)
is called immediately after setting up the panel. Confirm thatp.nsPanel
is 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 forsetButtonState
The function
setButtonState
takes avoid *button
parameter, whereas related functions likesetMinimiseButtonState
usevoid *window
. Ensure thatsetButtonState
is intended to operate on a button object and that the parameter type is correct.Please confirm whether
setButtonState
should 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_DARWIN
is 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
hiddenWindows
has been updated from[]*application.WebviewWindow
to[]application.Window
. Ensure thatapplication.Window
correctly represents the windows you intend to store and that all methods and comparisons used with this slice function as expected.
33-36
: Update tocurrentWindow
function enhances flexibility.Changing the
currentWindow
helper function to acceptapplication.Window
instead of*application.WebviewWindow
allows for greater flexibility, especially ifapplication.Window
is 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.WindowLevel
assignsMacWindowLevelNormal
if it's empty.Confirm that comparing
macOptions.WindowLevel
to an empty string (""
) is appropriate. IfWindowLevel
is 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.w
Access Is ValidIn the
startDrag:
method, you've changed the call to[window.w performWindowDragWithEvent:self.leftMouseEvent];
. Verify thatwindow.w
is properly initialized and thatwindow
is indeed an instance ofWebviewWindow
with a validw
property.Run the following script to ensure that all instances of
window
passed tostartDrag:
are of typeWebviewWindow
:This script searches for calls to
startDrag:
and provides context to help confirm that thewindow
parameter is correctly typed.Verification successful
To gather more information and verify our concerns, let's run the following scripts:
Verification Successful:
window.w
Access Is ValidThe code review has been verified. The
window
parameter passed tostartDrag:
is indeed an instance ofWebviewWindow
, and it has a validw
property. 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.WebviewWindow
instances consistently have aw
property used throughout the code.- The call to
startDrag:
inwebview_window_bindings_darwin.m
passes 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 incurrentWindow
functionThe
currentWindow
function now accepts a parameter of typeapplication.Window
instead of*application.WebviewWindow
. Please ensure that all methods invoked onwindow
within this function and its callbacks are part of theapplication.Window
interface and that no pointer-specific functionality is required.To confirm, you can run the following script to check for any methods called on
window
that might not be defined in theapplication.Window
interface:This script will list all unique methods called on
window
, allowing you to cross-reference them with theapplication.Window
interface.Verification successful
Type compatibility in
currentWindow
function verifiedThe change from
*application.WebviewWindow
toapplication.Window
in thecurrentWindow
function 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.Window
interface.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 ofkeyBindings
map usingsync.RWMutex
Good use of
RWMutex
to manage concurrent access to thekeyBindings
map. This ensures that key bindings are safely read and modified across multiple goroutines.
420-423
: Initialization of key binding structuresThe initialization of
a.keyBindings
and other related fields in theinit
function is appropriate and ensures that the application is correctly set up before use.
Line range hint
755-766
: Correctly updatedCurrentWindow
to returnWindow
interfaceThe
CurrentWindow
method has been updated to return theWindow
interface instead of*WebviewWindow
. This change aligns with the shift towards using the more generalWindow
type and enhances flexibility.
Line range hint
965-974
: Ensure key binding callbacks are thread-safeThe
processKeyBinding
method executes callbacks in separate goroutines to prevent blocking. Ensure that the callback functions provided inkeyBindings
are 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
addKeyBinding
ensures thread-safe modification of thekeyBindings
map. 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
frameless
istrue
, 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
windowSetFullScreen
function,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
fraudulentWebsiteWarningEnabled
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
invisibleTitleBarHeight
property to the providedheight
value. The logic is straightforward and there are no apparent issues.
177-181
: LGTM!The function correctly sets the window's
opaque
property toNO
and thebackgroundColor
toclearColor
to 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
invisibleTitleBarHeight
property to the providedheight
value. The logic is straightforward and there are no apparent issues.
190-195
: LGTM!The function correctly converts the provided
title
string to anNSString
and sets it as the window's title. It also frees thetitle
string 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
width
andheight
values. It then sets the window'scontentSize
andframe
properties 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
level
property based on the value ofalwaysOnTop
parameter. IfalwaysOnTop
is true, it sets the window to be always on top by usingNSFloatingWindowLevel
. IfalwaysOnTop
is 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
level
property 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
url
string to anNSURL
object and creates anNSURLRequest
object with the URL. It then loads the URL request in the window's webview using theloadRequest
method. The function also frees theurl
string 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
resizable
parameter. Ifresizable
is true, it adds theNSWindowStyleMaskResizable
flag to the style mask. Ifresizable
is false, it removes theNSWindowStyleMaskResizable
flag 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 toprocessKeyBindingOptions
Are UpdatedThe function
processKeyBindingOptions
has 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
windowProcessKeyBindingOptions
function and the context of otherprocessKeyBindingOptions
occurrences.
Renaming of
processKeyBindingOptions
is Correct and IntentionalThe renaming of
processKeyBindingOptions
towindowProcessKeyBindingOptions
inv3/pkg/application/webview_window.go
is correct and intentional. This change makes the function specific to WebviewWindow type.Other similarly named functions in the codebase serve different purposes:
processKeyBindingOptionsForPanel
inwebview_panel_options.go
for WebviewPanel typeprocessKeyBindingOptions
inapplication.go
for 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
Quality Gate failedFailed conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
createWindow
function correctly handles the creation of both window and panel types based on the providedWindowType
parameter. 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
printWindowStyle
function 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
setInvisibleTitleBarHeight
function correctly sets the invisible title bar height for the window by retrieving the window and its delegate, and then setting theinvisibleTitleBarHeight
property 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
windowSetTransparent
function correctly makes the window backdrop transparent by setting the window'sopaque
property toNO
and its background color toclearColor
. This function is useful for creating transparent windows.
189-195
: LGTM!The
windowSetSize
function 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
windowSetAlwaysOnTop
function correctly sets the window to be always on top by setting its level toNSFloatingWindowLevel
ifalwaysOnTop
is true, orNSNormalWindowLevel
otherwise. 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
, andsetTornOffMenuWindowLevel
correctly set the window level to different predefined levels using thesetLevel
method. These functions provide a convenient way to control the window's behavior and appearance in relation to other windows.
223-233
: LGTM!The
windowSetResizable
function correctly sets whether the window is resizable by retrieving the window's current style mask and adding or removing theNSWindowStyleMaskResizable
flag based on theresizable
parameter. The function then sets the updated style mask on the window, effectively controlling its resizability.
236-243
: LGTM!The
windowSetMinSize
function 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 thesetContentMinSize
method. The function also sets the window's minimum size using thesetMinSize
method, ensuring that the window cannot be resized below the specified dimensions.
246-255
: LGTM!The
windowSetMaxSize
function correctly sets the maximum size of the window by initializing the maximum size toFLT_MAX
for 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 thesetContentMaxSize
method. It also sets the window's maximum size using thesetMaxSize
method, 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
id
variable and its usage to setoptions.Name
and initialize theid
field is a good optimization that ensures consistency and reduces the chances of errors.The renaming of
processKeyBindingOptions
towindowProcessKeyBindingOptions
improves clarity and follows a consistent naming convention. The function call withinNewWindow
has 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.impl
isnil
or 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.impl
was 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofWebviewWindow
adding 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
WebviewWindow
andWebviewPanel
are working as expected (https://github.com/nixpare/wails-mac-panel).WebviewWindow
, panics (as expected) only when trying to useWebviewPanel
Test Configuration
Checklist:
website/src/pages/changelog.mdx
with details of this PRSummary by CodeRabbit
Release Notes
New Features
WebviewPanel
struct for managing webview panels with customizable options.Window
interface with new methods for flashing, printing, and enabling/disabling windows.Improvements
application.Window
type, improving flexibility and future extensibility.Bug Fixes