-
Notifications
You must be signed in to change notification settings - Fork 379
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: make adjusting screenshot coordinate configurable - screenshotOrientation settings api #277
fix: make adjusting screenshot coordinate configurable - screenshotOrientation settings api #277
Conversation
@@ -43,6 +43,7 @@ | |||
static NSString* const INCLUDE_NON_MODAL_ELEMENTS = @"includeNonModalElements"; | |||
static NSString* const ACCEPT_ALERT_BUTTON_SELECTOR = @"acceptAlertButtonSelector"; | |||
static NSString* const DISMISS_ALERT_BUTTON_SELECTOR = @"dismissAlertButtonSelector"; | |||
static NSString* const SKIP_ADJUSTING_SCREENSHOT_COORDINATE = @"skipAdjustingScreenshotCoordinate"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we adjust screenshot orientation rather than coordinates
Skip adjusting the screenshot coordinate. | ||
The adjustment helps to fix the screenshot coordinate when a user change the device orientation. | ||
But the logic sometimes cannot set the screenshot orientation properly. Skiping the logic can fix it. | ||
Xcode versions, OS versions or device models and simulator or real device could be related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...could influence this logic
@@ -43,6 +43,7 @@ | |||
static NSString* const INCLUDE_NON_MODAL_ELEMENTS = @"includeNonModalElements"; | |||
static NSString* const ACCEPT_ALERT_BUTTON_SELECTOR = @"acceptAlertButtonSelector"; | |||
static NSString* const DISMISS_ALERT_BUTTON_SELECTOR = @"dismissAlertButtonSelector"; | |||
static NSString* const SKIP_ADJUSTING_SCREENSHOT_ORIENTATION = @"skipAdjustingScreenshotOrientation"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about autoAdjustScreenshotOrientation
and set it to true
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. it also looks good. what about 8d033b2?
@@ -43,6 +43,7 @@ | |||
static NSString* const INCLUDE_NON_MODAL_ELEMENTS = @"includeNonModalElements"; | |||
static NSString* const ACCEPT_ALERT_BUTTON_SELECTOR = @"acceptAlertButtonSelector"; | |||
static NSString* const DISMISS_ALERT_BUTTON_SELECTOR = @"dismissAlertButtonSelector"; | |||
static NSString* const AUTO_ADJUST_SCREENSHOT_ORIENTATION = @"autoAdjustScreenshotOrientation"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could make it more flexible for clients by explicitly providing the appropriate orientation value, like UIImageOrientationLeft
or UIImageOrientationUp
and leaving it to auto
by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea
@@ -328,6 +329,12 @@ + (NSArray *)routes | |||
[FBConfiguration setDismissAlertButtonSelector:(NSString *)[settings objectForKey:DISMISS_ALERT_BUTTON_SELECTOR]]; | |||
} | |||
|
|||
#if !TARGET_OS_TV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
But the logic sometimes cannot set the screenshot orientation properly. Skiping the logic can fix it. | ||
Xcode versions, OS versions or device models and simulator or real device could influence this logic. | ||
|
||
@param orientation Set to NO in order to skip adjusting screenshot coordinate. Defaults to YES. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the docstring
// Only UIInterfaceOrientationUnknown is over iOS 8. Others are over iOS 2. | ||
// https://developer.apple.com/documentation/uikit/uiinterfaceorientation/uiinterfaceorientationunknown | ||
if (orientation == nil) { | ||
FBScreenshotOrientation = UIInterfaceOrientationUnknown; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be merged with else block
} else if (orientation == UIInterfaceOrientationPortraitUpsideDown) { | ||
imageOrientation = UIImageOrientationDown; | ||
// Apply auto rotation by default for iOS >= 11.0. In iOS < 11.0 screenshots are already adjusted properly. | ||
if (FBConfiguration.screenshotOrientation == UIInterfaceOrientationUnknown && SYSTEM_VERSION_GREATER_THAN_OR_EQUAL_TO(@"11.0")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather change this condition to always apply the current approach if the screenshotOrientation
is set to auto. Otherwise forcefully apply the method set by the client without any additional checks
/** | ||
@return The orientation as String for human read | ||
*/ | ||
+ (NSString *)screenshotOrientationForUser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humanReadableScreenshotOrientation
} else if (FBScreenshotOrientation == UIInterfaceOrientationLandscapeLeft) { | ||
return @"LandscapeLeft"; | ||
} | ||
return @"Auto"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make them starting with lowercase, like the other existing setting names
|
||
+ (NSString *)screenshotOrientationForUser | ||
{ | ||
if (FBScreenshotOrientation == UIInterfaceOrientationPortrait) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use switch
} else if (orientation == UIInterfaceOrientationPortraitUpsideDown) { | ||
imageOrientation = UIImageOrientationDown; | ||
if (FBConfiguration.screenshotOrientation == UIInterfaceOrientationUnknown) { | ||
if (orientation == UIInterfaceOrientationLandscapeRight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (SYSTEM_VERSION_LESS_THAN(@"11.0")) {
is missing here. The previous implementation should be kept unchanged
imageOrientation = UIImageOrientationDown; | ||
} else if (FBConfiguration.screenshotOrientation == UIInterfaceOrientationLandscapeRight) { | ||
imageOrientation = UIImageOrientationLeft; | ||
}else if (FBConfiguration.screenshotOrientation == UIInterfaceOrientationLandscapeLeft) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space
FBScreenshotOrientation = UIInterfaceOrientationPortraitUpsideDown; | ||
} else if ([orientation.lowercaseString isEqualToString:@"landscaperight"]) { | ||
FBScreenshotOrientation = UIInterfaceOrientationLandscapeRight; | ||
}else if ([orientation.lowercaseString isEqualToString:@"landscapeleft"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space
}else if ([orientation.lowercaseString isEqualToString:@"landscapeleft"]) { | ||
FBScreenshotOrientation = UIInterfaceOrientationLandscapeLeft; | ||
} else { | ||
FBScreenshotOrientation = UIInterfaceOrientationUnknown; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather throw an error if an unknown setting value is provided
return @"landscapeRight"; | ||
} else if (FBScreenshotOrientation == UIInterfaceOrientationLandscapeLeft) { | ||
return @"landscapeLeft"; | ||
switch (FBScreenshotOrientation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
NSError *error; | ||
if (![FBConfiguration setScreenshotOrientation:(NSString *)[settings objectForKey:SCREENSHOT_ORIENTATION] | ||
error:&error]) { | ||
return FBResponseWithStatus([FBCommandStatus invalidArgumentErrorWithMessage:error.description traceback:nil]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
@param orientation Set the orientation to adjust the screenshot. | ||
Case insensitive "portrait", "portraitUpsideDown", "landscapeRight" and "landscapeLeft" are available | ||
to force the coodinate adjust. Other wards are handled as "auto", which handles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wards?
FBScreenshotOrientation = UIInterfaceOrientationUnknown; | ||
} else { | ||
return [[FBErrorBuilder.builder withDescriptionFormat: | ||
@"No available orientation strategies. Available strategies are 'auto', 'portrate', " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather say The orientation value '%@' is not known. Only the following orientation values are supported: %@
I found the screenshot coordination should be handled as
SYSTEM_VERSION_LESS_THAN(@"11.0")
case, but it looked depending on the device under test environment.I found landscape screenshot should follow
SYSTEM_VERSION_LESS_THAN(@"11.0")
logic on various Xcode, iOS and model versions. It meant it was difficult to say onlySYSTEM_VERSION_LESS_THAN(@"11.0")
orSYSTEM_VERSION_LESS_THAN(@"12.0")
etc was always true in all Xcode 10, 11 environments for us (and in the future). It also could include on simulator/devices situations.So, let me add a setting to be able to configure the coordination on the user side when they face the screenshot orientation issue.
I tested on my local.
e.g.:
On Xcode 11.3 and iPhone 11 simulator,