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.

Sign up for GitHub

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

Adb parse device list add offline field improve #671

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Adb parse device list add offline field improve #671

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e85d0b
parseDeviceList add offline
18506711722 Sep 1, 2024
9f8eb27
parseDeviceList add offline
18506711722 Sep 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion libraries/adb/src/server/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export class AdbServerClient {
const parts = line.split(" ").filter(Boolean);
const serial = parts[0]!;
const status = parts[1]!;
if (status !== "device" && status !== "unauthorized") {
if (status !== "device" && status !== "unauthorized" && status !== "offline") {
continue;
}

Expand All @@ -169,11 +169,17 @@ export class AdbServerClient {
break;
}
}

if(status === 'offline' && !product && !model && !device){
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this needed? In which situation the device will (or won't) have these fields?

(these three fields are optional in the type definition)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phone status online -> offline ,There will be no product,model and device fields

Copy link
Owner

Choose a reason for hiding this comment

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

Won't this exclude all offline devices?

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, he doesn't exclude all offline devices, just filter out unwanted offline push messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any concern？

Copy link
Owner

Choose a reason for hiding this comment

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

filter out unwanted offline push messages

Which ones are "wanted" and which ones are "unwanted"?

I'd like the method to return all devices, and let users decide what to do (they can filter by !product && !model && !device themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous versions will filter out offline information, which is not friendly to devices that need to listen in real time

Copy link
Owner

Choose a reason for hiding this comment

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

I still don't understand what you mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

 if (status !== "device" && status !== "unauthorized" ) {
continue;
}

offline push message Will be filtered

 if (status !== "device" && status !== "unauthorized"  && status !== "offline" ) {
continue;
}

Copy link
Contributor Author

@18506711722 18506711722 Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

status === "offline" There will be multiple offline messages and not all of them will make sense

continue;
}

if (!transportId) {
throw new Error(`No transport id for device ${serial}`);
}
devices.push({
serial,
offline: status === "offline",
authenticating: status === "unauthorized",
product,
model,
Expand Down Expand Up @@ -610,6 +616,7 @@ export namespace AdbServerClient {
model?: string | undefined;
device?: string | undefined;
transportId: bigint;
offline?:boolean
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to use a single status field (and an enum) to replace offline and authenticating fields.

It might only contain Offline, Unauthorized and Online these three values initially, it can be extended later.

}

export class NetworkError extends Error {
Expand Down
Loading