Skip to content

Commit

Permalink
changes to Theme and ThemeTest from review feedback, refs JabRef#7177
Browse files Browse the repository at this point in the history
  • Loading branch information
@docrjp
docrjp committed Jan 9, 2021
1 parent 081fe9c commit 6e27c0e
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 28 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/preview/PreviewViewer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public PreviewViewer(BibDatabaseContext database, DialogService dialogService, S
}

public void setTheme(Theme theme) {
theme.additionalStylesheet().ifPresent(location -> previewView.getEngine().setUserStyleSheetLocation(location));
theme.getAdditionalStylesheet().ifPresent(location -> previewView.getEngine().setUserStyleSheetLocation(location));
}

private void highlightSearchPattern() {
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/org/jabref/gui/util/Theme.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,16 @@ private Optional<URL> additionalCssToLoad() {
}

/**
* Creates a data-embedded URL from a file or resource URL
* Creates a data-embedded URL from a file (or resource) URL.
*
* TODO: this is only desirable for file URLs, as protection against the file being removed (see
* {@link #MAX_IN_MEMORY_CSS_LENGTH} for details). However, there is a bug in OpenJFX, in that it does not
* recognise jrt URLs (modular java runtime URLs). This is detailed in
* <a href="https://bugs.openjdk.java.net/browse/JDK-8240969">JDK-8240969</a>.
* When we upgrade to OpenJFX 16, we should limit loadCssToMemory to only URLs where the protocol is equal
* to file, using {@link URL#getProtocol()}. Also rename to loadFileCssToMemory() and reword the
* javadoc, for clarity.
*
* @param url the URL of the resource to convert into a data: url
*/
private void loadCssToMemory(URL url) {
Expand Down Expand Up @@ -285,7 +294,7 @@ public String getCssPathString() {
* location will be a local URL. Typically it will be a {@code 'data:'} URL where the CSS is embedded. However for
* large themes it can be {@code 'file:'}.
*/
public Optional<String> additionalStylesheet() {
public Optional<String> getAdditionalStylesheet() {
if (cssDataUrlString.get().isEmpty()) {
additionalCssToLoad().ifPresent(this::loadCssToMemory);
}
Expand Down
50 changes: 25 additions & 25 deletions src/test/java/org/jabref/gui/util/ThemeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,41 +43,41 @@ void setUp(@TempDir Path tempFolder) {
public void lightThemeUsedWhenPathIsBlank() {
Theme blankTheme = new Theme("", preferencesMock);
assertEquals(Theme.Type.LIGHT, blankTheme.getType());
blankTheme.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet to be available; with CSS location " + location));
assertEquals(Optional.empty(), blankTheme.getAdditionalStylesheet(),
"didn't expect additional stylesheet to be available");
}

@Test
public void lightThemeUsedWhenPathIsBaseCss() {
Theme baseTheme = new Theme("Base.css", preferencesMock);
assertEquals(Theme.Type.LIGHT, baseTheme.getType());
baseTheme.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet to be available; with CSS location " + location));
assertEquals(Optional.empty(), baseTheme.getAdditionalStylesheet(),
"didn't expect additional stylesheet to be available");
}

@Test
public void darkThemeUsedWhenPathIsDarkCss() {
// Dark theme is detected by name:
Theme theme = new Theme("Dark.css", preferencesMock);
assertEquals(Theme.Type.DARK, theme.getType());
assertTrue(theme.additionalStylesheet().isPresent(),
assertTrue(theme.getAdditionalStylesheet().isPresent(),
"expected dark theme stylesheet to be available");
}

@Test
public void customThemeIgnoredIfDirectory() {
Theme baseTheme = new Theme(tempFolder.toString(), preferencesMock);
assertEquals(Theme.Type.CUSTOM, baseTheme.getType());
baseTheme.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet when CSS location is a directory; was called with CSS location " + location));
assertEquals(Optional.empty(), baseTheme.getAdditionalStylesheet(),
"didn't expect additional stylesheet to be available when location is a directory");
}

@Test
public void customThemeIgnoredIfInvalidPath() {
Theme baseTheme = new Theme("\0\0\0", preferencesMock);
assertEquals(Theme.Type.CUSTOM, baseTheme.getType());
baseTheme.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet when CSS location is just some null terminators!"));
assertEquals(Optional.empty(), baseTheme.getAdditionalStylesheet(),
"didn't expect additional stylesheet when CSS location is just some null terminators!");
}

@Test
Expand All @@ -97,7 +97,7 @@ public void customThemeAvailableEvenWhenDeleted() throws IOException {
assertEquals(Theme.Type.CUSTOM, theme.getType());
assertEquals(testCss.toString(), theme.getCssPathString());

Optional<String> testCssLocation1 = theme.additionalStylesheet();
Optional<String> testCssLocation1 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation1.isPresent(), "expected custom theme location to be available");
assertEquals(
"data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==",
Expand All @@ -108,7 +108,7 @@ public void customThemeAvailableEvenWhenDeleted() throws IOException {
// Consumer passed to additionalStylesheet() should still return the data url even though file is deleted.
// It shouldn't matter whether the file existed at the time the Theme object was created (before or after)

Optional<String> testCssLocation2 = theme.additionalStylesheet();
Optional<String> testCssLocation2 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation2.isPresent(), "expected custom theme location to be available");
assertEquals(
"data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==",
Expand All @@ -117,8 +117,8 @@ public void customThemeAvailableEvenWhenDeleted() throws IOException {
Theme themeCreatedWhenAlreadyMissing = new Theme(testCss.toString(), preferencesMock);
assertEquals(Theme.Type.CUSTOM, theme.getType());
assertEquals(testCss.toString(), theme.getCssPathString());
themeCreatedWhenAlreadyMissing.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet to be available because it didn't exist when theme was created; with CSS location " + location));
assertEquals(Optional.empty(), themeCreatedWhenAlreadyMissing.getAdditionalStylesheet(),
"didn't expect additional stylesheet to be available because it didn't exist when theme was created");

// Check that the consumer is called once more, if the file is restored
Files.writeString(testCss,
Expand All @@ -127,13 +127,13 @@ public void customThemeAvailableEvenWhenDeleted() throws IOException {
" -fx-font-family: monospace;\n" +
"}", StandardOpenOption.CREATE);

Optional<String> testCssLocation3 = theme.additionalStylesheet();
Optional<String> testCssLocation3 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation3.isPresent(), "expected custom theme location to be available");
assertEquals(
"data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==",
testCssLocation3.get());

Optional<String> testCssLocation4 = themeCreatedWhenAlreadyMissing.additionalStylesheet();
Optional<String> testCssLocation4 = themeCreatedWhenAlreadyMissing.getAdditionalStylesheet();
assertTrue(testCssLocation4.isPresent(), "expected custom theme location to be available");
assertEquals(
"data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==",
Expand All @@ -159,32 +159,32 @@ public void largeCustomThemeNotHeldInMemory() throws IOException {
assertEquals(Theme.Type.CUSTOM, theme.getType());
assertEquals(testCss.toString(), theme.getCssPathString());

Optional<String> testCssLocation1 = theme.additionalStylesheet();
Optional<String> testCssLocation1 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation1.isPresent(), "expected custom theme location to be available");
assertTrue(testCssLocation1.get().startsWith("file:"), "expected large custom theme to be a file");

Files.move(testCss, testCss.resolveSibling("renamed.css"));

// additionalStylesheet() will no longer offer the deleted stylesheet, because it's not been held in memory

theme.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet after css was deleted; was called with CSS location " + location));
assertEquals(Optional.empty(), theme.getAdditionalStylesheet(),
"didn't expect additional stylesheet after css was deleted");

Theme themeCreatedWhenAlreadyMissing = new Theme(testCss.toString(), preferencesMock);
assertEquals(Theme.Type.CUSTOM, theme.getType());
assertEquals(testCss.toString(), theme.getCssPathString());
themeCreatedWhenAlreadyMissing.additionalStylesheet().ifPresent(location -> fail(
"didn't expect additional stylesheet to be available; with CSS location " + location));
assertEquals(Optional.empty(), themeCreatedWhenAlreadyMissing.getAdditionalStylesheet(),
"didn't expect additional stylesheet to be available because it didn't exist when theme was created");

// Check that it is available once more, if the file is restored

Files.move(testCss.resolveSibling("renamed.css"), testCss);

Optional<String> testCssLocation2 = theme.additionalStylesheet();
Optional<String> testCssLocation2 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation2.isPresent(), "expected custom theme location to be available");
assertTrue(testCssLocation2.get().startsWith("file:"), "expected large custom theme to be a file");

Optional<String> testCssLocation3 = themeCreatedWhenAlreadyMissing.additionalStylesheet();
Optional<String> testCssLocation3 = themeCreatedWhenAlreadyMissing.getAdditionalStylesheet();
assertTrue(testCssLocation3.isPresent(), "expected custom theme location to be available");
assertTrue(testCssLocation3.get().startsWith("file:"), "expected large custom theme to be a file");
}
Expand All @@ -195,7 +195,7 @@ public void largeCustomThemeNotHeldInMemory() throws IOException {
*/
@Test
@EnabledOnOs(OS.WINDOWS)
public void installLiveReloadsCssData() throws IOException, InterruptedException {
public void liveReloadCssDataUrl() throws IOException, InterruptedException {

/* Create a temporary custom theme that is just a small snippet of CSS. There is no CSS
validation (at the moment) but by making a valid CSS block we don't preclude adding validation later */
Expand All @@ -210,7 +210,7 @@ public void installLiveReloadsCssData() throws IOException, InterruptedException
assertEquals(Theme.Type.CUSTOM, theme.getType());
assertEquals(testCss.toString(), theme.getCssPathString());

Optional<String> testCssLocation1 = theme.additionalStylesheet();
Optional<String> testCssLocation1 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation1.isPresent(), "expected custom theme location to be available");
assertEquals(
"data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==",
Expand Down Expand Up @@ -248,7 +248,7 @@ public void installLiveReloadsCssData() throws IOException, InterruptedException
}
}

Optional<String> testCssLocation2 = theme.additionalStylesheet();
Optional<String> testCssLocation2 = theme.getAdditionalStylesheet();
assertTrue(testCssLocation2.isPresent(), "expected custom theme location to be available");
assertEquals(
"data:text/css;charset=utf-8;base64,LyogQW5kIG5vdyBmb3Igc29tZXRoaW5nIHNsaWdodGx5IGRpZmZlcmVudCAqLwouY29kZS1hcmVhIC50ZXh0IHsKICAgIC1meC1mb250LWZhbWlseTogc2VyaWY7Cn0=",
Expand Down
1 change: 1 addition & 0 deletions src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
View file
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
# This Mockito extension allows us to mock final methods, which are very common in OpenJFX
mock-maker-inline

0 comments on commit 6e27c0e

Please sign in to comment.