8313424: JavaFX controls in the title bar (Preview) #1605
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/reviewers 2 reviewers |
|
@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mstr2 please create a CSR request for issue JDK-8313424 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
Hey, I'm glad to see this PR, but do you have any ideas for continuing to provide |
|
Very nice. I'll test on Linux and report back. |
The only difference between the two would be whether the default window buttons are provided. I don't see how a window without default window buttons would be more useful. Even heavily stylized apps like Spotify use window buttons that feel at home on the OS, that doesn't take away from a consistent look and feel. |
|
A few points observed on Linux:
Added later: Nice cleanup on |
|
I suggest we convert this PR to Draft and first discuss this in the mailing list. There are so many issues with the proposal that need to be ironed first: CSS support, height limitation (what happens when the app or CSS places too large of the component?), user-defined color/accents/transparency on Windows, to name just a few. This change also may add a significant maintenance burden to the platform, for what I feel is a very small payout. What do you think? |
This has already been discussed at various points in time, and was always received positively. The previous implementation is one of the most-upvoted feature proposals since OpenJFX moved to GitHub.
What about these things? I don't understand the question, but let me try to give some answers nontheless:
Popular demand says otherwise. |
|
Popular demand is good, but I would like to hear from the tech leads and the maintainers. |
|
To clarify about height: do all the platforms support arbitrary height? What if size set by the app/CSS exceeds the height supported by the platform? About platform preferences: will it support all the attributes of the window decorations provided by the platform? |
Yes, all platforms support header bars of arbitrary height. You can make the header bar the size of the entire window, in which case everything is draggable.
If by "attributes" you mean the default behavior of platform decorations, then the anwer is mostly. These are the behaviors that are available on every platform:
On Windows, native window decorations also have the "system menu", which is this thing that appears when you click on the program icon:
|
|
@tsayao Thanks for the comments. I'll have a look at the bugs that you found.
I know that dragging on interactive controls is a thing on Linux, but I don't think that we should be doing that. JavaFX applications are multi-platform apps, which means that their behavior should be consistent across platforms. Stealing mouse interactions on interactive controls is not a thing on Windows and macOS, and this has the potential to cause problems for application developers. If you want, you can declare any node in the header bar to be draggable on all platforms with
I'll have to look into that, but in general an
While that sounds useful at first, I don't think it carries its own weight. Many platforms use different styling for windows that are focused vs. windows that are not. This not only includes the header bar, but many other parts of the user interface as well. I don't think that we should be adding what would essentially be ad-hoc pseudo-classes only to HeaderBar. In addition to that, it is extremely easy for an application to do this by adding a listener to We should definitely not do pseudo-classes for light vs. dark mode. The correct way to solve this problem is with media queries (prefers-color-scheme). |
|
Mailing list message from quizynox at gmail.com on openjfx-dev: Hello, Thank you so much for your effort! I'm really glad this hasn't been Every modern platform supports this feature: Electron, Tauri, Wails, Qt, Unfortunately, the current UNDECORATED stage implementation lacks two That's why the implementation should be handled on the native side, which It's indeed a complex feature. For that reason, I believe the ??, 22 ???. 2024??. ? 03:24, Michael Strau? <mstrauss at openjdk.org>: -------------- next part -------------- |
|
I think we should look at use cases and design a simple solution that can be extended. Doing this with JavaFX as it is now will be hacky, since it would need to touch internals that are not exposed by default. Intellij Idea Nautilus Chrome Amberol (music player). App Center (Ubuntu Software Store) - This one uses flutter Some of them has no title at all. Some fuses the HeaderBar with the body, like nautilus. Even chrome, the tabs on the HeaderBar are not "isolated" from the body. On modern Gnome, everything is client side decorated. Server side decoration is legacy. It's better for rendering since the window manager does not need to calculate, draw and sync with the window. It's probably less flickery. Since JavaFx accounts the title as part of the window size, the current glass implementation is very hacky because it needs to request the decoration sizes from the window manager and then recalculate it. Having it on the client side is better, because no hacky solution is required. |
|
Adding some more examples. Vivaldi browser actually allows you to switch between two modes: which can be switched with to So in the first mode, the menu is compressed vertically into the Vivaldi button. Here is Discord: Very compact. As for the payout of this feature, while personally I don't have a need for it, I will probably use it if it's not too much trouble. Regardless of my own opinion, this has been one of the most requested features, along with tray icon support, and it's available in the main "competition" frameworks. I would say that a very strong case will need to be made for this to not be added. The question that remains, as it often does, is if this is the right implementation. Hopefully the review process will figure that out. |
|
Continuing on window attributes. I would like to know what is and is not supported by platform, by feature. For example: Windows
Did I miss anything? |
|
For the HeaderBar, I would like to see the explanation of different layout options, including the cases when all the information does not fit the window width. Which parts are contracted? How is overflow handled? |
|
May be I am late to the party, but I would suggest to discuss the JEP first. I would like to see the summary by platform by feature, I would like to see details of the layout, and the description if and how the proposed design responds to user preferences changes dynamically. I would also like to see alternatives. Perhaps the app developers has all the tools already (for example, creating an overlay transparent scene on top of the platform title bar?), or maybe this functionality should be rather implemented in a library. Lastly, there is a concern of adding a huge maintenance burden on the platform. Next time the major OS vendor changes the L&F or adds something to the window decorations, we are on the hook for supporting that. I am not even qualified to access the impact of this feature in the Linux world. There are so many frameworks and opinions - how do you propose to handle that? Is it going to be supported on Wayland? |
|
Gtk does work on Mac and Windows, maybe we can see how it handles it's HeaderBar, for reference. |
|
Another aspect is whether this should be a conditional feature. |
I'll eagerly invite you to dissect this PR for its merits, its pros and cons. However, your questions lead me to conclude that you haven't really looked at what I'm proposing. Half of your questions are answered just by looking at the documentation for Let me try to explain
Since there is no non-client title bar, all questions regarding the appearance or accessibility of the |
There was a problem hiding this comment.
I finished my review of the API. It looks good. I identified one minor formatting issue in the docs.
I also left a minor question about a test method. I'll review the CSR and do a bit of light testing. Others have reviewed the implementation, so I'll limit most of my code review to the native glass changes.
| * <p> | ||
| * This method is only used for testing purposes. | ||
| */ | ||
| public static void enableForTesting() { |
There was a problem hiding this comment.
Minor: As an alternative, have you considered adding a utility method in a test utility class that sets both the javafx.enablePreview and javafx.suppressPreviewWarning system properties? Unless there a reason that wouldn't work, it seems cleaner to me.
There was a problem hiding this comment.
I've modified the build script instead to always specify the system properties for tests.
| * Setting the preferred height to zero hides the system-provided header buttons, allowing applications to | ||
| * use custom header buttons instead (see {@link #setButtonType(Node, HeaderButtonType)}). | ||
| * <p> | ||
| * The default value {@code #USE_DEFAULT_SIZE} indicates that the platform should choose the button height. |
There was a problem hiding this comment.
Minor: The # is rendered since it using @code rather than @link. I recommend removing the #.
There was a problem hiding this comment.
Testing looks good. One thing I noticed on Linux when running on an older version of Ubuntu is that the buttons are always on the right, even when the window manager is configured to put them on the left for decorated windows. It might be worth considering a future enhancement to query the window manager for whether the buttons should be on the left or right.
I spot checked a few things in the code and at least looked at all of the native code. I didn't spot any concerns.
I did leave a couple questions, but nothing that would prevent my approving this. This is a nice piece of work, and I'm happy we will be getting it into JavaFX 25 as a preview feature!
| @@ -2182,6 +2182,8 @@ allprojects { | |||
| systemProperty 'unstable.test', IS_UNSTABLE_TEST | |||
| systemProperty 'junit.jupiter.execution.timeout.test.method.default', JUNIT_TEST_TIMEOUT | |||
| systemProperty 'junit.jupiter.execution.timeout.lifecycle.method.default', JUNIT_LIFECYCLE_TIMEOUT | |||
| systemProperty 'javafx.enablePreview', 'true' | |||
| systemProperty 'javafx.suppressPreviewWarning', 'true' | |||
There was a problem hiding this comment.
This seems a good approach.
it might make it a little more difficult if we wanted to test the logic that throws an exception when javafx.enablePreview is not "true", but that would still be possible by having that test explicitly set the property to the empty string in an @Before method, and then restoring it in an @After method. Probably not worth doing anyway.
There was a problem hiding this comment.
Yes, this seems like a good solution waiting for a problem. I agree that we can do that when we discover that problem.
|
|
||
| class GtkWindow extends Window { | ||
|
|
||
| public GtkWindow(Window owner, Screen screen, int styleMask) { | ||
| super(owner, screen, styleMask); | ||
|
|
||
| if (isExtendedWindow()) { | ||
| prefHeaderButtonHeightProperty().subscribe(this::onPrefHeaderButtonHeightChanged); |
There was a problem hiding this comment.
This subscription is never canceled. Could this result in a leak?
There was a problem hiding this comment.
The prefHeaderButtonHeight property is a field on the same class. We only subscribe to this property within the context of this class, so it won't prevent GtkWindow from being eligible for GC.
| @@ -1400,143 +1350,6 @@ protected void notifyLevelChanged(int level) { | |||
| } | |||
| } | |||
|
|
|||
| private class UndecoratedMoveResizeHelper { | |||
There was a problem hiding this comment.
Have you verified that removing this doesn't result in any loss of functionality?
There was a problem hiding this comment.
Yes, one of the first things that I discovered in this project was that UndecoratedMoveResizeHelper was effectively unused.
|
I am going to run a bunch of tests and report the results. The first thing I noticed:
Happens for any Modality setting other than NONE.
|
|
With this config, clicking on "Enter/Exit Full Screen" button locks up the app without going into the full screen mode. I was able eventually to get it unstuck, but opening a stage for the second time produced a blank window:
once that happens, the application breaks - for example, I cannot change any options in the Stage Tester window. |
|
Same full screen issue for this configuration:
no full screen, and once the "My Stage" is closed the app is broken. |
|
@andy-goryachev-oracle Are any of the problems you are discovering caused by this PR? Or are you running into preexisting problems? |
|
|
Looks like the problems are caused by changes in this PR (maybe recent changes since I did not see these issues before). The test app is difference since we don't have the stage tester tool in the master branch, but one can use the standalone monkey tester https://github.com/andy-goryachev-oracle/MonkeyTest which has a Stage page. Use iconify/full screen checkboxes to replicate the abovementioned scenarios, and it seems to work correctly in the master branch:
|
|
I can reproduce at least two of the things you reported with master. What I did to ensure an accurate comparison was to take the modified monkey tester that is included with this PR and removed all references to HeaderBar. The resulting app will run on either the master branch or this PR branch. With that, I can reproduce the following behaviors:
So far I haven't seen any regressions, but it's worth some additional testing. |
|
I'm testing the following configuration on macOS: Both with the current master, as well as this PR, I can confirm your observation that a non-fullscreen window shows up, and when it's closed, it freezes the application. However, I think we're testing unsupported configurations here. These are definitively bugs, because unsupported configurations should not crash or freeze the application. What we should probably do is collect these artifacts in a bug ticket. |
|
This is another configuration: I then click the "Iconify" button. I can confirm that both windows get iconified, a beep is heard, and both windows are restored. However, again, this is an unsupported configuration: you can't iconify a modal window. |
|
Apparently that stage tester utility has made it really easy to test various combinations of stage configurations, which we haven't been able to do before (at least not as easily). |
|
I've added popup menu to full screen/ minimize/ maximize to the standalone monkey tester, just in case. I can reproduce the iconify issue with EXTENDED and the following config (by invoking 'iconify' popup menu):
Looks like fx thinks it's iconified, but it is not. (please pull from the standalone monkey tester repo) |
|
I've created a ticket for this problem: https://bugs.openjdk.org/browse/JDK-8358620 |
|
should we add the full screen issue to the same ticket or is it a separate one? |
I haven't studied the problem extensively, but my guess is that it's a different problem. I don't see at first glance why a modal window shouldn't also be a full-screen window. This needs more investigation. |
|
On #1789 there's a Test utility for Stages called |
|
created * JDK-8358625 Full screen of a modal Stage breaks the application |
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/WindowManager.java
Outdated
Show resolved
Hide resolved
| GdkDisplay* display = gdk_display_get_default(); | ||
| if (!display) { | ||
| return; | ||
| } | ||
|
|
||
| GdkSeat* seat = gdk_display_get_default_seat(display); | ||
| GdkDevice* device = gdk_seat_get_pointer(seat); | ||
| if (!device) { | ||
| return; | ||
| } | ||
|
|
||
| gint rx = 0, ry = 0; | ||
| gdk_window_get_root_coords(gdk_window, x, y, &rx, &ry); | ||
|
|
||
| GdkEvent* event = (GdkEvent*)gdk_event_new(GDK_BUTTON_PRESS); | ||
| GdkEventButton* buttonEvent = (GdkEventButton*)event; | ||
| buttonEvent->x_root = rx; | ||
| buttonEvent->y_root = ry; | ||
| buttonEvent->window = (GdkWindow*)g_object_ref(gdk_window); | ||
| buttonEvent->device = (GdkDevice*)g_object_ref(device); | ||
|
|
||
| gdk_window_show_window_menu(gdk_window, event); | ||
| gdk_event_free(event); |
There was a problem hiding this comment.
The seat concept was introduced in gtk 3.20 and gdk_window_show_window_menu was in 3.14, and we require 3.8.
I would agree that it's time to bump it up.
There was a problem hiding this comment.
Is there anything we need to change here?
There was a problem hiding this comment.
We can bump up required Gtk version (with another PR) to 3.20 (it's from 2016, so pretty old), or replace the gdk_window_show_menu with X11 calls.
There was a problem hiding this comment.
I'm okay with bumping the required version. It seems like 3.8 was released 12 years ago, and we would be requiring a version that was released 9 years ago. Sounds fair.
There was a problem hiding this comment.
Our production build system is Oracle Linux 8, which has GTK 3.22.
The oldest system I now have access to is Ubuntu 16.04 (yeah, I know...it's probably time to retire it), which has GTK 3.18. I build JavaFX using GCC 14.2 (via a devkit) and compile against a GTK 3.22 header files and library. I'm able to run this PR and use the EXTENDED stage style with a HeaderBar, but when I try to use the context menu it crashes:
java: symbol lookup error: build/sdk/lib/libglassgtk3.so: undefined symbol: gdk_display_get_default_seat
We should make a deliberate decision as to whether or not to bump the minimum. I think bumping the minimum to 3.20 might be reasonable thing to, although I'd want to do it separately from this PR and provide a release note for that.
There was a problem hiding this comment.
Worth noting is that we only recently upgraded our production build env from Oracle Linux 7, which is now out of support. Bumping the minimum GTK version to 3.20 will preclude running on OL 7 / RHEL 7 at all (RHEL 7 has GTK 3.8, which is why that minimum was chosen). I think this is OK, but is another reason I want to see a separate JBS issue to bump the minimum.
There was a problem hiding this comment.
I think providing a release note that the new extended stage style requires GTK 3.20 sounds reasonable.
There was a problem hiding this comment.
I mis-remembered the GTK version in OL7. I just tried it on an OL 7 VM I still had lying around, and it also has GTK 3.22 and using this PR, the MonkeyTester with EXTENDED stage style and a HeaderBar runs just fine, including the context menu.
I think providing a release note that the new extended stage style requires GTK 3.20 sounds reasonable.
I was more thinking that we would file a new issue to bump the minimum to 3.20 and provide a release note using that bug that JavaFX requires 3.20 as a minimum. If we end up not doing that, then a release note that the extended stage style needs 3.20 would be good.
There was a problem hiding this comment.
I can work on the Gtk version bump.
| @@ -126,6 +129,7 @@ class WindowContext : public DeletedMemDebug<0xCC> { | |||
| virtual void set_title(const char*) = 0; | |||
| virtual void set_alpha(double) = 0; | |||
| virtual void set_enabled(bool) = 0; | |||
| virtual void set_system_minimum_size(int, int) = 0; | |||
There was a problem hiding this comment.
Maybe set_headerbar_mininum_size() ?
There was a problem hiding this comment.
Hmm... but it's not the minimum size of the header bar, it's the minimum size of the window.
Implementation of
StageStyle.EXTENDED.Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1605/head:pull/1605$ git checkout pull/1605Update a local copy of the PR:
$ git checkout pull/1605$ git pull https://git.openjdk.org/jfx.git pull/1605/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1605View PR using the GUI difftool:
$ git pr show -t 1605Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1605.diff
Using Webrev
Link to Webrev Comment