On Fri, 21 May 2021 04:30:28 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
> This PR adds style themes as a first-class concept to OpenJFX. A style theme
> is a collection of stylesheets and the logic that governs them. Style themes
> can respond to OS notifications and update their stylesheets dynamically.
> This PR also re-implements Caspian and Modena as style themes.
>
> ### New APIs in `javafx.graphics`
> The new theming-related APIs in `javafx.graphics` provide a basic framework
> to support application-wide style themes. Higher-level theming concepts (for
> example, "dark mode" detection or accent coloring) are not a part of this
> basic framework, because any API invented here might soon be out of date.
> Implementations can build on top of this framework to add useful higher-level
> features.
> #### 1. StyleTheme
> A style theme is an implementation of the `javafx.css.StyleTheme` interface:
>
> /**
> * {@code StyleTheme} is a collection of stylesheets that specify the
> appearance of UI controls and other
> * nodes in the application. Like a user-agent stylesheet, a {@code
> StyleTheme} is implicitly used by all
> * JavaFX nodes in the scene graph.
> * <p>
> * The list of stylesheets that comprise a {@code StyleTheme} can be modified
> while the application is running,
> * enabling applications to create dynamic themes that respond to changing
> user preferences.
> * <p>
> * In the CSS subsystem, stylesheets that comprise a {@code StyleTheme} are
> classified as
> * {@link StyleOrigin#USER_AGENT} stylesheets, but have a higher precedence
> in the CSS cascade
> * than a stylesheet referenced by {@link
> Application#userAgentStylesheetProperty()}.
> */
> public interface StyleTheme {
> /**
> * Gets the list of stylesheet URLs that comprise this {@code StyleTheme}.
> * <p>
> * If the list of stylesheets that comprise this {@code StyleTheme} is
> changed at runtime, this
> * method must return an {@link ObservableList} to allow the CSS
> subsystem to subscribe to list
> * change notifications.
> *
> * @implNote Implementations of this method that return an {@link
> ObservableList} are encouraged
> * to minimize the number of subsequent list change
> notifications that are fired by the
> * list, as each change notification causes the CSS subsystem
> to re-apply the referenced
> * stylesheets.
> */
> List<String> getStylesheets();
> }
>
>
> A new `styleTheme` property is added to `javafx.application.Application`, and
> `userAgentStylesheet` is promoted to a JavaFX property (currently, this is
> just a getter/setter pair):
>
> public class Application {
> ...
> /**
> * Specifies the user-agent stylesheet of the application.
> * <p>
> * A user-agent stylesheet is a global stylesheet that can be specified
> in addition to a
> * {@link StyleTheme} and that is implicitly used by all JavaFX nodes in
> the scene graph.
> * It can be used to provide default styling for UI controls and other
> nodes.
> * A user-agent stylesheets has the lowest precedence in the CSS cascade.
> * <p>
> * Before JavaFX 20, built-in themes were selectable using the special
> user-agent stylesheet constants
> * {@link #STYLESHEET_CASPIAN} and {@link #STYLESHEET_MODENA}. For
> backwards compatibility, the meaning
> * of these special constants is retained: setting the user-agent
> stylesheet to either {@code STYLESHEET_CASPIAN}
> * or {@code STYLESHEET_MODENA} will also set the value of the {@link
> #styleThemeProperty() styleTheme}
> * property to a new instance of the corresponding theme class.
> * <p>
> * Note: this property must only be modified on the JavaFX application
> thread.
> */
> public static StringProperty userAgentStylesheetProperty();
> public static String getUserAgentStylesheet();
> public static void setUserAgentStylesheet(String url);
>
> /**
> * Specifies the {@link StyleTheme} of the application.
> * <p>
> * {@code StyleTheme} is a collection of stylesheets that define the
> appearance of the application.
> * Like a user-agent stylesheet, a {@code StyleTheme} is implicitly used
> by all JavaFX nodes in the
> * scene graph.
> * <p>
> * Stylesheets that comprise a {@code StyleTheme} have a higher
> precedence in the CSS cascade than a
> * stylesheet referenced by the {@link #userAgentStylesheetProperty()
> userAgentStylesheet} property.
> * <p>
> * Note: this property must only be modified on the JavaFX application
> thread.
> */
> public static ObjectProperty<StyleTheme> styleThemeProperty();
> public static StyleTheme getStyleTheme();
> public static void setStyleTheme(StyleTheme theme);
> ...
> }
>
>
> `styleTheme` and `userAgentStylesheet` are correlated to preserve backwards
> compatibility: setting `userAgentStylesheet` to the magic values "CASPIAN" or
> "MODENA" will implicitly set `styleTheme` to a new instance of the
> `CaspianTheme` or `ModenaTheme` class. Aside from these magic values,
> `userAgentStylesheet` can be set independently from `styleTheme`. In the CSS
> cascade, `userAgentStylesheet` has a lower precedence than `styleTheme`.
>
> #### 2. PlatformPreferences
> `javafx.application.PlatformPreferences` can be used to query UI-related
> information about the current platform to allow theme implementations to
> adapt to the operating system. The interface extends `Map` and adds several
> useful methods, as well as the option to register a listener for change
> notifications:
>
> /**
> * Contains UI preferences of the current platform.
> * <p>
> * {@code PlatformPreferences} implements {@link Map} to expose platform
> preferences as key-value pairs.
> * For convenience, {@link #getString}, {@link #getBoolean} and {@link
> #getColor} are provided as typed
> * alternatives to the untyped {@link #get} method.
> * <p>
> * The preferences that are reported by the platform may be dependent on the
> operating system version.
> * Applications should always test whether a preference is available, or use
> the {@link #getString(String, String)},
> * {@link #getBoolean(String, boolean)} or {@link #getColor(String, Color)}
> overloads that accept a fallback
> * value if the preference is not available.
> */
> public interface PlatformPreferences extends Map<String, Object> {
> String getString(String key);
> String getString(String key, String fallbackValue);
>
> Boolean getBoolean(String key);
> boolean getBoolean(String key, boolean fallbackValue);
>
> Color getColor(String key);
> Color getColor(String key, Color fallbackValue);
>
> void addListener(PlatformPreferencesListener listener);
> void removeListener(PlatformPreferencesListener listener);
> }
>
> An instance of `PlatformPreferences` can be retrieved via
> `Platform.getPreferences()`.
>
> ### Usage
> In its simplest form, a style theme is just a static collection of
> stylesheets:
>
> Application.setStyleTheme(() -> List.of("stylesheet1.css", "stylesheet2.css");
>
> A dynamic theme can be created by returning an instance of `ObservableList`:
>
> public class MyCustomTheme implements StyleTheme {
> private final ObservableList<String> stylesheets =
> FXCollections.observableArrayList("colors-light.css", "controls.css");
>
> @Override
> public List<String> getStylesheets() {
> return stylesheets;
> }
>
> public void setDarkMode(boolean enabled) {
> stylesheets.set(0, enabled ? "colors-dark.css" : "colors-light.css");
> }
> }
>
> `CaspianTheme` and `ModenaTheme` can be extended by prepending or appending
> additional stylesheets:
>
> Application.setStyleTheme(new ModenaTheme() {
> {
> addFirst("stylesheet1.css");
> addLast("stylesheet2.css");
> }
> });
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!
modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line
93:
> 91:
> 92: FnRtlGetVersion* pRtlGetVersion;
> 93: if (!loadFunction(hLibNtdll, pRtlGetVersion, "RtlGetVersion")) {
Is this really the right way to get the Windows version these days? It used to
be that everything in NTDLL was off limits. Seems there's an API now for
detecting if you're on Windows 8+:
https://docs.microsoft.com/en-us/windows/win32/sysinfo/version-helper-apis
modules/javafx.graphics/src/test/java/test/javafx/application/ThemeTest.java
line 16:
> 14: public class ThemeTest {
> 15:
> 16: public static class PublicConstructor implements Theme {
Rather than support varying constructor types to pass in the properties map,
why not just specify the API as that platformThemeChanged will be called
immediately after construction? That way there's only one code path that
handles the properties map, and the way the class is constructed no longer
matters.
-------------
PR: https://git.openjdk.org/jfx/pull/511
