refactor(ADB): improve internal API #757

Rudxain · 2024-12-14T13:14:03Z

The goal is to make ADB commands:

❤️ more ergonomic: contextual auto-complete, RA suggestions, and pre-parsed stdout
✨ less ugly (cleaner code)
🧠 more maintainable (easier to debug)
⌨️ less typo-prone: not "stringly typed"
✅ correct & safer: no arbitrary cmd execution, and no env-vars

I am sorry to whoever is going to review this 😅. I didn't want the diff to be so big 💀

#653 (comment)

This commit fixes the style issues introduced in fed1580 according to the output from Rustfmt. Details: #653

This commit fixes the style issues introduced in 1da5b69 according to the output from Rustfmt. Details: #653

…es` API

deepsource-io · 2024-12-14T13:14:45Z

Here's the code health analysis summary for commits 1ed216a..fc72efc. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
Rust	❌ Failure	❗ 13 occurences introduced 🎯 15 occurences resolved	View Check ↗
Test coverage	⚠️ Artifact not reported	Timed out: Artifact was never reported	View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

…acks`

Also make `get_device_brand` more honest

Also merge some `use`s

adhirajsinghchauhan

Overall, there's only one or two good changes here (e.g. explicit serial, deduping android_sdk >= 21). Most of the other ones are subjective, so they may not be beneficial to others. It is my opinion that changes for the sake of changes should not be done.

I am not a fan of shortening variable names, as none of them were long enough that they needed abbreviation in the first place. For example, command => cmd, packages => packs, shell => sh is wholly unnecessary and adds noise to this PR. It's also not done consistently, as sometimes we see the full word being used (packages), and in other places two different short-forms. (e.g. packs and pack)

Even the renaming of Phone to Device was not necessary, especially when considering it's unrelated to this PR, and that some variables are named device, some dev, and some remain as phone.

Forgive the bluntness, but please revert any such subjective changes. We can discuss the specifics on Discord.

Typing in natural language form is more efficient for the developer. When we think of typing "command", we expect to start typing it and have autocomplete suggest it as-is. It will not surface "cmd", so we'd need to remember to type abbreviations everywhere, only to realise that abbreviations are not consistently used. This extra step is at odds with the basic methodology of good, clean code for everyone.

adhirajsinghchauhan · 2024-12-16T18:40:33Z

src/gui/views/settings.rs

