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

[macOS] Fix GesturePlatformManager.CalculatePosition when asked for a relative position #19371

Merged

Conversation

MartyIX
Copy link
Collaborator

@MartyIX MartyIX commented Dec 12, 2023

Description of Change

This PR follows what I wrote here #19327 (comment). I'm not sure if the fix is correct or not. I just figured that creating a PR is an easy way to get some feedback.

Issues Fixed

Fixes #19329
Fixes #19327

@MartyIX MartyIX requested a review from a team as a code owner December 12, 2023 18:54
@ghost ghost added the community ✨ Community Contribution label Dec 12, 2023
@ghost
Copy link

ghost commented Dec 12, 2023

Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@MartyIX MartyIX changed the title WIP [macOS] Fix GesturePlatformManager.CalculatePosition when asked for a relative position Dec 12, 2023
@MartyIX
Copy link
Collaborator Author

MartyIX commented Dec 12, 2023

Cursor is in both cases in the right-bottom corner of the red robot image.

Before
Before

After
After

@PureWeen PureWeen self-requested a review December 12, 2023 19:24
@MartyIX
Copy link
Collaborator Author

MartyIX commented Dec 13, 2023

Btw: The change this PR reverts was introduced here e84098b#diff-ad1a05553c13fe18565124521da4efa63ec857d8d9b6b3e248c1c4ea4497fed5R172 (#12108).

edit: Hm, so #12108 fixed #8330 so that is probably the thing that should be tested.

@PureWeen
Copy link
Member

Can you add a test?

Example of one here #19283

@MartyIX
Copy link
Collaborator Author

MartyIX commented Dec 13, 2023

I can try but it would be great to know if the fix looks ok to you or not.

@MartyIX MartyIX force-pushed the feature/2023-12-12-pointer-gesture-relative branch from 919e470 to ee002f2 Compare December 13, 2023 20:46
@PureWeen
Copy link
Member

PureWeen commented Dec 14, 2023

I can try but it would be great to know if the fix looks ok to you or not.

I need to understand a bit more about why the new code doesn't work, and if the fix here will break other cases. The view that it's currently using is a container view that sometimes gets created around the platform view. In some cases that container view will have visible parts. For example, if you add a background to a Label that background will be a view around the label that displays the background . So, I think this PR will cause some scenarios like that to become incorrect.

My guess at what is happening here is that the ContainerView is expanding to fill the entire horizontal space and then the image is just occupying the center. I'm curious if that's intentional or if there's a bug with ContainerView and it needs to be sized more correctly.

@MartyIX
Copy link
Collaborator Author

MartyIX commented Dec 14, 2023

@PureWeen I don't know the code in depth to add any meaningful comment right now. However, I believe it's worth taking a step back and trying to first establish what platform is actually affected by a bug. For that purpose I have modified #19329 (comment) and added animations of the behavior on Windows and macOs for the same MAUI application.

The fact that the same source code leads to different outcomes on both platforms leads me to believe it's a bug.

However, given your previous response, I'm not actually sure if Windows or macOS behaves differently than as expected.

Could you take a look at those two animations please?

@Eilon Eilon added the area-gestures Gesture types label Dec 14, 2023
@PureWeen
Copy link
Member

PureWeen commented Dec 15, 2023

@PureWeen I don't know the code in depth to add any meaningful comment right now. However, I believe it's worth taking a step back and trying to first establish what platform is actually affected by a bug. For that purpose I have modified #19329 (comment) and added animations of the behavior on Windows and macOs for the same MAUI application.

The fact that the same source code leads to different outcomes on both platforms leads me to believe it's a bug.

However, given your previous response, I'm not actually sure if Windows or macOS behaves differently than as expected.

Could you take a look at those two animations please?

Nevermind :-) I see what's going on here now. We just made a bad refactor and your revert makes sense

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX MartyIX force-pushed the feature/2023-12-12-pointer-gesture-relative branch from d152abd to 35dd0f7 Compare December 19, 2023 18:54
@MartyIX MartyIX force-pushed the feature/2023-12-12-pointer-gesture-relative branch from 35dd0f7 to c0b1672 Compare December 22, 2023 09:27
@PureWeen PureWeen force-pushed the feature/2023-12-12-pointer-gesture-relative branch from c0b1672 to ee64161 Compare January 4, 2024 15:49
@PureWeen
Copy link
Member

PureWeen commented Jan 4, 2024

/azp run

@dotnet dotnet deleted a comment from azure-pipelines bot Jan 4, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jan 4, 2024
@PureWeen
Copy link
Member

PureWeen commented Jan 4, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Jan 5, 2024

OpenQA.Selenium.WebDriverException : An unknown server-side error occurred while processing the command. Original error: Error Domain=io.appium.WebDriverAgentMac Code=1 "Only pointer type 'mouse' is supported. 'touch' is given instead for action with id '0164df41-a79d-42c1-9f78-c4742f81238c'" UserInfo={NSLocalizedDescription=Only pointer type 'mouse' is supported. 'touch' is given instead for action with id '0164df41-a79d-42c1-9f78-c4742f81238c'}

I'll work on this one tomorrow

we're close

PureWeen and others added 2 commits January 5, 2024 10:10
….xaml

Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
@PureWeen
Copy link
Member

PureWeen commented Jan 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen previously approved these changes Jan 5, 2024
PureWeen and others added 2 commits January 8, 2024 10:05
Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
@PureWeen
Copy link
Member

PureWeen commented Jan 8, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) January 8, 2024 17:49
@PureWeen PureWeen merged commit c1dfd70 into dotnet:main Jan 9, 2024
47 checks passed
@MartyIX MartyIX deleted the feature/2023-12-12-pointer-gesture-relative branch January 9, 2024 15:28
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-gestures Gesture types community ✨ Community Contribution
Projects
None yet
5 participants