revanced-integrations icon indicating copy to clipboard operation
revanced-integrations copied to clipboard
verified publisher icon ReVanced

feat(tiktok): `tiktok-feed-filter` , `tiktok-settings` and `tiktok-force-login`

Open d4rkk3y opened this issue 3 years ago • 2 comments

d4rkk3y avatar Sep 16 '22 11:09 d4rkk3y

Soon, every patch will have their own integrations. General integrations like this will also still exist providing basic utility methods like settings or methods to the Android API (which the patch integrations will not have access to), but patch specific code will be moved to the patches own integrations. The current problem with this PR is that using this integrations in other applications will still merge the integrations code for TikTok patches. Would it be possible to make this repository build separate integrations, in this case, one for TikTok and the other for YouTube? This way, we can then easily migrate to patch specific integrations and prevent unrelated code from getting merged.

oSumAtrIX avatar Sep 16 '22 23:09 oSumAtrIX

@oSumAtrIX I think it's better to leave the PR as-is right now. It's better to just have it be "wrong" and then fix it later than doing a half-assed implementation and having to fix it later anyway. But it's up to you.

Sculas avatar Sep 17 '22 00:09 Sculas

Actually, merging unrelated code is more of a half assed implementation then creating a proper solution which would be integrations specifically made for each patch, or until thats done, for these patches for now. This way, useless code would be prevented to be merged to our current apps and the same for the app @d4rkk3y patches. The solution is simply to create a separate integrations apk. Then either you use the current integrations or the new ones specifically for these patches.

oSumAtrIX avatar Sep 17 '22 01:09 oSumAtrIX

That'd require a whole new repository and new IntegrationsPatch dependency for.. just this? And for it then all be removed when everything is refactored into how you said it would be like? Don't think only about yourself if that's what you do, think about the author too who has to do all of that with.

Instead, if you insist on going with this route, merge this PR and make that huge change yourself or ask if the author would like to do that. Either way, you'll have to help because you need to make a repository for it.

Sculas avatar Sep 17 '22 01:09 Sculas

@oSumAtrIX @Sculas I have a idea. How about create a annotation to mark a class in integration belong to target app? And then implement merge method in patcher which will check it and merge if current patching app is target app.

d4rkk3y avatar Sep 17 '22 05:09 d4rkk3y

That'd require a whole new repository and new IntegrationsPatch dependency for.. just this? And for it then all be removed when everything is refactored into how you said it would be like? Don't think only about yourself if that's what you do, think about the author too who has to do all of that with.

Instead, if you insist on going with this route, merge this PR and make that huge change yourself or ask if the author would like to do that. Either way, you'll have to help because you need to make a repository for it.

No, that's not required, it can be done in this repo pretty straight forward. Less refactoring will happen if the integrations are being started to be separated now for each app.

oSumAtrIX avatar Sep 18 '22 14:09 oSumAtrIX

I've tested both this and #501 together and it seems to work flawless, is there anything left that is required in the PR's before merge?

E85Addict avatar Sep 20 '22 23:09 E85Addict

@oSumAtrIX @Sculas I have a idea. How about create a annotation to mark a class in integration belong to target app? And then implement merge method in patcher which will check it and merge if current patching app is target app.

This would require to annotate all classes which should be merged, including those which are shaded into the integrations to which an annotation can not be added. As an alternative every class which does not have this particular annotation, could be merged by default, while those who have, but do not match the selected app, are excluded. For the scope of this PR it is better to separate this to another PR, please revert the intruduction of the MergeIf annotation so we can merge this PR for now.

oSumAtrIX avatar Sep 21 '22 00:09 oSumAtrIX

Tested the corrections all is working!

E85Addict avatar Sep 21 '22 20:09 E85Addict

:tada: This PR is included in version 0.41.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] avatar Sep 21 '22 21:09 github-actions[bot]