jfx icon indicating copy to clipboard operation
jfx copied to clipboard

8267546: Add CSS themes as a first-class concept

Open mstr2 opened this issue 4 years ago • 3 comments

This draft PR adds Theme as a first-class concept to OpenJFX. A theme is a collection of stylesheets and the logic that governs them. Themes can respond to OS notifications and update their stylesheets dynamically. This PR also re-implements the existing Caspian and Modena themes as first-class Theme implementations.


Progress

  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed

Issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/511/head:pull/511
$ git checkout pull/511

Update a local copy of the PR:
$ git checkout pull/511
$ git pull https://git.openjdk.java.net/jfx pull/511/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 511

View PR using the GUI difftool:
$ git pr show -t 511

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/511.diff

mstr2 avatar May 21 '21 04:05 mstr2

:wave: Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar May 21 '21 04:05 bridgekeeper[bot]

Looks great! I read through the code and nothing wrong jumped out - all the obvious missing parts were already raised on the mailing list and reasonable arguments provided for why it's about reducing maintenance costs.

The only thing that seemed odd is the way you set a theme by providing a special string syntax+class name as a "stylesheet". Is there some reason a more direct API cannot be added instead, where you pass the actual Class<T> constant instead, or simply construct the theme object yourself?

Also the Theme interface looks suspiciously like an SPI. Rather than use a special pseudo-URI that devs would have to fish out of the theme's documentation to trigger loading, it might be better to use ServiceLoader to locate themes and then have some notion of priority, such that simply adding a theme module to your modulepath would automatically cause it to override the platform defaults. This would allow apps to take the next obvious step and allow themes to be plugins. As is this API wouldn't support it out of the box, because the app would have no way to know what the class name of the theme actually is, so a bunch of ad-hoc mechanisms would emerge to fill that gap.

It seems Kevin wanted some more info on cost/benefit analysis on the mailing list, but it's a little unclear what sort of analysis or evidence would be wanted. I'll say that JavaFX apps I've written in the past would definitely have benefited from this. My CSS files always ended up being a mishmash of patches to Modena and actual app-specific styling. Additionally, whilst various JavaFX theme libraries exist and are popular, you normally have to do some manual integration work to use them which is a pity given that theming is, ultimately, entirely subjective and users frequently enjoy theming apps they work with a lot.

I'll also say that the whole JavaFX theming system was a point of confusion for me when I first learned the API. It clearly does support themes, yet there's no actual API point called "theme" anywhere and exactly how themes, CSS and so on relate isn't obvious. So this PR has a learning and usability benefit too.

