From 6e27c0e9e1012c01f20bac7edf786c9c72db6efa Mon Sep 17 00:00:00 2001 From: Rob Platt <9541175+docrjp@users.noreply.github.com> Date: Sat, 9 Jan 2021 16:35:31 +0000 Subject: [PATCH] changes to Theme and ThemeTest from review feedback, refs #7177 --- .../org/jabref/gui/preview/PreviewViewer.java | 2 +- src/main/java/org/jabref/gui/util/Theme.java | 13 ++++- .../java/org/jabref/gui/util/ThemeTest.java | 50 +++++++++---------- .../org.mockito.plugins.MockMaker | 1 + 4 files changed, 38 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/jabref/gui/preview/PreviewViewer.java b/src/main/java/org/jabref/gui/preview/PreviewViewer.java index fd064557d67..b0949ee9a5d 100644 --- a/src/main/java/org/jabref/gui/preview/PreviewViewer.java +++ b/src/main/java/org/jabref/gui/preview/PreviewViewer.java @@ -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() { diff --git a/src/main/java/org/jabref/gui/util/Theme.java b/src/main/java/org/jabref/gui/util/Theme.java index 03094927d79..16f7d482e2e 100644 --- a/src/main/java/org/jabref/gui/util/Theme.java +++ b/src/main/java/org/jabref/gui/util/Theme.java @@ -182,7 +182,16 @@ private Optional 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 + * JDK-8240969. + * 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) { @@ -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 additionalStylesheet() { + public Optional getAdditionalStylesheet() { if (cssDataUrlString.get().isEmpty()) { additionalCssToLoad().ifPresent(this::loadCssToMemory); } diff --git a/src/test/java/org/jabref/gui/util/ThemeTest.java b/src/test/java/org/jabref/gui/util/ThemeTest.java index ec848a4f3a2..08471166620 100644 --- a/src/test/java/org/jabref/gui/util/ThemeTest.java +++ b/src/test/java/org/jabref/gui/util/ThemeTest.java @@ -43,16 +43,16 @@ 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 @@ -60,7 +60,7 @@ 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"); } @@ -68,16 +68,16 @@ public void darkThemeUsedWhenPathIsDarkCss() { 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 @@ -97,7 +97,7 @@ public void customThemeAvailableEvenWhenDeleted() throws IOException { assertEquals(Theme.Type.CUSTOM, theme.getType()); assertEquals(testCss.toString(), theme.getCssPathString()); - Optional testCssLocation1 = theme.additionalStylesheet(); + Optional testCssLocation1 = theme.getAdditionalStylesheet(); assertTrue(testCssLocation1.isPresent(), "expected custom theme location to be available"); assertEquals( "data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==", @@ -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 testCssLocation2 = theme.additionalStylesheet(); + Optional testCssLocation2 = theme.getAdditionalStylesheet(); assertTrue(testCssLocation2.isPresent(), "expected custom theme location to be available"); assertEquals( "data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==", @@ -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, @@ -127,13 +127,13 @@ public void customThemeAvailableEvenWhenDeleted() throws IOException { " -fx-font-family: monospace;\n" + "}", StandardOpenOption.CREATE); - Optional testCssLocation3 = theme.additionalStylesheet(); + Optional 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 testCssLocation4 = themeCreatedWhenAlreadyMissing.additionalStylesheet(); + Optional testCssLocation4 = themeCreatedWhenAlreadyMissing.getAdditionalStylesheet(); assertTrue(testCssLocation4.isPresent(), "expected custom theme location to be available"); assertEquals( "data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==", @@ -159,7 +159,7 @@ public void largeCustomThemeNotHeldInMemory() throws IOException { assertEquals(Theme.Type.CUSTOM, theme.getType()); assertEquals(testCss.toString(), theme.getCssPathString()); - Optional testCssLocation1 = theme.additionalStylesheet(); + Optional 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"); @@ -167,24 +167,24 @@ public void largeCustomThemeNotHeldInMemory() throws IOException { // 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 testCssLocation2 = theme.additionalStylesheet(); + Optional 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 testCssLocation3 = themeCreatedWhenAlreadyMissing.additionalStylesheet(); + Optional 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"); } @@ -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 */ @@ -210,7 +210,7 @@ public void installLiveReloadsCssData() throws IOException, InterruptedException assertEquals(Theme.Type.CUSTOM, theme.getType()); assertEquals(testCss.toString(), theme.getCssPathString()); - Optional testCssLocation1 = theme.additionalStylesheet(); + Optional testCssLocation1 = theme.getAdditionalStylesheet(); assertTrue(testCssLocation1.isPresent(), "expected custom theme location to be available"); assertEquals( "data:text/css;charset=utf-8;base64,LyogQmlibGF0ZXggU291cmNlIENvZGUgKi8KLmNvZGUtYXJlYSAudGV4dCB7CiAgICAtZngtZm9udC1mYW1pbHk6IG1vbm9zcGFjZTsKfQ==", @@ -248,7 +248,7 @@ public void installLiveReloadsCssData() throws IOException, InterruptedException } } - Optional testCssLocation2 = theme.additionalStylesheet(); + Optional testCssLocation2 = theme.getAdditionalStylesheet(); assertTrue(testCssLocation2.isPresent(), "expected custom theme location to be available"); assertEquals( "data:text/css;charset=utf-8;base64,LyogQW5kIG5vdyBmb3Igc29tZXRoaW5nIHNsaWdodGx5IGRpZmZlcmVudCAqLwouY29kZS1hcmVhIC50ZXh0IHsKICAgIC1meC1mb250LWZhbWlseTogc2VyaWY7Cn0=", diff --git a/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker index 1f0955d450f..83088d4ec16 100644 --- a/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker +++ b/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -1 +1,2 @@ +# This Mockito extension allows us to mock final methods, which are very common in OpenJFX mock-maker-inline