[PoC] Introduced read/write interface for preferences

[btut]

We want to avoid to pass the big JabRefPreferences Object around. This is mostly done by just passing the smaller PreferencesObjects, but this approach has two problems:

• The preferences cannot be written to disk
• Changes to the preferences are only visible locally or wherever that same object was passed to
• Changes by other classes are missed if they were not passed the sampe preferences-object (multiple calls to JabRefPreferences.get*Preferences).

We want to fix that.
The interfaces I propose here basically gives classes the possibility to load an up-to-date preference object whenever they need it. The ReadWriteInterface adds the possibility to store the preferences to disk.

I show a possible use case on hand of the ImportSettingsPreferences. I chose that class because it is small and not used very often, so the change was quick.

Advantages:

• Avoid passing around the big JabRefPreferencesObject
• Always have an up-to-date preferences object
• With the distinction between Read- and Read-Write interface, it is immediately clear to the caller if a class/method alters the preferences or not.

• [] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
• [] Tests created for changes (if applicable)
• [] Manually tested changed features in running JabRef (always required)
• [] Screenshots added in PR description (for UI changes)
• [] Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

[tobiasdiez]

Thanks for working on this! After the many refactorings of the preferences in the past, I think we are now finally in a position to migrate to a new system that addresses the shortcomings you mentioned.

What I don't understand in your proposal is how this is different to the existing PreferenceService that also provides load/store operations for each preference subobject.

A few more points that should be considered:

• The preference values need to be cached since using Java's bult-in preference getters are notoriously slow (this is one the raison d'etre for the preference subobjects).
• How is this handled in tests. You don't want to access the real preferences in the tests.
• How are updates propagated? For example, if you change the preferences that control the main table columns, the maintable should rerender itself without being explicitly told to do so. This is also the biggest flaw of the current system in my opinion.

When I last thought about this, my hope was that JavaFX observables can be used, especially to cover the last point. The idea was roughly the following:

• You have a PreferenceObservable<T> that corresponds to exactly one option, say PreferenceObservable<boolean> showRecommendations.
• The initial value of this observable is what is in the Java preferences.
• When the value of the observable is changed, then this new value is stored in the Java preferences.
• All write operations have to go through the preference observable, i.e. there is no direct access to the Java preferences anymore.
• One can register listeners that are notified when the value of the observable is changed.
• For tests, one uses ordinary observable values that don't persist in the Java preferences.

[btut]

What I don't understand in your proposal is how this is different to the existing PreferenceService that also provides load/store operations for each preference subobject.

• With this approach you don't need to pass the whole preferences-service around. That makes it easier to understand what classes read/write what preferences.
• As you load a fresh object each time you access a preference, it is always up-to-date.

The preference values need to be cached since using Java's bult-in preference getters are notoriously slow (this is one the raison d'etre for the preference subobjects).

I did not know that. This eliminates the second benefit I listed above.

How is this handled in tests. You don't want to access the real preferences in the tests.

You pass a lambda that returns a static preference object or mock with the preferences you need for the test.

How are updates propagated? For example, if you change the preferences that control the main table columns, the maintable should rerender itself without being explicitly told to do so. This is also the biggest flaw of the current system in my opinion.

Still in the old way: not at all...

When I last thought about this, my hope was that JavaFX observables can be used, especially to cover the last point.

I like that idea. We could still bundle sets of PreferenceObservable's into Preference-Classes to make the passing easier. The needed refactoring would also be minimal I think (except for places where you actually want to have a listener on a preference, but that would be an improvement rather than a refactoring).

[tobiasdiez]

Thanks for the explanation!

@calixtus what are your thoughts about this?

[calixtus]

Did not have much time to dive deep into this. Sorry, will do in the next days...

[calixtus]

So I finally got into the code. One thing I see is that the ReadInterface is just a simple supplier, the ReadWriteInterface almost a consumer, except it integrates the ReadInterface. My proposal would be to simplify the proposal to the following in a first step:

1. Remove the new interfaces and just use Suppliers<> and Consumers<>
2. Use the Suppliers in the higher high level meta preferences objects (like ImportFormatPreferences), they should provide always a freshly created high level preferences object.
3. Pass the Consumer storePreferences only when really needed (like for an opt-out dialog).
4. Keep the PreferencesService in the PreferencesDialog, because in almost every Tab you need multiple high level preferences objects.

In a second step, as soon as we have reached a consistent codebase in the preferences package we should be able to easily refactor the supplier-methods to maybe ReadOnlyObservables providing update listeners.

About the other questions:

• Caching of prefs is happening inside of JabRefPreferences and won't be seen on the outside. Cache will be updated each time preferences are stored.

• Testing of JabRefPreferences right now is really not an easy job, since JabRefPreferences is a singleton and it is creating it's own dependency on the java preferences. This is one reason why i want to split the logic for creating and caching the high level preferences objects in a high level class and the ugly low level interface to the java preferences in a low level class.

• One problem will occur: observables are part of JavaFX, not java itself, so we probably have to add an exception to the architecture test if we are using javafx stuff in the logic package...

I'll push my proposal on this branch, if you don't like it, you can always force push your branch again.

Some things to think about:

• Move the creation logic of some high level preference objects (like MainTablePreferences/ColumnPreferences) out of JabRefPreferences into a factory inside the preferences object?

edit: was late, when i made the comment... some clearifications made.

[calixtus]

I tried to keep changes commit by commit

[calixtus]

We really should move and rename ImportSettingsPreferences.
Only file in package org.jabref.logic.importer.importsettings
Both does really annoy me. REALLY. 😡 😉

[tobiasdiez]

I'm not sure if I like the additional overhead that comes with the load/store interface (either in the original form proposed by Benedikt or using Supplier/Consumer). It just feels too complicated.

What do you guys think about this proposal using observables:
tobiasdiez@a0fc4b1
Whenever someone changes the shouldOverrideDefaultFontSize property, it is automatically persisted (in production, for tests nothing happens). So no store method is needed anymore. Moreover, since the whole appearance preference object is cached, the performance should be really good. Also the changes to the code are minimal, since the public interface of the AppearancePreference class isn't changed. Finally, it is easy to be notified about changes. Only constrained about this design is that each key (e.g. OVERRIDE_DEFAULT_FONT_SIZE) is only used once in a preference object (which can of course be a child in another preference object).

[calixtus]

As I understand you proposal, this would mean that the PreferencesObjects are some kind of singletons. I still don't understand why for tests nothing should happen?
Although i like the general idea, i belive we have to change how we work with preferences that way all way through the code base (use setValue / getValue / set / get) instead of directly assigning values to the variables.
Wouldn't it be easier to make the whole high level preferences object observable instead of the small ones? The idea behind is: Let the PreferenceService decide, how he wants to put and read the preferences.

[calixtus]

I think the getters and setters are superfluous in the preferences object, instead one should only provide the property like we do in the viewmodels.

[tobiasdiez]

PreferencesObjects are some kind of singletons.

Kind of. They are still normal classes that can be initialized as often as you want, but in practice you use the PreferenceService to get them and then they are essentially singletons, yes.

I still don't understand why for tests nothing should happen?

In tests, you simply create a new instance of the preference object (vie the constructor). So, in this case, no listener is added and thus they are not persisted (which is the behavior that what you want in tests).

i belive we have to change how we work with preferences that way all way through the code base (use setValue / getValue / set / get) instead of directly assigning values to the variables.

The getters don't change and so far we don't have any setters. Thus the only thing that changes is how you store the new version, which happens mainly in the preference dialog. In this case, you can actually simplify the code quite a bit since you can simply bind the preferences to the ui components.

Wouldn't it be easier to make the whole high level preferences object observable instead of the small ones? The idea behind is: Let the PreferenceService decide, how he wants to put and read the preferences.

That would be possible, but is essentially equivalent to the above load/store procedure (accept that they are then called get/set). What advantages would this have?

I think the getters and setters are superfluous in the preferences object

In some way, yes. But it's actually javafx convention to add get + set + property methods. It's also handy for backwards compatibility since you don't need to change any getters.

[calixtus]

Took the liberty to add some flesh to your changes. Yes, i think this could be a way to go. Just want see how this works with those opt-in stuff.

02feee9

[btut]

One thing I see is that the ReadInterface is just a simple supplier, the ReadWriteInterface almost a consumer, except it integrates the ReadInterface.

I know. But in this case, I think it improves readability to have the interfaces. Passing a supplier and a consumer clutters the method signature IMO.

I really like the observables approach. I think it makes it very easy and clean to bind preferences to UI elements (in the preferences dialog) and to read them. I also like that there are still getters for the individual preferences in @tobiasdiez s code, so there is no need for a big refactoring. We just havte to use the observables where we actually want to react to changes / change the value.

One thought about reference keeping: Storing a reference to all Preference-Objects that were ever used means they will stay in memory until JabRef exits, even if they are not needed anymore. This could be seen as a feature - since we only have to read them once. On the other hand, I don't know how resource-demanding that would be.

[calixtus]

I don't think hat these few preferences objects (maybe 20-30) will take much memory. There are maybe one or two exceptions, that are more resource demanding (Theme?).

[btut]

I don't think hat these few preferences objects (maybe 20-30) will take much memory. There are maybe one or two exceptions, that are more resource demanding (Theme?).

Great!

[tobiasdiez]

@calixtus Great! I would suggest to convert the static variables like "OVERRIDE_DEFAULT_FONT_SIZE" to method-variables. In this way, you make sure that there is only one instance where the those variables are used and you prevent the potential bugs that would result otherwise. Also this has the advantage that the defaults object can be removed, with the defaults then being specified directly in in the getAppearencePreferences() method.

[calixtus]

What still makes me think is how we can uncouple the higher logic in JabRefPreferences from the low level java API preferences like in createColumns.
Also we probably have to rethink our all preferences dialog.
Still some questions unclear.

[tobiasdiez]

We can extract the getBoolean, getString etc methods in JabrefPreferences into a low-level preference service interface, and then mock this interface for tests.