Implement and use internal API for reading and writing preferences

[oakkitten]

At this point this is not a real PR but more like a RFC or something like that.

Currently, preferences are retrieved using SharedPreference APIs directly, e.g.

val context = AnkiDroidApp.getInstance().baseContext
val preferences = AnkiDroidApp.getSharedPrefs(context)
val answerButtonPosition = preferences.getString("answerButtonPosition", "bottom")

if (answerButtonPosition == "bottom") { ... }

There's a few problems with this:

• It's verbose;
• In most cases the string and the default values are hardcoded. These same values are also hardcoded in XML;
• The return values are “raw” objects like strings, which might be fine for stuff like names or milliseconds, but you probably want to have stuff like enums for choises or HttpUrls for HTTP URLs;
• This will crash if the stored value for answerButtonPosition is not a string;
• You have to think about nullability.

I propose to introduce a new internal API for retrieving and storing preferences, with which the above example will look like this:

if (settings.answerButtonPosition == Settings.AnswerButtonsPosition.Bottom) { ... }

The single runnable commit here is a proof-of-concept example of how this could look like. Only a few settings are fetched via the API.

• I named things “settings” simply to distinguish them from preferences. As in, preferences are the UI things, settings are internal things.

• I used context receivers as IMO these make the code a bit cleaner, but I am not sure if everyone will agree on this. As someone said on Reddit, Kotlin is going to develop diabetes with all the syntactic sugar it is building up. 😅

• Most of the setting keys are already conveniently in R.string. already, so we can use those; hopefully, this will be the only place where the string resource is used in code. E.g.

  var centerCardContentsVertically by booleanSetting(R.string.center_vertically_preference, false)

  Alternatively, it's possible to de-extract strings and revert to hardcoding them. As long as the string are not scattered all over the code, I am not sure if keeping strings in resources is of any substantial benefit. Especially if you consider that the default values are also hardcoded, as are the values for choice preferences.

• Most of the choice pereferences can be represented with enums. In XML, choice values ("top", "bottom") are often in string-arrays. We can hardcode these values, e.g.

  enum class AnswerButtonsPosition(override val stringValue: String) : StringValue {
      Top("top"),
      Bottom("bottom"),
      None("none"),
  }

  Or we can extract strings and use those, for instance:

  enum class AnswerButtonsPosition(override val stringValueResource: Int) : StringValueResource {
      Top(R.string.preference__answer_buttons_position__top),
      Bottom(R.string.preference__answer_buttons_position__bottom),
      None(R.string.preference__answer_buttons_position__none),
  }

• Writing settings is straightforward:

  settings.centerCardContentsVertically = false

  However, this will create a new editor under the hood. It looks like it isn't possible to mandate the above statement to have an editor context, as in settings.edit { centerCardContentsVertically = false }, without also mandating it for retrieval. I wonder if this is an issue. SharedPreferences.Editor is useful especially if you want to have atomic changes; I'm not sure if there are places in the app where this matters.

• The settings are retrieved on the first read as well as on change. It is also possible to read all settings in advance, perhaps in a worker thread; also, it's possible to only invalidate settings on change instead of re-reading them.

  Also, as long as the the settings are retrieved in sync, I guess it's possible to swap the underlying setting storage layer for something else, while keeping the API (phrases like settings.answerButtonPosition), so that's neat.

• I suggest using the Settings class to also attach all the change listeners, e.g.

  var language by stringSetting(R.string.pref_language_key, "")
      .apply { 
          whenChanged { functionThatSetsLanguage(value) }
      }

• The API can also validate preferences, which can be used by UI preferences to prevent people from entering bad values, e.g. invalid URLs:

  try {
      settings.validate(key, newValue /* Any? is accepted */)
  } except (e: Exception) {
      // show a toast with error
  }

  This is currently done in the preferences code; I think this belongs here.

I did not look too far into this, so there's probably a few edge cases that I'm missing.

[BrayanDSO]

I planned to do something to solve the same problem, but wanted more testing infrastructure/key-safety first, although not part of my GSoC project (which is basically finished). I was going for using simple POJOs though, like wikipedia app does + adapt the current settings that already use enums (e.g. themes) + add enums for whatever needs an Enum but doesn't have one.

I'm not fixed on any approach, so I'm +1 for the job as long as this is well tested. Left some considerations about your points below

---

I am not sure if keeping strings in resources is of any substantial benefit.

The benefit is being able to use the same key used on the XMLs (Preferences, Deck Options and Filtered deck options) and avoid problems caused by typos

I suggest using the Settings class to also attach all the change listeners, e.g.

var language by stringSetting(R.string.pref_language_key, "")
    .apply { 
        whenChanged { functionThatSetsLanguage(value) }
    }

For the screens that use preferences (the UI things), I believe that more simple to use the default setOnPreferenceChangeListener. So this only would be useful for sharedPreferences that don't have a preference, which are few. By memory, I don't remember any case where this would simplify things, so it would be nice if you could show some examples.

The API can also validate preferences, which can be used by UI preferences to prevent people from entering bad values, e.g. invalid URLs:

try {
    settings.validate(key, newValue /* Any? is accepted */)
} except (e: Exception) {
    // show a toast with error
}

For the Preference preferences, probably only useful for EditTextPreference and inheritors, but I don't remember any case where it's actually useful to extract the validation. For the non-Preference preferences, just validate on the setter

[oakkitten]

I am myself a bit conflicted, on one hand it feels that this is a bit too much sugar, on the other the API for the enums especially are rather neat if I do say so myself.

The benefit is being able to use the same key used on the XMLs (Preferences, Deck Options and Filtered deck options) and avoid problems caused by typos

I understand, I mean, I am not sure if this will actually prevent any mistakes. Never had a problem with this. It seems that even if you do make a mistake, it will be found right away. Besides, if you change stuff, you still have to check that you use the correct default value, or if you have a choice preference, you have to check that the stored values in code are the same as the ones in XML.

(I wonder if it's somehow possible to say settings.someSetting::key in preferences, or whether this is a good idea. If not, or not, string resorces are indeed great 😛)

I believe that more simple to use the default setOnPreferenceChangeListener.

I simply do not think that this stuff semantically belongs to UI. A preference may change without any UI involvement, e.g. you can make text size change by pressing volume buttons, or perhaps you can decided to duplicate a setting by using a context menu somewhere.

For the Preference preferences, probably only useful for EditTextPreference and inheritors, but I don't remember any case where it's actually useful to extract the validation. For the non-Preference preferences, just validate on the setter

The point here is to remove all validation from UI code entirely; it doesn't semantically belongs there either: validation does most of what reading a preference and making an useful object out of it does. Preferences can simply call settings.validate for every setting, and handle the exception, if any.

[BrayanDSO]

I simply do not think that this stuff semantically belongs to UI. A preference may change without any UI involvement, e.g. you can make text size change by pressing volume buttons, or perhaps you can decided to duplicate a setting by using a context menu somewhere.

For these cases it may be useful. But for most of the simple preferences that are changed only on their screens, I think that it's easier to just use the default API provided by the library, which will still need to be used even with these new changes, especially if the user wants to change any property of the preference (isEnabled, isVisible, summary) or affecting other preferences (UI).

The point here is to remove all validation from UI code entirely; it doesn't semantically belongs there either: validation does most of what reading a preference and making an useful object out of it does. Preferences can simply call settings.validate for every setting, and handle the exception, if any.

Not a pattern I've seen on other applications, and I think it's the same point. May be useful for some cases, so it's better to have actual examples on AnkiDroid's codebase where this will help, and validating inside the preference code seems simpler IMO

[oakkitten]

I would say separating the UI and business logic is a commendable endeavor on its own. ¯\_(ツ)_/¯

[BrayanDSO]

Probably why the preferences are on XMLs and why Android has a configurable PreferenceFragmentCompat and a whole API to configuring individual preferences, which is very sufficient IMO.

If the validation is tied/extends the default androidx.Preference API instead of adding another layer of complexity, I think it may be good.

[oakkitten]

Updated to sliiigthly change a method.

To clarify some things as discussed on Discord,

• You don't need to somehow call through to whenChanged {} blocks from UI. Changing a SharedPreferences value will do that automatically.

• As you can get settings by key and validate() accepts Any?, you would only have to call this method from one place. Something like this might work:

  override fun initSubscreen() {
      setupValidationOfNewPreferenceValues()
  }
  
  private fun setupValidationOfNewPreferenceValues() {
      preferenceScreen.forEach { preference ->
          preference.setOnPreferenceChangeListener { _, newValue ->
              try {
                  settings.getSetting(preference.key).validate(newValue)
                  true
              } catch (e: Exception) {
                  Timber.w(e, "Could not set setting ${preference.key} to new value $newValue")
                  showSnackbar(e.toString())  // More useful message here
                  false
              }
          }
      }
  }

  (This is needed as while you can set a listener that is called after a preference change, SharedPreferences API does not include a listener that is called before a preference change to determine if the new value is needed. I suppose it doesn't because while such an API sounds very much possible, showing an error to user with it would be problematic.)

  One problem with this would be translating the error message. The way I did this, I have a method such as Exception.toUserFriendlyString() that translates exceptions in a somewhat generic way using Android strings or falls back to whatever Java produces for unexpected exceptions. This method is used throughout the app, and in case of settings you would say something along "Bad new setting value: ${e.toUserFriendlyString()}" in a snackbar and call it a day.

  While this is simple and versatile, for some combinations of specific preferences and errors you might want to have custom messages. Perhaps something like this would work:

  class Settings(...) : ... {
      var syncServer by httpUrlSetting(..., ...)
          .apply {
              addValidationErrorMessage<IllegalArgumentException>(R.string.custom_sync_server_base_url_invalid)
          }
  }

  While this is nice as it's still a divorce of the UI and backend, and simple enough in itself, I am not entirely sure if the extra complexity is worth it given that not too many preferences require validation. I would still do it as this looks simple enough once you read through it once, and, well. I notice that once you have good API, even if unnecessarily overengineered, you rarely need to refactor that code, while imperfect API might need quite a bit of touch.

[BrayanDSO]

You don't need to somehow call through to whenChanged {} blocks from UI. Changing a SharedPreferences value will do that automatically.

If the changes are on the UI, the block will need to be on the UI, like the example you give of changing language with whenChanged{}, which needs to call requireActivity().recreate()

As you can get settings by key and validate() accepts Any?, you would only have to call this method from one place. Something like this might work:

In the example you give, someone may need to set a change listener later to change other things besides validating it. I am away from my PC now, but IIRC overriding callChangeListener may do the job here.

---

I understand the motivation of trying to divorce the androidx Prefererences and the rest. As dealing with the XMLs, data validation and storage in just one place difficults one's life (testing, code duplication, etc). But probably only death may do them part.

Maybe extending the Preferences or creating a whole set of settings views instead of using Android's library may be ideally the best thing, as some apps do out there.

Do we have a problem that needs to go such lengths? I don't think so.

Especially because only custom sync server preferences may benefit with the divorce, as I've discussed on Discord (while we have more than 100 other preferences that may get more complex without needing it)

random thought: maybe create a HttpUrlPreference to avoid repetition on custom sync url and media sync url

---

I still like the getters and setters for accessing SharedPreferences on other places. With tests, of course.

If whenChanged has a global scope, probably will be difficult to test and lead to bugs, as it may be changed on different places of the code.

The validation may be inside the setter to simplify things (i.e. have one less method on the interface) or outside if we have repeated uses of it

[github-actions]

Hello 👋, this PR has been opened for more than 2 months with no activity on it. 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

[oakkitten]

Oh, I forgot to reply! Sorry

If the changes are on the UI, the block will need to be on the UI, like the example you give of changing language with whenChanged{}, which needs to call requireActivity().recreate()

It would be more proper to perform UI changes when a setting change, rather than when user taps on a thing in preferences. In case of app language, it would be more proper to call setApplicationLocales or something similar that would change app configuration.

In the example you give, someone may need to set a change listener later to change other things besides validating it. I am away from my PC now, but IIRC overriding callChangeListener may do the job here.

If you want to, you can always exclude some preferences from auto-opting into validation. I'm not if there's going to be many cases where you'd want that

Maybe extending the Preferences or creating a whole set of settings views instead of using Android's library may be ideally the best thing, as some apps do out there.

I'm not sure I see the need. Preference UI and the storage is sufficiently separated I think, and any settings interface you may come up with is going to be built on top of the two, not instead of.

If whenChanged has a global scope, probably will be difficult to test and lead to bugs, as it may be changed on different places of the code.

Actual settings (settings.answerButtonsPosition, not Setting) are integrating elements that only make sense in the context of the entire application, and should probably not be tested separately. They would get tested through integration/e2e tests of other things, and help there. For instance, you would say settings.answerButtonsPosition = AnswerButtonsPosition.Top and would then veirify that the answer button is in fact appear on top.

The stuff in Setting.kt may be easily unit-tested separately, including whenChanged and everything else. SettingRegistry accepts arbitrary Context and SharedPreferences, I see no issues with testing it.

[BrayanDSO]

Forgot about this too.

Overall, I'm fine with the proposed API, but not sure if we would actually use it enough to compensate the churn. For the getters and setters I can see the benefit immediately, but for the other methods I'd like some real examples on the current codebase that have a actual benefit on maintainability or testability so we aren't just spending time writing things on a different way

[oakkitten]

YAGNI is a fine principle, but I prefer to slightly overengineer in cases like these. Validation of raw values as the idea seems to fit the design, as well as change listeners of sorts; while these require some additional code, it's rather straightforward. Of course, this is no more than a proof of concept. It remains to be seen what should be added or removed for this to work with all of the existing preferences. There's quite a few of them eh

[github-actions]

Hello 👋, this PR has been opened for more than 2 months with no activity on it. 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