[v3-Windows] New DIP system for Enhanced High DPI Monitor Support #3665
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent updates enhance multi-monitor management and window handling within the application. Key improvements include the introduction of a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Main
participant ScreenService
User->>UI: Selects screen layout
UI->>Main: Trigger layout update
Main->>ScreenService: Update screen configurations
ScreenService-->>Main: Return updated layout
Main->>UI: Render updated screen layout
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: 38
Outside diff range, codebase verification and nitpick comments (9)
v3/pkg/application/screen_windows.go (1)
13-18: Ensure comprehensive error handling.The function
processAndCacheScreenshandles errors when retrieving screen information and laying out screens. However, it should also consider handling potential errors during the screen processing loop where screen information is converted and appended to thescreensslice.
- Lines where errors are handled:
- After calling
w32.GetAllScreens()- After calling
m.parent.screenManager.LayoutScreens(screens)Consider adding error handling within the loop to cover potential issues during screen information processing.
Analysis chain
Ensure comprehensive error handling.
The function retrieves screen information and handles errors, but ensure that all potential edge cases are covered.
Let's gather the complete function body to verify comprehensive error handling throughout the
processAndCacheScreensfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify comprehensive error handling in `processAndCacheScreens`. # Test: Search for error handling in the function. Expect: Proper handling of all potential errors. rg --type go -A 5 'func \(m \*windowsApp\) processAndCacheScreens\(\)' | rg 'if err != nil'Length of output: 147
Script:
#!/bin/bash # Description: Retrieve the entire `processAndCacheScreens` function to verify comprehensive error handling. # Search for the complete function body of `processAndCacheScreens` in the Go file. rg --type go -A 50 'func \(m \*windowsApp\) processAndCacheScreens\(\)' v3/pkg/application/screen_windows.goLength of output: 1742
v3/examples/screen/screens.go (1)
18-28: Potential Issue: Lack of Error Handling inProcessExampleScreensFunctionThe
ProcessExampleScreensfunction does not include error handling for type assertions and missing keys in the input maps. This could lead to runtime panics if the input data is not formatted as expected. Consider adding checks to handle these potential edge cases gracefully.
- File:
v3/examples/screen/screens.go- Lines: 18-54
Analysis chain
LGTM! Clear and concise parsing logic.
The parsing logic for example screens is clear and concise, but ensure that it handles all potential edge cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the parsing logic in `ProcessExampleScreens`. # Test: Search for the parsing logic. Expect: Proper handling of all potential edge cases. rg --type go -A 5 'func \(s \*ScreenService\) ProcessExampleScreens' | rg 'parseRect'Length of output: 179
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the `ProcessExampleScreens` function. # Search for the complete implementation of the `ProcessExampleScreens` function in the Go files. ast-grep --lang go --pattern $'func (s *ScreenService) ProcessExampleScreens($_) $_ { $$$ }'Length of output: 2591
v3/pkg/application/screen_darwin.go (1)
27-27: Action Required: UpdatescaletoscaleFactorThe
scalemethod inscreenmanager.goand thescalefield inlinux/screen.goshould be updated toscaleFactorfor consistency with the changes made to theScreenstruct.
- File:
v3/pkg/application/screenmanager.go
- Lines: Multiple occurrences of
func (s Screen) scaleand its usage.- File:
v2/internal/frontend/desktop/linux/screen.go
- Lines:
int scale;and its usage.Please update these references to ensure consistency across the codebase.
Analysis chain
LGTM! But verify the usage of the renamed
scaleFactorfield.The changes to the
Screenstruct are approved.However, ensure that all references to the old
scalefield are updated toscaleFactoracross the codebase.
Let's verify if the occurrences in
screenmanager.goandlinux/screen.goare related to theScreenstruct and need to be updated toscaleFactor.
Let's refine our approach and directly search for the
scalemethod and any references to theScreenstruct inscreenmanager.goandlinux/screen.go.
Let's correct the file path and re-run the search for the
scalemethod and any references to theScreenstruct. Additionally, let's search for any remaining occurrences of thescalefield in all Go files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the old `scale` field are updated to `scaleFactor`. # Test: Search for the old `scale` field. Expect: No occurrences of the old `scale` field. rg --type go '\bscale\b'Length of output: 2692
Script:
#!/bin/bash # Description: Verify if the occurrences of "scale" in `screenmanager.go` and `linux/screen.go` are related to the `Screen` struct. # Search for the definition of the `Screen` struct and its fields in `screenmanager.go`. ast-grep --lang go --pattern 'struct Screen { $$$ }' | rg 'screenmanager.go' # Search for the definition of the `Screen` struct and its fields in `linux/screen.go`. ast-grep --lang go --pattern 'struct Screen { $$$ }' | rg 'linux/screen.go'Length of output: 842
Script:
#!/bin/bash # Description: Verify if the occurrences of "scale" in `screenmanager.go` and `linux/screen.go` are related to the `Screen` struct. # Search for the `scale` method in `screenmanager.go`. rg 'func (s Screen) scale' v3/pkg/application/screenmanager.go # Search for the `Screen` struct in `linux/screen.go`. rg 'struct Screen' v3/pkg/application/linux/screen.goLength of output: 195
Script:
#!/bin/bash # Description: Verify if the occurrences of "scale" in `screenmanager.go` and `linux/screen.go` are related to the `Screen` struct. # Search for the `scale` method in `screenmanager.go`. rg 'func (s Screen) scale' v3/pkg/application/screenmanager.go # Search for the `Screen` struct in the correct file path for `linux/screen.go`. rg 'struct Screen' v2/internal/frontend/desktop/linux/screen.go # Search for any remaining occurrences of the `scale` field in all Go files. rg '\bscale\b' --type goLength of output: 2847
v3/examples/screen/assets/index.html (1)
11-16: Optimize body styles for better performance.Setting the margin to 0, and ensuring the body occupies the full viewport dimensions is good. Consider using a more specific font stack and a background color that adapts to the theme (e.g., dark mode).
- background-color: white; + background-color: var(--background-color, white);v3/pkg/application/screenmanager.go (2)
12-15: Consider initializingscreensandprimaryScreenin the constructor.This can help avoid potential nil pointer dereferences.
429-445: Improve error messages for better debugging.Provide more detailed error messages to help with debugging.
if m.primaryScreen == nil { return fmt.Errorf("no primary screen found in the provided screens list") } else if len(remainingScreens) != len(m.screens)-1 { return fmt.Errorf("invalid primary screen found; expected %d remaining screens but found %d", len(m.screens)-1, len(remainingScreens)) }v3/pkg/w32/user32.go (3)
140-141: Add comments to explain the purpose of new procedures.Adding comments will help other developers understand the purpose of these new procedures.
// procSetProcessDpiAwarenessContext sets the DPI awareness context for the process. procSetProcessDpiAwarenessContext = moduser32.NewProc("SetProcessDpiAwarenessContext") // procSetThreadDpiAwarenessContext sets the DPI awareness context for the thread. procSetThreadDpiAwarenessContext = moduser32.NewProc("SetThreadDpiAwarenessContext")
394-400: Improve error handling inSetProcessDpiAwarenessContext.Provide more detailed error messages to help with debugging.
func SetProcessDpiAwarenessContext(ctx uintptr) error { status, r, err := procSetProcessDpiAwarenessContext.Call(ctx) if status == 0 { return fmt.Errorf("SetProcessDpiAwarenessContext failed with status %d: %v %v", status, r, err) } return nil }
402-408: Improve error handling inSetThreadDpiAwarenessContext.Provide more detailed error messages to help with debugging.
func SetThreadDpiAwarenessContext(ctx uintptr) error { status, r, err := procSetThreadDpiAwarenessContext.Call(ctx) if status == 0 { return fmt.Errorf("SetThreadDpiAwarenessContext failed with status %d: %v %v", status, r, err) } return nil }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (30)
- v3/examples/screen/assets/examples.js (1 hunks)
- v3/examples/screen/assets/index.html (1 hunks)
- v3/examples/screen/assets/main.js (1 hunks)
- v3/examples/screen/main.go (2 hunks)
- v3/examples/screen/screens.go (1 hunks)
- v3/examples/systray/main.go (1 hunks)
- v3/examples/window/assets/index.html (1 hunks)
- v3/examples/window/main.go (4 hunks)
- v3/internal/runtime/desktop/@wailsio/runtime/package.json (1 hunks)
- v3/internal/runtime/desktop/@wailsio/runtime/src/screens.js (2 hunks)
- v3/internal/runtime/desktop/@wailsio/runtime/types/screens.d.ts (3 hunks)
- v3/pkg/application/application.go (2 hunks)
- v3/pkg/application/application_windows.go (5 hunks)
- v3/pkg/application/linux_cgo.go (7 hunks)
- v3/pkg/application/linux_purego.go (2 hunks)
- v3/pkg/application/screen_darwin.go (3 hunks)
- v3/pkg/application/screen_windows.go (1 hunks)
- v3/pkg/application/screenmanager.go (1 hunks)
- v3/pkg/application/screenmanager_test.go (1 hunks)
- v3/pkg/application/systemtray_windows.go (5 hunks)
- v3/pkg/application/webview_window.go (6 hunks)
- v3/pkg/application/webview_window_darwin.go (1 hunks)
- v3/pkg/application/webview_window_linux.go (2 hunks)
- v3/pkg/application/webview_window_options.go (1 hunks)
- v3/pkg/application/webview_window_windows.go (16 hunks)
- v3/pkg/w32/constants.go (1 hunks)
- v3/pkg/w32/screen.go (3 hunks)
- v3/pkg/w32/shcore.go (1 hunks)
- v3/pkg/w32/typedef.go (2 hunks)
- v3/pkg/w32/user32.go (4 hunks)
Files skipped from review due to trivial changes (2)
- v3/examples/systray/main.go
- v3/internal/runtime/desktop/@wailsio/runtime/package.json
Additional context used
Biome
v3/examples/screen/assets/main.js
[error] 405-405: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (131)
v3/pkg/w32/shcore.go (4)
15-15: LGTM! The variable declaration is correct.The variable
procSetProcessDpiAwarenessis correctly defined usingmodshcore.NewProc("SetProcessDpiAwareness").
18-21: LGTM! The function implementation is correct.The function
HasSetProcessDpiAwarenessFunccorrectly checks for the availability of theSetProcessDpiAwarenessprocedure.
23-29: LGTM! The function implementation is correct.The function
SetProcessDpiAwarenesscorrectly sets the DPI awareness level for the process and handles errors appropriately.
14-14: LGTM! The variable declaration is correct.The variable
procGetDpiForMonitoris correctly defined usingmodshcore.NewProc("GetDpiForMonitor").v3/examples/screen/main.go (5)
6-6: LGTM! The import is necessary and correctly used.The
log/slogpackage is imported for logging and is used appropriately in the code.
8-8: LGTM! The import is necessary and correctly used.The
ospackage is imported for file operations and is used appropriately in the code.
32-34: LGTM! The service management is correctly implemented.The
Servicesfield is correctly initialized with a newScreenService.
35-35: LGTM! The logging level is correctly set.The
LogLevelis set toslog.LevelError, which is appropriate for error-level logging.
38-57: LGTM! The middleware for asset handling is correctly implemented.The middleware function correctly disables caching for asset files and handles requests appropriately.
v3/internal/runtime/desktop/@wailsio/runtime/types/screens.d.ts (5)
21-25: LGTM! The type definition is correct.The
Sizetype has been correctly modified to change the properties fromXandYtoWidthandHeight, which enhances clarity.
55-57: LGTM! The property name change is appropriate.The
Scaleproperty has been correctly renamed toScaleFactor, which provides a clearer definition.
59-65: LGTM! The property changes are appropriate.The
Positiontype has been removed and its properties have been integrated into theScreentype asXandYfields, simplifying the representation of screen coordinates.
74-85: LGTM! The new properties are appropriate.The new properties
PhysicalBoundsandPhysicalWorkAreahave been correctly added to theScreentype, providing more detailed information about the screen's characteristics.
Line range hint
29-37: LGTM! The type definition is correct.The
Recttype has been correctly modified to include new properties for the X and Y coordinates, width, and height.v3/internal/runtime/desktop/@wailsio/runtime/src/screens.js (3)
15-16: LGTM! Improved clarity.Renaming the properties from
XandYtoWidthandHeightenhances the clarity of theSizetypedef.
22-23: LGTM! Improved semantic understanding.Refining the description of the
XandYproperties to specify that they denote the coordinates of the origin improves the semantic understanding of the coordinate system.
33-35: LGTM! Enhanced clarity and usability.The updates to the
Screentypedef, including renaming properties, splitting properties, and adding new properties, enhance the clarity and usability of the type definitions.Also applies to: 38-38, 40-41, 43-46
v3/pkg/application/screen_windows.go (9)
19-29: LGTM! Efficient processing of screen information.The code efficiently processes screen information by converting it to the required format.
30-35: LGTM! Clear and concise work area calculation.The work area calculation is clear and concise.
37-50: LGTM! Well-structured screen information storage.The screen information is well-structured and stored efficiently.
62-64: LGTM! Straightforward retrieval of cached screen information.The function retrieves cached screen information and includes a note for a potential future change.
67-69: LGTM! Straightforward retrieval of primary screen information.The function retrieves primary screen information and includes a note for a potential future change.
86-88: LGTM! Straightforward conversion of monitor handle to screen ID.The function converts a monitor handle to a screen ID.
75-84: LGTM! Straightforward retrieval of screen information for a window handle.The function retrieves screen information for a window handle, but ensure that the screen ID logic is robust.
Verification successful
LGTM! Straightforward retrieval of screen information for a window handle.
The function retrieves screen information for a window handle, and the screen ID logic appears robust, assuming
hMonitoris unique and stable for each screen.
hMonitorToScreenIDimplementation:func hMonitorToScreenID(hMonitor uintptr) string { return strconv.Itoa(int(hMonitor)) }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the screen ID logic in `getScreenForWindowHwnd`. # Test: Search for the screen ID logic. Expect: Robust logic for determining the screen ID. rg --type go -A 5 'func getScreenForWindowHwnd' | rg 'hMonitorToScreenID'Length of output: 152
Script:
#!/bin/bash # Description: Locate and review the implementation of `hMonitorToScreenID`. # Find the implementation of `hMonitorToScreenID`. rg --type go 'func hMonitorToScreenID'Length of output: 128
Script:
#!/bin/bash # Description: Extract and review the implementation of `hMonitorToScreenID`. # Extract the implementation of `hMonitorToScreenID`. rg --type go -A 10 'func hMonitorToScreenID' v3/pkg/application/screen_windows.goLength of output: 172
71-73: LGTM! Straightforward retrieval of screen information for a window.The function retrieves screen information for a specific window, but ensure that the logic for determining the nearest screen is robust.
53-56: Ensure proper handling of layout errors.The function handles layout errors, but ensure that all potential edge cases are covered.
v3/examples/screen/screens.go (5)
12-15: LGTM! Straightforward retrieval of system screens.The function retrieves system screens.
30-50: LGTM! Efficient processing of example screens.The code efficiently processes example screens by converting them to the required format.
72-90: LGTM! Well-structured point transformation with double-rounding check.The point transformation logic with double-rounding check is well-structured, but ensure that it handles all potential edge cases.
Verification successful
LGTM! Well-structured point transformation with double-rounding check.
The
transformPointmethod correctly handles the conversion based on thetoDIPflag and theisExampleLayoutcondition. This integration within theTransformPointmethod ensures that potential edge cases are managed effectively.
v3/examples/screen/screens.go:56-70: Implementation oftransformPointmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the double-rounding check logic in `TransformPoint`. # Test: Search for the double-rounding check logic. Expect: Proper handling of all potential edge cases. rg --type go -A 5 'func \(s \*ScreenService\) TransformPoint' | rg 'transformPoint'Length of output: 83
Script:
#!/bin/bash # Description: Search for the implementation of `transformPoint` method in the codebase. # Search for the definition of `transformPoint` method within the codebase. ast-grep --lang go --pattern 'func (s *ScreenService) transformPoint($_, $_) $_'Length of output: 968
92-113: LGTM! Well-structured rectangle transformation logic.The rectangle transformation logic is well-structured, but ensure that it handles all potential edge cases.
Verification successful
LGTM! Well-structured rectangle transformation logic.
The rectangle transformation logic is well-structured and thoroughly tested, covering various scenarios and edge cases.
v3/pkg/application/screenmanager_test.goincludes comprehensive tests forPhysicalToDipRectandDipToPhysicalRect.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rectangle transformation logic in `TransformRect`. # Test: Search for the rectangle transformation logic. Expect: Proper handling of all potential edge cases. rg --type go -A 5 'func \(s \*ScreenService\) TransformRect' | rg 'PhysicalToDipRect|DipToPhysicalRect'Length of output: 105
Script:
#!/bin/bash # Search for the definitions of the PhysicalToDipRect and DipToPhysicalRect functions rg --type go 'func PhysicalToDipRect' -A 10 rg --type go 'func DipToPhysicalRect' -A 10Length of output: 1667
Script:
#!/bin/bash # Search for the usages of the PhysicalToDipRect and DipToPhysicalRect functions within the codebase rg --type go 'PhysicalToDipRect' rg --type go 'DipToPhysicalRect'Length of output: 3831
Script:
#!/bin/bash # Search for test cases related to PhysicalToDipRect and DipToPhysicalRect in screenmanager_test.go rg --type go 'PhysicalToDipRect|DipToPhysicalRect' v3/pkg/application/screenmanager_test.go -A 10Length of output: 3809
56-69: LGTM! Well-structured point transformation logic.The point transformation logic is well-structured, but ensure that it handles all potential edge cases.
v3/pkg/w32/screen.go (7)
Line range hint
21-30:
LGTM!The
getMonitorNamefunction correctly retrieves the monitor name based on the device name and handles errors appropriately.
75-83: Improved error handling for cursor position retrieval.The function now retrieves the cursor position at the beginning to determine the current monitor and handles errors appropriately.
Line range hint
85-96:
Enhanced monitor enumeration with better error handling.The monitor enumeration logic has been improved with better error handling, ensuring that the function exits gracefully upon encountering errors.
99-130: Improved monitor information retrieval and error handling.The function now retrieves monitor information, including name, DPI, and rotation, with improved error handling and immediate association of monitor handles with their respective
Screenobjects.
132-134: Enhanced enumeration with error feedback.The function now provides clearer feedback for debugging by including error messages if enumeration fails.
140-141: LGTM!The
rectContainsPointfunction correctly checks if a point is within a rectangle.
13-18: LGTM! But verify the usage of the renamedScaleFactorfield.The changes to the
Screenstruct are approved.However, ensure that all references to the old
Scalefield are updated toScaleFactoracross the codebase.Verification successful
The renaming of the
Scalefield toScaleFactorhas been correctly applied across the codebase. No occurrences of the oldScalefield were found.
- The changes to the
Screenstruct are approved.- No further action is required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the old `Scale` field are updated to `ScaleFactor`. # Test: Search for the old `Scale` field. Expect: No occurrences of the old `Scale` field. rg --type go '\bScale\b'Length of output: 263
Script:
#!/bin/bash # Description: Verify all references to the old `Scale` field are updated to `ScaleFactor`. # Test: Search for the old `Scale` field as a struct field. Expect: No occurrences of the old `Scale` field as a struct field. rg --type go '\.Scale\b'Length of output: 26
v3/examples/window/assets/index.html (5)
1-6: LGTM!The HTML structure and metadata are correctly defined and follow standard HTML practices.
7-11: LGTM!The CSS styles are minimal and correctly set the text color to white.
13-17: LGTM!The
callBindingfunction correctly handles binding calls with appropriate parameters.
54-87: LGTM!The JavaScript functions for window management tasks, such as setting position, size, and bounds, as well as retrieving bounds, are correctly implemented and follow appropriate logic and practices.
18-51: LGTM!The HTML body content includes interactive elements for window management and follows standard HTML practices.
v3/pkg/application/screen_darwin.go (5)
39-39: LGTM!The
processScreenfunction correctly initializes thescaleFactorfield in theScreenstruct.
146-150: LGTM!The
cScreenToScreenfunction correctly converts thescaleFactorfield from the CScreenstruct to the GoScreenstruct.
Line range hint
153-156:
LGTM!The
getPrimaryScreenfunction correctly retrieves the primary screen.
Line range hint
158-167:
LGTM!The
getScreensfunction correctly retrieves all screens and converts them to GoScreenstructs.
Line range hint
169-171:
LGTM!The
getScreenForWindowfunction correctly retrieves the screen for a given window.v3/examples/screen/assets/index.html (14)
7-9: Ensure consistent box-sizing.The universal selector (*) sets box-sizing to border-box, which is a good practice for consistent layout behavior.
19-22: Flexbox container for layout.Using flexbox for the container ensures a flexible and responsive layout. The padding ensures some spacing around the content.
25-26: User-select and cursor properties for SVG.Preventing text selection and setting a crosshair cursor for SVG elements enhances the user experience.
28-29: Padding for control divs.Adding padding to control divs ensures better spacing and layout.
32-33: Span width for control labels.Setting a fixed width for the first child span in control divs ensures consistent alignment.
36-45: Styling for radio buttons.The styles for radio buttons ensure they are visually distinct and interactive. The use of background color, box-shadow, and border-radius enhances their appearance.
47-50: Active state for radio buttons.The styles for the active state of radio buttons provide visual feedback to the user.
51-56: Conditional display for probe buttons.The styles ensure that only the relevant probe button is displayed based on the active state.
57-58: Advanced settings display.The advanced class is initially hidden, which can be toggled based on user interaction.
71-93: Advanced input fields and buttons.The advanced section provides input fields for dimensions and coordinates, along with buttons for updating and setting rectangles. Ensure that the functions
updateDipRectanddrawRectare defined in the JavaScript.Verification successful
Verified: Functions
updateDipRectanddrawRectare defined in the JavaScript files.
updateDipRectis defined inv3/examples/screen/assets/main.js.drawRectis defined inv3/examples/screen/assets/main.js.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of functions `updateDipRect` and `drawRect` in JavaScript files. # Test: Search for the function definitions. Expect: Functions are defined. rg --type js -A 5 $'function updateDipRect' rg --type js -A 5 $'function drawRect'Length of output: 983
63-70: Advanced controls section.The advanced controls section provides buttons for reloading and testing, which are initially hidden. Ensure that the functions
test1andtest2are defined in the JavaScript.Verification successful
Advanced controls section.
The advanced controls section provides buttons for reloading and testing, which are initially hidden. The functions
test1andtest2are defined inv3/examples/screen/assets/main.js, ensuring that the buttons will function as intended.
- Functions
test1andtest2are confirmed to be defined inv3/examples/screen/assets/main.js.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of functions `test1` and `test2` in JavaScript files. # Test: Search for the function definitions. Expect: Functions are defined. rg --type js -A 5 $'function test1' rg --type js -A 5 $'function test2'Length of output: 1066
94-122: Controls section.The controls section provides radio buttons for selecting screens and coordinates, along with a checkbox for retaining the viewbox and a button for showing advanced settings. Ensure that the functions
radioBtnClick,setExamplesType,showAdvanced, andprobeLayoutare defined in the JavaScript.Verification successful
Verified: The functions
radioBtnClick,setExamplesType,showAdvanced, andprobeLayoutare all defined in thev3/examples/screen/assets/main.jsfile.
radioBtnClick: Defined on line 1.setExamplesType: Defined on line 6.showAdvanced: Defined on line 11.probeLayout: Defined on line 15.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of functions `radioBtnClick`, `setExamplesType`, `showAdvanced`, and `probeLayout` in JavaScript files. # Test: Search for the function definitions. Expect: Functions are defined. rg --type js -A 5 $'function radioBtnClick' rg --type js -A 5 $'function setExamplesType' rg --type js -A 5 $'function showAdvanced' rg --type js -A 5 $'function probeLayout'Length of output: 1949
123-134: SVG element for screen layout.The SVG element provides a visual representation of the screen layout. Ensure that the SVG elements with IDs
point1andpoint2are used correctly in the JavaScript.
137-139: New script imports.The new script imports include
examples.jsandmain.js. Ensure that these scripts are loaded correctly and do not cause any errors.v3/examples/screen/assets/examples.js (20)
1-9: Screen examples for normal scenarios.The screen examples for normal scenarios provide various monitor configurations. Ensure that the screen properties (id, width, height, scale factor, name) are correct and consistent.
10-16: Two monitors example.The two monitors example provides a configuration with two monitors. Ensure that the parent alignment and offset are correct.
17-23: Two monitors example (2).The two monitors example (2) provides a configuration with two monitors with different scale factors. Ensure that the parent alignment and offset are correct.
24-31: Three monitors example.The three monitors example provides a configuration with three monitors. Ensure that the parent alignment and offset are correct.
32-40: Four monitors example.The four monitors example provides a configuration with four monitors. Ensure that the parent alignment and offset are correct.
43-50: Test case: Child scaled, Start offset.The test case demonstrates a child monitor with scaling and a start offset. Ensure that the parent alignment and offset are correct.
51-57: Test case: Child scaled, End offset.The test case demonstrates a child monitor with scaling and an end offset. Ensure that the parent alignment and offset are correct.
58-64: Test case: Parent scaled, Start offset percent.The test case demonstrates a parent monitor with scaling and a start offset percentage. Ensure that the parent alignment and offset are correct.
65-71: Test case: Parent scaled, End offset percent.The test case demonstrates a parent monitor with scaling and an end offset percentage. Ensure that the parent alignment and offset are correct.
72-78: Test case: Parent scaled, Start align.The test case demonstrates a parent monitor with scaling and a start alignment. Ensure that the parent alignment and offset are correct.
79-85: Test case: Parent scaled, End align.The test case demonstrates a parent monitor with scaling and an end alignment. Ensure that the parent alignment and offset are correct.
86-92: Test case: Parent scaled, in-between.The test case demonstrates a parent monitor with scaling and an in-between alignment. Ensure that the parent alignment and offset are correct.
95-105: Edge case: Parent order (5 is parent of 4).The edge case demonstrates a complex parent-child relationship with multiple monitors. Ensure that the parent alignment and offset are correct.
106-115: Edge case: de-intersection reparent.The edge case demonstrates a de-intersection scenario with reparenting. Ensure that the parent alignment and offset are correct.
116-124: Edge case: de-intersection (unattached child).The edge case demonstrates a de-intersection scenario with an unattached child. Ensure that the parent alignment and offset are correct.
125-134: Edge case: Multiple de-intersection.The edge case demonstrates multiple de-intersection scenarios. Ensure that the parent alignment and offset are correct.
135-144: Edge case: Multiple de-intersection (left-side).The edge case demonstrates multiple de-intersection scenarios on the left side. Ensure that the parent alignment and offset are correct.
145-153: Edge case: Parent de-intersection child offset.The edge case demonstrates a parent de-intersection scenario with child offset. Ensure that the parent alignment and offset are correct.
155-157: Map screen layouts.The screen layouts are mapped using the
parseLayoutfunction. Ensure that the function correctly parses the layout.
159-205: Parse layout function.The
parseLayoutfunction parses the screen layout and calculates the screen positions. Ensure that the calculations for screen positions and alignments are correct.v3/pkg/application/systemtray_windows.go (6)
56-59: Calculate new window position.The new logic calculates the new position of the window based on the screen bounds and window bounds. Ensure that the calculations are correct and consider all edge cases.
69-70: Initialize variables for tray bounds and center alignment.The variables
trayBounds,centerAlignX, andcenterAlignYare initialized to store the tray bounds and center alignment values.
77-79: Convert tray bounds to DIP coordinates.The tray bounds are converted to DIP coordinates using the
PhysicalToDipRectfunction. Ensure that the conversion is correct.
89-104: Adjust window position based on taskbar location.The window position is adjusted based on the location of the taskbar and the tray icon. Ensure that the logic correctly handles all taskbar positions.
107-110: Set new window position.The new window position is set using the
relativeToAbsoluteDipPointfunction and theSetBoundsmethod. Ensure that the new position is correct.
151-151: Rename function for clarity.The function
getScreenhas been renamed togetScreenForWindowHwndto clarify its purpose. The new name accurately reflects the function's behavior.v3/pkg/application/application_windows.go (3)
190-198: Verify error handling and approve changes.The new logic for reprocessing and caching screen information when display settings change improves the application's responsiveness. Ensure that the error handling is sufficient and consistent with the rest of the function.
277-299: Approve changes and verify DPI awareness methods.The new
setupDPIAwarenessfunction enhances the application's compatibility with high-DPI displays by checking for various DPI awareness methods supported by the Windows API and setting the appropriate context. Ensure that all necessary DPI awareness methods are covered and errors are handled appropriately.
303-305: Verify error handling and approve changes.The updated
newPlatformAppfunction ensures that DPI awareness is set up correctly and that screen information is processed and cached. Ensure that the error handling is sufficient and consistent with the rest of the function.Also applies to: 315-319
v3/pkg/application/webview_window_linux.go (2)
108-109: Approve changes and verify consistency.The updated
disableSizeConstraintsfunction replaces the variablescalewithscaleFactor, suggesting a clearer semantic intent regarding the scaling of dimensions. Ensure that the new variable name is used consistently throughout the function.
207-218: Approve changes and verify DPI scaling.The new functions
bounds,setBounds,physicalBounds, andsetPhysicalBoundsimprove the handling of window dimensions, particularly concerning DPI scaling. Ensure that the functions handle DPI scaling appropriately and verify if there are any potential issues with the new logic.Also applies to: 220-226, 227-231, 232-236
v3/pkg/application/webview_window_options.go (1)
Line range hint
1-1:
Verify impact of removal and approve changes.The removal of the
WebviewWindowDefaultsvariable indicates a shift away from using hardcoded defaults, potentially allowing for more flexible or dynamic configurations of web view options. Ensure that the removal does not impact any other parts of the codebase.v3/pkg/application/screenmanager_test.go (11)
38-203: LGTM! The functionexampleLayoutsis comprehensive and well-structured.The function covers a wide range of scenarios, including normal, test cases, and edge cases, which is beneficial for thorough testing.
283-291: LGTM! The functionmatchRectsis correctly implemented.The function is useful for verifying rectangle transformations.
295-333: LGTM! The functionTestScreenManager_ScreensLayoutis well-structured.The function covers various scenarios for child and parent scaling, which is beneficial for thorough testing.
337-370: LGTM! The functionTestScreenManager_BasicTranformationis correctly implemented.The function covers essential transformation scenarios between physical and DIP coordinates.
372-395: LGTM! The functionTestScreenManager_PrimaryScreenis well-structured.The function covers various scenarios for identifying the primary screen.
397-428: LGTM! The functionTestScreenManager_EdgeAlignis correctly implemented.The function covers essential edge alignment scenarios between transformations.
430-493: LGTM! The functionTestScreenManager_ProbePointsis correctly implemented.The function covers various scenarios for point transformations across the screen.
495-524: LGTM! The functionTestScreenManager_TransformationDriftis correctly implemented.The function covers essential scenarios for transformation drift over time.
527-623: LGTM! The functionTestScreenManager_ScreenNearestRectis well-structured.The function covers various scenarios for identifying the nearest screen to a given rectangle.
626-673: LGTM! The functionTestScreenManager_UnsolvedEdgeCasesis correctly implemented.The function highlights unsolved edge cases, which is useful for future improvements.
676-715: LGTM! The benchmark functions are correctly implemented.The functions cover essential scenarios for performance testing.
v3/pkg/w32/typedef.go (2)
13-14: LGTM! The import statements have been adjusted for clarity.The changes improve readability.
682-682: LGTM! The new constantSPI_SETWORKAREAis correctly defined.The constant expands the set of system parameters for configuring or retrieving information about the working area of the desktop environment.
v3/pkg/application/linux_purego.go (2)
624-627: LGTM!The renaming of the
Scalefield toScaleFactorimproves clarity and aligns with common terminology.Ensure that all references to the
Scalefield are updated accordingly throughout the codebase.Verification successful
Verified: The renaming of the
Scalefield toScaleFactorin theScreenstruct is correctly implemented and does not affect other parts of the codebase.
- The occurrences of the term
Scalefound in the search results are related to different contexts and do not refer to theScalefield in theScreenstruct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `Scale` field are updated to `ScaleFactor`. # Test: Search for the old `Scale` field. Expect: No occurrences of the old `Scale` field. rg --type go -A 5 $'Scale[^F]'Length of output: 8858
Line range hint
722-729:
LGTM!The renaming of the
scaleparameter toscaleFactorimproves clarity and aligns with the changes in thegetScreenByIndexfunction.Ensure that all references to the
scaleparameter are updated accordingly throughout the codebase.Verification successful
To ensure that all references to the
scaleparameter are updated toscaleFactor, we need to refine our search to focus on relevant instances in the context of monitor geometry and scaling factors.
LGTM!
The renaming of the
scaleparameter toscaleFactorimproves clarity and aligns with the changes in thegetScreenByIndexfunction. No missed updates were found in the relevant context.
- Ensure that the function
scaleinv3/pkg/application/screenmanager.gois not confused with thescaleFactorparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `scale` parameter are updated to `scaleFactor`. # Test: Search for the old `scale` parameter. Expect: No occurrences of the old `scale` parameter. rg --type go -A 5 $'scale[^F]'Length of output: 31951
Script:
#!/bin/bash # Description: Refine the search to identify specific references to the `scale` parameter in the context of monitor geometry and scaling factors. # Search for instances of `scale` in function signatures and variable declarations related to monitor geometry and scaling factors. rg --type go -A 5 -w 'scale' | grep -E 'func|var|type|struct'Length of output: 148
v3/pkg/application/linux_cgo.go (5)
747-750: LGTM! The renaming of thescalefield toscaleFactorenhances clarity.The function retrieves screen information and returns a
Screenstruct. The renaming of thescalefield toscaleFactoris a positive change.
832-836: LGTM! The renaming of thescalefield toscaleFactorenhances clarity.The function retrieves the current mouse position and returns it along with a
Screenstruct. The renaming of thescalefield toscaleFactoris a positive change.
861-866: LGTM! The renaming of thescalevariable toscaleFactorenhances clarity.The function sets the window to fullscreen mode. The renaming of the
scalevariable toscaleFactoris a positive change.
885-889: LGTM! The renaming of thescalevariable toscaleFactorenhances clarity.The function retrieves the current screen information for the window and returns a
Screenstruct. The renaming of thescalevariable toscaleFactoris a positive change.
900-908: LGTM! The renaming of thescalevariable toscaleFactorenhances clarity.The function retrieves the geometry of the current monitor and returns it along with the
scaleFactor. The renaming of thescalevariable toscaleFactoris a positive change.v3/pkg/application/webview_window_windows.go (11)
191-205: Verify bounds calculation and default position logic.Ensure that the initial bounds and physical bounds are calculated correctly. The logic for handling the default window position (setting
physicalBounds.XandphysicalBounds.Ytow32.CW_USEDEFAULT) appears correct.
378-380: LGTM!The
updatemethod is straightforward and does not require further changes.
382-392: Verify correctness of border size calculations.Ensure that the calculations for the border sizes using extended frame bounds are correct and handle edge cases appropriately.
395-406: Verify appropriateness of usingGetWindowRect.Ensure that using
GetWindowRectto retrieve the window's physical bounds is appropriate for all cases.
408-422: Verify correctness of bounds setting and flag usage.Ensure that the logic for setting the physical bounds and handling DPI changes is correct. Verify that the flag
ignoreDPIChangeResizingis used appropriately to prevent double resizing issues.
424-427: Verify correctness of conversion to DIP coordinates.Ensure that the conversion from physical to DIP coordinates is correct in the
boundsmethod.
429-432: Verify correctness of conversion from DIP to physical coordinates.Ensure that the conversion from DIP to physical coordinates is correct and that the bounds are set appropriately in the
setBoundsmethod.
434-436: Verify correctness of size retrieval.Ensure that the size retrieval in DIP coordinates is correct in the
sizemethod.
447-453: Verify correctness of size setting and conversion.Ensure that the size setting is correct and that the conversion from DIP to physical coordinates is accurate in the
setSizemethod.
455-457: Verify correctness of position retrieval.Ensure that the position retrieval in DIP coordinates is correct in the
positionmethod.
460-466: Verify correctness of position setting and conversion.Ensure that the position setting is correct and that the conversion from DIP to physical coordinates is accurate in the
setPositionmethod.v3/pkg/w32/constants.go (2)
75-80: DPI Awareness Constants: ApprovedThe constants for
SetProcessDpiAwarenessare correctly defined and consistent with standard definitions.
82-90: DPI Awareness Context Constants: ApprovedThe constants for
SetProcessDpiAwarenessContextare correctly defined and use appropriate bitwise operations onuintptrfor their values.v3/examples/screen/assets/main.js (1)
315-378: Add error handling forawaitcalls.To ensure robustness, add error handling for the
awaitcalls.- ptDblTransformed = await probePoint(pt) + try { + ptDblTransformed = await probePoint(pt) + } catch (error) { + console.error('Error probing point:', error); + }Likely invalid or redundant comment.
v3/pkg/application/application.go (1)
273-274: LGTM!The addition of the
screenManagerfield aligns with the PR objectives.v3/examples/window/main.go (1)
Line range hint
74-676: LGTM!The changes to the
mainfunction align with the PR objectives.v3/pkg/application/screenmanager.go (1)
169-171: Ensure theContainsmethod handles edge cases.Add validation to handle edge cases where the point is on the edge of the rectangle.
func (r Rect) Contains(pt Point) bool { return pt.X >= r.X && pt.X < r.X+r.Width && pt.Y >= r.Y && pt.Y < r.Y+r.Height }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- mkdocs-website/docs/en/changelog.md (1 hunks)
Additional comments not posted (1)
mkdocs-website/docs/en/changelog.md (1)
20-21: LGTM!The new entry accurately describes the addition of the DIP system for enhanced high DPI monitor support. Proper attribution and formatting are used.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/examples/window/main.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- v3/examples/window/main.go
|
Thanks for this amazing PR @mmghv! 🙏 A couple of quick questions to start us off :-)
|
You're right, I might have added it when I was experimenting with DPI awareness and forgot to remove it, will do now.
There should be no effect because the screen manager layer is not implemented there yet, I just added the needed method for the new public APIs I tested on Linux (on WSL) to make sure it still works but Mac need to be tested. Edit: |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
v3/pkg/w32/user32.go (3)
365-367: Ensure consistency in function naming.Rename the function to match the naming convention used for other similar functions.
func HasSetProcessDPIAwareFunc() bool { err := procSetProcessDPIAware.Find() return err == nil }to
func HasSetProcessDpiAwareFunc() bool { err := procSetProcessDPIAware.Find() return err == nil }
388-390: Ensure consistency in function naming.Rename the function to match the naming convention used for other similar functions.
func HasSetProcessDpiAwarenessContextFunc() bool { err := procSetProcessDpiAwarenessContext.Find() return err == nil }to
func HasSetProcessDpiAwarenessContextFunc() bool { err := procSetProcessDpiAwarenessContext.Find() return err == nil }
393-399: Ensure consistency in function naming and approve the implementation.Rename the function to match the naming convention used for other similar functions.
func SetProcessDpiAwarenessContext(ctx uintptr) error { status, r, err := procSetProcessDpiAwarenessContext.Call(ctx) if status == 0 { return fmt.Errorf("SetProcessDpiAwarenessContext failed %d: %v %v", status, r, err) } return nil }to
func SetProcessDpiAwarenessContext(ctx uintptr) error { status, r, err := procSetProcessDpiAwarenessContext.Call(ctx) if status == 0 { return fmt.Errorf("SetProcessDpiAwarenessContext failed %d: %v %v", status, r, err) } return nil }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/w32/user32.go (4 hunks)
Additional comments not posted (2)
v3/pkg/w32/user32.go (2)
15-15: LGTM! The import statement is necessary for Windows-specific system calls.The import of
golang.org/x/sys/windowsis appropriate for accessing Windows API functions.
140-140: LGTM! The new procedure is necessary for DPI management.The addition of
procSetProcessDpiAwarenessContextis appropriate for managing DPI awareness contexts.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/application/webview_window_darwin.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/webview_window_darwin.go
|
@mmghv - There was a compile issue on Mac and interestingly the 2x probe buttons end up issuing an error in the console: |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- v3/pkg/application/linux_cgo.go (8 hunks)
- v3/pkg/application/screen_darwin.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- v3/pkg/application/linux_cgo.go
- v3/pkg/application/screen_darwin.go
|
@leaanthony I assume you're talking about the screens example, The probe buttons just probe multiple points on the screens, the first button probes the current layout only, the second one probes the current + all example layouts, Try probe manually by clicking on the screen svg, if it still crashes try to select an example layout first. I tested it now on Linux (WSL Ubuntu) it actually crashes but only when |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
v3/examples/window/main.go (1)
29-39: Add error handling forSetPos,SetSize, andSetBoundsmethods.To ensure robustness, add error handling for the
SetPos,SetSize, andSetBoundsmethods.// Example for SetPos: func (s *WindowService) SetPos(relative bool, x, y float64) { win := application.Get().CurrentWindow() initX, initY := win.Position() if relative { x += float64(initX) y += float64(initY) } if err := win.SetPosition(int(x), int(y)); err != nil { fmt.Printf("Error setting position: %v\n", err) } currentX, currentY := win.Position() fmt.Printf("SetPos: %d, %d => %d, %d\n", initX, initY, currentX, currentY) }Also applies to: 40-50, 51-62
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- mkdocs-website/docs/en/changelog.md (1 hunks)
- v3/examples/window/main.go (4 hunks)
- v3/pkg/application/application.go (2 hunks)
- v3/pkg/application/application_windows.go (5 hunks)
- v3/pkg/application/systemtray_windows.go (5 hunks)
- v3/pkg/application/webview_window.go (6 hunks)
- v3/pkg/application/webview_window_darwin.go (1 hunks)
- v3/pkg/application/webview_window_linux.go (2 hunks)
- v3/pkg/application/webview_window_options.go (1 hunks)
- v3/pkg/application/webview_window_windows.go (16 hunks)
Additional comments not posted (26)
v3/pkg/application/systemtray_windows.go (4)
56-56: Use ofwindowBoundsfor clarity and efficiency.The introduction of
windowBoundsimproves readability and efficiency by reducing repeated calls towindow.Bounds(). This change is beneficial for maintaining clean and understandable code.
69-79: Improved window centering logic.The addition of
centerAlignXandcenterAlignYvariables enhances the logic for centering the window over the tray icon. This change ensures that the window is positioned more accurately relative to the system tray.
107-110: Setting window position withSetBounds.The use of
window.SetBounds(windowBounds)to update the window's position based on calculated coordinates is a clear and efficient way to manage window positioning.
151-151: Method renamed togetScreenForWindowHwndfor specificity.Renaming the method to
getScreenForWindowHwndclarifies its purpose, indicating that it retrieves the screen associated with a specific window handle. This enhances code readability and maintainability.v3/pkg/application/application_windows.go (3)
186-195: Handle display setting changes inwndProc.The addition of logic to reprocess and cache screens when display settings change ensures that the application remains responsive to changes in display configuration. This is crucial for maintaining a consistent user experience.
273-295: Encapsulation of DPI awareness logic insetupDPIAwareness.The
setupDPIAwarenessfunction centralizes the logic for setting DPI awareness, improving code clarity and maintainability. This approach also allows for easier updates and modifications to DPI handling.
Line range hint
299-314: Error handling innewPlatformApp.The error handling for
setupDPIAwarenessandprocessAndCacheScreensensures that any issues during initialization are logged and addressed, which is essential for robust application behavior.v3/pkg/application/webview_window_linux.go (5)
109-110: Improved size constraints handling withscaleFactor.The use of
scaleFactorindisableSizeConstraintssuggests a more accurate handling of monitor geometry, enhancing the precision of size constraints.
208-219: Implementation ofboundsmethod.The
boundsmethod provides a structured approach to retrieve window dimensions, but the TODO comment indicates a need for proper DPI scaling. Ensure that DPI scaling is implemented for accurate results.
221-226: Implementation ofsetBoundsmethod.The
setBoundsmethod allows setting window dimensions using aRectobject, enhancing the interface for window manipulation. However, the TODO comment suggests that DPI scaling needs to be addressed.
228-232: Implementation ofphysicalBoundsmethod.The
physicalBoundsmethod currently mirrorsboundsand includes a TODO for DPI scaling. Ensure that DPI scaling is implemented for handling physical screen properties accurately.
233-237: Implementation ofsetPhysicalBoundsmethod.The
setPhysicalBoundsmethod provides a way to set window dimensions based on physical properties, but the TODO comment indicates missing DPI scaling. Verify the need for DPI scaling implementation.mkdocs-website/docs/en/changelog.md (1)
21-22: Changelog entry for DIP system and window class name option.The entries for the new DIP system and window class name option are clear and provide appropriate credit to contributors. Ensure these changes are accurately reflected in the codebase.
v3/pkg/application/application.go (2)
773-778: Track the TODO comments forGetScreensandGetPrimaryScreen.The comments indicate a transitional state. Ensure to address these in future updates.
Also applies to: 780-784
276-278: Track the integration ofscreenManager.The
screenManageris added to theAppstruct. Ensure its integration aligns with future updates for DPI support.Verification successful
Integration of
screenManageris well-distributed and aligns with future DPI support plans.The
screenManageris effectively integrated across the codebase, managing screen layouts and coordinate conversions. Future updates should ensure it directly supports DPI across all platforms, as indicated by the TODO comments.
- Files involved:
v3/examples/screen/screens.gov3/pkg/application/screen_windows.gov3/pkg/application/screenmanager.gov3/pkg/application/application.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and integration of `screenManager` in the codebase. # Test: Search for occurrences of `screenManager`. Expect: Proper usage and integration. rg --type go 'screenManager'Length of output: 2243
v3/pkg/application/webview_window.go (1)
791-801: Ensure unit test coverage for new and modified methods.The new methods for handling physical and DIP bounds should be covered by unit tests to ensure correctness.
Also applies to: 803-811, 813-823, 825-833, 835-845, 847-855, 857-867, 869-879
v3/pkg/application/webview_window_darwin.go (4)
1316-1316: Address the TODO comment for DPI scaling.The method retrieves the current position and size of the window and returns a
Rectstructure. However, the TODO comment indicates that proper DPI scaling needs to be implemented.- // DOTO: do it in a single step + proper DPI scaling + // TODO: Implement proper DPI scaling
1332-1332: Address the TODO comment for DPI scaling.The method sets the position and size of the window using the provided
Rectstructure. However, the TODO comment indicates that proper DPI scaling needs to be implemented.- // DOTO: do it in a single step + proper DPI scaling + // TODO: Implement proper DPI scaling
1338-1338: Address the TODO comment for DPI scaling.The method returns the physical bounds of the window by calling the
boundsmethod. However, the TODO comment indicates that proper DPI scaling needs to be implemented.- // TODO: proper DPI scaling + // TODO: Implement proper DPI scaling
1343-1343: Address the TODO comment for DPI scaling.The method sets the physical bounds of the window by calling the
setBoundsmethod with the providedRectstructure. However, the TODO comment indicates that proper DPI scaling needs to be implemented.- // TODO: proper DPI scaling + // TODO: Implement proper DPI scalingv3/pkg/application/webview_window_windows.go (6)
201-201: Consider providing a way to set an initial position.The TODO comment suggests providing a way to set
(0,0)as an initial position. This could enhance flexibility in window positioning.- // TODO: provide a way to set (0,0) as an initial position? + // TODO: Consider providing a way to set (0,0) as an initial position
1216-1218: LGTM!The
DPImethod correctly retrieves the DPI settings for the window, with fallbacks for different Windows versions.
1249-1251: LGTM!The
scaleWithWindowDPImethod correctly scales dimensions based on the window's DPI settings.
397-408: LGTM!The
physicalBoundsmethod correctly retrieves the physical bounds of the window.
410-424: LGTM!The
setPhysicalBoundsmethod correctly sets the window's physical bounds and manages DPI-related resizing issues.
1089-1098: Verify the usage ofignoreDPIChangeResizingflag.Ensure that the
ignoreDPIChangeResizingflag is used correctly to prevent unintended resizing during DPI changes. This is crucial for maintaining consistent window sizes across different DPI settings.Verification successful
The
ignoreDPIChangeResizingflag is used correctly.The flag is consistently used to manage window resizing behavior during DPI changes, ensuring that resizing is prevented when the flag is set. This aligns with its intended purpose to maintain window size consistency across different DPI settings.
- The flag is set to
truetemporarily and then restored, which is a common pattern for managing state during specific operations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ignoreDPIChangeResizing` flag in the codebase. # Test: Search for the flag usage. Expect: Consistent usage across methods handling DPI changes. rg --type go 'ignoreDPIChangeResizing'Length of output: 447
|
@mmghv - I can confirm that selecting an example layout then pressing "Probe Layout" doesn't raise the error. However, clicking "Probe all examples" errors every time. This is on Mac. |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (3)
v3/pkg/application/webview_window_windows.go (1)
191-205: Initialization of window bounds and position.The initialization of
boundsandphysicalBoundsusing the new DIP system is well implemented. However, the handling of default positions usingCW_USEDEFAULTcould be clarified in the comments to explain why this choice is made, especially since it's marked with a TODO.Consider expanding the comment on lines 200-201 to explain the choice of default positions more clearly.
v3/pkg/application/application_windows.go (2)
186-194: Approve the handling of display settings changes.The updates to the
wndProcmethod to handle display settings changes are crucial for maintaining correct application behavior in dynamic multi-monitor setups. The addition ofprocessAndCacheScreensensures that the application's internal state is updated in response to these changes.Consider enhancing error handling around the
processAndCacheScreenscall to manage potential failures more gracefully.
Line range hint
298-315: Approve the application initialization logic with a suggestion for enhanced error handling.The initialization process in
newPlatformAppis well-structured, setting up DPI awareness and caching screen information effectively. However, consider enhancing the error handling to potentially halt the application if critical errors occur during initialization, as these might prevent the application from functioning correctly.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- mkdocs-website/docs/en/changelog.md (1 hunks)
- v3/examples/window/main.go (4 hunks)
- v3/pkg/application/application.go (2 hunks)
- v3/pkg/application/application_windows.go (5 hunks)
- v3/pkg/application/linux_cgo.go (8 hunks)
- v3/pkg/application/systemtray_windows.go (5 hunks)
- v3/pkg/application/webview_window.go (6 hunks)
- v3/pkg/application/webview_window_darwin.go (1 hunks)
- v3/pkg/application/webview_window_linux.go (2 hunks)
- v3/pkg/application/webview_window_windows.go (16 hunks)
Files not reviewed due to server errors (1)
- v3/pkg/application/webview_window_darwin.go
Additional comments not posted (32)
v3/pkg/application/systemtray_windows.go (2)
153-153: Function renaming approved for enhanced clarity.Renaming
getScreentogetScreenForWindowHwndimproves clarity by explicitly stating that the function retrieves the screen based on the window handle. This change helps in understanding the code's intent at a glance.
Line range hint
58-112: Refined window positioning logic: Approved with suggestions for improvement.The modifications to the
positionWindowmethod enhance the positioning logic by using actual window dimensions and better alignment calculations. The introduction ofcenterAlignXandcenterAlignYis a positive change for centering the window relative to the tray icon.However, consider adding detailed comments explaining the purpose of these calculations, especially the use of
PhysicalToDipRect, to aid future maintainability.Run the following script to verify the behavior across different DPI settings:
Verification successful
DPI Handling Verified: The application appears to handle DPI settings appropriately.
The codebase contains several references to DPI handling, including functions and constants related to DPI awareness and scaling. This suggests that the application is designed to handle different DPI settings, which aligns with the changes made to the
positionWindowfunction.
- Functions like
GetDpiForWindow,SetProcessDPIAware, andSetProcessDpiAwarenessContextindicate DPI handling.- Constants and functions related to DPI scaling, such as
ScaleWithDPIandWM_DPICHANGED, are present.These elements confirm that the application is equipped to manage different DPI settings effectively.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify window positioning across different DPI settings. # Test: Search for DPI-related issues in the application logs. rg --type log "DPI issue"Length of output: 176
Script:
#!/bin/bash # Search for any code handling DPI settings or adjustments in the codebase. ast-grep --lang go --pattern 'func $_($$$) { $$$ // Handle DPI settings $$$ }'Length of output: 86
Script:
#!/bin/bash # Search for terms related to DPI handling in the codebase. rg -i 'scale|resolution|DPI|display' --type go -A 5Length of output: 152941
mkdocs-website/docs/en/changelog.md (3)
21-21: Changelog entry is clear and well-documented.The entry for the new DIP system is concise, provides essential details, and correctly links to the PR. Good job on maintaining clarity and traceability.
21-21: Changelog entry is clear and well-documented.The entry for the window class name option is concise, provides essential details, and correctly links to the PR. Good job on maintaining clarity and traceability.
21-21: Changelog entry is clear and well-documented.The entry for expanded services providing plugin functionality is concise, provides essential details, and correctly links to the PR. Good job on maintaining clarity and traceability.
v3/examples/window/main.go (2)
23-24: Proper use of embedded assets.The use of
go:embedfor including assets directly in the binary is correctly implemented. This approach is beneficial for simplifying deployment and ensuring all necessary assets are included.
72-85: Correct integration ofWindowServiceand asset handling.The main function effectively integrates the
WindowServiceinto the application's service architecture. The use ofBundledAssetFileServerfor asset handling is appropriately implemented to serve embedded assets.v3/pkg/application/application.go (2)
279-281: Strategic addition ofscreenManager.The inclusion of
screenManagerin theAppstruct is a crucial enhancement for managing screen layouts and DPI settings more efficiently. This change aligns well with the PR's objectives to enhance DPI handling.
820-825: Informative comments on future use ofscreenManager.The comments detailing the future direct use of
screenManagerare clear and provide valuable insights into the planned evolution of screen management. Ensure these comments are kept updated as development progresses to reflect any changes or decisions.Also applies to: 827-831
v3/pkg/application/linux_cgo.go (6)
148-148: Approved: Addition ofscaleFactorto theScreenstruct.The new
scaleFactorfield enhances clarity and supports the improved DPI handling features introduced in this PR.
Line range hint
747-780: Approved: UpdatedgetScreenByIndexfunction to usescaleFactor.The function now correctly initializes the
Screenstruct with the newscaleFactorfield, aligning with the PR's objectives to enhance DPI handling.
850-861: Approved: UpdatedgetMousePositionfunction to includescaleFactor.The function now returns a
Screenobject with the newscaleFactorfield, ensuring DPI scaling is considered in mouse position calculations.
918-926: Approved: UpdatedgetCurrentMonitorGeometryfunction to returnscaleFactor.This function now correctly retrieves and returns the scale factor of the current monitor, aligning with the PR's focus on improved DPI handling.
879-884: Approved: Updatedfullscreenfunction to usescaleFactor.The function now uses the
scaleFactorto correctly size the window according to the current monitor's DPI settings.
903-914: Approved: UpdatedgetScreenfunction to includescaleFactor.The function now correctly retrieves the current screen's properties, including the new
scaleFactor, ensuring accurate DPI handling.v3/pkg/application/webview_window_windows.go (14)
51-51: New field added for DPI change handling.The addition of
ignoreDPIChangeResizingto thewindowsWebviewWindowstruct is a crucial part of managing resizing behavior when the DPI changes. This is in line with the PR's objectives to handle DPI changes more gracefully.
232-235: Setting initial window position and size.The use of
physicalBoundsfor setting the initial window position and size is a good application of the new DPI handling system. This ensures that the window is correctly placed according to the physical dimensions of the display, which is crucial for multi-monitor setups.
245-250: Handling window resizing across different DPI screens.This block effectively handles the scenario where the window might open on a secondary monitor with a different scale factor. It's a practical implementation to ensure the window size is adjusted correctly when the scale factor differs from the initial screen.
384-394: Method to calculate extended border sizes.The
getBorderSizesmethod calculates the extended border sizes, which is essential for accurate window sizing and positioning. This method helps in determining the non-client area of the window, which is crucial for precise layout calculations.
397-408: Physical bounds calculation method.The
physicalBoundsmethod correctly calculates the physical dimensions of the window. This method is vital for the new DPI handling system, ensuring that all window dimensions are based on physical rather than logical units.
410-424: Setting physical bounds with DPI change considerations.This method adjusts the window's physical bounds while temporarily ignoring DPI change resizing events. This is a smart approach to prevent unwanted resizing effects when moving the window across screens with different DPI settings.
426-434: Conversion between DIP and physical bounds.The methods
boundsandsetBoundsprovide a seamless way to convert between DIP and physical measurements, which is central to the functionality introduced in this PR. These methods ensure that the window management system can operate with logical DIP units internally while still respecting the physical pixel dimensions of the display.
436-455: Window size management using DIP units.The
size,setSize,width, andheightmethods have been updated to use DIP units, aligning with the PR's goals to handle high DPI settings more effectively. This change ensures that the window size is managed consistently across different DPI settings.
457-468: Window position management using DIP units.The
positionandsetPositionmethods have been updated to handle window positioning in DIP units. This update is crucial for maintaining consistent window positions across displays with varying DPI settings.
470-487: Relative positioning for multi-monitor setups.The methods
relativePositionandsetRelativePositionare well-designed to handle window positioning relative to the work area of the screen. This is particularly useful in multi-monitor setups where absolute positioning could lead to windows appearing in unexpected locations.
854-854: Screen retrieval method.The
getScreenmethod is crucial for obtaining the screen associated with the window. This method is essential for the new DIP system to function correctly, as it relies on accurate screen information to calculate positions and sizes.
1089-1098: Handling DPI changes dynamically.The handling of the
WM_DPICHANGEDmessage has been updated to respect theignoreDPIChangeResizingflag. This update is crucial for preventing unwanted resizing when the DPI changes, which is a key part of the PR's objectives to enhance DPI handling.
1216-1216: DPI retrieval method.The
DPImethod has been updated to useGetDpiForWindowwhen available, which provides more accurate DPI values. This update is important for ensuring that DPI calculations are as precise as possible, which is critical for the new DIP system.
1249-1250: Utility function for scaling with DPI.The
ScaleWithDPIfunction provides a utility for scaling pixel values according to a given DPI. This function is essential for the new DIP system, ensuring that all pixel measurements are scaled correctly based on the current DPI.v3/pkg/application/application_windows.go (2)
186-194: Note the absence ofprocessAndCacheScreensimplementation.The function
processAndCacheScreensis crucial for updating screen information but is not visible in the provided code. Ensure to review this function's implementation to confirm it handles screen updates correctly and efficiently.
Line range hint
1-315: Approve the overall structure and consistency of the file.The file is well-organized, and the code is consistently styled and documented, which aids in maintainability and readability.
v3/pkg/application/webview_window_linux.go (1)
108-109: Approved: Improved handling of DPI scaling in size constraints.The use of
scaleFactorto adjust window size constraints is a positive change, enhancing the accuracy of window sizing across different DPI settings.Run the following script to verify the functionality across different DPI settings:
Verification successful
Verified: Correct use of
scaleFactorin window size constraints.The methods
getCurrentMonitorGeometryandsetMinMaxSizeare correctly implemented and used to adjust window size constraints based on the monitor's DPI settings. The use ofscaleFactorensures accurate scaling of window sizes across different DPI environments.
getCurrentMonitorGeometryis defined inv3/pkg/application/linux_cgo.go.setMinMaxSizeis defined inv3/pkg/application/webview_window_linux.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the functionality of `disableSizeConstraints` across different DPI settings. # Test: Check the application behavior on monitors with different DPI settings. echo "Manually test the application on monitors with varying DPI settings to ensure correct window sizing."Length of output: 210
Script:
#!/bin/bash # Find the implementation of `getCurrentMonitorGeometry` function ast-grep --lang go --pattern 'func getCurrentMonitorGeometry() ($_, $_, $_, $_, $_) { $$$ }'Length of output: 94
Script:
#!/bin/bash # Find the implementation of `setMinMaxSize` function ast-grep --lang go --pattern 'func setMinMaxSize($_, $_, $_, $_) { $$$ }'Length of output: 75
Script:
#!/bin/bash # Search for `getCurrentMonitorGeometry` as a method ast-grep --lang go --pattern 'func ($_).getCurrentMonitorGeometry() ($_, $_, $_, $_, $_) { $$$ }'Length of output: 99
Script:
#!/bin/bash # Search for `setMinMaxSize` as a method ast-grep --lang go --pattern 'func ($_).setMinMaxSize($_, $_, $_, $_) { $$$ }'Length of output: 80
Script:
#!/bin/bash # Search for `getCurrentMonitorGeometry` in the codebase rg 'getCurrentMonitorGeometry'Length of output: 1328
Script:
#!/bin/bash # Search for `setMinMaxSize` in the codebase rg 'setMinMaxSize'Length of output: 673
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/examples/screen/screens.go (1 hunks)
Additional comments not posted (2)
v3/examples/screen/screens.go (2)
9-12: Review ofScreenServicestruct definition.The
ScreenServicestruct is well-defined with clear responsibilities. It holds a reference toscreenManagerand a booleanisExampleLayoutto toggle between real and example screen layouts. This struct serves as a foundation for the methods that manipulate screen data, which is crucial for the new DIP system.
14-18: Review ofGetSystemScreensmethod.This method correctly retrieves the system screens using the
application.Get().GetScreens()call and sets theisExampleLayouttofalse, indicating that the screens being processed are real system screens. The method is straightforward and effectively serves its purpose without any apparent issues.
|
@leaanthony I've fixed that by skipping DPI transformation on Linux & Mac for the system layout because there're no screens present in the screen manager until DPI is implemented there. By the way, on Linux (WSL) I was not getting |
|
Awesome 😎 Thanks for that. Do you have a list of things that need to be done before this can be merged? Just thinking how I can best help |
There's something I forgot to mention regarding Windows, the window is created with an invisible extended border around it, so setting the position to 0,0 results in offsetting the window by about 7px to the right, to overcome this you added a function that retrieves the extended border size and then offset the position by that much (https://github.com/wailsapp/wails/blob/v3-alpha/v3/pkg/application/webview_window_windows.go#L80-L105), I was able to use it while taking care of DPI transformation, but there's a problem with the initial position, this function doesn't work until the window is created and is visible, so to do the same with the initial position we have to first show the window then get the extended border size then offset it, causing an annoying position shift whenever the window is created, so for now I stopped offsetting the border but I kept the code there commented out until a solution is found. |
|
I would also like to propose (if it makes sense) changing the inputs & outputs for position & size methods from multiple params & returns |
|
I've simplified |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/application/screenmanager.go (1 hunks)
Additional comments not posted (17)
v3/pkg/application/screenmanager.go (17)
12-15: Struct definition looks good.The
ScreenManagerstruct definition is clear and concise, with appropriately named and typed fields.
17-30: Struct definition looks good.The
Screenstruct definition is comprehensive and includes all the necessary properties of a screen. The field names are descriptive and the types are appropriate.
32-37: Struct definition looks good.The
Rectstruct definition is simple and straightforward, with fields representing the properties of a rectangle.
39-42: Struct definition looks good.The
Pointstruct definition is simple and straightforward, with fields representing the coordinates of a point.
43-46: Struct definition looks good.The
Sizestruct definition is simple and straightforward, with fields representing the dimensions of a size.
75-81: Struct definition looks good.The
ScreenPlacementstruct definition represents the placement of a screen relative to its parent screen. The field names are descriptive and the types are appropriate.
83-88: Method implementation looks good.The
Originmethod forRectstruct returns the expected origin point of the rectangle. The implementation is straightforward and correct.
90-95: Method implementation looks good.The
Originmethod forScreenstruct returns the expected origin point of the screen. The implementation is straightforward and correct.
97-102: Method implementation looks good.The
Cornermethod forRectstruct returns the expected corner point of the rectangle. The implementation is straightforward and correct.
104-109: Method implementation looks good.The
InsideCornermethod forRectstruct returns the expected inside corner point of the rectangle. The implementation is straightforward and correct.
111-113: Method implementation looks good.The
rightmethod forRectstruct returns the expected right coordinate of the rectangle. The implementation is straightforward and correct.
115-117: Method implementation looks good.The
bottommethod forRectstruct returns the expected bottom coordinate of the rectangle. The implementation is straightforward and correct.
119-121: Method implementation looks good.The
rightmethod forScreenstruct returns the expected right coordinate of the screen by delegating to therightmethod of theBoundsrectangle. The implementation is straightforward and correct.
123-125: Method implementation looks good.The
bottommethod forScreenstruct returns the expected bottom coordinate of the screen by delegating to thebottommethod of theBoundsrectangle. The implementation is straightforward and correct.
127-137: Method implementation looks good.The
scalemethod forScreenstruct correctly scales a value based on the screen's scale factor. The rounding strategy of rounding up when scaling down and rounding down when scaling up is used to prevent drift over time when applying multiple scaling back and forth. The method uses themath.Ceilandmath.Floorfunctions for rounding. The implementation follows the scaling strategy described in the comments.
139-144: Method implementation looks good.The
Sizemethod forRectstruct returns the expected size of the rectangle. The implementation is straightforward and correct.
146-148: Method implementation looks good.The
IsEmptymethod forRectstruct correctly determines if the rectangle is empty based on its dimensions. The implementation is straightforward and correct.
|
Thanks for taking the time to push this issue forward @mmghv ! Considering this is a windows only solution for now, I propose that we create an experimental API specifically for it, like |
|
@leaanthony This is just an upgrade from the incomplete DPI system that was used, if you hidden it under If anything, this gives the developer more control over the system he wants to use, he can use default logical system, or use the newly added physical methods |
|
@leaanthony What were your thoughts for the default system if this was hidden under an experimental API? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Ok, thanks so much @mmghv for the insane amount of time this would have taken 🙏 I'm happy to get this into alpha and get feedback. 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
v3/pkg/application/screenmanager.go (1)
388-435: Add unit tests forcalculateScreensDipCoordinatesto ensure correct screen layoutsThe
calculateScreensDipCoordinatesmethod is critical for arranging screens correctly. Adding unit tests will help verify that it handles various screen configurations and edge cases as expected.Would you like assistance in creating unit tests for this method?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- v3/pkg/application/screenmanager.go (1 hunks)
Additional comments not posted (1)
v3/pkg/application/screenmanager.go (1)
127-138: LGTM: Thescalemethod handles scaling correctlyThe
scalemethod effectively manages rounding to prevent drift when converting between DIP and physical coordinates.
| // Heavily inspired by the Chromium project (Copyright 2015 The Chromium Authors) | ||
| // Chromium License: https://chromium.googlesource.com/chromium/src/+/HEAD/LICENSE | ||
|
|
There was a problem hiding this comment.
Ensure compliance with Chromium's license requirements
The code references and is inspired by the Chromium project. Please ensure that all necessary licensing information and attributions are included to comply with the Chromium license terms.
| if !isPhysical && len(excludedScreens) < len(m.screens)-1 { | ||
| // Make sure to give the same screen that would be given by the physical rect | ||
| // of this dip rect so transforming back and forth always gives the same result. | ||
| // This is important because it could happen that a dip rect intersects Screen1 | ||
| // more than Screen2 but in the physical layout Screen2 will scale up or Screen1 | ||
| // will scale down causing the intersection area to change so transforming back | ||
| // would give a different rect. | ||
| physicalRect := nearestScreen.dipToPhysicalRect(rect) | ||
| physicalRectScreen := m.screenNearestRect(physicalRect, true, nil) | ||
| if nearestScreen != physicalRectScreen { | ||
| if excludedScreens == nil { | ||
| excludedScreens = make(map[string]bool) | ||
| } | ||
| excludedScreens[nearestScreen.ID] = true | ||
| return m.screenNearestRect(rect, isPhysical, excludedScreens) | ||
| } | ||
| } | ||
| return nearestScreen |
There was a problem hiding this comment.
Prevent potential infinite recursion in screenNearestRect
The recursive call to screenNearestRect may lead to infinite recursion if the termination condition isn't met. Consider adding a maximum recursion depth or revising the logic to ensure that the recursion will always terminate.
| func (m *ScreenManager) getScreenAlignment(screen, parent *Screen) Alignment { | ||
| maxLeft := max(screen.X, parent.X) | ||
| maxTop := max(screen.Y, parent.Y) | ||
| minRight := min(screen.right(), parent.right()) | ||
| minBottom := min(screen.bottom(), parent.bottom()) | ||
|
|
||
| // Corners touching | ||
| if maxLeft == minRight && maxTop == minBottom { | ||
| if screen.Y == maxTop { | ||
| return BOTTOM | ||
| } else if parent.X == maxLeft { | ||
| return LEFT | ||
| } | ||
| return TOP | ||
| } | ||
|
|
||
| // Vertical edge touching | ||
| if maxLeft == minRight { | ||
| if screen.X == maxLeft { | ||
| return RIGHT | ||
| } else { | ||
| return LEFT | ||
| } | ||
| } | ||
|
|
||
| // Horizontal edge touching | ||
| if maxTop == minBottom { | ||
| if screen.Y == maxTop { | ||
| return BOTTOM | ||
| } else { | ||
| return TOP | ||
| } | ||
| } | ||
|
|
||
| return -1 // Shouldn't be reached | ||
| } |
There was a problem hiding this comment.
Define an explicit InvalidAlignment constant instead of returning -1
Returning -1 as an Alignment value can lead to unexpected behavior. It's better to define a constant like InvalidAlignment to represent invalid cases explicitly.
Apply this diff to define an explicit invalid alignment:
const (
TOP Alignment = iota
RIGHT
BOTTOM
LEFT
+ InvalidAlignment
)
...
func (m *ScreenManager) getScreenAlignment(screen, parent *Screen) Alignment {
...
- return -1 // Shouldn't be reached
+ return InvalidAlignment // Shouldn't be reached
}Committable suggestion was skipped due to low confidence.
|
Summary
This PR introduces a new Device Independent Pixels (DIP) system to enhance support for high DPI monitors and multi-monitor setups, it uses an algorithm that is highly inspired by Chromium which in turn is used in Electron.
Currently, this is implemented for Windows only but the algorithm part of it (in
screenmanager.go) is platform-agnostic and should work for the other systems (even on Mac where the Y axis is inverted).What's wrong with the current system?
Currently, there's a limited support for high DPI by scaling the window based on the the monitor DPI which the window is on, but this doesn't work with the initial window position/size because we don't know yet which monitor it will end up on, and scaling the size based on one screen could make it end up on another with incorrect size.
This is not only a problem with the initial placement, but also when changing the position of the window, you need to use logical values for both the position and size to be consistent, so using these logical values you can't determine which monitor the window will end up on to scale and position based on its DPI.
How does the new system work
The core of the new system is an algorithm, based on Chromium but simplified and enhanced with some changes and bug fixes.
The algorithm works by first retrieving all the system screens on startup and arranges them in a logical (DIP) space in a way that makes sense with the physical layout.
For example :
Now each screen has logical coordinates beside the physical ones, The screen list is cached for the application's lifetime and updated on system changes like monitor addition/removal, resolution changes, or taskbar movement.
Using this screen list, we now can map any
PointorRectbetween the physical and logical space using this set of methods:The conversion starts by finding the screen containing the point or the majority of the rect, then using this screen's scale factor we scale position and size and map it to the correct placement.
This system integrates with the position and size APIs, using DIP by default, so the developer can define/set/get in DIP, ensuring correct physical sizes on HiDPI screens, it also could be used to save & restore the window placement using DIP values.
Considerations
1- For the window, we have to always use the rect methods even if we're just changing the position, because for the proper DPI scaling the whole rect position & size should be talking into consideration.
But the developer doesn't have to worry about that, he just calls the available window APIs to get/set the position or size and internally the conversion is handled for him.
2- You should be carful when using WinAPIs as they use physical coordinates, Ensure all values are in the same coordinate system when combining coordinates, as shown in the system tray window positioning:
https://github.com/wailsapp/wails/pull/3665/files#diff-5b6ae6228f85608c5ea32108356e93d0233b4a7f13a1955c241adcd0102be235R73-R77
Disclaimer
Algorithm speed
The algorithm is actually very fast, here're some benchmarks:
It outperforms WinAPI calls because the screen list is cached.
Algorithm shortcomings
This algorithm is not perfect, it has its flaws.
For starter, you could face a problem if you wanted to map a point (e.g. mouse coordinates) to the window rect when it's in between screens with different DPI, the window will be scaled based on one DPI but the mouse coordinates mapping will change according to the screen you're on :
Additionally, there're also at least couple of edge cases with certain monitors setup, the first one where there could be DIP rects that don't have corresponding physical ones, and also there could be a physical rect that could be produced by two different DIP rects, causing one of them to incorrectly double-transform (
physical > dip > physical).(The red rectangle is the double transformed one, this should map to the original white rect)
However, these issues only occur when the window spans multiple monitors with different DPI (in certain layouts). I couldn't find a suitable solution for these issue for the time being.
Beside, Windows has even weirder bugs when dealing with DPI when the application is DPI unaware, so I don't think this system is worse than how Windows handles it.
Here's an example comparing the two while programmatically sliding the window:
Dpi.Unaware.Window.Bug.-.Position.Jumping.mp4
It's also better than the current state of Electron which has many bugs right now regarding DPI.
One additional flaw: Currently, this system may cause the application to crash when all the monitors are disconnected, this could be avoided however by doing some checks.
Physical Methods
To give the developer more control (maybe to overcome some of the edge cases when needed), we offer physical coordinate APIs, beside the screens having
PhysicalBounds, there're 4 new public window APIs :References
https://learn.microsoft.com/en-us/windows/win32/hidpi/high-dpi-desktop-application-development-on-windows
https://learn.microsoft.com/en-us/windows/win32/learnwin32/dpi-and-device-independent-pixels
https://learn.microsoft.com/en-us/windows/win32/gdi/the-virtual-screen
https://source.chromium.org/chromium/chromium/src/+/main:ui/display/win/screen_win.cc;l=317
@leaanthony @stffabi Please take your time reviewing this, Let me know if you need any additional info.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
ScreenManagerfor managing multiple displays, ensuring accurate positioning and scaling.ScreenServicefor managing screen-related functionalities and transformations.Bug Fixes
Documentation
Tests
Chores