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

feat: add device.tap() and device.longPress(). #4542

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

gayablau
Copy link

@gayablau gayablau commented Aug 1, 2024

Description

In this pull request, I have added api for device tap and long press for android and IOS
fixed review comments in #4534


For features/enhancements:

  • I have added/updated the relevant references in the documentation files.

For API changes:

  • I have made the necessary changes in the types index file.

@gayablau gayablau changed the base branch from master to feat/device-tap August 1, 2024 07:24
@gayablau gayablau changed the title Feat device.tap() ios feat: add device.tap() and device.longPress(). Sep 7, 2024
@gayablau gayablau changed the base branch from feat/device-tap to master September 7, 2024 08:42
@gayablau gayablau marked this pull request as ready for review September 7, 2024 09:31
@asafkorem asafkorem self-assigned this Sep 11, 2024
@asafkorem
Copy link
Contributor

Unit test failed due to code coverage issues.
@gayablau please make sure your tests cover the following lines (or remove them if they are dead-codes).

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
invocationTraceDescriptions.js 98.07 93.33 98.03 98.07 6
mapDeviceLongPressArguments.js 91.66 94.44 100 91.66 34-36

Copy link
Contributor

@asafkorem asafkorem left a comment

Choose a reason for hiding this comment

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

Relatively small comments compared to this massive, impressive, awesome work. This is REALLY great work, @gayablau. 100 / 100! 🫡 We are close!


@Override
public void perform(UiController uiController, View view) {
int adjustedY = shouldIgnoreStatusBar ? y + UiAutomatorHelper.getStatusBarHeight(view) : y;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's extract that to private method that will be shared with the longPress's perform (reuse & separation of concerns) → e.g. calculateAdjustedY(y, shouldIgnoreStatusBar) or something similar.

Comment on lines +119 to +124
@SuppressLint({"DiscouragedApi", "InternalInsetResource"})
public static int getStatusBarHeight(View view) {
Context context = view.getContext();
int resourceId = context.getResources().getIdentifier("status_bar_height", "dimen", "android");
return (int) (context.getResources().getDimensionPixelSize(resourceId) / ((float) context.getResources().getDisplayMetrics().densityDpi / DisplayMetrics.DENSITY_DEFAULT));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to avoid confusion, could you please exctrat the method with the same name from isDisplayingAtLeastDetoxMatcher to this file? They perform a similar task but return coordinates in different units (px vs dp), so I would suggest creating two distinct method names to differentiate them:

  • getStatusBarHeightPixels
  • getStatusBarHeightDps

@SuppressLint("InternalInsetResource", "DiscouragedApi")
private fun getStatusBarHeight(view: View): Int {
val resourceId = view.context.resources.getIdentifier("status_bar_height", "dimen", "android")
return if (resourceId > 0) view.context.resources.getDimensionPixelSize(resourceId) else 0
}

Copy link
Contributor

Choose a reason for hiding this comment

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

just a scout rule that'll make the code cleaner 🙏

Comment on lines +11 to +21
func findElement(from params: InvocationParams, predicateHandler: PredicateHandler) -> XCUIElement {

let element = predicateHandler.findElement(using: params)

let exists = element.waitForExistence(timeout: .defaultTimeout)
DTXAssert(
exists,
"Action failed, element with matcher `\(params.matcherDescription)` does not exist"
)
return element;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reindent

Comment on lines +29 to +33
let screenFrame = try XCUIApplication.appUnderTest().frame
let normalizedX = CGFloat(x) / screenFrame.width
let normalizedY = CGFloat(y) / screenFrame.height
let normalizedPoint = CGVector(dx: normalizedX, dy: normalizedY)
let coordinate = try XCUIApplication.appUnderTest().coordinate(withNormalizedOffset: normalizedPoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's avoid repeated calls to XCUIApplication.appUnderTest() by storing it in a variable

Comment on lines +36 to +38
} catch {
throw Error.failedToTapDeviceByCoordinates
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you're going to throw this error anyway if you catch anything (Error.failedToTapDeviceByCoordinates) since you already wrapped this method with try-catch on the switch-case (on handle)

Copy link
Contributor

Choose a reason for hiding this comment

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

You may avoid the try-catch here and just let it be caught by the caller, or remove the duplicated try-catch from the caller(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not even sure if we need any of these catches since they might hide other errors that are more specific. Please revisit the error throwing logic.

func getNormalizedCoordinate(from params: InvocationParams) throws -> XCUICoordinate {
do {
guard let x = Int(params.params?.first ?? "100"), let y = Int(params.params?[1] ?? "100") else {
throw Error.missingTypeTextParam
Copy link
Contributor

Choose a reason for hiding this comment

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

you're throwing this error but catching it and hiding it (with failedToTapDeviceByCoordinates, see below).

Copy link
Contributor

Choose a reason for hiding this comment

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

also this is not the right error to throw, you might want to make this error more generic by introducing params to it, e.g. missingParam(paramName, paramType)

Comment on lines +419 to +430
### `device.tap(point, shouldIgnoreStatusBar)`

Perform a tap at arbitrary coordinates on the device's screen.

#### tap parameters

| parameter | platform | description | default value
| -------- | ------- | ------- | ------- |
| point | Android & IOS | Coordinates in the element's coordinate space | `{x: 100, y: 100}`
| shouldIgnoreStatusBar | Android | Coordinates will be measured starting from under the status bar. | `true`

#### tap examples
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be nitpicky and that's an nice structure, but we usually describe the parameters within bullet points and then just put examples without the headlines. For example: https://wix.github.io/Detox/docs/api/actions#swipedirection-speed-normalizedoffset-normalizedstartingpointx-normalizedstartingpointy

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.

Introduce device.tap(point), device.longPress(point) API's
2 participants