Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

test: add failing/passing sparse/not-sparse test #3201

Merged
merged 4 commits into from
Jul 23, 2023

Conversation

iBotPeaches
Copy link
Owner

@iBotPeaches iBotPeaches commented Jul 23, 2023

This has a sparse packed and not application - iBotPeaches/TestApks@a09e2ca

fixes: #3199

@iBotPeaches iBotPeaches marked this pull request as ready for review July 23, 2023 11:48
@iBotPeaches
Copy link
Owner Author

I'm confident in the tests here as they fail if I revert fix, but I'll leave this open for a few hours if you'd like to confirm @IgorEisberg

@IgorEisberg
Copy link
Contributor

Works perfectly here, thanks for the quick patch.

@IgorEisberg
Copy link
Contributor

One question regarding

            if (! mResTable.getSparseResources()) {
                mResTable.setSparseResources(true);
            }

Isn't the if check redundant? It's just setting a boolean, doesn't seem like making sure it's not set has much of a purpose.

@iBotPeaches
Copy link
Owner Author

Isn't the if check redundant? It's just setting a boolean, doesn't seem like making sure it's not set has much of a purpose.

Yeah since that function has evolved to just a bool scalar set - it probably doesn't need that check anymore since its very cheap to do.

@IgorEisberg
Copy link
Contributor

IgorEisberg commented Jul 23, 2023

Yeah since that function has evolved to just a bool scalar set - it probably doesn't need that check anymore since its very cheap to do.

Oh, I missed that piece of history. In any case, since I decompile/recompile whole ROMs I can confidently say that this was the last broken piece of the puzzle - can't find any other regressions. Neat release.

@iBotPeaches
Copy link
Owner Author

Oh, I missed that piece of history. In any case, since I decompile/recompile whole ROMs I can confidently say that this was the last broken piece of the puzzle - can't find any other regressions. Neat release.

Yeah sorry about that, but I figured at some point I needed to rewrite some lower parts of code. I knew there was a chance of some regressions, but now I understand it far more than past and can iterate a bit quicker.

@IgorEisberg
Copy link
Contributor

Yeah sorry about that, but I figured at some point I needed to rewrite some lower parts of code. I knew there was a chance of some regressions, but now I understand it far more than past and can iterate a bit quicker.

All par for the course, that's how development goes. I actually use a flavor of Apktool that I customized especially for proper MIUI handling (including the Xiaomi-invented "nxhdpi" specifier), but always try to make a pull request for generic/non-MIUI fixes that others could use. ;)

@iBotPeaches iBotPeaches merged commit 5483650 into master Jul 23, 2023
@iBotPeaches iBotPeaches deleted the regression-test-3199 branch July 23, 2023 15:20
@iBotPeaches iBotPeaches modified the milestones: 2.3.4, v2.8.2 Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Regression: sparseResources is always true
2 participants