Conclusion: to me the code looks really quite small, the benefits large (as most "real" JavaFX apps don't simply use Modena as-is), and it is at any rate mostly a refactoring and exposure of far more complex machinery already in the toolkit. Thumbs up!

mikehearn avatar Jul 01 '21 09:07 mikehearn

Here's an update on the current state of this feature. The API has evolved a bit to offer more flexibility, and can be used in several different ways:

1. Ad-hoc themes

An ad-hoc theme is a way to programmatically tie together a bunch of stylesheets with very little effort:

var theme = Theme.of("stylesheet1.css", "stylesheet2.css");

Similarly, existing themes can be extended with additional stylesheets. The stylesheet order is retained such that base stylesheets always come before extension stylesheets, even if the list of base stylesheets changes.

var theme = Theme.extensionOf(new ModenaTheme(), "stylesheet1.css", "stylesheet2.css");

2. Theme classes

When more control is desired, a theme can be implemented by extending the Theme class. In this example, a custom theme toggles a high-contrast stylesheet:

public class MyTheme extends Theme {
    private final ObservableList<String> stylesheets = FXCollections.observableArrayList();

    public MyTheme() {
        stylesheets.add("stylesheet1.css");
        platformThemeChanged(getPlatformThemeProperties());
    }

    @Override
    public ObservableList<String> getStylesheets() {
        return stylesheets;
    }

    @Override
    protected void platformThemeChanged(Map<String, String> properties) {
        String value = properties.get("Windows.SPI_HighContrastOn");
        if (value != null) {
            if (Boolean.parseBoolean(value)) {
                stylesheets.add("highconstrast.css");
            } else {
                stylesheets.remove("highconstrast.css");
            }
        }
    }
}

Similarly, an existing theme class can be extended by subclassing:

public class MyTheme extends ModenaTheme {
    // concat existing stylesheets with new stylesheets, react to platform changes, etc.
}

3. Usage

Themes are collections of user-agent stylesheets, and as such, they can be used in all places where user-agent stylesheets can be used (that's in Application, Scene and SubScene).

Themes, like user-agent stylesheets, must be specified with a URL to be able to use them with the existing setUserAgentStylesheet(String) APIs. There are three ways to specify a theme:

3.1. Class URLs

The easiest option to reference a theme class is with the theme scheme:

Application.setUserAgentStylesheet("theme:com.example.MyTheme");

The theme scheme works when the theme class is instantiable with a parameterless constructor.

3.2. Instance URLs

Ad-hoc themes, or theme classes that require custom instantiation, can be referenced by their instance URL:

var theme = Theme.of("stylesheet1.css", "stylesheet2.css");
Application.setUserAgentStylesheet(theme.toURL());

The URL returned by Theme.toURL() can be dereferenced as long as the theme instance is alive. It is not required to keep a reference to the theme instance around after setUserAgentStylesheet has been called, as the theming subsystem will keep the theme instance alive.

3.3. Legacy names

Backwards compatibility is preserved by interpreting the names CASPIAN and MODENA as aliases for their corresponding theme class. Since themes can be used with all user-agent stylesheet APIs, we can now also use different themes in the same application:

mstr2 avatar Aug 26 '21 04:08 mstr2

Are there any plans for this to be merged or is there any work remaining for this? I'd love to use this instead of what I'm currently doing which is accessing private API in JavaFX with --add-exports.

airsquared avatar Nov 06 '22 19:11 airsquared

I've iterated on this feature for several months, and I'm quite comfortable with the latest version. The API was changed considerably: there are no "theme URIs" masquerading as user-agent stylesheets any more. Instead there's an application-wide Application.styleTheme property. This simplifies the enhancement quite a lot. I've updated this PR with an overview of the API and some usage examples.

mstr2 avatar Nov 09 '22 22:11 mstr2

This will take a fair bit of discussion regarding how various applications might use such a feature, what the API should look like, etc.

/reviewers 3 /csr

kevinrushforth avatar Nov 09 '22 22:11 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

openjdk[bot] avatar Nov 09 '22 23:11 openjdk[bot]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8267546 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk[bot] avatar Nov 09 '22 23:11 openjdk[bot]

This will take a fair bit of discussion regarding how various applications might use such a feature, what the API should look like, etc.

I think there are basically three categories of how applications might use this feature:

  1. Extend a built-in theme.
  2. Create a new static theme.
  3. Create a dynamic theme that responds to OS preferences.

The proposed feature addresses all three categories, with the third option (a dynamic theme) being the most difficult to implement. It requires quite a bit of effort to create such a theme, and a good amount of that effort will be figuring out how OS-level preferences interact with themes (platform independence is a complicating factor).

I've made it an explicit non-goal of this feature to provide an API for higher-level concepts like dark mode, high contrast, etc., since I think APIs for OS design trends should be left to third-party libraries instead.

mstr2 avatar Nov 20 '22 00:11 mstr2

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/theming
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Dec 12 '22 18:12 openjdk[bot]

Hi all,

I’ve seen in the mailing list a request for commenting on this PR and as a long-time theme developer (JMetro and other themes) I'd thought I'd give my 2 cents.

I think it’s a good idea to add the concept of a theme to the JavaFX API! So, thanks for this.

1 - Reading through the javadocs in this PR and the description, I think it’s not clear whether the stylesheets of a StyleTheme will be used as user agent stylesheets or as author stylesheets. It says that StyleThemes have higher precedence than a user agent stylesheet so I suppose they are going to be author stylesheets (?), but there’s no point in the Javadoc where that is explicitly said. If that’s not the case, then I think the opposite should then be explicitly written. I.e. that it will have higher precedence than a user agent stylesheet but it’s not an author stylesheet.

2 – I think the ability to specify in the StyleTheme whether it is a user agent stylesheet, or an author stylesheet could be of interest. Most of the themes I know are composed of author stylesheets (they use the getStylesheets() API in Scene) so migration to using the StyleTheme API would be easier if one could specify this. Conversely specifying that the StyleTheme is a user agent stylesheet will also be very useful in the cases where we’re creating a theme but don’t want it to override styles specified through code, etc.

3 – I would really love for JavaFX to have first class support for dark and light modes, and I think this would be the ideal place for it. One of the problems with how things are right now is that if you create a dark theme with this API or the previous API (using stylesheets) the frames of windows (main windows and dialogs) will still show with a light theme (I see this on Windows, haven’t tested on Mac but I suppose it will be the same). So as it is, you can’t fully create a dark theme in JavaFX. The only way would be to call on native code to change the frame of windows (by making a request for the native window to change its appearance to dark mode) which isn’t trivial and would have to be done for the various operating systems and it’s subject to break. The other way would be to create your own frames of the windows which I think would be even worse. You’d have to create the frame buttons yourself and all other decorations. If they don’t visually exactly match the ones from the native platform they’re going to look off. You’d also have to do this for all operating systems and for their various versions (various versions of the same OS might have different frame decorations, e.g. win10 vs win11). Given that dark and light theme concepts are pervasive across all major operating systems (Windows, Mac, iOS, Android, etc) and it’s a concept that has lasted for years and continues to exist, I think it would be of value to fully support this.

Thanks

dukke avatar Jan 13 '23 19:01 dukke

@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Mar 31 '23 23:03 bridgekeeper[bot]

@mstr2 This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bridgekeeper[bot] avatar May 27 '23 02:05 bridgekeeper[bot]

No don't, don't close this omg 🤦

palexdev avatar May 28 '23 11:05 palexdev

No don't, don't close this omg 🤦

I’ll re-open once the prerequisite PRs are integrated. The discussion continues in #1014

mstr2 avatar May 28 '23 19:05 mstr2

No don't, don't close this omg 🤦

I’ll re-open once the prerequisite PRs are integrated. The discussion continues in #1014

Oh I see, thanks for your work!

palexdev avatar May 29 '23 06:05 palexdev

@mstr2 any news on this feature?

palexdev avatar Feb 18 '24 12:02 palexdev

@mstr2 any news on this feature?

I've been working on this feature lately, and will soon be sharing more about it.

mstr2 avatar Feb 18 '24 12:02 mstr2

@mstr2 First of all thank you very much for your work on this! 👍👍

Are there any news regarding StyleTheme? This is a much needed feature for custom theme development. You can't really create a full blown theme in JavaFX that's easy to override and works well with other libraries because this piece of API is missing from JavaFX (can't set more than 1 css file to be the user agent stylesheet, etc).

Thanks again!

dukke avatar Apr 11 '24 23:04 dukke