feat: option to choose backup target directory #1354
Conversation
| override fun onCreatePreferences(savedInstanceState: Bundle?, rootKey: String?) { | ||
| hideKeyboard() | ||
| setPreferencesFromResource(R.xml.settings_updates, rootKey) | ||
| //val settingsManager = PreferenceManager.getDefaultSharedPreferences(requireContext()) | ||
| val settingsManager = PreferenceManager.getDefaultSharedPreferences(requireContext()) |
There was a problem hiding this comment.
using this top level declaration instead of in each listener. any reason to use separate calls previously?
There was a problem hiding this comment.
There is not, good thinking!
I presume it was like that because using it.context removed the need for requireContext() which can crash even when it should not.
There was a problem hiding this comment.
Does it? if thats the case we could have a null check usage of it.context.
There was a problem hiding this comment.
No, you misunderstand.
requireContext(): can crash, but no wet code
it.context: can't crash, but wet code
In this case requireContext() should be relatively safe because it is in onCreatePreferences.
|
|
||
| // No access rights after restore data from backup | ||
| "download_path_key", | ||
| "download_path_pref", |
There was a problem hiding this comment.
added download_path_pref in ignoreForBackup list, check if that is something we left deliberately.
There was a problem hiding this comment.
also check this value in strings.xml, why are we not keeping it translatable=false?
There was a problem hiding this comment.
Looks like a big mistake, download_path_pref is used both as a display string and as a key, it should never be used as both.
There was a problem hiding this comment.
correct, I would fix it but would let one of the maintainers comment on this.
| @@ -371,7 +351,7 @@ class SettingsGeneral : PreferenceFragmentCompat() { | |||
| ?: context?.let { ctx -> VideoDownloadManager.getDefaultDir(ctx)?.filePath() } | |||
|
|
|||
| activity?.showBottomDialog( | |||
| dirs + listOf("Custom"), | |||
| dirs + listOf(getString(R.string.custom)), | |||
| override fun onCreatePreferences(savedInstanceState: Bundle?, rootKey: String?) { | ||
| hideKeyboard() | ||
| setPreferencesFromResource(R.xml.settings_updates, rootKey) | ||
| //val settingsManager = PreferenceManager.getDefaultSharedPreferences(requireContext()) | ||
| val settingsManager = PreferenceManager.getDefaultSharedPreferences(requireContext()) |
There was a problem hiding this comment.
There is not, good thinking!
I presume it was like that because using it.context removed the need for requireContext() which can crash even when it should not.
|
|
||
| // No access rights after restore data from backup | ||
| "download_path_key", | ||
| "download_path_pref", |
There was a problem hiding this comment.
Looks like a big mistake, download_path_pref is used both as a display string and as a key, it should never be used as both.
|
fixed conflicts, clean for review again. |
Fixes: #1152 and #1106