Update providing of SharedPreferences

[adamszewe]

Description

Refactor a bit the way the default preferences are provided by making it more explicit they are the default ones. There are only the default ones at the moment but it may change in the future.

In the next step I'd also want to make the SyncthingModule the single source of these SharedPreferences. At the moment there are 19 places where PreferenceManager.getDefaultSharedPreferences(...) is invoked. There should be only one source of it, the SyncthingModule, and there should be only one way of getting these preferences: injecting them where needed.

Changes

• move the dagger component and module to a dedicated package
• add an annotation, DefaultSharedPreferences, to distinguish which preferences are provided/injected
• annotate the provider function and injection sites with DefaultSharedPreferences

Why differentiate between different SharedPrefernces? For additional clarity. It may not be needed now, as everything is stored inside a single instance of SharedPrefences, the default one, but we may want to change that in the future. Ideally each feature screen that needs to store small bits of data should be able to do so in its own dedicated SharedPreferences instance (also dedicated xml file on the device) that is independent from other features. This also helps to decouple features as it's always painful to decouple screens that are tightly coupled because of same storage. Example:

The same key is used in five different places. Trying to modify anything related to that key in one screen may break unexpectedly functionalities in other screens. This is fine for now, but we should strive to avoid this kind of accidental complexity in the future in my opinion.

[adamszewe]

@imsodin could you check this PR again?

[imsodin]

The same key is used in five different places. Trying to modify anything related to that key in one screen may break unexpectedly functionalities in other screens.

I don't understand the example: That's the same setting used in five different places - starting on boot is not something that can differ in separate activities. You inherently can't modify that between screens.

I assume what you really look for is something like having separate preference categories for maybe folders, devices, app runtime, ...?

Generally I very much like iterative, small PRs - thanks a lot for that. However please do show me (drafts) of future improvements building on changes/PRs like this. Unfortunately I need to weight the potential impact/improvement a change brings with the effort to review it, as I have very limited time for maintaining this app. I wont review/accept refactors that don't bring an improvement on their own. I assume you do have some tangible improvement planned/in the works already?

[adamszewe]

You inherently can't modify that between screens.

It can be done unfortunately. SharedPreferences offer no type safety, thus another screen could save an integer instead of a boolean with the same key name or a wrong key could be used to save the data that should end up under another name.

This may never happen as the app is still small but there are multiple keys used in multiple parts of the application, thus the possibility of a mistake only increases.

Currently the logic for reading/writing the value is duplicated in each location where a particular key is used. This is bad for multiple reasons:

1. possibility of using the wrong key or type, as mentioned above
2. tight coupling with the particular storage solution: SharedPreferences - the fragments/activities/etc that read or write the value should be oblivious to how this information is stored as in a foreseeable future one may want to switch to another solution instead (like DataStore but that's a much bigger effort and discussion)
3. hard to test - having to write unit tests for any component that relies directly on SharedPreferences is a painful experience - using UI tests instead is recommended but those are slower and more expensive (require a device to run on, and more costly to maintain)

For the PREF_START_SERVICE_ON_BOOT the situation looks as follows:

Current	Possible Future
Each usage location has its own way of reading the value. The key name or type could be wrong. A different default value could be used instead. Hard to test and maintain.	Use the Repository pattern to hide the details of how the data is stored and offer a simpler interface for each class that depends on a particular value. Only one implementation to test.

This PR makes sure that that there is only one way to provide the default SharedPreferences instance that is injected in other classes.

The next step is going to be that of making sure there is only one place that provides the default SharedPreferences in the entire app: the dagger SyncthingModule. This should remove any other use of PreferenceManager.getDefaultSharedPreferences(...) and have only the dagger module know how this instance is created.

A future step would introduce the Repository pattern and replace the direct usage of SharedPreferences in favor of calling a method to get/set a value while hiding the intricacies of how that value is read/stored/cached.

Note:

The Possible Future is not a definitive solution. The view layer of the app should not access the data layer directly and we should introduce a presentation pattern like MVVM and move the logic from the Activity/Fragment to the ViewModel instead. The diagram on the right is a step in the right direction as it's easier to refactor from that situation than from the current one.

[imsodin]

Cool, that end result I understand and agree with.

This PR makes sure that that there is only one way to provide the default SharedPreferences instance that is injected in other classes.

I guess that refers to moving the dagger providers into a separate module? The getPreferences method is still public though, so it doesn't actually prevent any access.

The next step is going to be that of making sure there is only one place that provides the default SharedPreferences in the entire app: the dagger SyncthingModule. This should remove any other use of PreferenceManager.getDefaultSharedPreferences(...) and have only the dagger module know how this instance is created.

That also makes sense, I just don't get why the default preferences introduced in this PR are relevant for that. Nor for the matter moving dagger providers to a separate module. You could just replace those usages with an injected preferences member and you'd achieve the same.

And a step further I don't get how that helps towards reaching the repo pattern: It doesn't make that step smaller, both from the current state or from injected preferences, you'll replace the same preferences with the repo pattern.

[adamszewe]

I guess that refers to moving the dagger providers into a separate module? The getPreferences method is still public though, so it doesn't actually prevent any access.

This is because the methods that provide dependencies in Dagger modules cannot be private. The goal here is to have only one way of providing this dependency and get rid of getDefaultSharedPreferences(...) calls in other places of the app.

And a step further I don't get how that helps towards reaching the repo pattern: It doesn't make that step smaller, both from the current state or from injected preferences, you'll replace the same preferences with the repo pattern.

I think introducing the repo pattern right away would be too big of a jump at the moment.
I intend to untangle how the dependencies are provided in the app first to make the components easier to work with and to test. I'd also avoid adding the repository pattern or presentation patterns until we have some unit tests in place to make sure the refactors do not break core functionalities.

[imsodin]

The goal here is to have only one way of providing this dependency and get rid of getDefaultSharedPreferences(...) calls in other places of the app.

I really do get (and agree with) that goal. What I don't see is how adding @DefaultSharedPreferences is related to that. You could exchange those get calls with injection now, without adding this decorator. You explained why you want the decorator, but that's an orthogonal thing: You want to have separate preferences for different activities (eventually, maybe). However I don't want to review and add code/decorators that aren't used - generally PRs with no value on their own. I am happy to accept this default decorating, if you actually do separate settings - if you want to do it in two commits, send both commits for review at once please. I am also happy to accept replacing getters with injections.

[adamszewe]

I really do get (and agree with) that goal. What I don't see is how adding @DefaultSharedPreferences is related to that. You could exchange those get calls with injection now, without adding this decorator. You explained why you want the decorator, but that's an orthogonal thing: You want to have separate preferences for different activities (eventually, maybe). However I don't want to review and add code/decorators that aren't used - generally PRs with no value on their own. I am happy to accept this default decorating, if you actually do separate settings - if you want to do it in two commits, send both commits for review at once please. I am also happy to accept replacing getters with injections.

Agreed 👍 Let me close this PR and do the changes in a new smaller ones as this one has been opened for too long.