-
-
Notifications
You must be signed in to change notification settings - Fork 767
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
refactor: reorganize and rename settings #1307
Conversation
assets/i18n/en_US.json
Outdated
@@ -241,15 +240,15 @@ | |||
"importPatchesHint": "Import patches selection from a JSON file", | |||
"importedPatches": "Patches selection imported", | |||
|
|||
"resetStoredPatchesLabel": "Reset patches", | |||
"resetStoredPatchesLabel": "Reset patches selection", |
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.
Im not sure if it should be "Reset patches selection" or "Reset patch selection" (same question for the similar options)
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.
Maybe "Reset selection of patches"?
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.
I think that sounds clunky. If this is changed the other options need to be changed too for consistency (Ie: "Export selection of patches")
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.
What do you think @PalmDevs
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.
Technically, it should be patch selections
, since it is multiple selections of patches. This can be combined with options which would result in something along the lines of:
Reset patch selections and options
Patches selection
just doesn't sound right in my opinion.
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.
Okay, ill make these changes today
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.
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.
🥞 LGTM
Should the 'Allow experimental patches' be renamed to something like "Ignore version restraints"? The current title is generic |
Noticed some inconsistencies:
|
What does CLI say for the |
Even for importing a keystore? Saying "Import the keystore used to sign apps" makes it sounds like there is only one keystore in existence. I think "a keystore" is more appropriate in this context |
Good point, use |
Im not sure, and idk how to find out |
Here it is. This is unfortunately too nerdy/advanced for an average user to understand however. Any suggestions based on this? |
Thanks. I cant think of anything except that "force" may sound like the user doesn't have a choice |
I asked on discord. osum, ushie, and taku discussed it and came up with these
I dont think it matters too much. All these are better than the current title, but more context will be needed in the description anyway |
5 is out of the question, it's not necessarily incompatible at times. 1 is unfortunately too long. However, combinations of 2 and 3 or 4 might work.
|
How about negating and calling the switch "Version compatibility check" and having it on by default? |
Good idea. It makes you feel like you're turning off a feature. |
changed a file name and string
Should I rename all strings relating to the Info section to refer to it as the Debug section instead so it is easier to understand the code? And should I do the same while renaming the "experimental patches" to "Version compatibility check"? (edit: I reflected these changes locally and can push them. I just don't know how to revert a push or commit so I'm waiting for a "go-ahead") Also, I'm not sure how to change the default value for the Version compatibility check toggle so ill need someone to help me out. I asked validcube on discord. Will see if he's down Lastly, do I need to build the apk after the final changes are made to test? Some of you may know that I cant figure flutter out 😅 |
This implies that the app will be slower in general, but only the app list loading speed is affected |
changes all references to the experimental patches support to "version compatibility check"
2023-10-13.23-03-02.mp4 |
We ended up keeping the 'Reset patch options' in the import/export section because it is closely related to the 'Reset patch selections'. |
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.
Needs a few string changes and documentation changes.
@PalmDevs I'm not sure if it sends notifications for code review replies (sorry for the extra ping if it did) |
@KobeW50 Press change any "Patch selections" to "Patch selection", the pluralized form of selection is not needed. Thank you! 🙏 |
What about what was discussed here? Are we considering all selections as one collective selection? I think what you and oSum said earlier is the correct wording When we are referring to the selection of all apps, it should be |
Yes, we'll be considering every selection as one collective selection. |
It seems confusing to use the same term to refer to a single selection and the collective of multiple selections. When |
If you can say |
I believe this is ready to be merged. |
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.
A few changes I've missed (sorry!) and we're good.
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.
LGTM!
Closes #1296
Closes #1297
I renamed some of the options, partially based off of palms comment in #1296 (comment)
Huge thanks to @validcube for compiling and testing (bec i have no clue what im doing)