refactor: source and tests#4071
Conversation
There was a problem hiding this comment.
Pull request overview
This PR performs a comprehensive refactoring of the Apktool codebase focused on improving maintainability and code organization. The changes include merging AbstractDirectory into Directory, refactoring ApkBuilder and ApkDecoder with improved resource handling logic, migrating test utilities from TestUtils into BaseTest, and simplifying exception handling across all tests.
Changes:
- Refactored directory hierarchy by merging
AbstractDirectoryintoDirectorybase class - Improved
ApkBuilderwith clearer order of operations for resource building and proper file lock handling - Changed
ApkDecoder.decode()return type fromApkInfoto void with separate getter method - Migrated
getIncludeFiles()fromApkBuildertoAaptInvokerand removed unused aapt2 parameters - Merged
TestUtilsintoBaseTestand simplified test exception declarations tothrows Exception - Renamed
XmlUtils.loadDocumentContent()toparseDocument()with performance improvement usingStringReader
Reviewed changes
Copilot reviewed 68 out of 68 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| XmlUtils.java | Renamed method and optimized XML parsing with StringReader |
| Directory.java, AbstractDirectory.java | Merged abstract class into base class to simplify hierarchy |
| FileDirectory.java, ZipRODirectory.java | Refactored to extend Directory directly with cleaned implementations |
| ExtFile.java | Changed exception type from IOException to DirectoryException |
| ApkBuilder.java | Major refactoring with improved resource building logic and file handling |
| ApkDecoder.java | Changed API to void return with separate getter for ApkInfo |
| AaptInvoker.java | Added getIncludeFiles() method and removed unused parameters |
| BaseTest.java | Merged TestUtils functionality and simplified helper methods |
| Test files | Simplified exception declarations and removed unused imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I wish I properly organized all my samples that broke arsc parsing since trying to follow if these string pool changes are semantically the same. |
I pretty much just merged |
ApkBuilder: RefactoredbuildResourceswith well-documented order of operation.ApkBuilder: Determine whetherAndroidManifest.xmlis raw usingResChunkHeaderinbuildResources.ApkBuilder: MergedbackupManifestFileintobuildManifestOnlyandbuildResourcesFullywhere editing happens.ApkBuilder: Made surebuildcloses the APK file even if an exception occurred to release file lock.ApkBuilder,ApkDecoder: Normalized comments and logging.AaptInvoker: Removed unused parameters (aapt2:-Aand-Roptions are neither used nor needed).AaptInvoker: MigratedgetIncludeFilesfromApkBuilder.BinaryXmlResourceParser: Initial usage ofResChunkPullParser(the rest is WIP for later).AbstractDirectoryintoDirectory.FileDirectoryandZipRODirectory.FileDirectory/ZipRODirectorywhenever possible,ExtFileis ambiguous and hides possible file lock.XmlUtils: RenamedloadDocumentContenttoparseDocumentand usedStringReaderfor performance.TestUtilsintoBaseTest.The reasoning behind
ExtFileis being reevaluated but kept unchanged for now.Maintenance PR: No changes in usage or output.