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

bug: Inconsistencies in patch name/description #740

Closed
3 tasks done
ILoveOpenSourceApplications opened this issue Jun 18, 2023 · 17 comments · Fixed by #2554
Closed
3 tasks done

bug: Inconsistencies in patch name/description #740

ILoveOpenSourceApplications opened this issue Jun 18, 2023 · 17 comments · Fixed by #2554
Labels
Bug report Something isn't working

Comments

@ILoveOpenSourceApplications
Copy link
Contributor

Type

Cosmetic

Bug description

For most hide-whatever-patches, their description startes with "hides whatever" according to the their respective patch names. But this is not followed throughout, like in the case of hide-ads, the description is "Removes general ads." and for hide-player-buttons and hide-video-action-buttons the descriptions are "Adds the option to hide video player previous and next buttons." and "Adds the options to hide action buttons under a video."
Then there's enable-debugging and it's description is "Adds debugging options." whereas minimized-playback description is "Enables minimized and background playback."
There are other cases as well which I won't be listing here. But if needed I can go through and list them, but I hope what I meant is clear.

Steps to reproduce

Open the manager and select the YouTube apk and go through the patch list.

Relevant log output

Nil

Screenshots or videos

No response

Solution

Follow a more consistent naming scheme and description for patches as long as it does something similar to an existing patch.

Additional context

No response

Acknowledgements

  • I have searched the existing issues and this is a new and no duplicate or related to another open issue.
  • I have written a short but informative title.
  • I filled out all of the requested information in this issue properly.
@ILoveOpenSourceApplications ILoveOpenSourceApplications added the Bug report Something isn't working label Jun 18, 2023
@oSumAtrIX
Copy link
Member

But if needed, I can go through and list them, but I hope what I meant is clear.

Sure, please list all of them.

@ILoveOpenSourceApplications
Copy link
Contributor Author

  1. always-autorepeat - "Always repeats the playing video again." Patch name could be changed to always-repeat.
  2. comments - Hides components related to comments.
  3. disable-auto-captions - "Disable forced captions from being automatically enabled." Should've been "Disables forced auto captions", simple.
  4. custom-branding - "Changes the YouTube launcher icon and name to your choice (defaults to ReVanced)" whereas custom-video-buffer - "Lets you change the buffers of videos." Should be made uniform.
  5. old-quality-layout - "Enables the original video quality flyout in the video player settings."

All hides patches provide an option within the app to turn it off/on. So is it better that all of them be changed to adds option to hide .... or to keep it as it is, which is start of the description with hides ...

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Jul 14, 2023

It would be great if there was an annotation to indicate a patch is selectable/configurable inside the target app.

Then the website patch list and the patcher could show that as a tag or icon, instead of relying on the patch description of literally 50 YouTube patches saying the same "adds an option of..."

It would also allow a "simple patch mode" for the patcher, where all in app selectable patches are always included and the user then only picks from the permanent patches (the patches that have no in app options and are always active). The patcher would still have an 'expert' patch mode that allows the user to not included those patches, but 99% of users would never use it as there's no reason to (just turn off the feature inside the app).

It would make patching way simpler and less daunting to new users. And when new YouTube patches come out or are renamed, users would automatically get the patch without them needing to pay attention and include the new patch.

@oSumAtrIX
Copy link
Member

Adding options to YouTube is merely a patch, not a ReVanced patcher feature. While options are added by YouTube patches for example, none are added for Twitter.

@LisoUseInAIKyrios
Copy link
Contributor

I know it's not a patcher feature, but it would be to indicate the patch should basically always be included (except for users who just insist to remove the option from the settings, or to debug if a patch is causing an issue).

And yes, twitter and reddit would never have that indicator (since picking those patches are important, since they cannot be turned off later)

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Jul 14, 2023

Some patches may inject options that can toggle certain things but not be disabled; they are merely a feature added and decided by patches. Incorporating a feature that is merely introduced and controlled by patches to the patcher may be the wrong approach. As an alternative solution the description of patches can be utilized as that is in control of the patches but still a feature by ReVanced Patcher.

@RZMTL
Copy link

RZMTL commented Aug 7, 2023

This is extremely confusing to me as well. At the very least, the patch descriptions need to be updated to say 'adds the option'. The current system of trial and error is a big pain in the ass.

Also, the 'hide layout components' patch is named horribly, since literally half of the patches do that or add options to do that.

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Aug 7, 2023

That's why I suggested some way for Manager to let the user include/exclude a patches only the patch makes permanent changes. Then for YouTube the user only needs to decide to include/exclude one patch (remove the player background overlay, which is a permanent change), because all other patches can be changed in app.

Basically the patch annotation would have two options, recommended (always include, no matter what, unless the 'expert mode' described below is turned on). And a suggested option, where the patch makes permanent changes but it's still suggested to include (example is remove ads for Twitter). Change the existing "include" patch annotation field to an enum of "default exclude", "recommended", and "suggested", or something to the effect of these 3 options.
All recommend patches are automatically included, and perhaps even hide these patches from the UI to make the experience simpler.

Manager would have an option to "allow excluding recommend patches", that allows the current system of picking and choosing from all 50+ Youtube patches. It would be used mostly to debug and exclude patches when potential bugs show up, and for the few obsessive users who really want to remove setting options instead of simply turning off the feature in app.

Something like this would dramatically simplify the new user experience, because currently users are presented with making decisions to include/exclude from 50+ patches and they don't know the patches can be turned off in app and it's best to include them all. Adding a describe of "adds an option to" would help, but it still shows 50+ patches and prompts the user to pick from them when for YouTube the user only needs to make 1 decision (hide the player overlay).

Obviously for Twitter, Reddit, etc, the user would still pick from the patches since they cannot be changed in app. So this 'always include' feature would only be used for YouTube, Twitch, and TikTok.

@RZMTL
Copy link

RZMTL commented Aug 7, 2023

That's overcomplicated. Just make the patch descriptions accurate. Your fix takes actual development time while fixing the descriptions takes 5 minutes.

There is already a 'recommended patches' option in the manager. As long as the recommended patches option only includes patches that add additional options (plus microG), you're good.

@LisoUseInAIKyrios
Copy link
Contributor

Adding an annotation field and an extra option in Manager is not complicated. The result is a way simpler patching experience.

Why show the user 50+ "adds an option of" patches, when instead only 1 patch could be shown?

@oSumAtrIX
Copy link
Member

So this 'always include' feature would only be used for YouTube, Twitch, and TikTok.

This sounds like a feature solely made for a constrained set of use cases such as YouTube, Twitch and TikTok. Patches usually do not offer options at all, it's a single patch like the Settings patch, which does, and unless patches for other apps provide such a patch, this annotation loses all purpose.

What would work is changing our conventions of describing patches using the description annotation.
The description could be in-depth, have a short description and long description, images or link fields and much more, giving each patch its individual flexibility, unlike the proposed annotation, which would only work for a constrained set of use cases instead of maximizing the potential that the description can offer.

@RZMTL
Copy link

RZMTL commented Aug 7, 2023

I mean, can't the repo managers just require that patch maintainers give an accurate and concise description of what each patch does? The problem is that some of the patches have inaccurate or incomplete descriptions.

That should be the first step. Adding a bunch of code and new metadata to populate is pointless if the metadata we already have isn't even being filled in correctly.

@oSumAtrIX
Copy link
Member

The repo maintainers are also patch maintainers.

Adding a bunch of code and new metadata to populate is pointless if the metadata we already have isn't even being filled in correctly.

This has already been said here.

@RZMTL
Copy link

RZMTL commented Aug 7, 2023

The following patches for youtube have incomplete descriptions:

'always autorepeat', 'comments', 'disable shorts on startup', 'disable auto captions', 'disable fullscreen panels', 'disable popup player panels', 'hide shorts components', 'hide ads', 'hide album cards', 'hide captions button', 'hide cast button', 'hide endscreen cards', 'hide layout components', 'hide load more button', 'hide seekbar', 'hide timestamp', 'hide video action buttons', 'hide watermark'

These patches need to say if they actually disable/hide a component or if they add an option to do so.

Most of them add an option, but some of them don't.

Contrast with the 'navigation buttons' patch, which correctly states that it "Adds options to hide or change navigation buttons" as well as the 'Hide video action buttons' patch, which correctly states that it "Adds the options to hide action buttons under a video".

To clarify, the convention should be:

If a patch adds an option (to hide/disable/remove) functionality from the app, that needs to be specified in the patch description. If the description does not specify that an option is added, the patch should hide/disable/remove that feature unconditionally.

@DmitriiShamrikov
Copy link

DmitriiShamrikov commented Nov 20, 2023

Hello, do we have any info about the patches anywhere? I mean maybe in this repo or in some other documentation if we don't have it in the app for now.

I might be an idiot but I cannot comprehend why anyone would want to remove comments, captions, seekbar, timestamps or navigation buttons - that's almost all functionality - from the app. Especially considering that Revanced Manager strongly suggests using recommended settings and not unchecking anything manually.

@oSumAtrIX
Copy link
Member

The patches are included but disabled by default.

@oSumAtrIX oSumAtrIX transferred this issue from ReVanced/revanced-patches-template Dec 14, 2023
@oSumAtrIX oSumAtrIX transferred this issue from another repository Dec 14, 2023
@KobeW50
Copy link
Contributor

KobeW50 commented Dec 20, 2023

@ILoveOpenSourceApplications I opened a pull request in which I reworded many patch descriptions. If you have time, please look over it and provide feedback and suggestions if necessary. Thanks!

@oSumAtrIX oSumAtrIX linked a pull request Jan 1, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug report Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants