Fixed exception about missing custom css file #7292

docrjp · 2021-01-04T18:19:22Z

Fixes #7177 "Java Exception when trying to open the preferences with missing custom theme"

After some testing I found that there two main paths that could result in the same failure: the CSS file being missing before application start, and changing the name of the CSS file while the application is running. To resolve the latter, I think the best approach is to check the custom CSS file exists.

This is already done for the Theme as used for most of the application, however the Preview Viewer has it's own CSS URL logic.

Rather than add file checking logic to the Preview Viewer, I have refactored and moved the theme logic of Preview Viewer into Theme, so that all CSS URL logic is encapsulated in that class. I have also put the logic that checks that the URL is valid and filters it to an Optional.empty() into one new method, additionalCssToLoad(), in place of the field of the same name. This ensure that the file existence check is performed in all cases, rather than just at construction of the Theme object.

I have kept the Optional field so there should be no overhead to this approach for the built in themes, and only the minor and necessary overhead of file existence checking for the custom theme.

For manual testing I checked:

previews and overall appearance for default theme are correct, after restart
previews and overall appearance for dark theme are correct, after restart
previews and overall appearance with a custom theme with a valid name are correct, after restart
live changes to the custom theme still work, in general and for preview
rename custom theme so that the configured theme is no longer correct, before application start. The preview is no longer themed but there is no error.
the preview theme is restored when the filename is restored to the configured value, without restarting the application
Restart. Rename custom theme so that the configured theme is no longer correct, while application is running. The preview remains themed (typically sized CSS themes are data URL embedded and so survive file removal).

These tests were run both with the Gradle run task, and on the built binary (rationale: gradle presents the dark theme as a file, while the binary has it built in on a jrt: URL, so the results could be different).

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)~~ (N/A)
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.

calixtus

Hi, thanks for your work already!
I like your changes very much and i think they not only fix a bug, but also contribute to code quality. Just one remark so far.

About testing file changes: I believe that this would take way more effort, and i have absolutely no experience with this sort of checks, but there are some tests for FileUtil.class that work on temp directories that are maybe helpful...

src/main/java/org/jabref/gui/util/Theme.java

Siedlerchr

Thanks for the work and also thanks for adding a test. Is this ready now?

docrjp · 2021-01-08T17:10:43Z

Thanks for the work and also thanks for adding a test. Is this ready now?

I've justed add some test coverage for theme live reloading, the change to it was minor but this should cover all the changes now. All being well from CI pipeline I will remove the draft status 👍

docrjp · 2021-01-08T17:28:43Z

I get an NPE with the file watching ThemeTest in the github CI pipeline that I don't get when running the test locally, and the only thing I can see on the line with NPE that could do that is closing the WatchService, which functionally looks correct, but perhaps is open to memory consistency issues. So, I've added volatile to the WatchService and I take a local copy to null check before closing. In commit 9d77318

docrjp · 2021-01-08T18:04:27Z

I've put a condition on that last test I've added, so it only runs on Windows. I'll try to figure out why it is behaving differently in the pipeline without holding up this PR, as it is very likely a test artifact -- it's quite complicated with needing to run up the file watching service and mock the JavaFX Scene.

I've given a final quick manual test after the last commit.

Fetcher Tests fail with 429 Too Many Requests, probably just because I'm hammering the CI and this is usual. I guess the failing of the AUTHORS is usual for a first contribution, so nothing to worry about.

Only thing that I am unsure of is why the Mac OSX create installer task fails consistently, if that's something I need to fix let me know.

From my perspective, happy for this to be given a final review and merge 👍

Siedlerchr · 2021-01-08T18:20:05Z

@docrjp The mac os will always fail because it requires some keys/secrets. However, GithubAction does not pass these to forks. Linux and file watching/locking is a complex issue, Linux file system works completely different from windows, e.g. there is no kind of lock for a file.
Fetcher test are another, non-related issue, only relevant if you worked on them.

tobiasdiez

Thanks a lot for your work. This is a very nice improvement.

A few nitpickings and questions from my side.

src/test/java/org/jabref/gui/util/ThemeTest.java

src/main/java/org/jabref/gui/util/Theme.java

src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

calixtus · 2021-01-10T12:10:19Z

src/main/java/org/jabref/gui/util/Theme.java

+    /**
+     * Provides the raw, configured custom CSS location. This should be a file system path, but the raw string is
+     * returned even if it is not valid in some way. For this reason, the main use case for this getter is to
+     * storing or display the user preference, rather than to read and use the CSS file.
+     *
+     * @return the raw configured CSS location
+     */
+    public String getCssPathString() {
+        return cssPathString;
+    }


Hi @docrjp , I took a closer look into your changes.

I really did not get why you were removing the Path object in your changes.
The Path object in the java.nio.file package is just providing this: a simple Path, which is file system independent by design, not linked to a concrete file, but rather just the Path. Abstraction is a major difference between the old-style java io and the nio package. You can test the validity of the Path with Files.exists().

If you are unsure, whether a syntactically correct path is delivered by the PreferencesService: It should deliver a Path optional. I think this was not implemented before, but this would be a good addition. (I'm still in progress of refactoring JabRefPreferences and its surroundings. If possible, PreferencesService should always deliver high abstraction objects instead of raw strings - see ColumnPreferences for example)

I believe using raw Strings here is unnecessary and somewhat inconvenient.

Digging even deeper, I realized it's still more complicated.

Currently JabRefPreferences is providing the Theme itself. I'm not yet sure, if it should...

I thought a little about this. Besides that our complete theme architecture desires some attention and love, I think that keeping three different references in the class will provoke sooner or later unexpected bugs. The best would probably just to work inside the class with URLs and abolish the Path object, only return it, if asked by the prefs.

I don't like it either! But it is making true on design decisions already in place both upstream and downstream of the getter.

Every use of the Path getter was immediately converted to a String i.e. throwing away the abstraction.

The constructor of Theme took a String already, passing in magic values of the path for the LIGHT and DARK themes.

In the case of the LIGHT theme, the magic value is an empty string, and therefore an empty Path, which can never be used to open a regular file at all, it just happens to be OK to have an empty Path on Windows/Linux/Mac file systems. Usually an empty path means "current working directory" -- that has no meaningful relationship to the LIGHT theme.

For the DARK theme, there is also some magic. 'Dark.css' is passed to the constructor, it then works backwards: by detecting that the URL is a resource, it guesses that it is the DARK theme, since this is the only theme at present that is stored in the jar. For a binary build, this will never exist, as even though it happens to be a valid Path, it is is never intended to be used as a file system Path. It implies something that isn't true.

Converting this string always to Path is therefore a misleading abstraction. It really fooled me, when I first came to it.

Related to this, is that I didn't want to second guess how the preference service might interact with the "path" string (as I say, it throws away the high level abstraction). Since there was no check for Path validity before, it might well be assuming that the getter always returns a value which when it converts back to a string, i.e. reflects the constructor value. Changing that to Optional will make that no longer be true. That could create new bugs, e.g. the path configuration being "lost", at the surprise of the user. It is a behavioural change to introduce Optional to this getter.

To really use the Path consistently as an abstraction, I'd really have to rip up the Theme class, maybe with a factory pattern and removing these magic constructor values, and also look at changing how the preferences uses Theme. That makes the change much bigger and may better be in a separate PR.

How about this plan:

I will create two new issues, for follow up PRs:

add more built in themes alongside Dark, e.g. a high contrast Dark theme for enhanced accessibility. In this we will eliminate the magic handling specific to Light and Dark theme, allowing for easier extensibility to built in themes

add validation of user input of custom theme file path. In this we will refactor how the Theme and preference interact in relation to paths.

Between these we should be able to get into a good state. Happy to give theme preferences some love!

I have created issues #7322 and #7323 for follow up.

tobiasdiez

LGTM. Thanks for the quick follow-up!

I agree, the current theme class needs to be reworked. As a starting point I would suggest to have a Theme interface (with install and getCss methods), one implementation in form of a enum for built-in themes (Dark, Light at the moment) and one implementation for custom themes based on an external file. The current constructor can then go in the preference class providing the correct theme instance based on the string stored in the preferences.

calixtus · 2021-01-10T15:36:03Z

I agree with @tobiasdiez , we should merge this one now, as this is an improvement already, bug has been fixed and especially extracting theme logic out of foreign classes is very good, even though the theme architecture is not perfect, but this goes beyond this PR. Codewise looks good also to me, tests have passed, so i'll merge this.

calixtus · 2021-01-10T15:41:37Z

Thank you for your work here and for accepting our commnts about it. I hope you liked working on JabRef. We would be happy if you decide to continue to contribute.

docrjp · 2021-01-10T16:10:22Z

Thank you for your work here and for accepting our commnts about it. I hope you liked working on JabRef. We would be happy if you decide to continue to contribute.

Absolutely!

Thank you all @Siedlerchr @calixtus @tobiasdiez for your valuable feedback and support, all of it was very much appreciated, it's a great community here!

I will definitely keep contributing. Jabref caught my eye on CodeTriage because I used over 10 years ago as a postgraduate. In private industry since and now wanting to give back, it couldn't be more perfect! 😄

…dtask * upstream/master: Fixed exception about missing custom css file (#7292) Update the templates for opening a new issue (#7321) Entitlements file Mac (#7317) Make CONTRIBUTING.md much shorter. Move long text to docs/contributing.md (#7293) Include Github-optimized screenshot into repository (#7318)

* upstream/master: (34 commits) Fixed exception about missing custom css file (#7292) Update the templates for opening a new issue (#7321) Entitlements file Mac (#7317) Make CONTRIBUTING.md much shorter. Move long text to docs/contributing.md (#7293) Include Github-optimized screenshot into repository (#7318) Remove obsolete registry patch file (#7316) Fix AUTHORS GitBook: [master] one page modified Remove broken Sonarqube integration (#7315) GitBook: [master] 5 pages and 32 assets modified docs: update license year (#7314) Add javafx version number + update javafx (#7312) Add missing authors Adjust zbmath fetcher (#7298) Add "acm-siggraph.csl" required by CitationStyle.java Added Keyboard shortcuts (clear/set read status) (#7302) Add special fields ADR (#7300) Overwrite local copies Squashed 'buildres/csl/csl-locales/' content from commit ecb8e70233 Squashed 'buildres/csl/csl-styles/' content from commit 737ffa1 ...

fixes JabRef#7177 exception when custom css missing

48c276d

calixtus previously approved these changes Jan 4, 2021

View reviewed changes

src/main/java/org/jabref/gui/util/Theme.java Outdated Show resolved Hide resolved

docrjp added 4 commits January 5, 2021 01:02

custom theme css logging, refs JabRef#7177

deb7a45

add test coverage for fix 7177

6e3abf4

tidy ThemeTest and remove the deliberate failing assertion

0910a12

unit test coverage for passing Base.css to Theme

64f34cb

docrjp marked this pull request as draft January 5, 2021 12:17

docrjp added 3 commits January 5, 2021 18:31

handle invalid paths in custom css, refs JabRef#7177

a8ed912

small theme styelsheets held in memory, refs JabRef#7177

573ce22

fix checkstyle violations in Theme

c7d260b

Siedlerchr reviewed Jan 8, 2021

View reviewed changes

src/main/java/org/jabref/gui/util/Theme.java Outdated Show resolved Hide resolved

Siedlerchr previously approved these changes Jan 8, 2021

View reviewed changes

docrjp added 2 commits January 8, 2021 16:33

improve javadoc, refs JabRef#7177

667dc5c

test coverage of Theme live reload for preview, refs JabRef#7177

5cb311a

docrjp added 2 commits January 8, 2021 17:14

fix import formating in ThemeTest

65e611c

fix possible memory consistency issue with file watching

9d77318

docrjp added 2 commits January 8, 2021 17:33

more friendly error if mocking issue with ThemeTest

fc93e40

only run liveReloadCssDataUrl test on Windows, for now

081fe9c

docrjp marked this pull request as ready for review January 8, 2021 17:58

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 8, 2021

tobiasdiez requested changes Jan 8, 2021

View reviewed changes

tobiasdiez added the status: changes required Pull requests that are not yet complete label Jan 8, 2021

docrjp added 2 commits January 9, 2021 16:35

changes to Theme and ThemeTest from review feedback, refs JabRef#7177

6e27c0e

refactor Theme to eliminate Path field, refs JabRef#7177

f54feac

docrjp dismissed stale reviews from Siedlerchr and calixtus via f54feac January 10, 2021 01:31

fix use of Paths architecture violation

5abef78

docrjp requested review from Siedlerchr and calixtus January 10, 2021 11:22

calixtus reviewed Jan 10, 2021

View reviewed changes

tobiasdiez approved these changes Jan 10, 2021

View reviewed changes

tobiasdiez removed the status: changes required Pull requests that are not yet complete label Jan 10, 2021

This was referenced Jan 10, 2021

Add some more built in themes, especially high contrast dark #7322

Closed

Validate user input of custom theme paths and provide visual feedback #7323

Open

calixtus approved these changes Jan 10, 2021

View reviewed changes

calixtus changed the title ~~fixes #7177 exception when custom css missing~~ Fixed exception about missing custom css file Jan 10, 2021

calixtus merged commit 8adb3a4 into JabRef:master Jan 10, 2021

docrjp mentioned this pull request Jan 12, 2021

Change theme preferences without requiring a restart #7335

Closed

docrjp mentioned this pull request Mar 11, 2021

Live reloading when switching themes #7336

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed exception about missing custom css file #7292

Fixed exception about missing custom css file #7292

docrjp commented Jan 4, 2021 •

edited

Loading

calixtus left a comment

Siedlerchr left a comment

docrjp commented Jan 8, 2021

docrjp commented Jan 8, 2021

docrjp commented Jan 8, 2021 •

edited

Loading

Siedlerchr commented Jan 8, 2021

tobiasdiez left a comment

calixtus Jan 10, 2021

calixtus Jan 10, 2021

calixtus Jan 10, 2021

docrjp Jan 10, 2021 •

edited

Loading

docrjp Jan 10, 2021 •

edited

Loading

docrjp Jan 10, 2021

tobiasdiez left a comment

calixtus commented Jan 10, 2021

calixtus commented Jan 10, 2021

docrjp commented Jan 10, 2021

Fixed exception about missing custom css file #7292

Fixed exception about missing custom css file #7292

Conversation

docrjp commented Jan 4, 2021 • edited Loading

calixtus left a comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

docrjp commented Jan 8, 2021

docrjp commented Jan 8, 2021

docrjp commented Jan 8, 2021 • edited Loading

Siedlerchr commented Jan 8, 2021

tobiasdiez left a comment

Choose a reason for hiding this comment

calixtus Jan 10, 2021

Choose a reason for hiding this comment

calixtus Jan 10, 2021

Choose a reason for hiding this comment

calixtus Jan 10, 2021

Choose a reason for hiding this comment

docrjp Jan 10, 2021 • edited Loading

Choose a reason for hiding this comment

docrjp Jan 10, 2021 • edited Loading

Choose a reason for hiding this comment

docrjp Jan 10, 2021

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

calixtus commented Jan 10, 2021

calixtus commented Jan 10, 2021

docrjp commented Jan 10, 2021

docrjp commented Jan 4, 2021 •

edited

Loading

docrjp commented Jan 8, 2021 •

edited

Loading

docrjp Jan 10, 2021 •

edited

Loading

docrjp Jan 10, 2021 •

edited

Loading