Live reloading when switching themes

[docrjp]

This resolves enhancement #7335 ( fixes #7335 )

Theme preferences can be changed without reloading. Supporting the enhancement is a refactoring of Theme which lays the groundwork for further enhancements #7322 and #7323.

This is a draft PR, for visibility and Continuous Integration purposes. Some code changes are outstanding:

• Live reload when changing between light/dark/custom
• Review live reloading for web engine (Preview Viewer)
• If all live reloading is fully effective, remove reload warning
• Rework ThemeTest now that the Theme interface has changed for web engine stylesheets
• Review JavaDoc and comments to explain solution
• Summarise solution here to assist review

Followed by standard PR tasks:

• Change in CHANGELOG.md described (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 a lot, this already looks extremely nice. The new Stylesheet classes are a good idea, they make things a lot clearer.

I'm not sure if it works, but it would be nice to profit even more from the introduction of these classes and further simplify the Theme / ThemeImpl / ThemePreference classes. The following would be my idea:

• Add Optional<Stylesheet> getAdditonialStylesheet in AppearencePreference. The preference class uses Stylesheet.create(...) to create one based on the string stored in the preferences.
• The Theme class uses this method to determine the additional stylesheets to install.
• You can now remove ThemeState, ThemeImpl and ThemePreferences.

What do you think?

[tobiasdiez]

Maybe make this an Optional<StyleSheet>? Than you would not need to introduce the "EmptyStylesheet" class.

[docrjp]

The idea here is to have an invariant: "there is always an additional stylesheet".

This is so that it is much simpler to transition between states where an additional stylesheet is available, and states where it is not (either because it is missing, or because the theme does not require one).

[tobiasdiez]

ok, but "there is always an additional stylesheet" doesn't seem to be valid from a business logic (the light theme doesn't have additional css) as well as from the current implementation (EmptyStylesheet is not a a stylesheet, it's something that pretends to be one).

On the other hand, the current solution works and is fine codewise so take my comment as a suggestion that doesn't necessarily need to be implemented.

[docrjp]

Ah, perhaps it wasn't clear! EmptyStylesheet loaded a real resource called Empty.css so it wasn't pretending!

However, given it is confusing, I've updated to remove StyleSheetEmpty and rename Empty.css to Light.css, which has a comment in it explaining that it doesn't have to add anything over Base.css

This is meaningful, because we could add something specific to Light.css. A practical example would be theming for Preview. That can't be done in Base.css, because WebEngine (which preview uses) can only take one style sheet and that is the additional one. So Light.css would be the place for that.

So hopefully that clears things up (and removes a class) while keeping the useful invariant.

[docrjp]

Thanks a lot, this already looks extremely nice. The new Stylesheet classes are a good idea, they make things a lot clearer.

I'm not sure if it works, but it would be nice to profit even more from the introduction of these classes and further simplify the Theme / ThemeImpl / ThemePreference classes. The following would be my idea:

• Add Optional<Stylesheet> getAdditonialStylesheet in AppearencePreference. The preference class uses Stylesheet.create(...) to create one based on the string stored in the preferences.
• The Theme class uses this method to determine the additional stylesheets to install.
• You can now remove ThemeState, ThemeImpl and ThemePreferences.

What do you think?

I have gone somewhat in this direction, just pushed a change that cleans up the AppearancePreferences and eliminates ThemeState, by having the Stylesheet created by the ThemePreference. Theme no longer has a circular relationship with the PreferenceService. Instead it just needs AppearancePreferences and this is the state object.

It might be a bit cleaner still if I can move AppreancePreferences into the theme package, at the moment all the preference objects sit together but we really want it to live with ThemePreference so it can have some package-private methods to help ThemeImpl.

Not sure I could go further with removing the added classes without compromising the Single Responsibility Principle which are the rationale behind them:

• Theme (ThemeImpl) is responsible for live updating changes to appearance. It is a singleton so that it can transition between old and new preferences
• Stylesheet and its implementations abstract the different places that a stylesheet can be stored, and the complexities of WebEngine and Scene having different URL handling
• AppearancePreferences hold all the chosen preferences at a point in time that can be transitioned by Theme
• ThemePreference is the specific appearance preference that determines the chosen additional stylesheet. It is responsible for determining the correct additional Stylesheet for a chosen theme. This is the class that will be refactored as part of Add some more built in themes, especially high contrast dark #7322

If we for example combined Stylesheet and ThemePreference into a single class, then Stylesheet would no longer be useable for the base stylesheet, it would be coupled to the additional stylesheet. The general stylesheet handling doesn't need to be impacted when we get to #7322, which really only concerns how ThemePreference interacts with the appearance tab and settings storage. Stylesheet shouldn't have to care about such things, especially as it is also used by the base CSS.

[tobiasdiez]

That looks really good. Thanks! Based on your description of the responsibilities of the classes, one might consider to rename Theme to ̀ThemeManager (and ThemePreferences to Theme). Also maybe it's possible to merge ThemePreference with AppearencePreference since the former is never used as a standalone anyway.

[calixtus]

It might be a bit cleaner still if I can move AppreancePreferences into the theme package, at the moment all the preference objects sit together but we really want it to live with ThemePreference so it can have some package-private methods to help ThemeImpl.

Great work so far, however, please keep in mind that the XxxPreferences objects should be plain stupid (immutable) data holder objects, that are created by a getter method by a PreferencesService. One of our goals is to clean up the whole codebase of JabRef, which has grown in almost 20 years, modularize it and implement design patterns. I know, this is still a very long road to walk...

[calixtus]

Hi @docrjp , do you have any updates on this PR? We are so impressed of your work you already contributed in such a short period of time in this and the other PR.
Tonight is devcall, and we normally walk through the PRs to check for their status.

[docrjp]

Hi @docrjp , do you have any updates on this PR? We are so impressed of your work you already contributed in such a short period of time in this and the other PR.
Tonight is devcall, and we normally walk through the PRs to check for their status.

Hi! So, I'm somewhat undecided about how to finish up the PR.

Eliminating ThemeState, and moving the Stylesheet static factory to the XxxPreferences, conflicts to some extent with the principle that the preferences are just data holder objects. Since now, the static factory Stylesheet.create() is called from the preferences. On the other hand, maybe that's taking the principle too literally. It's not too much different from java NIO Path. If we used Path in a preference class, then we're using static factory methods i.e. Path.of() and these static factories have a lot of file system logic behind them, with Path itself being an interface.

I think it depends on the goal behind the preference classes being data holders.

If it is so that we can deserialize by reflection with a library (e.g. Jackson), where it helps to not have complex factory or constructor logic, then I think it safest to move this logic back out of ThemePreference and reintroduce ThemeState as per the initial PR commit. Doing so eliminates the static factory from the ThemePreference and will keep that class really plain and simple.

On the other hand, if the purpose of keeping the preferences as data holder objects is to encourage loose coupling -- i.e. avoid the preferences needing application services injecting into them -- then the Stylesheet.create() static factory itself is harmless. It doesn't need to depend upon any other services, less so even than Path.of() which depends on file system providers.

However, what remains not so great is that Stylesheet is in the theme package but the factory is used from the preference package -- and the preference is in turn used by the ThemeManager in the .theme package. So that's a cyclic dependency of packages.

My instinct is to keep ThemePreference, and keep Stylesheet.create() as being called by ThemePreference, and then move ThemePreference into its own sub-package of the preferences package along with Stylesheet. But I don't want to do that if it messes up other plans with preferences.

[calixtus]

Thanks for your very quick response!
The big plans were to uncouple the current implementation of JabRefPreferences from the other code and to clean up the project structure, so we reach someday a clean distinction between model logic and gui (and in the far far future in a galaxy far far away maybe someday provide a webbased interface for jabref).

My idea was to collect the xxxPreferences classes in the model package unbound to any concrete implementation either of the preferences or the gui. On the other hand I think that the theme preferences definitely are part of the gui implementation.
I think we'll discuss this in the devcall tonight.

[tobiasdiez]

the purpose of keeping the preferences as data holder objects is to encourage loose coupling -- i.e. avoid the preferences needing application services injecting into them

That's their main goal as of now. Personally, I don't mind if a bit of logic is needed to construct a preference object.

However, what remains not so great is that Stylesheet is in the theme package but the factory is used from the preference package -- and the preference is in turn used by the ThemeManager in the .theme package. So that's a cyclic dependency of packages.

I don't think this is a big deal. For unit tests, you can mock the preferences and still create a ThemeManager. I think, this is the most important thing that one desires from the architecture.

[docrjp]

Did anything come out of the devcall that is worth taking into consideration?

If not, then based on discussion above, I think we can just stick with what we've got for now, and I finish up the outstanding work of fixing the unit tests. I'll also update the tracked master to make sure that the PR is clean.

With the related follow up issues #7322 and #7323. there will be some more natural opportunities to refactor and discuss :)

[calixtus]

Sorry, I was a bit buried in work today.

We had a brief discussion yesterday, but no quick response, so it's more or less up to me, since I'm currently refactoring the whole preferences complex.

I believe there was a misunderstanding on my side: I thought that ThemePreference is a preferences class just like MainTablePreferences, but it rather seems to be the representation of the actual theme. So please rename it to Theme, like @tobiasdiez mentioned earlier. I'd like to ask you to provide the ThemeManager in JabRefGui or in Globals, but not to construct it in JabRefPreferences, since there we only want to return values from the low level preferences repository there. That way we can avoid bloating the JabRefPrefences too much with logic and gui stuff that does not belong there. That's because my future target is to abstract the getXXXPrefrences and storeXXXPreferences methods in JabRefPreferences to be able to provide multiple implementations of a PreferencesService. So JabRefPreferences should only implement an adapter for the java prefs.

Please keep the naming convention (getAppearancePreferences instead of getCurrentAppearancePreferences). Also please use these methods only to store preferences or return from the java prefs or from a cache variable.

I know, JabRefPreferences is still a bit a messy place, but I'm working on it. It has a code history of almost 20 years now.

Thank you for your patience and all your impressive work!

As soon as you are ready, we are doing the regular double review and can then merge.

[tobiasdiez]

@docrjp could you please implement the last remaining changes so that we can get this merged? Thanks!

[docrjp]

Sorry! My spare time has been unusually lacking this last month but I hope to get to it soon! All the best Rob

…

On Wed, 10 Mar 2021, 09:59 Tobias Diez, ***@***.***> wrote: @docrjp <https://github.com/docrjp> could you please implement the last remaining changes so that we can get this merged? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7336 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIZMN3X3VZ2V33CKJORJPTTC47G3ANCNFSM4V7P6JVQ> .

[calixtus]

Hi, I took the liberty to dive into this PR a bit last week and made some commits regarding the remaining issues. If you don't mind I'll push them on your branch?

[Siedlerchr]

@calixtus What's the status here? You still had the OutOfBounds Exception?
Since someone in the forum asked https://discourse.jabref.org/t/new-feature-change-light-dark-theme-without-restarting/3041

[calixtus]

There still seems to be a concurrency bug producing the exception, and the ThemeManagerTest not completely working. Had to put work on this pr on hold because of more pressing matters, both private and jabref-related
(observable prefs project)

[ghost]

Is this feature ready now? I'm trying to make themes and would like to have this.
It works in 5.9 when selecting a new theme file but I just wanted somebody to confirm it's officially working always :)

[calixtus]

Well, live reloading worked once when it was implemented, and I believe there are some test ensuring it works. Be aware that currently the css is a bit of a mess, as there are some styles still hidden in the source, especially measurements are not yet scaling. But most of jabref should be customizeable.