fix(spoof-wifi-connection): use updated instruction indices

[Linus789]

Not using a mutableMethod resulted in using the original method for locating the instruction indices to patch. When another patch already patched that method and the indices got offset, we still used the old indices which resulted in invalid smali.

This is intended to be a temporary fix until a better solution for mutability is found.
Tracking issue: ReVanced/revanced-patcher#178

reference: ReVanced/revanced-patches#2177

[oSumAtrIX]

I have changed how methods to be transformed are collected, reducing the number of required checks to a minimum. Please test if everything still works properly.

[Linus789]

I have changed how methods to be transformed are collected, reducing the number of required checks to a minimum.

If I’m not mistaken, the number of checks are the same. I used a Sequence before, which executes lazily. If you agree with me, it would be nice if you would remove your commit.

[oSumAtrIX]

The key difference is that the previous code has collected all instructions indices if it matched filterMap, whereas the current one returns, when a single index has been found

[Linus789]

Could you point out to me where it collected all instructions indices?

[oSumAtrIX]

I believe in line 36 here:

https://github.com/revanced/revanced-patches/blob/bd094580013718541e1874f8e95b46852a33c920/src/main/kotlin/app/revanced/util/patch/AbstractTransformInstructionsPatch.kt#L24-L36

[Linus789]

But that doesn’t collect the patch indices, it returns a Sequence of patch indices.

[oSumAtrIX]

Lazily, I see; in that case, the latest commit doesn't contain any performance regressions and instead skips creating a Sequence

[Linus789]

Your perf commit doesn’t contain any performance improvements. It would be nice if you would remove that commit.

[oSumAtrIX]

What would be the benefit of removing the commit? It skips having to depend on a Sequence

[Linus789]

The code would be more clean and concise since with your commit we have two lines where we filterMap an instruction. Before that it was all in one place.

It skips having to depend on a Sequence

Sounds like premature optimization to me.

[oSumAtrIX]

The code would be more clean and concise since with your commit we have two lines where we filterMap an instruction.

I believe redundancy comes in favor of readability here to begin with. Previously, it was not able to infer that the code works lazily without knowing Sequence is lazy, thus assuming it collects all indices obfuscating the readability of the code. With the current "redundancy" of filterMap existing twice, it is now inferable that first only the method is checked if it's a mutable candidate by checking if an instruction is present that needs to be transformed, without having to first know anything about Sequence or lazy loading.

[Linus789]

Previously, it was not able to infer that the code works lazily without knowing Sequence is lazy, thus assuming it collects all indices obfuscating the readability of the code.

Yes, because you didn’t know of Sequences. Now that you know what a Sequence is, the code should be readable. And if you really want to be clear about it being lazy, we could add a comment.

[oSumAtrIX]

Previously, it was not able to infer that the code works lazily without knowing Sequence is lazy, thus assuming it collects all indices obfuscating the readability of the code.

Yes, because you didn’t know of Sequences. Now that you know what a Sequence is, the code should be readable. And if you really want to be clear about it being lazy, we could add a comment.

I think having to know about something used in a code to justify that the code is readable when it is possible to make it readable solely by reading it obfuscates readability. The code abuses the fact that Sequence is lazy to work as expected, but reading the code where Sequence is used provides no inference about Sequence being lazy, contributing to obfuscating readability. I understand that code quality may outweigh this. If that is your motivation for using a Sequence, commenting on how it is leveraged should solve our issue.

[Linus789]

If that is your motivation for using a Sequence, commenting on how it is leveraged should solve our issue.

Thanks for understanding! The other reason why I prefer a sequence over the current code is that I would like to avoid the assumption that the index is never used for filtering (currently always 0 is passed as index).

I will update the code with some comments, but before that I would like to ask another question.

Couldn’t we avoid the copies (referring to my first naive solution) by checking whether a mutable class has already been resolved via the resolved field and use the mutableClass if it has been resolved and the immutable class if not?

This would have at least two advantages:

• We wouldn’t have to call filterMap twice for some instructions
• If another patch adds a new instruction which should have been patched, it would be detected because we are now checking the updated class

The field’s getter visibility would have to be changed to public, though.

Disadvantages:

• The ClassProxy for every class is being looked up and created if it didn’t already exist (the latter should be avoidable with a little bit refactoring, I think and for the former it should be possible to improve the speed; but perhaps it will already be fast enough without these changes, I don’t know)

[Linus789]

Well, since this is intended to be a temporary fix, I don’t think it matters that much. I have updated the code with comments regarding the Sequence.

[Linus789]

Thanks for reviewing this PR and helping me with it! 🎉

[oSumAtrIX]

Thank you too for your continuous contribution :)