-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
Make scroll tracking configurable with a cap #284
Conversation
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.
looks good, couple small things
if (session.hasCapability(TRACK_SCROLL_EVENT_CAP)) { | ||
try { | ||
trackScrollEvents = (Boolean) session.getCapability(TRACK_SCROLL_EVENT_CAP); | ||
Logger.error("Capability '" + TRACK_SCROLL_EVENT_CAP + "': " + trackScrollEvents); |
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.
is this meant to be an error log?
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, no. That was just all that would show up in my logs without fiddling. I'll fix.
setResult(doTouchUp(x, y)); | ||
} | ||
}); | ||
} else { |
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.
spacing seems off in this block
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.
LGTM. i do still worry about breaking any existing use cases, so would prefer for this cap to default to true
on the appium side, maybe with a deprecation notice for people not including the cap that the behavior will change?
That is the plan. In |
Logger.debug("Capability '" + TRACK_SCROLL_EVENT_CAP + "': " + trackScrollEvents); | ||
} catch (Exception e) { | ||
Logger.error("Could not set '" + TRACK_SCROLL_EVENT_CAP + "' from capability: ", e); | ||
trackScrollEvents = true; |
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 is redundant
try { | ||
trackScrollEvents = (Boolean) session.getCapability(TRACK_SCROLL_EVENT_CAP); | ||
Logger.debug("Capability '" + TRACK_SCROLL_EVENT_CAP + "': " + trackScrollEvents); | ||
} catch (Exception e) { |
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.
is getCapability supposed to throw an exception?
Boolean trackScrollEvents = true; | ||
if (session.hasCapability(TRACK_SCROLL_EVENT_CAP)) { | ||
try { | ||
trackScrollEvents = (Boolean) session.getCapability(TRACK_SCROLL_EVENT_CAP); |
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 using String.format?
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.
same below
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 personally think the else
ought to be there when it is, logically, an else, even if the return
makes it technically "redundant".
setResult(doTouchDown(x, y)); | ||
} | ||
}); | ||
} else { |
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.
else is redundant
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.
same below
@@ -32,6 +36,7 @@ | |||
private static final String METHOD_TOUCH_DOWN = "touchDown"; | |||
private static final String METHOD_TOUCH_UP = "touchUp"; | |||
private static final String METHOD_TOUCH_MOVE = "touchMove"; | |||
private static final String TRACK_SCROLL_EVENT_CAP = "trackScrollEvents"; |
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 make it an appium setting
Add
trackScrollEvents
cap, to configure the tracking of scroll movement. This improves performance of touch actions significantly:Using a simple touch action:
trackScrollEvents
set totrue
, averages 1710mstrackScrollEvents
set tofalse
, averages 194msThis could be changed to a setting if it is found to be necessary to switch between configurations mid-session.