-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Support explicit operation for when to load full resource table (#3217)
- Loading branch information
Showing
4 changed files
with
92 additions
and
53 deletions.
There are no files selected for viewing
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
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
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
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
7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iBotPeaches So now you have duplicate identical
hasManifest
andhasResources
functions in bothApkDecoder
andResourcesDecoder
.Something here doesn't seem right from a separation of concerns standpoint, and I can see multiple redundancy instances within
ResourcesDecoder
left over (like expensive calls togetResTable
that callshasManifest
andhasResources
after those checks have already been performed within the usage case).7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep guess I reviewed that poorly. I'll try and remove the dupes unless @sv99 beats me to it. I won't have time till tomorrow morning.
7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right place for this methods in the ApkInfo class, but for this need add field mApkFile in this class.
Now this field will be saved in the apktool.yml
7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has to be a way to mark a field as non-serializable, Java happens to have the keyword
transient
for that but I'm not sure if the YAML library respects that.The ideal way would be
private final transient ExtFile mApkFile;
and then pass theExtFile
instance in the constructor forApkInfo
.7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, SnakeYAML respects the
transient
keyword. Working on cleaning redundancy.7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IgorEisberg - So you don't waste some effort. There is a PR here that completely removes SnakeYAML - #3191
7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be a problem, I don't do anything SnakeYAML-specific, that PR is not much of a conflict to this.
7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iBotPeaches I see that that PR changes a lot more than just YAML serialization/deserialization, I'll wait for you to merge that first.
7a4a20b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IgorEisberg - Sounds good. I'm still running it through some tests so not sure on timeline on that. Its quite large.