Skip to content

Structure of preferences #1058

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

Closed
oscargus opened this issue Mar 29, 2016 · 5 comments
Closed

Structure of preferences #1058

oscargus opened this issue Mar 29, 2016 · 5 comments
Labels
dev: code-quality Issues related to code or architecture decisions [outdated] type: question

Comments

@oscargus
Copy link
Contributor

Based on the Wiki and #658 I would like to discuss what the preferences should look like. As an example consider the OpenOfficePreferences in #1047. When designing that I faced a number of problems which, well, were not solved as in #658. Let me outline the issues, how #658 and #1047 differs and how it might be solved.

First, the purpose, I assume, is to provide an intermediate "layer" which handles preferences for a specific subset of the code. In #1047 it was all the preferences related to the OO connection. This makes sense since one can have nicely named methods and not have to access JabRefPreferences from a lot of different classes.

"logic and gui should not know JabRefPreferences":
Where should the Preferences class be stored? Should there be another main branch, preferences, where OpenOfficePreferences are in preferences.openoffice? In this way one can enforce this through tests.

What about splitting the current JabRefPreferences in three classes?

  1. Constants
  2. Default values
  3. The actual preference handling
    This would simplify the maintenance a bit as it provides a better overview. JabRefPreferences is quite huge at the moment...

Where are the default preferences set and how are an object constructed?
In #658 the default values are added to JabRefPreferences and the class has a special method that takes a JabRefPreferences, and returns an instance with a special call in JabRefMain(?) as the constructor taken the preference values. In #1047 the constructor has JabRefPreferences as an argument and inserts default values on construction. The object in constructed in OpenOfficePanel, i.e., where it is in needed. Advantage in #658, easier to construct an object with a different set of preference values. (I'm actually a bit confused here as it wasn't obvious where the ProxyPreference object is actually used and I am too tired at the moment to check). Drawback with #1047: default preferences are only put in the preferences if OpenOfficePanel is instantiated. Advantage with #1047: the default values are "near" the preference class.

I realize that this is a bit vague and I was hoping for a more stringent proposal, still I hope this can raise some discussion.

Finally, would it make sense to add some method that removes selected preferences in the preference list? For example, all (except for maybe those in #1047 unless OpenOfficePanel is used) preferences with a null default value are probably obsolete and currently there are quite a few in at least my preference list.

@oscargus oscargus added [outdated] type: question dev: code-quality Issues related to code or architecture decisions labels Mar 29, 2016
@Siedlerchr
Copy link
Member

From my point of view I like the idea of splitting up the preferences a bit more. Instead of having one huge class. I would rather suggest to split up the Preferences by their Context, like it is done with all classes in packages, e.g. groups.

I think the quote

"logic and gui should not know JabRefPreferences":

is about the direct access of the "raw Preferences". From what I think it it as meant as having an encapsulated layer over them which is for reading/updating preferences.
This layer, however, could then be accessed from the relevant classes that need em.

@oscargus
Copy link
Contributor Author

I agree on your interpretation. The question is just where that encapsulating layer should be placed? Most of the time, gui or logic is the natural place for it and I guess we do not really want to place it somewhere else just because it cannot be placed in the natural place.

#859 is also relevant here.

@simonharrer
Copy link
Contributor

At the moment we have the large plus that all preference keys are in a single place, which makes it very easy to add or remove or simply look up preferences. We should try to keep this, I think, because it has proven very useful many times during development and debugging.

@tobiasdiez
Copy link
Member

  • Place for default values and preference keys: As Simon explains, putting them all in the JabrefPreference class has its advantages. For the moment I would leave it that way and maybe reevaluate at a later point when more special preference classes are introduced.
  • Smaller preference classes encapsulating only a subset of similar preferences:
    • Advantage: preferences are encapsulated by context
    • There a mainly two ways to implement it. One way is to construct a proxy class which directly passes gets/sets through to JabRefPreferences, this is done in Refactoring of OO connection #1047. The other way is to use loadFromPreferences and saveToPreference methods as in Refactor proxy registration and preferences #658.
    • Advantages of proxy method: easy to implement, you can't forget to call saveToPreference so that the preference class is always in sync with JabRefPreferences.
    • Advantages of having dedicated load/save methods: similar functionality can share the same preference class (for example save and export both use SavePreferences but just have different loadFromPreference methods), some performance advantages since JabRefPreferences.getValue calls are rather expensive (so the mid-layer also serves as a cache). The main advantage is that testing is rather easy since you can completely ignore JabRefPreferences in your tests.
    • In my opinion the advantages for dedicated load/save methods outperform the advantages of the proxy method.

@lenhard
Copy link
Member

lenhard commented Jul 29, 2016

Refs: #1579

I think we have a consensus here, so I am closing this issue. Feel free to reopen for further discussion if needed.

@lenhard lenhard closed this as completed Jul 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev: code-quality Issues related to code or architecture decisions [outdated] type: question
Projects
None yet
Development

No branches or pull requests

5 participants