-
-
Notifications
You must be signed in to change notification settings - Fork 435
feat(Spotify): Add Change lyrics provider
patch
#4937
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
base: dev
Are you sure you want to change the base?
feat(Spotify): Add Change lyrics provider
patch
#4937
Conversation
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/Fingerprints.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/Fingerprints.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
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.
Upon skimming the patch I am not sure whats going on in a fair amount of time. The complexity needs to be addressed. Organize the code into blocks of related code and add // region comments and comments explaining the strategy, magic constants etc
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
After more tests, I noticed that 2 changes were not needed for the patch to work, so they have been removed. I have added comments both in the fingerprint file and in the execute file, explaining what is being done. Most of the comments above have been resolved, I've left open the one that to me are still to clarify. |
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/CustomLyricsPatch.kt
Outdated
Show resolved
Hide resolved
Added default value (lyrics.natanchiodi.fr) Added domain resolver check
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/ChangeLyricsProviderPatch.kt
Outdated
Show resolved
Hide resolved
Should this has |
Fails on 46.496
|
…lyrics-provider # Conflicts: # patches/api/patches.api
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/ChangeLyricsProviderPatch.kt
Show resolved
Hide resolved
@oSumAtrIX you want to re-review or is this ready to merge? |
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/ChangeLyricsProviderPatch.kt
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/ChangeLyricsProviderPatch.kt
Outdated
Show resolved
Hide resolved
patches/src/main/kotlin/app/revanced/patches/spotify/misc/lyrics/ChangeLyricsProviderPatch.kt
Outdated
Show resolved
Hide resolved
parameters() | ||
custom { method, _ -> | ||
method.indexOfFirstInstruction { | ||
getReference<MethodReference>() == httpClientBuilderFingerprint.originalMethod |
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.
This repeatedly calls the getter of originalMethod property, should set the return value to a variable
|
||
MutableMethod(method).apply { | ||
name = "patch_getCustomLyricsProviderHttpClient" | ||
classDef.methods.add(this) | ||
}.also { | ||
method.addInstruction( | ||
urlBuilderIndex - 1, | ||
"const-string v$urlRegister, \"$lyricsProviderHost\"" | ||
) | ||
} | ||
} |
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.
MutableMethod(method).apply { | |
name = "patch_getCustomLyricsProviderHttpClient" | |
classDef.methods.add(this) | |
}.also { | |
method.addInstruction( | |
urlBuilderIndex - 1, | |
"const-string v$urlRegister, \"$lyricsProviderHost\"" | |
) | |
} | |
} | |
MutableMethod(method).apply { | |
name = "patch_getCustomLyricsProviderHttpClient" | |
addInstruction( | |
urlBuilderIndex - 1, | |
"const-string v$urlRegister, \"$lyricsProviderHost\"" | |
) | |
}.apply(classDef.methods::add) | |
} |
classDef.methods.add(this) | ||
}.also { | ||
method.addInstruction( | ||
urlBuilderIndex - 1, |
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.
This will still add the instruction to the original method because the implementation is copied by reference when you call MutableMethod()
The user will enter an URL. This URL will be used when fetching lyrics.
The implemented logic is the same as https://github.com/Natoune/SpotifyMobileLyricsAPI/blob/main/scripts/xmanager.patch.js.
Closes #4906