Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: improve resource names #2316

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Conversation

pubiqq
Copy link
Contributor

@pubiqq pubiqq commented Oct 22, 2024

  • Makes sanitized resource names more human-friendly (before: jadx_deobf_<id>, after: <sanitized_name>_<id>).
  • Improves detection of incorrect characters in resource names.
  • Fixes an issue where "better name" determination was not performed for styles.
  • Fixes an issue where keywords could be used as a field name in the R class.

@skylot
Copy link
Owner

skylot commented Oct 22, 2024

@pubiqq this change looks great!
Although, I am hoping that someone also check and test this change 😅

@zhongqingsong
Copy link
Contributor

@pubiqq this change looks great! Although, I am hoping that someone also check and test this change 😅

hi, it looks interesting, i want test it. But i am not clear how to test, should I write a test class, or run the test locally? 😅

@jpstotz
Copy link
Collaborator

jpstotz commented Oct 24, 2024

@skylot I will try to make a review in the next days. Based on the description it sounds good. Biggest problem is to find good example APKs to test it's functionality.

@zhongqingsong Pull Requests are effectively branches you can check-out, compile. In the end have a modified Jadx version with the PR changes: https://docs.github.com/de/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@jpstotz
Copy link
Collaborator

jpstotz commented Nov 1, 2024

Sorry for the delay, I needed to code some utilities first to be able to process multiple APKs one after another to get a better understanding when renaming is applied.

I started with jadx.core.xmlgen.ResTableBinaryParser#getNewResName. For the few APK files I tested it the most common case was a renaming because the resource name started with a $ like this:

$avd_hide_password__0 renamed to _avd_hide_password__0_res_0x7f080000 however the new resource name doesn't seem to be used anywhere (this was from the APK linked in #1232).

@pubiqq
Copy link
Contributor Author

pubiqq commented Nov 1, 2024

I started with jadx.core.xmlgen.ResTableBinaryParser#getNewResName. For the few APK files I tested it the most common case was a renaming because the resource name started with a $ like this:

$avd_hide_password__0 renamed to _avd_hide_password__0_res_0x7f080000

The dollar sign is not a valid character for resource names in aapt2, so ResNameUtils#sanitizeAsResourceName replaces it with an underscore and appends a postfix with the resource id.

however the new resource name doesn't seem to be used anywhere

They are used in drawable/avd_hide_password.xml.

@jpstotz
Copy link
Collaborator

jpstotz commented Nov 4, 2024

I started with jadx.core.xmlgen.ResTableBinaryParser#getNewResName. For the few APK files I tested it the most common case was a renaming because the resource name started with a $ like this:
$avd_hide_password__0 renamed to _avd_hide_password__0_res_0x7f080000

The dollar sign is not a valid character for resource names in aapt2, so ResNameUtils#sanitizeAsResourceName replaces it with an underscore and appends a postfix with the resource id.

OK, got it. I saw that it was already renamed before your PR, but the new format used by your PR is indeed much better.

Could you please extend your PR and add JavaDoc where you describe why something is done? The info what is done can be derived from the code but the reason why is often lost. That would really be helpful for the future...

They are used in drawable/avd_hide_password.xml.

Seems like Jadx resources search does not yet allow searching for file-names, so I directly hit the blind spot of Jadx-Gui ;)

@pubiqq pubiqq force-pushed the improve-resource-names branch from e692cff to cc02e84 Compare November 5, 2024 15:36
@pubiqq
Copy link
Contributor Author

pubiqq commented Nov 5, 2024

Could you please extend your PR and add JavaDoc where you describe why something is done?

I'm not sure I understand what kind of description you're expecting. I've added javadoc to ResNameUtils#sanitizeAsResourceName, maybe that's enough.

@jpstotz
Copy link
Collaborator

jpstotz commented Nov 6, 2024

@pubiqq I didn't had something special in mind, therefore what you have written is in my opinion sufficient. Thanks for your patience and of course of PR.

@skylot skylot merged commit 5d064d3 into skylot:master Nov 6, 2024
4 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants