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 iOS landscape mode #1809

Merged
merged 8 commits into from
Aug 3, 2024
Merged

Fix iOS landscape mode #1809

merged 8 commits into from
Aug 3, 2024

Conversation

bartekpacia
Copy link
Contributor

Proposed changes

Aims to fix #565 by mapping coordinates according to current simulator orientation.

Testing

Tested locally.

I would like to have automated tests as well, but not sure yet how to go about it.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Jul 18, 2024

I think I got it working:

demorec.mp4

Current limitations:

  • breaks if orientation is changed during flow execution

  • breaks when simulator is in landscape, but app is portrait. This worked before these changes.

    Details
    demobug.mp4

    EDIT Maybe this could be useful here?

@bartekpacia
Copy link
Contributor Author

Another approach idea: instead of mapping input coordinates (e.g in tapOn), let's map output coordinates when querying the view hierarchy.

This will probably work better with Maestro Studio.

case .portrait: (requestBody.x, requestBody.y)
case .landscapeLeft: (width - requestBody.y, requestBody.x)
case .landscapeRight: (requestBody.y, height - requestBody.x)
default: fatalError("Not implemented yet")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honeslty not sure what's the right indentation here, I haven't been writing much Swift recently.

I created #1827 to make this situation better in the future:)

@amanjeetsingh150
Copy link
Collaborator

What do you think about following?

  1. How would a maestro user test this? Does this unblock all the recent requests that were inbound for landscape mode testing?
  2. Is android working on landscape? We don't need any change there?
  3. What do you think about this configuration to cloud?
  4. The scenario we discussed about mid way changing the orientation? For this lets' keep that scope out of this pr we can first roll this out and see how users react and then plan to pick that. WDYT?

@bartekpacia
Copy link
Contributor Author

How would a maestro user test this? Does this unblock all the recent requests that were inbound for landscape mode testing?

I believe it unblocks ~80% of the users who complained about landscape mode.

Is android working on landscape? We don't need any change there?

That's right, Android landscape has always worked.

What do you think about this configuration to cloud?

I consider this a quick-fix to unblock some people, who run tests locally. AFAIK Maestro Cloud doesn't allow for handling orientation changes. Maestro doesn't have a command for that as well (see #1221).

The scenario we discussed about mid way changing the orientation? For this lets' keep that scope out of this pr we can first roll this out and see how users react and then plan to pick that. WDYT?

I agree, yes!

Sum up

After this PR gets merged, there will be still 3 problems:

  • (Android & iOS) Both Maestro & Maestro Studio break if device is rotate mid-flow
    • possible fix: Don't cache deviceSize
  • (iOS) breaks when simulator is "physically" in landscape, but app is locked to portrait. I consider this a rare edge case and suggest we are fine with it.
    • possible fix: find iOS/XCUITest APIs that allow us to detect this situation and handle accordingly
  • (iOS) Maestro Studio is broken when device is in horizontal mode

But I believe this PR gets us the most value for now.

@bartekpacia
Copy link
Contributor Author

Last night I was trying out this approach, but it failed.

Yet another idea:

Maybe it's enough to just pass the current orientation there?

@amanjeetsingh150
Copy link
Collaborator

Maybe it's enough to just pass the current orientation there?

Sure you can check if it works. Btw heads up this is a private API so you have to check if its even available as an option.

@amanjeetsingh150
Copy link
Collaborator

I'm not sure if we discussed this. This could also be something we do in separate PR:

Right now maestro start-device doesn't,t support start device by default in landscape. I'm not even sure if that's possible. But it would be good if user also has an option to pass orientation while starting device.

What do you think?

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Jul 26, 2024

Maybe it's enough to just pass the current orientation there?

Sure you can check if it works. Btw heads up this is a private API so you have to check if its even available as an option.

I did, but it doesn't work (admittedly though I didn't try very hard). Here's the result of that small investigation (and here is what didn't work)

Right now maestro start-device doesn't support start device by default in landscape. I'm not even sure if that's possible. But it would be good if user also has an option to pass orientation while starting device.

That's right, maestro start-device doesn't have this functionality right now. Changing virtual device rotation is very easy (⌘ + ←/→), but I understand it may be very useful for users using maestro directly on CI (directly=not on managed CI like Maestro Cloud).

I made a separate issue for this: #1833

@bartekpacia bartekpacia merged commit ac0bb66 into mobile-dev-inc:main Aug 3, 2024
2 of 3 checks passed
@bartekpacia bartekpacia deleted the fix/ios_landscape_mode branch August 3, 2024 18:28
@chiefchief
Copy link

doesn't work on iPad

@bartekpacia
Copy link
Contributor Author

@chiefchief Can you tell more? Did you build Maestro yourself locally?

This is not yet released.

@chiefchief
Copy link

chiefchief commented Aug 4, 2024

@chiefchief Can you tell more? Did you build Maestro yourself locally?

This is not yet released.

as i understand, after the merge of this PR, new version was released, then i ran the command

curl -Ls "https://get.maestro.mobile.dev" | bash

which install maestro from the latest release
and the issue still exist on iPad

@bartekpacia
Copy link
Contributor Author

So this is expected. New version (1.37.7) was released before this PR was merged.

@chiefchief
Copy link

So this is expected. New version (1.37.7) was released before this PR was merged.

Got it, thank you, i'll check once new version will be released

rasyid7 pushed a commit to rasyid7/maestro that referenced this pull request Aug 14, 2024
bartekpacia added a commit that referenced this pull request Aug 15, 2024
bartekpacia added a commit that referenced this pull request Aug 15, 2024
bartekpacia added a commit that referenced this pull request Aug 29, 2024
bartekpacia added a commit that referenced this pull request Aug 29, 2024
bartekpacia added a commit that referenced this pull request Aug 29, 2024
bartekpacia added a commit that referenced this pull request Sep 9, 2024
bartekpacia added a commit that referenced this pull request Sep 9, 2024
bartekpacia added a commit that referenced this pull request Sep 9, 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.

Landscape support for iOS
3 participants