-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix that shared preferences use description strings instead of unique keys #454
Fix that shared preferences use description strings instead of unique keys #454
Conversation
chrgernoe
commented
Feb 18, 2024
- Fixes Shared preferences uses description strings instead of unique keys #453
- Needs merge of Use Android Preference Library for App Settings #429 before.
We should also make the naming in the code consistent. Currently there are two different wordings:
The Android library uses the the word preferences. Created a new issue for this: #456 |
* Check whether the settings need to be migrated or not. | ||
*/ | ||
private fun _checkForSettingsMigration() { | ||
if (_customSettings.getInt(getString(R.string.pref_key_pref_version), 1) == 1) { |
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.
Wouldn't it make more sense to check for the latest version? Otherwise, we would always need to remember adapting this line when we increase the version.
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.
The point is that the migration is only necessary from the current format version 1 to the version 2, which is the format introduced with this PR. Assume, the user does not update its app and meanwhile we introduce another version with another migration path. In this scenario, we first need to migrate from 1 to 2 and afterwards from 2 to 3.
app/src/main/java/com/yacgroup/yacguide/utils/SearchBarHandler.kt
Outdated
Show resolved
Hide resolved
This is a quite critical change. I suggest to trigger a dev release after merging it to have some more time for testing. |
32d9369
to
bf83bbe
Compare