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

Use UIScreen over keyWindow to get size, so that size can be used without host application #79

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

cltnschlosser
Copy link
Contributor

Before without a host app the keyWindow would be nil, resulting in the screen size being reported as "0x0", now the size will be correct.

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2019

CLA assistant check
All committers have signed the CLA.

@cltnschlosser
Copy link
Contributor Author

@alanzeino Can I get some feedback here? This is an improvement to using this framework for libraries which don't use host apps.

@rudro rudro requested a review from reidmain May 22, 2019 17:44
Copy link

@reidmain reidmain left a comment

Choose a reason for hiding this comment

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

I am sorry it has taken so long to respond @cltnschlosser. This PR looks solid and I don't any reason why we shouldn't merge it immediately.

I originally had some trepidation because I thought there would be some scenario where the main screen and key window wouldn't be the same size (perhaps some sort of iPad multiple screens testing or something). But the more I thought about it the more this current solution makes even less sense.

Why does adding the dimensions of the key window or main screen matter? It is because we're making the assumption that you'd do this if you wanted to snapshot a view controller's view and its dimensions should be the same as the window or screen? Shouldn't we just embed the dimensions of the view that is being snapshot then?

Regardless I tested your changes on projects that both have and don't have a host application and it worked great so 👍🏻.

@cltnschlosser
Copy link
Contributor Author

@reidmain Thanks for the review :) Now just need someone to merge!

The dimensions matter because without them the file names aren't unique, for example when using the following devices, here are the suffixes. Without the screensize the names would conflict.

'platform=iOS Simulator,name=iPhone 7,OS=11.3' -> _iPhone_11_3_375x667@2x.png
'platform=iOS Simulator,name=iPhone SE,OS=11.3' -> _iPhone_11_3_320x568@2x.png

'platform=iOS Simulator,name=iPhone X,OS=12.2' -> _iPhone_12_2_375x812@3x.png
'platform=iOS Simulator,name=iPhone 8 Plus,OS=12.2' -> _iPhone_12_2_414x736@3x.png
'platform=iOS Simulator,name=iPhone Xs Max,OS=12.2' -> _iPhone_12_2_414x896@3x.png

Long term the size of the view might be a better solution, but this works out fine :)

@reidmain
Copy link

@reidmain Thanks for the review :) Now just need someone to merge!

The dimensions matter because without them the file names aren't unique, for example when using the following devices, here are the suffixes. Without the screensize the names would conflict.

'platform=iOS Simulator,name=iPhone 7,OS=11.3' -> _iPhone_11_3_375x667@2x.png
'platform=iOS Simulator,name=iPhone SE,OS=11.3' -> _iPhone_11_3_320x568@2x.png

'platform=iOS Simulator,name=iPhone X,OS=12.2' -> _iPhone_12_2_375x812@3x.png
'platform=iOS Simulator,name=iPhone 8 Plus,OS=12.2' -> _iPhone_12_2_414x736@3x.png
'platform=iOS Simulator,name=iPhone Xs Max,OS=12.2' -> _iPhone_12_2_414x896@3x.png

Long term the size of the view might be a better solution, but this works out fine :)

Ah of course. This is an option mask so if you don't include the name of the device you can't unique it. That was the assumption I was making.

Alright to not make any breaking changes I'll merge this right now and we can look into addressing the larger issue in the future. Thanks again @cltnschlosser.

@reidmain reidmain merged commit 484d6e1 into uber:master Jun 25, 2019
lukaskukacka pushed a commit to lukaskukacka/ios-snapshot-test-case that referenced this pull request Jan 3, 2020
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.

3 participants