Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(macos/input): incorrect mouse input for non-main display #2461

Merged
merged 25 commits into from
May 2, 2024

Conversation

TimmyOVO
Copy link
Contributor

Description

fix: abs mouse input not working in macOS
fix: wrong abs mouse coordinates when capturing non-main display in macOS
fix: wrong mouse coordinates correction when capturing non-main display in macOS

Screenshot

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

TimmyOVO added 22 commits April 20, 2024 04:03
fix: wrong abs mouse coordinates when capturing non-main display in macOS
fix: wrong mouse coordinates correction when capturing non-main display in macOS
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 6.18%. Comparing base (7fb8c76) to head (4d89d73).

Additional details and impacted files
@@            Coverage Diff             @@
##           nightly   #2461      +/-   ##
==========================================
+ Coverage     6.17%   6.18%   +0.01%     
==========================================
  Files           86      86              
  Lines        17546   17548       +2     
  Branches      8190    8186       -4     
==========================================
+ Hits          1083    1085       +2     
+ Misses       15410   14655     -755     
- Partials      1053    1808     +755     
Flag Coverage Δ
Linux 4.26% <ø> (ø)
Windows 2.04% <ø> (ø)
macOS-12 8.73% <18.18%> (+0.14%) ⬆️
macOS-13 ?
macOS-14 8.19% <18.18%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/platform/macos/display.mm 42.15% <100.00%> (+1.15%) ⬆️
src/platform/macos/input.cpp 2.39% <0.00%> (ø)

... and 27 files with indirect coverage changes

@ReenigneArcher ReenigneArcher requested a review from cgutman April 23, 2024 13:09
@ReenigneArcher
Copy link
Member

I don't have the ability to test these changes. Ideally unit tests would be added around the changed code, but I understand that's not always the easiest thing to do.

Can you explain how you tested your changes?

Copy link
Contributor Author

@TimmyOVO TimmyOVO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all i done here in order to get correct input coordinates after we support specific display capture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this commit, there is a change that would require both env_width and env_height to have non-zero values, but these values are not properly set in that commit, which could break absolute mouse input on macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we support change the display we need to capture in macOS, the location we get here according to Apple Document should be The current location of the specified mouse event in global display coordinates, we can not just simply assuming the display's bounds to [0,display_width] and [0,display_height]. So we need to get the correspond display bounds for display we are capturing using CGDisplayBounds which return The bounds of the display, expressed as a rectangle in the global display coordinate space (relative to the upper-left corner of the main display)..


CGPoint location = CGPointMake(x * scaling, y * scaling);

CGRect display_bounds = CGDisplayBounds(display);
// in order to get the correct mouse location for capturing display , we need to add the display bounds to the location
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The location we received are relative to the display we are capturing ,so we get the display bounds using CGDisplayBounds then add them up to get the right input coordinates.

@BayLee4
Copy link
Contributor

BayLee4 commented May 2, 2024

Hi, just wanted to report that I tested the changes (by forking the homebrew tap repository and then pointing it to the right branch/commit) and confirm the issue is now gone, everything[1] works perfectly 👍

[1]: Well except the CTRL + ALT + SHIFT + N shortcut which still doesn't hide the cursor in anything but the main display, but that behavior is already present upstream so that would be a different issue

@ReenigneArcher ReenigneArcher merged commit 9d5ee2f into LizardByte:nightly May 2, 2024
51 of 53 checks passed
@Hazer Hazer mentioned this pull request May 30, 2024
11 tasks
KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants