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

Add CSS Customisation #6725

Merged
merged 37 commits into from
Aug 31, 2020
Merged

Add CSS Customisation #6725

merged 37 commits into from
Aug 31, 2020

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Jul 30, 2020

This PR continues the abandoned PR #6036
As discussed in the DevCall, we are now aggressivly finishing those PRs we would like to include in JabRef.
Works so far, yet I still want to make some changes with the optics...
Introduces also AppearancePreferences

(Screenshots are out of date, see below)

fixes #5790

  • 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)
  • 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.

nilsstre and others added 26 commits February 26, 2020 11:45
… CSS file, started on import functionality
* Add CSS file type, add button in preferences to import a custom CSS file, started on import functionality

* Change so that the log uses format specifiers instead of string concatenation

* Add RadioButton for toggling custom theme

* Add preference for setting the path to custom CSS theme

* Load custom CSS if toggled

* Add missing language keys

* Remove check if the current theme is applied again, the check is removed since we don't need it

* Save path to the custom CSS file in program preferences
… once, disable custom theme radio button i no custom theme has been imported
* Add method for saving theme to file

* Add modal for selection witch theme to export as CSS
…2/jabref into DD2480-Group-22-addCSSCustomisation
@Siedlerchr
Copy link
Member

As you are working on that theme and preferences stuff, maybe you can also check for this issue/PR? #6429

@tobiasdiez
Copy link
Member

I don't really see the value in the export button. Instead I would focus more on a good documentation on how to create a custom theme, linking the main and dark.css in our repo. Or do I miss something?

@calixtus
Copy link
Member Author

calixtus commented Aug 1, 2020

To be honest, I don't see any advantage in the export button too, but in the first step I just wanted to preserve the effort the students put in it and to address the raised issues in the predecessor PR. Now I'm thinking about how to improve it. So ideas are welcome.

Btw. We should really think of cleaning up our css-files and to distinguish between changeable stuff and unchangeable. This is kind of messed up atm all through the codebase...

@tobiasdiez
Copy link
Member

Ok, then I would propose to remove the export dialog - also makes the implementation more streamlined.

Not sure what you mean with "changeable" vs "non-changable" in the css code. I think themes can override everything.

@calixtus
Copy link
Member Author

calixtus commented Aug 1, 2020

There are many specialized css files eg for the entry editor, for the preferences dialog, for the groups (I think I remember), partially because we do some detail styling in these specialized css files, which are not customizable.

@calixtus
Copy link
Member Author

calixtus commented Aug 12, 2020

I took some time to debug the issue in #6429
I tried to introduce a listener

EasyBind.wrapNullable(dialogPaneProperty()).subscribe(pane ->
                Globals.getThemeLoader().installCss(getDialogPane().getScene(), Globals.prefs));

in the BaseDialog constructor. The problem seems to be, that Java only calls the subscriber when already this dialog was opened before. So it works, but only on the second attempt. Strange... Maybe @Siedlerchr has a clue. But since this issue is not directly connected to this PR I suggest we follow it in #6429. Also i noticed, we have to make the controls dependend on the selected font size, since they wont be displayed properly if the font size is increased.

Therefor this PR should be ready review.

grafik

@calixtus calixtus marked this pull request as ready for review August 12, 2020 14:43
@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 12, 2020
@calixtus calixtus changed the title [WIP] Add CSS Customisation Add CSS Customisation Aug 12, 2020
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.

Nice work! Looks good to me. I've one or two minor remarks, and a couple more suggestions.

fontOverrideProperty.setValue(initialAppearancePreferences.shouldOverrideDefaultFontSize());
fontSizeProperty.setValue(String.valueOf(initialAppearancePreferences.getMainFontSize()));

String currentTheme = initialAppearancePreferences.getPathToTheme();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this makes things clearer, but just wanted to throw the idea in: What about having an enum for the different kinds of themes (light / dark / custom), and then an additional Path getPathToCustomTheme in case the "custom" enum option is chosen. Right now it feels like getPathToTheme is mixing these two aspects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, possible, but It escalated a bit. Again. 🙈

src/main/java/org/jabref/gui/util/ThemeLoader.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/util/ThemeLoader.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/util/ThemeLoader.java Outdated Show resolved Hide resolved
@@ -70,6 +70,8 @@

void setWorkingDir(Path dir);

String setLastPreferencesExportPath();
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Partly. I would like to postpone refactor of this until later when I'm working on FilePreferences.

this.additionalCssPath = Path.of(path);
}

public static void initialize(FileUpdateMonitor fileUpdateMonitor, PreferencesService preferences) {
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I'm not a big fan of these static methods. Static methods should only be used when there is no internal state to manage. So for example its ok that the methods in StringUtil are static, as they mostly follow the pattern that they take an input, process it and return something.

However, here, there is a state that is managed: the fileUpdateMonitor and the additionalCssToLoad private fields. This leads to the hidden convention, that it's only allowed to call the installCss method after initalize is called. It's better to make this requirement explicit by making initalize a constructor of a normal class. This also has the advantage that you can mock/replace the themeloader in tests or for whatever reason. (I agree that it's somewhat academic in this particular case for the themeloader, as its not widely used. But it's good to follow the practice that dependencies are explicitly stated and not hidden in untestable static classes).

Copy link
Member Author

@calixtus calixtus Aug 15, 2020

Choose a reason for hiding this comment

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

Ok, I have reworked the class, the static objects are now part of the theme object. Should feel now somewhat like a cell factory with 'fluent' api (getTheme().installCss(scene)). The fileUpdateMonitor dependency is still bound to the Globals object, but I did not have the time nor the energy to refactor everything now, so I would like to stop here and to leave the refactor of the Globals object for sometime later, as I really would like to finish the preferences refactor project first.

@github-actions
Copy link
Contributor

The JabRef maintainers will add the following name to the AUTHORS file. In case you want to use a different one, please comment here and adjust your name in your git configuration for future commits.

Marcel Luethi
Nils Streijffert
Qing
ShiqingLiu
WangAooa

@Siedlerchr Siedlerchr merged commit 43c8ba2 into master Aug 31, 2020
@Siedlerchr Siedlerchr deleted the CSSCustomisation branch August 31, 2020 14:56
@github-actions
Copy link
Contributor

The JabRef maintainers will add the following name to the AUTHORS file. In case you want to use a different one, please comment here and adjust your name in your git configuration for future commits.

Nils Streijffert

Siedlerchr added a commit that referenced this pull request Aug 31, 2020
* upstream/master: (120 commits)
  Follow up fix for copy paste (#6820)
  Add CSS Customisation (#6725)
  Separate signing and notarizing (#6822)
  Remove checkstyle hack. 8.36 got released (#6816)
  Feature/enable lucene query parsing (#6799)
  New release cycle
  Bump WyriHaximus/github-action-wait-for-status from v1.1.2 to v1.2 (#6814)
  Bump mockito-core from 3.5.5 to 3.5.7 (#6813)
  Bump classgraph from 4.8.87 to 4.8.89 (#6812)
  Bump me.champeau.gradle.jmh from 0.5.0 to 0.5.1 (#6811)
  Bump checkstyle from 8.35 to 8.36 (#6810)
  Improve Changelog
  Refactor edit action (#6808)
  Fixed typo in BuildInfo (#6807)
  disable checkstyle for generated
  fix checkstyle
  Simplify check-links.yaml (markdown-link-check) (#6720)
  Rename /gen to /generate (#6800)
  Disable CSL refresh on push (#6803)
  New Crowdin updates (#6804)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/JabRefFrame.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to load customized css
4 participants