[Multiple Profiles] - PR1- Add a switch in developer options to toggle the Switch Profiles in Settings preferences#18484
Conversation
|
Important Maintainers: This PR contains Strings changes
|
fa89ba4 to
cd19c6b
Compare
lukstbit
left a comment
There was a problem hiding this comment.
Thanks, left some comments.
Also, please squash the two commits together.
| @@ -0,0 +1,10 @@ | |||
| <vector xmlns:android="http://schemas.android.com/apk/res/android" | |||
There was a problem hiding this comment.
Is this icon from the studio vector assets library?
There was a problem hiding this comment.
No this one is from Google Material Icons here -
| } | ||
|
|
||
| // Helps to enable/disable the switch profiles feature in settings | ||
| findPreference<SwitchPreferenceCompat>(getString(R.string.enable_multiple_profiles_pref_key)) |
There was a problem hiding this comment.
This entire code is not needed:
- we are just interested in the value found in the associated key so there's no need for manual setup
- there's no need for the toast, the switch behavior is clear
- there's no need to recreate the activity
| </com.ichi2.preferences.HeaderPreference> | ||
|
|
||
| <!-- Switch Profiles Preferences --> | ||
| <com.ichi2.preferences.HeaderPreference |
There was a problem hiding this comment.
Was this extra settings category discussed? I would think we don't need/have enough content for it especially as all sections have multiple groups of settings.
There was a problem hiding this comment.
This was proposed by me during the GSoC proposal. I also suggested to add an icon on the top menu in DeckPicker, to which @david-allison suggested that it was too prominent and should rather be in Settings.
So I created an option in Settings. I agree that all sections have multiple groups of settings, so what do you suggest - should we keep it like this or move into some other section?
I suggest maybe we keep it like this and work on the switch profiles feature itself and later on if required we can just move it somewhere or keep it there. But to begin with Multiple profiles it is a good place and as it's under developer settings so it should be fine
There was a problem hiding this comment.
Also, this makes it consistent with ankimobile, which have the list of profiles in the section.
@oyeraghib I'd be interested if you could link to your discussion with David please.
I don't even think an icon is useful here. I think we could imitate ankimobile and just have a text at the top stating which profile is used (if here are multiple profiles)
In any case, I agree that it'd be easy to move if we want to move it later.. would you have any idea of a better place to put the list of profile (and option to rename/delete one) @lukstbit ?
| ) { | ||
| setPreferencesFromResource(R.xml.preference_headers, rootKey) | ||
|
|
||
| fun shouldDisplaySwitchProfilesOption(context: Context): Boolean = |
There was a problem hiding this comment.
The oneline method is used only once, use a variable.
| <string name="switch_profiles_title" maxLength="41">Switch Profiles</string> | ||
|
|
||
| <string name="pref_switch_profiles_list">switch_profiles_list</string> | ||
|
|
There was a problem hiding this comment.
Only add strings that are actually used in the PR.
| <!-- Switch Profiles --> | ||
| <string name="multiple_profiles_screen_pref_key">multiple_profiles_screen</string> | ||
| <string name="enable_multiple_profiles_pref_key">enable_multiple_profiles</string> | ||
| <string name="switch_profiles_title" maxLength="41">Switch Profiles</string> |
There was a problem hiding this comment.
Wait a bit for other opinions on the new setting section, but consider:
- the three strings above should be in 10-preferences.xml
- use a name like the other keys use: pref_multiple_profiles_screen_key
- the category title follow a specific pattern: pref_cat_switch_profiles
There was a problem hiding this comment.
Not putting them inside the 10-preferences.xml as @Arthur-Milchior suggested that we should be keeping the string in 12-dont-translate.xml
So translators do not end up translating a string which would later change. So we keep it in dont-translate and when everything is completed we can move the strings to their respective places.
Please pay more attention to the message in the default PR template, and fill
|
|
I am looking at the mentioned changes and these conflicts, will update the PR soon. |
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
* common & libanki
I tried using `buildSrc`, but:
* it doesn't support version catalogs out the box
* it doesn't support `android { }` out the box
Given we have such little logic, it felt prudent to stick
with a `.gradle`
`AnkiDroid/jacoco.gradle` exists, so use `jacocoSupport.gradle`
Prep for 16618: common doesn't run tests
Tested by failing 'StringUtilsTest' and 'AnalyticsConstantsTest' Fixes 16618
Make it clickable in Android Studio terminal
GSoC 2025: Review Reminders - Makes `ReviewReminder` Parcelable so that it can be passed to and from the AddEditReminderDialog -- the AddEditReminderDialog will receive a `ReviewReminder` to edit via arguments and return one via a FragmentResult, `ReviewReminder` must be Parcelable to allow this to happen
`TrimLambda` inspection https://cs.android.com/android-studio/platform/tools/base/+/mirror-goog-studio-main:lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/TrimDetector.kt The Kotlin standard library `trim()` call takes an optional lambda to specify which characters are considered whitespace. When converting Java code to Kotlin code, the converter will convert calls for Java's `s.trim()` into `s.trim() { it <= ' ' }`. This preserves the exact semantics of the Java code, but is likely not what you want: the default in Kotlin uses a better definition of what constitutes a whitespace character (`Char::isWhitespace`) and also results in less bytecode at the call-site.
Issue 18716
GSoC 2025: Review Reminders - When initializing the deck spinner in AddEditReminderDialog, I wanted to use initializeNoteEditorDeckSpinner, but it was annotated with MainThread which meant I couldn’t use withCol and would have had to use getColUnsafe, which I think we’re not supposed to use anymore? I wasn’t sure if I was allowed to refactor everything, so I created a new method that calls withCol internally and tried to copy the format of the other initialization methods. Are these methods supposed to be annotated with MainThread? There’s an annotation at the top of the file marked “this class is a mess…” so I assume it’s due for a refactoring?
this is the same idea from the unit test workflow that lets us dynamically expand our matrix based on manual workflow input from GitHub web UI
MIUI doesn't support Material You well - the color is based on the system background color, with no means to disable it. https://www.reddit.com/r/Xiaomi/comments/y6zclf/material_you_on_miui_13/ Since we are using a widget, we can't use custom themes as a fix So to fix the issue: duplicate the assets/layout as unthemed and fix them there AdaptionUtil code copied from 43151b6 and converted to Kotlin `circle_background` did not need to be duplicated as the background was tintable just tinting the star was not possible **Duplicate unthemed widget icons:** `@android:color/system_neutral1_100` => #E2E2E9 `@android:color/system_neutral1_600` => #5D5E64 `@android:color/system_neutral1_800` => #2F3036 `@android:color/system_neutral1_900` => #1A1B20 `@android:color/system_neutral1_1000` => #000000 `@android:color/system_accent1_300` => #94AAE4 `@android:color/system_accent1_400` => #7A90C8 `@android:color/system_accent1_500` => #6076AC `@android:color/system_accent1_600` => #475D92 `ic_anki_unthemed.xml`: accent1_300 => 500 `ic_anki_unthemed_finish.xml`: accent1_400 => 600 **widget_small_unthemed** `material_dynamic_primary60` => #7A90C8 Fixes 18869
329a34d to
769f945
Compare
769f945 to
baf1ea2
Compare
Thanks! I have updated the PR description |
baf1ea2 to
b8aba82
Compare
Arthur-Milchior
left a comment
There was a problem hiding this comment.
I'm fine with PR, once the changes in 12-dont-translate are removed.
| </com.ichi2.preferences.HeaderPreference> | ||
|
|
||
| <!-- Switch Profiles Preferences --> | ||
| <com.ichi2.preferences.HeaderPreference |
There was a problem hiding this comment.
Also, this makes it consistent with ankimobile, which have the list of profiles in the section.
@oyeraghib I'd be interested if you could link to your discussion with David please.
I don't even think an icon is useful here. I think we could imitate ankimobile and just have a text at the top stating which profile is used (if here are multiple profiles)
In any case, I agree that it'd be easy to move if we want to move it later.. would you have any idea of a better place to put the list of profile (and option to rename/delete one) @lukstbit ?
| <!-- Switch Profiles: In progress for Switch Profile feature --> | ||
| <string name="pref_switch_screen_key">switch_profile_screen</string> | ||
| <string name="pref_enable_switch_profile_key">enable_switch_profile</string> | ||
| <string name="switch_profile_title" maxLength="41">Switch Profile</string> |
There was a problem hiding this comment.
If I recall correctly, we ended up deciding not to use 12-dont-translate, but to hard code the string in kotlin until we are actually certain of what UI we'll want. And just use a @Suppress to remove the warning
There was a problem hiding this comment.
Here is the link to the comment by David -
https://docs.google.com/document/d/1vvfdsReSLuSqvK0TrhGrzWZAbGf2r_o1NehVz9fm6iA/edit?disco=AAABfwHvoy8
| <string name="schedule_reminders_do_not_translate" maxLength="28" comment="Do not translate this string, this feature is still in development. Name of the screen that appears when review reminders are being scheduled.">Schedule reminders</string> | ||
|
|
||
| <!-- Switch Profiles: In progress for Switch Profile feature --> | ||
| <string name="pref_switch_screen_key">switch_profile_screen</string> |
There was a problem hiding this comment.
This file is fro strings that the user will see.
To see where to put such a key, please look at where similar keys are.
For example, you could see that pref_appearance_screen_key, which similaryl is used in preference_headers.xml, is declared in values/preferences.xml.
| override fun onResume() { | ||
| super.onResume() | ||
| Timber.d("onResume") | ||
| updateSwitchProfileVisibility() | ||
| } |
There was a problem hiding this comment.
This doesn't work on tablets
There was a problem hiding this comment.
yeah, remove it and call ActivityCompat.recreate in the dev options switch
| requirePreference<HeaderPreference>(R.string.pref_notifications_screen_key).isVisible = !Prefs.newReviewRemindersEnabled | ||
|
|
||
| // switch profile | ||
| updateSwitchProfileVisibility() |
There was a problem hiding this comment.
keep this code similar to the above (inline the function)
| override val analyticsScreenNameConstant: String | ||
| get() = "pref.switchProfiles" | ||
|
|
||
| override fun initSubscreen() { |
There was a problem hiding this comment.
Call this on change to handle tablet mode
ActivityCompat.recreate(requireActivity())| * You should have received a copy of the GNU General Public License along with * | ||
| * this program. If not, see <http://www.gnu.org/licenses/>. * | ||
| ****************************************************************************************/ | ||
| package com.ichi2.anki.preferences.switchProfiles |
There was a problem hiding this comment.
package names should be in lower case https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html
also, profiles is enough. Doesn't need the switch prefix IMO
| import androidx.preference.Preference | ||
| import androidx.preference.PreferenceViewHolder | ||
|
|
||
| class ProfileListPreference( |
There was a problem hiding this comment.
This class does nothing. If you are going to do something with it later, add it as a separate commit in a future PR. As of now, I can't tell if this will be necessary.
| import com.ichi2.anki.R | ||
| import com.ichi2.anki.preferences.SettingsFragment | ||
|
|
||
| class SwitchProfilesSettingsFragment : SettingsFragment() { |
| /** | ||
| * Whether the switch profiles feature is enabled. | ||
| */ | ||
| var switchProfileEnabled by booleanPref(R.string.pref_enable_switch_profile_key, false) |
There was a problem hiding this comment.
| var switchProfileEnabled by booleanPref(R.string.pref_enable_switch_profile_key, false) | |
| val switchProfileEnabled by booleanPref(R.string.pref_enable_switch_profile_key, false) |
Also, put it below the Developer options heading if this is just a flag for a WIP dev option
| requirePreference<HeaderPreference>(R.string.pref_review_reminders_screen_key).isVisible = Prefs.newReviewRemindersEnabled | ||
| requirePreference<HeaderPreference>(R.string.pref_notifications_screen_key).isVisible = !Prefs.newReviewRemindersEnabled | ||
|
|
||
| // switch profile |
| override fun onResume() { | ||
| super.onResume() | ||
| Timber.d("onResume") | ||
| updateSwitchProfileVisibility() | ||
| } |
There was a problem hiding this comment.
yeah, remove it and call ActivityCompat.recreate in the dev options switch
| get() = R.xml.preferences_switch_profiles | ||
|
|
||
| override val analyticsScreenNameConstant: String | ||
| get() = "pref.switchProfiles" |
There was a problem hiding this comment.
| get() = "pref.switchProfiles" | |
| get() = "pref.profiles" |
b8aba82 to
da9712c
Compare
Purpose / Description
Add a switch in developer options to toggle the Switch Profiles option in Settings Preferences
Fixes
Approach
How Has This Been Tested?
Manually tested on Google Pixel 6A
Steps:
Video changes:
WhatsApp.Video.2025-07-15.at.11.15.30.mp4
Learning (optional, can help others)
Describe the research stage
Checklist
Please, go through these checks before submitting the PR.