On Tue, 31 Jan 2023 20:00:00 GMT, Andy Goryachev <[email protected]> wrote:
> In the context of adding theme support in javafx, I think this PR is a step > in the right direction. I also like a small set of platform-independent > properties like fg and bg colors. I wonder if the same approach can be > extended for other aspects of platform L&F, like fonts, spacing, and may be > other aspects (like, hiding scrollbars setting on Mac?) That could indeed be a useful enhancement. > 1. Appearance enum seems unnecessary - there might be more choices in a > specific platform (Mac Ventura has three: dark/light/auto). Perhaps using > fg/bg color intensity is sufficient to find out whether the overall theme is > "dark" or "light". While dark/light mode can indeed be detected just by comparing foreground and background color, the reason for the `Appearance` enumeration and the `Preferences.appearance` property is to support dark window frames. In [this gist](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) (see "Stage appearance"), I've described how the stage appearance and platform appearance APIs interact. > 2. ObservableMap. Similarly to Node.getProperties(), I wonder if there might > be a better way to observe the changes. May be a different metaphor > (subscription?), like adding a value change listener to a specific key. We > do need a set of keys (perhaps that can be an ObservableSet). Having said > that, ObservableMap is good enough solution, and forgive me for stating the > obvious, it should not initialize anything if the platform properties have > not been requested by the application code. The use of `ObservableMap` is debatable. I think that always initializing platform properties makes it easier to reason about the code. Saving the allocation of just a single map with 20-or-so key-value pairs is not a convincing reason to complicate the implementation, especially since most of the platform preferences implementation lives in the Glass toolkit, and not in the user-facing framework. ------------- PR: https://git.openjdk.org/jfx/pull/1014
