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

feat: merge patch RVX/anddea #55

Merged
merged 43 commits into from
Jun 8, 2024
Merged

feat: merge patch RVX/anddea #55

merged 43 commits into from
Jun 8, 2024

Conversation

anddea
Copy link

@anddea anddea commented May 31, 2024

inotia00/revanced-integrations#43

  1. I reordered the YouTube settings, moving General to the top and rearranging elements as they appear in the UI. Additionally, I moved similar preferences closer to each other. The icons in the settings make it much easier to find elements, as they are displayed in YouTube's UI. But this reordering changed your defaults, so feel free to change it.
  2. Since I didn't have any time, I couldn't improve the search functionality as I wanted (for now). However, I left TODOs, so you or other contributors can continue this work. But still, search is functioning and is fine for everyone as it is right now.
  3. Overall, I left comments and documentation in some places. I suggest you to do the same: leave comments and documentation so other contributors can continue after us.

To inform about each feature, you can copy the commit messages. Feel free to change code/resources however you want, as I currently don't have the time.

Some notes:

  1. The Force snackbar theme does not change the snackbar for the player (e.g., quality change, captions, etc.). I couldn't find the responsible resource; it is probably implemented in the code (Jetpack Compose? I'm not sure, as I didn't have time to check).
  2. The Add splash animation patch can be integrated into the Custom branding icon patch and is needed only for the MMT animation.

TODO:

  • Minimize changes in dev branch
  • Check if a resource is valid
  • Lint code
  • Clarify patch option

@inotia00 inotia00 changed the title Killer feat: merge patch RVX/anddea May 31, 2024
@inotia00
Copy link
Owner

Because there are so many changes, reviewing PRs can take a long time.

I will try to review as quickly as possible, but I recommend not adding new changes to the PR branch if possible.

@inotia00

This comment was marked as resolved.

@inotia00

This comment was marked as resolved.

@anddea
Copy link
Author

anddea commented May 31, 2024

I reordered the YouTube RVX settings

Also, for other localizations, it appears messy, so ordering alphabetically (for English) does not seem like a good idea after all.

@inotia00
Copy link
Owner

The refactored RVX doesn't adhere to lexicographic arrangement as perfectly as before, but it nonetheless tries to preserve lexicographic arrangement as much as possible
(Because most users still speak English)

The arrangement of settings in RVX follows the official ReVanced settings, and I recommend not altering this order as much as possible to keep track of the official ReVanced source

@anddea
Copy link
Author

anddea commented May 31, 2024

to keep track of the official ReVanced source

I don't think the keys (such as key, title, summary, etc.) are the same as in ReVanced. Are they?

If you revert to the original settings order but include settings icons, you'll notice the mess in some places. But of course, it's up to you. You can just revert that one revanced-prefs.xml file.

If you include icons, you'll navigate by them once you get used to them. However, if you see a different order than in the UI, it may cause confusion.

@inotia00
Copy link
Owner

I don't think the keys (such as key, title, summary, etc.) are the same as in ReVanced. Are they?

In revanced-patches-4.7.1 I added a number of changes to match ReVanced's keys, titles, and summaries as much as possible.

If you include icons, you'll navigate by them once you get used to them. However, if you see a different order than in the UI, it may cause confusion.

I think this still needs to be considered.
Honestly, I've never used the Visual preferences icons patch so I don't think I understand it.

I will think about it for another day and then make a decision.

@anddea
Copy link
Author

anddea commented May 31, 2024

needs to be considered

It's just a customization option that can be turned off like any other patch, it can be also integrated inside Settings patch as boolean patch option.

Multiple people already opened issues in your repo asking for it and there are a lot of people coming to my repo for this feature that's why I opened it, so people don't use multiple forks. Of course, all those icons and resources take more space and increase patches size. And with new changes, this patch might need to be updated as well.

I created a poll for this feature if it's helpful.
anddea/revanced-patches#130

Edit: It's not a problem if you remove it, I just let you know about these changes and why they were done

Feel free to change code/resources however you want

@inotia00
Copy link
Owner

I mean, I'm not saying I won't merge this PR

I am talking about a change in the arrangement order of settings

I reorganized the settings arrangement to match the order of the official ReVanced settings, but PR is reverting this change
(For example, this PR places the General PreferenceScreen first)

If possible I would like to keep the arrangement order of the settings current

@Francesco146
Copy link

he's talking about the Visual preferences icons patch, not the order of the settings which is best kept the same from ReVanced

@anddea
Copy link
Author

anddea commented May 31, 2024

Ofc, you can revert it back by just copy-pasting your current arrangement. I've fixed some typos in keys, so only those have to be kept.

3609314

And added key for this preference (used for visual preferences icons)

<Preference android:title="@string/gms_core_settings_title" android:key="gms_core_settings" android:summary="@string/gms_core_settings_summary">

I think that's it, don't remember if there are other changes for settings icons.

Although, for me it seems better when General is on top. Also, there was a request for this before by someone.

Copy link
Collaborator

@KobeW50 KobeW50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings

…a more appropriate resource path, Clarify patch option, Excluded by default
@inotia00
Copy link
Owner

inotia00 commented Jun 8, 2024

Thanks

@vippium
Copy link

vippium commented Jun 11, 2024

In Visual Preference icons, as it is known that @inotia00 haven't included the "Extended name" and "extended icon" for options.json. It is good to see this. But the default icon for "Revanced Extended settings" is still set to "extension" which should be replaced with "Revanced" icon, to match it properly. As extension icon doesn't suits to Revanced settings.

So, pls change the "ExtendedIcon" from "Extension" to "Revanced" or something like that in the upcoming dev releases or stable.

Here is the icon image:
Screenshot_2024_0611_064101

@anddea
Copy link
Author

anddea commented Jun 11, 2024

the default icon for "Revanced Extended settings" is still set to "extension" which should be replaced with "Revanced" icon, to match it properly. As extension icon doesn't suits to Revanced settings.

"Extension" icon is used because these settings are Extensions for RV Extended, just like in the browser, you have normal settings and you can extend them with extensions, but you have plenty of options there if you want to change the dafault, including custom branding icons.

@vippium
Copy link

vippium commented Jun 11, 2024

you have plenty of options there if you want to change the dafault.

No option...for the change, there should be extendedIcon key, which is not there. As I think, he removed it from options file

@Git-Boy-Slim
Copy link

No option...for the change, there should be extendedIcon key, which is not there. As I think, he removed it from options file

You have indeed an option in the options.json under "Visual preferences icons", where you can change the settings icon of the revanced settings ("key": "RVXSettingsMenuIcon"). The name ("key": "RVXSettingsMenuName") and position ("key": "InsertPosition") is changeable with the "Settings" option.

@SaintNikku
Copy link

After this merge, "Revancify Yellow" can't be used with inotia00 patches whereas it was possible in anddea patches.

@inotia00
Copy link
Owner

After this merge, "Revancify Yellow" can't be used with inotia00 patches whereas it was possible in anddea patches.

"Revancify Yellow" icon was not included in this PR
If you want, you can open a new PR

@SaintNikku
Copy link

SaintNikku commented Jun 15, 2024

After this merge, "Revancify Yellow" can't be used with inotia00 patches whereas it was possible in anddea patches.

"Revancify Yellow" icon was not included in this PR If you want, you can open a new PR

@anddea
I have no idea what files are related to this yellow icon, can you kindly open a new PR to include that or guide me to correct commits and I'll do so.

Thank you.

@anddea
Copy link
Author

anddea commented Jun 15, 2024

I'll open more PRs with new fixes and features when I have time and patience.

There was no Revancify Yellow or new splash animations for AFN icons at the time when this PR was created.

@Francesco146
Copy link

I'll take care of the pr about Revancify yellow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants