-
-
Notifications
You must be signed in to change notification settings - Fork 725
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: Can't enter "null" in patch options #1704
Comments
ReVanced/revanced-patches#2762 (comment) as mentioned here i am now told ro redirect to here |
Doesn't deleting the option count as null? |
Deleting resets the option value to default. If the default is null, it will reset to null |
That sounds weird, if the default is a non-null value then the option by default is added in ReVanced Manager, removing it then should make it null If the default is null, it should be manually added by the user |
This is not the case. If you haven't configured any options, the options are not added in this view. Instead you have to click on plus to add them and change them. The reason for this is to cover this scenaro: An option value is null by default. You want to change it, so you add it. Now you want to set it back to null. As you can not input "null", you click on the icon to delete it which resets the value to null. This issue refers to the problem, when the option accepts null, but isn't null by default, so you have no way to set it to null. To improve UX some changes can be made:
This would fix the current issue and the slightly weird UX |
You're overcomplicating it, the issue here is that "Delete" and "Reset" are being used in a single button, simply adding a Reset button is enough I don't agree with showing all options, if an option is null by default then it shouldn't be added by default, that introduces some weird UX, especially accompanied with your "Set to null" option which is a glorified delete button (that you want to replace with reset) |
I'd like an elaboration on what is being complicated here. From what I am seeing, my suggestion simplifies the usage.
No, as the current delete functionality is to reset. By adding a reset button, you try repurposing this as a way to set it to null, whereas the dropdown menu is the correct purpose for abstract inputs such as paths to folders files, or null in this case. |
Null is just the computer way of saying nothing, a Delete button makes something nothing. The issue here is that the delete button isn't a delete button, it's a reset button, a proper delete button should be added and the existing button should have the icon updated to fix the weird UX and impossibility of manually setting null, without introducing an odd interface to the user. This is an end-user app, explain to me the difference between "Set to null" and "Delete", there's no difference, so why complicate it and make it less recognizable? what the hell is "null"? |
Please explain to me the different to the user. |
"Delete option" is a semantical problem. This already disqualifies it as a solution. Null being unclear is a problem, that does not justify using a semantically incorrect problem instead. Scenario: You want to change the icon but not the label. But with delete option you completely break the semantic: You want to change the icon but not the label. A solution that fixes this can be done on the patches side. |
I think the label could say "Value set to null" is better than my previous example. Little more clearer |
Bug description
In the patch 'Custom Branding' there should be an option to not change the icon, just like you can choose not to touch the app name
The only options are "ReVanced logo" and "Custom value"
Version of ReVanced Manager and version & name of application you tried to patch
Version - 1.18.0
Application - YouTube Revanced
Installation type
None
Device logs
.
Patcher logs
No response
Acknowledgements
The text was updated successfully, but these errors were encountered: