Dark Theme Visibility Fixes #5646

ageofadz · 2019-11-20T00:26:05Z

There had been some visibility issues with the interface when in dark theme.

Changed the color of the vertical scroll bar in dark mode to allow it to be more visible against the other colors.
Changed the color of the accent behind the number selected which appears in the group tree when an item in a group is selected to allow for the number inside to be more readable.

fixes #5522

Then:

Now:

[x ] Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

ageofadz · 2019-11-20T00:28:08Z

Sorry if I did anything wrong, this is my first pull request on a public repo.

Also happy birthday if you're reading this dad!

koppor · 2019-11-20T06:05:24Z

scripts/generate-authors.sh

@@ -52,6 +52,7 @@ cd "$(dirname "$(readlink -f "$BASH_SOURCE")")/.."
 	Samin Muhammad Ridwanul Karim
 	Stefan Robert
 	Bernhard Tempel
+	Samuel Robertson


No need to add yourself. You'll be added correctly by generate-authors.sh based on the data in your commit, In this place in generate-authors.sh, the authors contributing in our subversion times on sourceforge are tracked. With git, we don't need that system any more.

koppor · 2019-11-20T06:09:37Z

@ageofadz Thank you for being brave and submitting a pull request. This is how open source should work 🎉

Regarding the red cross. It's because our test coverage checker sometimes fails due to time outs. You can ignore this error. (Refs #5618).

Siedlerchr · 2019-11-20T07:47:36Z

Thank you very much for your contribution! 🥇 Looks good to me! Would you mind fixing the preview (ciation styles) panel for OpenOffice/LibreOffice as well? This is the same one as in the entry editor, so it should be easy to adapt.

AEgit · 2019-11-20T09:57:52Z

Just for clarification: @Siedlerchr is referring to the issue mentioned here:
#5522 (comment)

tobiasdiez

Thanks for your first contribution!

The changes look good to me, but I have two suggestions that make the code a bit more maintainable in the future.

tobiasdiez · 2019-11-20T11:45:21Z

src/main/java/org/jabref/gui/Dark.css

@@ -64,3 +64,16 @@
    -fx-background-color: -fx-light-text-color, -fx-control-inner-background;
 }

+.scroll-bar:horizontal .track,


Similar code is already in the Base.css file:

jabref/src/main/java/org/jabref/gui/Base.css

Lines 704 to 717 in 5376dcf

.scroll-bar:horizontal .track,

.scroll-bar:vertical .track {

-fx-background-color: -jr-scrollbar-track;

-fx-opacity: 0.6;

-fx-background-radius: 0em;

}

.scroll-bar:horizontal .thumb,

.scroll-bar:vertical .thumb {

-fx-background-color: -jr-scrollbar-thumb;

-fx-background-insets: 0, 0, 0;

-fx-background-radius: 0em;

}

Thus, it should be sufficient if you overwrite the following color specifications:

jabref/src/main/java/org/jabref/gui/Base.css

Lines 89 to 91 in 5376dcf

/* Specs for the scrollbars */

-jr-scrollbar-thumb: derive(-fx-outer-border, -30%);

-jr-scrollbar-track: derive(-fx-control-inner-background, -10%);

here in the root part of Dark.css.

tobiasdiez · 2019-11-20T11:47:06Z

src/main/java/org/jabref/gui/groups/GroupTree.css

@@ -30,13 +30,12 @@
 }

 .numberColumn > .hits:any-selected {
-    -fx-background-color: derive(-jr-green, 70%);
+    -fx-background-color: derive(-jr-gray-3, 70%);


With these changes, the number column is now also displayed in gray in the light theme. This is probably also not that readable.

I would propose to add two colors (-jr-groups-any-hits and -jr-groups-all-hits), which are used here. Then the dark theme can overwrite them.

ageofadz · 2019-11-20T17:06:13Z

Thank you very much for your contribution! 🥇 Looks good to me! Would you mind fixing the preview (ciation styles) panel for OpenOffice/LibreOffice as well? This is the same one as in the entry editor, so it should be easy to adapt.

Thanks so much for the feedback everyone!
Started working on a few of the changes. As for the changes to the OpenOffice style selection, there's a method in /gui/preview/PreviewViewer.java called setTheme() that can apply a theme to previewArticle and previewBook in /gui/openoffice/StyleSelectDialogView.java, with a note about an open issue on openJDK. I was able to add previewArticle.setTheme("DARK_CSS"); to get the text in the OpenOffice style selection to fit the dark mode, but because it only takes strings, I haven't been able to get a string value for the current theme applied so that the style selection's appearance matches the theme selected.
Here's the method's code:

public void setTheme(String theme) {
if (theme.equals(ThemeLoader.DARK_CSS)) {
// We need to load the css file manually, due to a bug in the jdk
// TODO: Remove this workaround as soon as https://github.com/openjdk/jfx/pull/22 is merged
URL url = JabRefFrame.class.getResource(ThemeLoader.DARK_CSS);
String dataUrl = "data:text/css;charset=utf-8;base64," +
`Base64.getEncoder().encodeToString(StringUtil.getResourceFileAsString(url).getBytes());`
previewView.getEngine().setUserStyleSheetLocation(dataUrl);
}
}

tobiasdiez · 2019-11-20T20:23:09Z

What do you mean with the following?

but because it only takes strings, I haven't been able to get a string value for the current theme applied so that the style selection's appearance matches the theme selected.

The method allows you to load the Dark.css, in which you then can add/change the necessary style information.

ageofadz · 2019-11-21T15:04:41Z

What do you mean with the following?

but because it only takes strings, I haven't been able to get a string value for the current theme applied so that the style selection's appearance matches the theme selected.

The method allows you to load the Dark.css, in which you then can add/change the necessary style information.

Because it checks if "Dark.css" is passed in as an argument and switches to dark theme if it is, it needs a string that says which theme is active for it to change according to what theme the user selected.
At the time I couldn't find one, but I found the hashmap in JabrRefPreferences and passed that into it now with JabRefPreferences prefs = JabRefPreferences.getInstance(); and previewArticle.setTheme(prefs.get(JabRefPreferences.FX_THEME));, which appear to work!

Testing right now, will update my pull requests later today with that addition and your earlier suggestions.

Edited dark theme scrollbar fix to dark.css to overwrite color -jr-scrollbar-thumb and -jr-scrollbar-track in dark.css. Switched base theme background for group selection number highlight back to green. Added dark theme to OO/Libreoffice style selection pane. fixes #5646

Siedlerchr · 2019-11-21T15:49:07Z

JabRefPreferences prefs = JabRefPreferences.getInstance(); and previewArticle.setTheme(prefs.get(JabRefPreferences.FX_THEME));, which appear to work!

In the Style Select DialogView you have access to the preferences via the Preferences Service which is a wrapper (for dependency injection via the FXML loader) for the JabRefPreferences.
We try to pass dependencies always as constructor arguments (Inversion of Control principle) to avoid direct dependency calls.
So the idea would be to add a method e.g. getActiveTheme to the PreferencesService which returns the theme for the preferences. Then you can call setTheme on the PreviewViewer.

Edited dark theme scrollbar fix to dark.css to overwrite color -jr-scrollbar-thumb and -jr-scrollbar-track in dark.css. Switched base theme background for group selection number highlight back to green. Added dark theme to OO/Libreoffice style selection pane. fixes #5646

ageofadz · 2019-11-23T18:43:57Z

Hey, many thanks once again for everyone's feedback!
The newest commit has updates to dark.css based on @TobiasDiaz's suggestion to overwrite scroll bar and group hit colors, in addition to @Siedlerchr's suggestion to update the StyleSelectDialogView using the Preferences Services wrapper injection as opposed to a hard dependency to JabRefPreferences.

Attached is a screenshot displaying the changes to scrollbar and group selected indicator (visually unchanged since first commit) and the OO style selection dialog view.

[] Tests created for changes
[x] Manually tested changed features in running JabRef
[x] Screenshots added in PR description (for bigger UI changes)
[x] Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

tobiasdiez · 2019-11-23T20:05:44Z

This looks good! Thanks for the follow-up.

I think you latest commit reverted the changes in the previous commit ageofadz@b812b62. You can use git cherry-pick to integrate these changes as well.

ageofadz · 2019-11-23T21:38:58Z

This looks good! Thanks for the follow-up.

I think you latest commit reverted the changes in the previous commit ageofadz@b812b62. You can use git cherry-pick to integrate these changes as well.

To fix that, should I use git cherry-pick ^aa06737230927cd0c9cd32a9860d26e20c367797..b812b62b6b854795fc9c012aa458959ed581bcfe?
The commit from two hours ago should have just added getTheme() to preferencesServices and jabRefPreferences and updated the StyleSelectDialogView to remove directly calling jabRefPreferences to refer to the preferencesServices getTheme().

tobiasdiez · 2019-11-24T12:11:29Z

Mr. @koppor git wizard, can you please assist with the git issue.

koppor · 2019-11-24T13:52:07Z

If it is only the commit b812b62, git cherry-pick b812b62b6b854795fc9c012aa458959ed581bcfe is enough (no need for commit ranges). Possibly, conflicts have to be resolved. Just resolve it with the IDE and then commit the result. I use git gui and click on the envelope buttons at the upper left.

changelog.md (63) StyleSelectDialogView (83-84)

ageofadz · 2019-11-24T15:37:47Z

Newest commit cherry-picks b812b62 and resolved conflicts in CHANGELOG.md and StyleSelectDialogView.java.
Thank you git wizard @koppor!

koppor · 2019-11-24T16:03:22Z

I think a git push is missing? Maybe you cherry-picked on the wrong branch? You can always check the current overall branch status with gitk --all&. Then, it displays where which branch currently points to. (Maybe the articleh https://lostechies.com/joshuaflanagan/2010/09/03/use-gitk-to-understand-git/ helps a bit?)

ageofadz · 2019-11-24T16:29:03Z

The latest commit is, 4411dce. gitk --all&, shows it as HEAD with 508c2be as parent, git status says I'm up to date.

Unsure why it isn't appearing in the PR, but appears on my fork

tobiasdiez · 2019-11-24T16:31:35Z

You pushed it to your master branch but not to the base branch of this PR.

tobiasdiez · 2019-11-24T19:01:03Z

This looks good now. Thanks again and looking forward to your next PR 😄 !

ageofadz · 2019-11-24T20:45:21Z

Couldn’t wish for a better first PR. Learned a lot and had a blast, I’m stoked to do many more.
Thank you all very much for being so helpful and patient commit after commit 😂

3a6a0a7 Update masarykova-univerzita-pravnicka-fakulta.csl (#5655) 136653a Corrections for Conservation Biology style (#5661) af148f8 Update biophysics-and-physicobiology.csl (#5646) 8842ed1 Create production-and-operations-management.csl (#5654) 6b4965f update style file (#5656) e5f6066 Update to Ruby 3.0.2 (#5657) 61c530c Reindent/reorder (#5653) 118c217 Update gems (#5652) 46cd9ab harvard-university-of-bath.csl: correct et-al-min (#5651) 37ba705 Create focaal-journal-of-global-and-historical-anthropology.csl (#5649) e22b8a5 Create developmental-medicine-and-child-neurology.csl (#5644) f7bc32c Bump nokogiri from 1.11.4 to 1.12.5 (#5640) git-subtree-dir: buildres/csl/csl-styles git-subtree-split: 3a6a0a7

koppor approved these changes Nov 20, 2019

View reviewed changes

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 20, 2019

koppor requested a review from tobiasdiez November 20, 2019 07:09

tobiasdiez requested changes Nov 20, 2019

View reviewed changes

ageofadz added 2 commits November 21, 2019 15:32

Fixed formatting problem with changelog entry

99d6994

Added override to getTheme

508c2be

Resolved conflicts

4411dce

changelog.md (63) StyleSelectDialogView (83-84)

tobiasdiez added 2 commits November 24, 2019 19:33

Remove unused variable

036aa89

Fix checkstyle

b96bd0d

tobiasdiez merged commit 8780a56 into JabRef:master Nov 24, 2019

ageofadz deleted the fix-for-issue-5522 branch November 25, 2019 16:01

sambo57u mentioned this pull request Apr 20, 2020

Dark theme: Visibility problems with current colouring scheme (group colouring and scroll bars) #5522

Closed

koppor mentioned this pull request Apr 26, 2020

Fix UX Anti Patterns (Improve Duplicate detection and merge dialog) #6334

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dark Theme Visibility Fixes #5646

Dark Theme Visibility Fixes #5646

ageofadz commented Nov 20, 2019

ageofadz commented Nov 20, 2019

koppor Nov 20, 2019

koppor commented Nov 20, 2019

Siedlerchr commented Nov 20, 2019

AEgit commented Nov 20, 2019

tobiasdiez left a comment

tobiasdiez Nov 20, 2019

tobiasdiez Nov 20, 2019

ageofadz commented Nov 20, 2019

tobiasdiez commented Nov 20, 2019

ageofadz commented Nov 21, 2019

Siedlerchr commented Nov 21, 2019

ageofadz commented Nov 23, 2019

tobiasdiez commented Nov 23, 2019

ageofadz commented Nov 23, 2019

tobiasdiez commented Nov 24, 2019

koppor commented Nov 24, 2019

ageofadz commented Nov 24, 2019

koppor commented Nov 24, 2019

ageofadz commented Nov 24, 2019

tobiasdiez commented Nov 24, 2019

tobiasdiez commented Nov 24, 2019

ageofadz commented Nov 24, 2019

	.scroll-bar:horizontal .track,
	.scroll-bar:vertical .track {
	-fx-background-color: -jr-scrollbar-track;
	-fx-opacity: 0.6;
	-fx-background-radius: 0em;
	}


	.scroll-bar:horizontal .thumb,
	.scroll-bar:vertical .thumb {
	-fx-background-color: -jr-scrollbar-thumb;
	-fx-background-insets: 0, 0, 0;
	-fx-background-radius: 0em;
	}

	/* Specs for the scrollbars */
	-jr-scrollbar-thumb: derive(-fx-outer-border, -30%);
	-jr-scrollbar-track: derive(-fx-control-inner-background, -10%);

Dark Theme Visibility Fixes #5646

Dark Theme Visibility Fixes #5646

Conversation

ageofadz commented Nov 20, 2019

Now:

ageofadz commented Nov 20, 2019

koppor Nov 20, 2019

Choose a reason for hiding this comment

koppor commented Nov 20, 2019

Siedlerchr commented Nov 20, 2019

AEgit commented Nov 20, 2019

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Nov 20, 2019

Choose a reason for hiding this comment

tobiasdiez Nov 20, 2019

Choose a reason for hiding this comment

ageofadz commented Nov 20, 2019

tobiasdiez commented Nov 20, 2019

ageofadz commented Nov 21, 2019

Siedlerchr commented Nov 21, 2019

ageofadz commented Nov 23, 2019

tobiasdiez commented Nov 23, 2019

ageofadz commented Nov 23, 2019

tobiasdiez commented Nov 24, 2019

koppor commented Nov 24, 2019

ageofadz commented Nov 24, 2019

koppor commented Nov 24, 2019

ageofadz commented Nov 24, 2019

tobiasdiez commented Nov 24, 2019

tobiasdiez commented Nov 24, 2019

ageofadz commented Nov 24, 2019