wails icon indicating copy to clipboard operation
wails copied to clipboard

[v3-Windows] New DIP system for Enhanced High DPI Monitor Support

Open mmghv opened this issue 1 year ago • 6 comments

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 :

1

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 Point or Rect between the physical and logical space using this set of methods:

DipToPhysicalPoint(dipPoint Point) Point
PhysicalToDipPoint(physicalPoint Point) Point
DipToPhysicalRect(dipRect Rect) Rect
PhysicalToDipRect(physicalRect Rect) Rect

2

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.

// Get window dip bounds
func (w *windowsWebviewWindow) bounds() Rect {
	return PhysicalToDipRect(w.physicalBounds())
}

// Set window dip bounds
func (w *windowsWebviewWindow) setBounds(bounds Rect) {
	w.setPhysicalBounds(DipToPhysicalRect(bounds))
}

// Set dip position
func (w *windowsWebviewWindow) setPosition(x int, y int) {
	bounds := w.bounds()
	bounds.X = x
	bounds.Y = y

	w.setBounds(bounds)
}

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

  • I don't have access to HiDPI monitors so all the testing was done using Windows scaling settings which has the same effect as using a different DPI monitors (with some differences).
  • I'm not a Mac user/developer so I don't know how they handle HiDPI there or whether or not this system is even needed.

Algorithm speed

The algorithm is actually very fast, here're some benchmarks:

  • Initial screens layout: 2us (500K opr/s) for 3 screens
  • DipToPhysicalPoint(): 30ns (33M opr/s)
  • DipToPhysicalRect(): 200ns (5M opr/s)

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 :

3

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).

4

(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:

https://github.com/user-attachments/assets/92072807-60ed-47d5-8df3-918caf9ceb5c

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 :

// PhysicalBounds returns the physical bounds of the window
(w *WebviewWindow) PhysicalBounds() Rect

// SetPhysicalBounds sets the physical bounds of the window
(w *WebviewWindow) SetPhysicalBounds(physicalBounds Rect)

// Bounds returns the DIP bounds of the window
(w *WebviewWindow) Bounds() Rect

// SetBounds sets the DIP bounds of the window
(w *WebviewWindow) SetBounds(bounds Rect)

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.

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

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.

  • [x] Windows
  • [ ] macOS
  • [ ] Linux

Checklist:

  • [x] I have updated website/src/pages/changelog.mdx with details of this PR
  • [x] My code follows the general coding style of this project
  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • New Features

    • Introduced a comprehensive set of examples for multi-monitor setups, showcasing configurations for normal, test, and edge cases.
    • Added interactive UI elements for manipulating window properties, including size and position.
    • Enhanced window management with new services for handling physical dimensions and DPI awareness.
    • Implemented a robust ScreenManager for managing multiple displays, ensuring accurate positioning and scaling.
    • Integrated a new DPI system to improve support for high DPI monitors.
    • Added methods to manage and retrieve window dimensions and positions more effectively.
    • Enhanced macosWebviewWindow with methods for handling physical bounds and window dimensions.
  • Bug Fixes

    • Improved error handling and feedback for screen enumeration and DPI management processes.
  • Documentation

    • Updated documentation to clarify new functionalities and configurations related to screen management and window properties.
    • Added a new section in the changelog regarding enhanced DPI support for high DPI monitors.
  • Tests

    • Added a test suite to validate the behavior of screen layout management and transformation functionalities.

mmghv avatar Aug 05 '24 08:08 mmghv

[!CAUTION]

Review failed

The pull request is closed.

Walkthrough

The recent updates enhance multi-monitor management and window handling within the application. Key improvements include the introduction of a ScreenService for managing screen properties, a comprehensive ScreenManager for layout handling, and refined methods for window positioning. Additionally, enhancements to DPI awareness for high-resolution displays have been implemented, collectively elevating the user experience across diverse display configurations.

Changes

Files Change Summary
v3/examples/screen/assets/examples.js Introduced a global window.examples array for multi-monitor configurations, structured into various example sections.
v3/examples/screen/assets/index.html Overhauled HTML and CSS for improved layout and interactivity; added new script imports for enhanced functionality.
v3/examples/screen/assets/main.js Added functionalities for managing screen layouts with interactive UI elements and layout processing logic.
v3/examples/screen/main.go Integrated a new logging mechanism and service management for enhanced screen handling capabilities.
v3/examples/screen/screens.go Introduced ScreenService for managing screen data, including methods for processing and transforming screen info.
v3/pkg/application/application.go Added screenManager field to App struct to enhance screen management capabilities.
v3/pkg/application/application_windows.go Removed methods for retrieving screen details; added setupDPIAwareness function for improved DPI handling.
v3/pkg/application/linux_cgo.go Updated Screen struct to use scaleFactor, enhancing naming consistency.
v3/pkg/application/linux_purego.go Standardized scaling terminology from Scale to ScaleFactor in screen-related functions.
v3/pkg/application/screen_darwin.go Improved Screen struct by renaming scale to scaleFactor, enhancing semantic clarity.
v3/pkg/application/screenmanager.go Added ScreenManager for comprehensive screen management, handling layout and DPI transformations.
v3/pkg/application/screenmanager_test.go Created a test suite for ScreenManager, validating layout management and transformations.
mkdocs-website/docs/en/changelog.md Added entry for new DPI system enhancing support for high-DPI monitors.
v3/examples/systray/main.go Modified webview window height parameter from 800 to 500 pixels.

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

🐰 In a world of screens so wide,
A rabbit hops with joy and pride.
With DPI and bounds, all set to play,
Multi-monitor magic brightens the day!
Hopping through code, with leaps so spry,
In this pixel paradise, we’ll soar high! 🌈


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example: -- I pushed a fix in commit <commit_id>, please review it. -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples: -- @coderabbitai generate unit testing code for this file. -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase. -- @coderabbitai read src/utils.ts and generate unit testing code. -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format. -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Aug 05 '24 08:08 coderabbitai[bot]

Thanks for this amazing PR @mmghv! 🙏 A couple of quick questions to start us off :-)

  • It looks like SetThreadDpiAwarenessContext isn't being used anywhere. Should we remove it?
  • Do you know what the implications are to the change in APIs for Mac/Linux, where previously they were dealing in pixels?

leaanthony avatar Aug 06 '24 05:08 leaanthony

@leaanthony

  • It looks like SetThreadDpiAwarenessContext isn't being used anywhere. Should we remove it?

You're right, I might have added it when I was experimenting with DPI awareness and forgot to remove it, will do now.

  • Do you know what the implications are to the change in APIs for Mac/Linux, where previously they were dealing in pixels?

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 Bounds & PhysicalBouns which both passthrough to the other ones right now. But there's a breaking change regarding the screen struct where I renamed Scale to ScaleFactor.

I tested on Linux (on WSL) to make sure it still works but Mac need to be tested.

Edit: By the way, there seemed to be a bug regarding moving the window on Linux, moving in the X axis caused the window to also move on the Y axis somehow, not sure if that was a WSL thing, I tested using the window example using the new UI controls.

mmghv avatar Aug 06 '24 05:08 mmghv

@mmghv - There was a compile issue on Mac and interestingly the 2x probe buttons end up issuing an error in the console: ERR Error calling method: runtime error: invalid memory address or nil pointer dereference. When debugging, it seems the first probe button calls in once and the second calls in twice. The second button's first call works but then the second comes in and it errors. I haven't had chance to track it down yet

leaanthony avatar Aug 06 '24 10:08 leaanthony

@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 system layout is selected and not the examples, it gives cryptic errors and I'm not able to debug it right now because my WSL is busy with other stuff and acting weird, anyways I've pushed a commit filling missing screens struct fields on Linux and Mac so the screens example renders nicely on the system layout.

mmghv avatar Aug 06 '24 13:08 mmghv

@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.

leaanthony avatar Sep 09 '24 10:09 leaanthony

@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 nil dereference error because it was swallowed by signal 11 received but handler not on signal stack which I believe is related to #2134

mmghv avatar Sep 09 '24 16:09 mmghv

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

leaanthony avatar Sep 09 '24 19:09 leaanthony

  • To be thoroughly reviewed (would love to hear @stffabi opinion)
  • To be tested on real HiDPI monitors
  • Possibly looking into the algorithm shortcomings
  • Researching Linux and macOS implementation (but that could come after this is merged).

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.

mmghv avatar Sep 09 '24 20:09 mmghv

I would also like to propose (if it makes sense) changing the inputs & outputs for position & size methods from multiple params & returns (int, int) to Point & Size which are already utilized by the internal methods.

mmghv avatar Sep 09 '24 20:09 mmghv

I've simplified distanceFromRectSquared() using the suggested changes from AI after making sure the distance is calculated exactly the same in all cases.

mmghv avatar Sep 09 '24 22:09 mmghv

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 SetPositionDPI, as it is a reasonable way off from being a complete alternative. What are your thoughts on that?

leaanthony avatar Sep 10 '24 21:09 leaanthony

@leaanthony This is just an upgrade from the incomplete DPI system that was used, if you hidden it under SetPositionDPI, what would SetPosition be? the old system? doesn't make sense, no DPI system at all? would be a regression .. so IMHO I don't see a good reason for hiding this under a special API.

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 PhysicalBounds(), SetPhysicalBounds() beside physical and logical coordinates for the screen.

mmghv avatar Sep 10 '24 22:09 mmghv

@leaanthony What were your thoughts for the default system if this was hidden under an experimental API?

mmghv avatar Sep 10 '24 22:09 mmghv

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. 🙏

leaanthony avatar Sep 21 '24 22:09 leaanthony

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarqubecloud[bot] avatar Sep 21 '24 22:09 sonarqubecloud[bot]