Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4130 provide dark theme #4372

Merged
merged 17 commits into from
Oct 17, 2018
Merged

4130 provide dark theme #4372

merged 17 commits into from
Oct 17, 2018

Conversation

Ali96kz
Copy link
Contributor

@Ali96kz Ali96kz commented Oct 15, 2018

Add ability to change theme via settings

@Ali96kz Ali96kz closed this Oct 15, 2018
@Ali96kz Ali96kz reopened this Oct 15, 2018
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, despite some minor things, the code looks good


String cssFileName = JabRefPreferences.getInstance().get(JabRefPreferences.FX_THEME);
if (cssFileName != null) {
String themeName = JabRefFrame.class.getResource(cssFileName).getPath();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the nio methods for accessing the resource. Have a look at the exporter tests to see how

@Ali96kz
Copy link
Contributor Author

Ali96kz commented Oct 16, 2018

Ready for review

CHANGELOG.md Outdated
@@ -34,7 +34,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We add auto url formatting when user paste link to URL field in entry editor. [#254](https://github.com/koppor/jabref/issues/254)
- We added a minimal height for the entry editor so that it can no longer be hidden by accident. [#4279](https://github.com/JabRef/jabref/issues/4279)
- We added a new keyboard shortcut so that the entry editor could be closed by <kbd>Ctrl<kbd> + <kbd>E<kbd>. [#4222] (https://github.com/JabRef/jabref/issues/4222)

- Change theme to dark or light via settings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please formulate the changelog as the above ones and link the issue


String cssFileName = jabRefPreferences.get(JabRefPreferences.FX_THEME);
if (cssFileName != null) {
CSS_SYSTEM_PROPERTY = new File("src/main/java/org/jabref/gui/" + cssFileName).getAbsolutePath();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we misunderstood us ;) Paths.get(JabRefFrame.class.getResource(cssFileName).toURI())
This is what I wanted to point to

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 16, 2018
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR. The code looks good to me. I've only one remark about the priority of preferences versus system property. Apart from that +1 for merge.

Currently, the build fails. Please have a look at the report and fix the checkstyle issues mentioned: https://travis-ci.org/JabRef/jabref/jobs/442113462

this.fileUpdateMonitor = Objects.requireNonNull(fileUpdateMonitor);

String cssFileName = jabRefPreferences.get(JabRefPreferences.FX_THEME);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First and foremost CSS_SYSTEM_PROPERTY should be System.getProperty("jabref.theme.css"), if the latter is not empty /null (StringUtil.isBlank) so that one can override the theme from the preference using command line arguments (mainly for developers). The preference should be consulted only if the system property is empty.
Please also rename CSS_SYSTEM_PROPERTY to userCss or something like this.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up! I'll merge this now.

@tobiasdiez tobiasdiez merged commit 453e7b5 into JabRef:master Oct 17, 2018
@Ali96kz Ali96kz deleted the 4130-provide-dark-theme branch April 3, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants