-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Feat: Add path picker GUI #19855
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
base: main
Are you sure you want to change the base?
Feat: Add path picker GUI #19855
Conversation
|
@david-allison I did apply your patch for this, but I wasn't sure if you were planning on making a pr for it since I didn't really do much so if you are then I will close this. Otherwise, if there's more changes to be made UI wise, I'd be happy to continue working on it |
|
It's been a long while, thanks!!
I don't recall my patch, but I wasn't planning on submitting it any time soon, thanks for seeing it through!! |
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
|
Will resolve all the changes, I thought I had reacted sorry TT |
a8d65c5 to
7957d5d
Compare
|
The new UI look for the changes I just made are: While using android 10 (i.e. no scoped storage) While using android 15 (playDebug) While using android 15 (fullDebug) Note: it looks truncated but you can scroll down there, as seen in the videos |
|
There should be enough space on the screen that it doesn't need to be truncated |
Do you think it would be better to reduce the existing text like say shorten it by removing "/storage/emulated/0" or should I resize the dialog altogether, the latter will have a lot of changes since the only method I've found is to make a subclass of ListPreferenceTrait and use setlayout there to change the size according to the screen |
7957d5d to
bb41f23
Compare
|
Screen_recording_20251221_044059.webm |
| return null | ||
| } else { | ||
| dialogLayoutResource = initialLayoutResId | ||
| return FullWidthListPreferenceDialogFragment() |
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.
This was changed to return the new subclass
| private val initialLayoutResId = dialogLayoutResource | ||
|
|
||
| override fun makeDialogFragment(): DialogFragment? { | ||
| val useTextBox = (forceCustomSelector || isEditText(context)) |
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.
This variable was added because I felt that the user should be able to see the list dialog again even if they choose custom path. Previously, the behaviour was that if they chose custom path, the text field would open and they could make their actions but when they went back to choose again, the text field would still open and not give them the list dialog
|
@david-allison just a reminder since the needs author reply tag wasn't removed ^^ |
|
Still on my to-do list, I'll get to it shortly! |
david-allison
left a comment
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.
- Select Custom Path
/storage/emulated/0/Android/data/com.ichi2.anki.debug/files/AnkiDroi
- Open Dialog Again
- 🪲 Dialog does not show the current path as an option
- 🪲 Selecting the top option does not apply the provided path
I would move the 'custom path' to a button at the bottom of the screen, with a very brief name. This means it won't be truncated at the bottom of the list if there's a number of entries
| val label = it.absolutePath.replace("/Android/", "\n/Android/") | ||
| ListPreferenceTrait.Entry(label, it.absolutePath) | ||
| }.toMutableList() | ||
| .apply { add(ListPreferenceTrait.Entry("Select custom path...", "CUSTOM_PATH_SENTINEL")) } |
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.
This would need to be translatable
bb41f23 to
ab78a9d
Compare
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
|
Screen_recording_20251222_221717.webm Edit: video seems to be buffering, hope its just on my end |
david-allison
left a comment
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.
My main concern is that the logic to switch between EditText and ListPreference is complicated.
Could you look into alternate solutions which require fewer variables/state?
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
| fun findAnkiDroidSubDirectories(f: File): List<File> { | ||
| // either returns '/AnkiDroid1...AnkiDroid100' from storage migration | ||
| // OR returns '/AnkiDroid' if no directories are available | ||
| val found = f.listFiles(ANKI_DIR_FILTER)?.toList() | ||
| return if (!found.isNullOrEmpty()) { | ||
| found | ||
| } else { | ||
| listOf(File(f, "AnkiDroid")) | ||
| } | ||
| } |
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.
This isn't good logic: multiprofile support needs to be considered, as does someone creating a custom directory on their SD Card/External Stroage via USB
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.
This was the only thing I wasn't sure about, with the new commit it should handle custom directories with the custompath var but as for multiprofile, I believe we haven't implemented it yet so I wasn't sure how to handle support for it, maybe I could put a TODO for multiprofile support.
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
|
I'll force push with the changes soon since there's a lot to be done I think, thank you for such a comprehensive review! |
|
just had a question regarding the file picker ui issue im doing. Right now, I have loadDirectories() running on the main thread and I had asked gemini to try and spot any issues and it said that having loadDirectories() on the main thread can cause ANR with devices having an older SD card and it's better to put it in the background (to me this makes sense since if the sd card can't load all the directories fast enough, the main thread is going to take too long and ANR occurs). I tried looking this up but wasn't able to find anything concrete, so I wanted to know if it's an actual issue and if its a better practice to throw it in the background? Flagging the above for discussion, I'll leave this as a TODO in the code |
ab78a9d to
cb5b6c0
Compare
2ee9832 to
9d60055
Compare
|
I believe I've made all the changes I wanted to push. I removed the very fragile logic that was there before, with the flags. Instead, I'm opening the dialogs directly and also I changed the text box dialog to the one that is being used in other places i.e. AnkiR.layout.dialog_generic_text_input |
|
|
||
| data class Entry( | ||
| val key: String, | ||
| val key: CharSequence, |
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 don't think this is the best way to go about it, but unless I do this I get errors in lines 103 and 104
david-allison
left a comment
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've asked for a second reviewer on Discord
Please ping me in 48 hours here, I'll need to give this a deeper dive, and likely submit a patch
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
4a6fbe0 to
7eb9ad7
Compare
|
@david-allison super sorry for the late ping, had a lot of college work to finish off but I've addressed all the comments best I could! |
bfd4a04 to
81d2155
Compare
david-allison
left a comment
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.
This looks solid!
Are tests possible here?
Fixing the AnkiDroid directory was critical functionality back in the day, and still is on older devices.
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
| CollectionHelper | ||
| .getAppSpecificExternalDirectories(context) | ||
| .filterNotNull() | ||
| .flatMap { findAnkiDroidSubDirectories(it) } | ||
| .distinct() | ||
| .filter { it.absolutePath != defaultDir?.absolutePath } | ||
| .forEach { add(createEntry(it)) } |
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.
If there is 1 exception, it stops further iteration.
- Extract this to a method which is more exception-safe
- Maybe use
addAllon the value of that method call
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
| listEntries = loadDirectories() | ||
| entries = listEntries.map { it.key }.toTypedArray() | ||
| setEntryValues(listEntries.map { it.value as CharSequence }.toTypedArray()) | ||
| listValue = value ?: defaultAnkiDir?.absolutePath ?: "" | ||
| setValue(listValue) | ||
| return FullWidthListPreferenceDialogFragment() |
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.
This code feels unusual - I know it's an unusual abstraction, but flagging to see if we can do better.
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 it's because there's a lot going on here and also I did have to change the entry class to allow for the mapping, so it's not the best implementation definitely
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Show resolved
Hide resolved
| <string name="pref_directory_not_set">Not set</string> | ||
| <string name="pref_custom_path" maxLength="41">Custom path</string> | ||
| <string name="pref_enter_custom_path" maxLength="41">Enter custom path</string> | ||
| <string name="pref_cannot_create_directory">Cannot create directory</string> |
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.
Is could_not_create_dir not usable?
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.
Didn't realise we had an existing string for it oops
| <string name="pref_custom_path" maxLength="41">Custom path</string> | ||
| <string name="pref_enter_custom_path" maxLength="41">Enter custom path</string> | ||
| <string name="pref_cannot_create_directory">Cannot create directory</string> | ||
| <string name="pref_directory_not_writable">Directory is not writable</string> |
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.
Consider using the nio (Java new input/output) methods, which throw good exceptions, then displaying a stack trace with to the user.
To be honest, tests have been something I've been meaning to learn more about, I'll look into whether we can add them here once I clear the reviews and give another update! |
81d2155 to
cd1e5fe
Compare
|
Still have to switch to using nio methods, will do this and look into the testing tomorrow |
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/preferences/ExternalDirectorySelectionPreference.kt
Outdated
Show resolved
Hide resolved
This feature allows users to choose a file path that exists on their device or an external storage device. If the user is using a full release version, or a pre android 11 version then they have the option to edit their path to a custom one
cd1e5fe to
ad9be12
Compare




Purpose / Description
This pr adds a path picker gui when a user wishes to select a path through the advanced settings, if the user is using the google play version, it restricts their options to their internal storage or to an external sd card (it automatically gives them the sd card option, whereas before they would have to manually find the path). If the user is on the fullrelease or full debug, then they can give permissions to manage all storage and they can manually enter their path since scoped storage wouldn't be an issue.
Fixes
Approach
David's patch ended up working very nicely and I just implemented his TO-DO's with the exception of one which I wasn't too sure about.
How Has This Been Tested?
Checklist
Please, go through these checks before submitting the PR.