refactor: Config/Tests/misc rework #3746
Open
+2,664
−3,233
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Config is now propagated to ResValue instead of being a globally
accessible instance that gets set each time a new Config is instantiated.
The old way violted every principle of an instance and even a singleton.
This caused working with Tests extremely error-prone, because the Config
had to be carefully reset to default for each Test to get the expected
behavior, and ResValue was only aware of the global instance.
Now, only Main and BaseTest are calling new Config().
BaseTest provides a fresh Config for every Test which turns the boilerplate
into something shorter, predictable and more robust.
Config class is now encapsulated to give control over the values passed
to it. Having both publically-modifyable fields and setters with the
verfications was more of a dirty hack than proper coding principles.
General-purpose XML utilities extracted from ResXmlUtils into XmlUtils
in the brut.xml library. It contains Document creation, parsing and saving.
In addition, it has a generic evaluateXPath method that's now used more
widely wherever manual looping over nodes was used, like in some tests.
ResXmlUtils reworked and cleaned, now takes advantage of XmlUtils.
ResXmlUtils only retains methods relevant specifically to resource XMLs
and Android Manifest.
Minor tweaks and code cleanup in various classed like ApktoolProperties
and the main work classes (Framework, ApkDecoder, ApkBuilder, etc.),
as well as utility classes like AaptInvoker and OS.
Fixed locks being held by certain ExtFiles to zips with loaded Directory,
which prevented proper temp file cleanup during Test stage.
ExternalEntityTest resources ("doctype") moved to aapt1 as it's an
aapt1-only test.
Overall, mainly a code refactor with no end-user changes,
to make development less prone to errors.
|
50 files to go... almost done |
We actually don't need to pass Config explicitly to all ResValue subtypes. It's only needed by 3 subtypes of ResBagValue, so we get the Config via ResBagValue's mParent.getPackage().getConfig(). Encapsulate ResConfigFlags. Encapsulate ResID and make it a numeric and comparable type. Sort resource specs by resource ID in generatePublicXml. Duo is redundant, replace with Apache Common's Pair. ResIdValue is redundant, it's never actually instantiated. Tweak equals and hashCode overrides to standard formats. Misc style and variable name tweaks.
|
I think I'm done for now. I could go on, but it's solid enough of a refactor for the future of Apktool. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Configis now propagated toResValueinstead of being a globally accessible instance that gets set each time a newConfigis instantiated. The old way violated every principle of an instance and even a singleton. This caused working with Tests extremely error-prone, because theConfighad to be carefully reset to default for each Test to get the expected behavior, andResValuewas only aware of the global instance. Now, onlyMainandBaseTestare callingnew Config().BaseTestprovides a freshConfigfor every Test which turns the boilerplate into something shorter, predictable and more robust.Configclass is now encapsulated to give control over the values passed to it. Having both publicly modifiable fields and setters with the verifications was more of a dirty hack than proper coding principles.General-purpose XML utilities extracted from
ResXmlUtilsintoXmlUtilsin thebrut.xmllibrary. It containsDocumentcreation, parsing and saving. In addition, it has a genericevaluateXPathmethod that's now used more widely wherever manual looping over nodes was used, like in some tests.ResXmlUtilsreworked and cleaned, now takes advantage ofXmlUtils.ResXmlUtilsonly retains methods relevant specifically to resource XMLs and Android Manifest.Minor tweaks and code cleanup in various classes like
ApktoolPropertiesand the main work classes (Framework,ApkDecoder,ApkBuilder, etc.), as well as utility classes likeAaptInvokerandOS.Fixed locks being held by certain
ExtFiles to zips with loadedDirectory, which prevented proper temp file cleanup during Test stage.ExternalEntityTestresources ("doctype") moved to aapt1 as it's an aapt1-only test.Overall, mainly a code refactor with no end-user changes, to make development less prone to errors.