@@ -137,7 +138,7 @@ impl Settings {
                    None => {
                        self.device = DeviceSettings {
                            device_id: phone.adb_id.clone(),
-                            multi_user_mode: phone.android_sdk > 21,
+                            multi_user_mode: supports_multi_user(phone),


Potential bug: the function checks >=; while the previous code was simply >. The official Android API to check for this was added in API 24; however, before this it was manufacturer-dependent. IIRC multiple users were possible even before API 21, via ADB (pm create-user). Not sure if it was exposed in a user-facing way.

See https://source.android.com/docs/devices/admin/multi-user#applying_the_overlay:

The multi-user feature is disabled by default. To enable the feature, device manufacturers must define […]

Yep, I noticed some inconsistencies, so I tried to find docs about which API level should be checked, and I defaulted to Android 5.0

adhirajsinghchauhan · 2024-12-16T18:48:15Z

src/core/sync.rs

-pub async fn perform_adb_commands(
+/// Runs a shell command on the device.
+/// See [`adb_cmd`] for details.
+pub async fn adb_sh_cmd<S: AsRef<str>>(


Not a fan of shortening this, and other definitions. It does not help readability; in fact, it makes it more ambiguous at times.

For example, while we obviously know sh => shell and cmd => command, others may assume /bin/sh or Windows CMD, leading to confusion: does this function only work over sh and cmd, or is it a generic platform-agnostic abstraction?

adhirajsinghchauhan · 2024-12-16T18:58:40Z

src/core/sync.rs

+
+/// `pm list packages` flag/state/type
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum PmLsPackFlag {


Enums are for our benefit, so we should use full, descriptive names here. Please change to:

IncludeUninstalled

OnlyEnabled

OnlyDisabled

For completeness, we could also add entries for all other possible flags (f, d, s, 3, i) (docs). Unused code should be stripped out of the final release binary anyway.

we should use full, descriptive names

I agree. Single-letter is bad. And it even looks ugly because of the relative length asymmetry with the enum name.

I tried to mirror the ADB CLI, but I've already deviated from it when I renamed list_users to ls_users, so I guess it doesn't hurt to improve the flag names.

For completeness, we could also add entries for all other possible flags (f, d, s, 3, i) (docs).

3 and s should probably be on a distinct enum, as they are a different kind of filter. Same for i and f.

About completeness, what about #[non_exhaustive] 👀 ? That way, we can easily add the other flags if we need them

Rudxain · 2024-12-16T22:02:35Z

Most of the other ones are subjective, so they may not be beneficial to others. It is my opinion that changes for the sake of changes should not be done.

I agree with the sentiment, as major refactors could introduce bugs. I tried my best to document all possible places where I could've created one.

I am not a fan of shortening variable names, as none of them were long enough that they needed abbreviation in the first place. For example, command => cmd, packages => packs, shell => sh is wholly unnecessary and adds noise to this PR

yeah 😅, I'll revert that.

Even the renaming of Phone to Device was not necessary, especially when considering it's unrelated to this PR, and that some variables are named device, some dev, and some remain as phone.

I'll revert the rename and defer that to its own PR. I did too much "opportunistic refactoring" (mostly just "opportunistic renaming", which is not very valuable)

Forgive the bluntness, but please revert any such subjective changes. We can discuss the specifics on Discord.

Ok 😅 🙁, I'll try to revert all out-of-scope changes. I can't do it today. I'll have more time on the weekend.

at odds with the basic methodology of good, clean code for everyone.

Agreed. One of the motivations I had in mind for this PR is to make the code-base more accessible to newcomers (#731), but the strongly context-bound names might go against that goal

Rudxain · 2024-12-26T22:40:59Z

This week, I've been busier than I expected, so I couldn't work on this

Rudxain and others added 22 commits September 24, 2024 17:38

refactor(sync): const pm {list packages | clear}

8ddbe68

refactor(set_var-serial): define set_adb_serial

8a80cdc

build(lint): improve warns about unsafe

6032b8e

style: rm unused imports

8bead68

style(sync): readable consts

63707f3

#653 (comment)

chore: derive Copy for Error

c0fa888

docs: Phone -> Device, and explain fields

cf054c8

refactor: avoid set_var by specifying serial

fed1580

style: format code with Rustfmt

db15305

This commit fixes the style issues introduced in fed1580 according to the output from Rustfmt. Details: #653

refactor: strs are Options already!

5300bef

Merge branch 'main' into refactor/sync-and-set_var

5761e8d

Merge branch 'main' into refactor/sync-and-set_var

65349ec

refactor: cargo clippy --fix

1da5b69

style: format code with Rustfmt

7052646

This commit fixes the style issues introduced in 1da5b69 according to the output from Rustfmt. Details: #653

docs: vec cap can't overflow

f0f6c51

style: vec macro

dc1ce19

perf: Optimize *system_packages and improve `list_all_system_packag…

ac056dd

…es` API

perf: use more Vec::with_capacity

89680fc

docs(sync): mention pm list format assumption

04b3ef5

feat: add type-state cmd builders

b74947e

docs(sync): Mention type-sys patterns on AdbCmd

87fa332

refactor: use AdbCmd, finish list_packs and add list_users

30359cb

Rudxain added documentation Improvements or additions to documentation refactor Rework of code — behaviour doesn't change labels Dec 14, 2024

Rudxain self-assigned this Dec 14, 2024

Rudxain changed the title ~~Refactor/adb api~~ refactor(ADB): improve internal API Dec 14, 2024

This was referenced Dec 14, 2024

refactor(sync): remove set_var in favor of adb_id #653

Closed

type-state API? cocool97/adb_client#76

Closed

fix: User not &User

c172bae

Rudxain added 10 commits December 14, 2024 12:56

refactor: add PackId, replace list_all_system_packages by `list_p…

a566733

…acks`

refactor: create adb module, and add **LOTS** of docs

240dde1

refactor: more pre-parsing

77a52be

docs(ADB): more docs for ls_packs*, and validated returns Set

4c61dc3

refactor: replace hashset_system_packages by new API

4ec1acd

refactor: user_id: u16 not u: User

9aeba11

docs(ADB): more specific; mention assumptions

2e2114d

refactor(ADB): add getprop API, inline adb_cmd into adb_sh_cmd

e84e081

Also make `get_device_brand` more honest

refactor(utils): less dupes in open_url

5e27a52

docs: iced::Command as ICmd, and potential RCE warn

fc72efc

Also merge some `use`s

This was referenced Dec 16, 2024

bug(backup): Likely ACE vulnerability when restoring backup #760

Closed

Likely ACE vulnerability when restoring backup 0x192/universal-android-debloater#1049

Closed

adhirajsinghchauhan requested changes Dec 16, 2024

View reviewed changes

Rudxain mentioned this pull request Jan 12, 2025

fix: use pm block and pm clear for sdk versions below 19 #620

Merged

AnonymousWP mentioned this pull request Jan 20, 2025

feat(ADB): retry adb devices only 3 times in debug builds #807

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ADB): improve internal API #757

refactor(ADB): improve internal API #757

Rudxain commented Dec 14, 2024 •

edited

Loading

deepsource-io bot commented Dec 14, 2024 •

edited

Loading

Analysis Summary

adhirajsinghchauhan left a comment

adhirajsinghchauhan Dec 16, 2024

Rudxain Dec 16, 2024

adhirajsinghchauhan Dec 16, 2024

adhirajsinghchauhan Dec 16, 2024

Rudxain Dec 16, 2024

Rudxain commented Dec 16, 2024 •

edited

Loading

Rudxain commented Dec 26, 2024

refactor(ADB): improve internal API #757

Are you sure you want to change the base?

refactor(ADB): improve internal API #757

Conversation

Rudxain commented Dec 14, 2024 • edited Loading

deepsource-io bot commented Dec 14, 2024 • edited Loading

Analysis Summary

adhirajsinghchauhan left a comment

Choose a reason for hiding this comment

adhirajsinghchauhan Dec 16, 2024

Choose a reason for hiding this comment

Rudxain Dec 16, 2024

Choose a reason for hiding this comment

adhirajsinghchauhan Dec 16, 2024

Choose a reason for hiding this comment

adhirajsinghchauhan Dec 16, 2024

Choose a reason for hiding this comment

Rudxain Dec 16, 2024

Choose a reason for hiding this comment

Rudxain commented Dec 16, 2024 • edited Loading

Rudxain commented Dec 26, 2024

Rudxain commented Dec 14, 2024 •

edited

Loading

deepsource-io bot commented Dec 14, 2024 •

edited

Loading

Rudxain commented Dec 16, 2024 •

edited

Loading