Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: get installed packages information #502
feat: get installed packages information #502
Changes from 2 commits
f0ca48b
a058f4a
22350a0
1fb0fea
45f9e58
48e1575
6d7b9b2
569febd
839febf
0515fa4
d97e538
fab7723
0a05c3f
615cb37
13b4fc6
1131105
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
the package manager instance should be retrieved once.
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.
Well first command will only give information of the activities information of the current opened application, and also the main issue is reading the dumpsys is another headache, maybe we can achieve but this seems the more efficient way of doing
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.
Thank you. So, this pr aims to show a few information which could be part of
adb shell dumpsys package <packagename>
via UIA2 server process instead ofadb
command, correct? (to clarify the intention)package name, an activity name which is specified as launcher category and the package name.
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.
Yes exactly, So is packageName and activityName correct? or should i rename to any other name for better understanding?
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 not a standard w3c endpoint, so we should not expose it like this. Consider adding this data into a custom /appium/device/... endpoint instead. Also, do you know how it is going to be called from the driver?
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.
For driver, we have to add in the driver Wrapper SDK right?
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 have changed the endpoint to /appium/device/apps
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.
does it make sense to have this endpoint without a session id?
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.
There should be a driver endpoint, which would expose this information. You could implement it in two ways, either using a
mobile: ...
extension or by adding a custom API endpoint and assume that clients would add a support for it.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.
No they can utilize the API with driver also right, see i need it before creating session but it's not necessary for everyone, they can also utilize it after creating a session like every other APIs
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'm ok to add a new endpoint, if not a complicated feature to keep maintaining, but I'd request to add test code to check the function (in this repo, or via uia2 driver). Endpoints which are not in regular WebDriver protocol and not used by UIA2 driver regularly could break because we don't care so much for as our project. (Actually, fork is the most flexible way to maintain your own way as Mykola addressed.)
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.
Well i can maintain it, but i think it's a quite a useful feature for the End User to have and i think it can be easily maintained and should not affect the existing implementation
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 have already commented appium/appium-uiautomator2-driver#585
It is just not going to work without a session id.
If you really need to have the endpoint in this rep I can propose to create two handlers for it - one with session id and one without (
session/:sessionId/appium/device/apps
and/appium/device/apps
). The latter could be then used in custom calls, but since the code would be shared, we can guarantee it works as expected.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.
Okay makes sense, i will do that