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: get installed packages information #502

Merged
merged 16 commits into from Mar 9, 2023

Conversation

shashank2086
Copy link
Contributor

@shashank2086 shashank2086 commented Mar 4, 2023

Getting all Installed and System apps of your device with Name and Main Activity for easily selecting and starting automation of any Application.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 4, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@mykola-mokhnach
Copy link
Contributor

What is the purpose of this extension? Packages querying could be easily implemented on the driver level via ADB.

@shashank2086
Copy link
Contributor Author

Well with Adb it only gives me package name of the application, Like for my requirement, i wanted to get more information like the app name for easy selection and The Activity name which i need to for creating a session. And also if appium server is giving me directly the app list then i don't need to run the terminal command.

@mykola-mokhnach
Copy link
Contributor

Adb it only gives me package name of the application

Probably you just need to use a different command then. Generally, adb shell pm can fetch all needed information about installed packages.

@shashank2086
Copy link
Contributor Author

I have tried alot but couldn't find the commands to get exact information i wanted that's why i wrote this enhancement, I think it's more useful and efficient if the appium server is able to get me these informations as well.

@KazuCocoa
Copy link
Member

adb shell dumpsys activity activities and adb shell dumpsys package <packagename> as well? Perhaps they will give more information though.

I'm curious about what information could get only via this

@@ -161,6 +162,7 @@ private void registerPostHandler() {

private void registerGetHandler() {
register(getHandler, new Status("/status"));
register(getHandler,new Getpackages("/apps"));
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 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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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 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.

Copy link
Contributor Author

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

Copy link
Member

@KazuCocoa KazuCocoa Mar 7, 2023

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.)

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

List<ApplicationInfo> apps = ServerContext.getPackageManager().getInstalledApplications(PackageManager.GET_META_DATA);
for(ApplicationInfo appInfo: apps) {
PackageModel model = new PackageModel();
if(ServerContext.getPackageManager().getLaunchIntentForPackage(appInfo.packageName)!=null) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adb shell dumpsys activity activities and adb shell dumpsys package <packagename> as well? Perhaps they will give more information though.

I'm curious about what information could get only via this

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

Copy link
Member

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 of adb command, correct? (to clarify the intention)
package name, an activity name which is specified as launcher category and the package name.

Copy link
Contributor Author

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?

@mykola-mokhnach
Copy link
Contributor

Please assign a proper name to the pull requests according to https://www.conventionalcommits.org requirements

@KazuCocoa KazuCocoa changed the title Appium packages feat: get installed packages information Mar 5, 2023
Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

forgot to submit a comment in addition to a question about adb shell dumpsys activity activities and adb shell dumpsys package <packagename>

@shashank2086 shashank2086 reopened this Mar 6, 2023
// Filtering out unnecessary sub packages without Intent
if (manager.getLaunchIntentForPackage(appInfo.packageName) != null) {
appDetails.add(new PackageModel(appInfo.packageName,
manager.getLaunchIntentForPackage(appInfo.packageName).getComponent().getClassName(),
Copy link
Member

Choose a reason for hiding this comment

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

What happen when getLaunchIntentForPackage(appInfo.packageName) is null, or getComponent is null. They are nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They both are Nullable, if an application has an intent then for sure it will have a component, so application with no component will automatically get filter out from the 46 condition and also getComponent is also Nullable

Copy link
Member

Choose a reason for hiding this comment

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

ah, missed here called the method twice.
This endpoint tries to show installed packages and each information, then maybe if would be nice to keep the package name (since it exists) but no activities rather than filtering the package out entirely. What do you think?
So, the user can understand some packages exist in the device but they has no launchable activities by this endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intend was to show all the main applications since the package without Intent is just a subpackage of it's parent package which can just create confusion to the user and actually it's pretty much useless, if later on anyone needs that then surely we can modify accordingly but i feel it's unnecessary right now to show?

Copy link
Member

Choose a reason for hiding this comment

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

Then, could you leave the intention as docstring in this class for future maintenance?
GetPackages made me think the app package is primary, then app activities etc as additional information. The current intention is to get app activities information than its package (since filter by package).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already given a comment on that condition, You want me to create docstring with more info?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, could you leave what kind of activities will be available, and no activities packages won't be available?
I mean not a comment for the implementation, just this method's entire behavior as the module's description. Also, it wole be appreciate to add this method uses android.permission.QUERY_ALL_PACKAGES here, or a comment in the manifest to link each

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, missing commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now

@shashank2086
Copy link
Contributor Author

Can it be merged now?

@shashank2086 shashank2086 requested review from mykola-mokhnach and KazuCocoa and removed request for mykola-mokhnach March 9, 2023 10:44
@mykola-mokhnach
Copy link
Contributor

@KazuCocoa Do you have any additional comments?

@mykola-mokhnach mykola-mokhnach merged commit 94dcd37 into appium:master Mar 9, 2023
11 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 9, 2023
## [5.8.0](v5.7.8...v5.8.0) (2023-03-09)

### Features

* get installed packages information ([#502](#502)) ([94dcd37](94dcd37))
@KazuCocoa
Copy link
Member

no, just waited for the ci result

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

🎉 This PR is included in version 5.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants