8351867: No UI changes while iconified #1733
Conversation
|
👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into |
|
@hjohn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 7 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
changed the title
Webrevs
|
|
When the window is restored glass calls notifySize with RESTORED and then notifyRepaint with valid dimensions. Why is the repaint being dropped? |
Can you give me a bit more detail what you mean here? I suspect it might be related to the size not actually changing, and so a size update with the same size does nothing? |
Sorry, I'm asking a general question about how JavaFX deals with repaint events issued by the OS. I've never understood how JavaFX's painting model interfaces with OS level calls that expect drawing to occur on demand and synchronously. But that conversation should probably be moved to another forum. |
|
My thought on this bug was to fix it in the scenegraph, rather than forcing a complete repaint in the toolkit. The scenegraph tracks the state of what is dirty and would only need to repaint if something has changed (and probably only those parts that have changed). @arapte Can you take a look at this and see what you think? I am not able to look at it for at least a week. |
|
/reviewers 2 |
|
@kevinrushforth |
Yeah, I'm sort of mimicking what happens when a RESCALE or MOVE occurs, so it is similar to what some of the other handlers do (MOVE does |
|
@hjohn this did not fix the issue for me on macOS 15.3.1 M1 |
|
Yeah, your solution might be fine. I just wanted to take a look at an alternative (or rather ask Ambarish to). |
@andy-goryachev-oracle Hm, I can't test it on Mac, but this solution works on Windows. I just tested again with your reproducer, and it fails without this change and works with it. Maybe someone with a Mac can take a closer look? For mac, perhaps also do |
|
I just verified that this PR does not fix the original issue on the Mac. Note: I could not reproduce the bug on my Mac 15.3.2 M2 Max using the latest master. Then I went to System Settings > Desktop & Dock and turned on "Minimize windows into application icon". Then the problem reproduced. But when I toggled that switch back off and the bug still reproduced. This may be irrelevant but I did notice a difference between Mac and Windows. After glass sends notifySize RESTORE it sends notifyRepaint on Windows but not on Mac. In fact I don't think the Mac ever sends notifyRepaint. |
|
Adding |
|
This does fix the issue on windows. The change seems to achieve the intended but the trigger check may not be the best way. |
|
At least on the Mac there seems to be a timing issue. If the MIMIMIZE notification is sent before a scheduled draw occurs the drawing code will clear the scene dirty bit but then skip the actual drawing since the window is minimized. But I've also seen cases where drawing completes before MINIMIZE is sent. When the window is restored notifyRepaint will be called on Windows and Linux which looks like it will call entireSceneNeedsRepaint to set the scene dirty bit (based on my reading of the code). Windows calls notifyRepaint whenever it gets a WM_PAINT event and I've verified that that happens after a window is restored. Looking at the sources it seems that Linux calls notifyRepaint only when a window is restored (!) Mac never calls notifyRepaint. There's code to call it but given where it's located I don't think it will ever be executed. It would be nice to get some clarification on when notifyRepaint should be called and get the platforms to behave more consistently. |
I think the bug can also appear without the scene graph having changed, when you iconify immediately programmatically (ie. before showing the window). The change detection would need to make sure it also detects the case where a scene was not drawn yet at all: public class App extends Application {
public static void main(String[] args) {
Application.launch(args);
}
@Override
public void start(Stage stage) {
Scene scene = new Scene(new Label("This should be modified when the signal is received"), 500, 500);
stage.setScene(scene);
stage.setIconified(true);
stage.show();
}
} |
|
Reviewers: @arapte @lukostyra |
|
This doesn't look like an easy fix; although it fixes the issue for me on Windows, I don't have access to a Mac to fix the problem on that platform as well. Perhaps someone else wants to take a look at this issue. |
|
First an errata: I wrote earlier that notifyRepaint is only called on Linux when restoring a window. I was wrong, it's also called for EXPOSE events. Anyway, I can reproduce this bug on all three platforms. JavaFX won't paint to a minimized window. This suggests to me that when we transition out of the minimized state someone needs to call I can tweak the Mac code to call I have verified that on all platforms Reproducing this on macOS 15 is complicated by some weirdness going on under the hood. The address of the main screen changes unexpectedly. This causes glass to send |
|
I think this PR is the correct fix on Windows and Linux. The scene state tracks the minimized state of the window. When the window is restored someone needs to tell the scene state to go update the isWindowMinimized flag. The Mac will require another tweak. Personally I would just modify glass to send notifyRepaint in this specific instance. I suppose that should be treated as a separate bug. Unfortunately it's going to be a bear to test given the strange things happening under the hood with NSScreen (at least on my system). |
|
I can help with the testing: I have 15.3.2 with one or two external monitors. Do you want to address the macOS side in a separate PR? |
Thanks for the offer. I'm also running 15.3.2. I suspect I'll have to try test on an older version of the OS to avoid all those unexpected screen change notifications.
Maybe? It's clear that someone needs to call The alternative is to update the Mac glass code to more closely match what Windows and Linux are doing. In that case it should be a separate PR. But it's beginning to look like notifyPaint is sort of vestigial. Outside of this bug the Mac is just fine not calling it because the core handles everything internally. From my newbie perspective it looks like this is just another area where the core code should be calling |
|
I spotted this while working on Linux glass. The test case I was doing was "showing" an iconified fullscreened stage. Linux glass has a repaint call, but it seems wrong. |
|
Hmm, it also relates to something else I was intrigued by: What happens if the Stage is both Maximized and Iconified? If From the Stage docs:
Update: Just a note about another possibly related bug involving the RESTORED logic not matching the documentation. |
|
About the comment on the change: After some experiments, I’d say the size can change because you can call Anyways, this is just a comment for a case to consider, but the change does make the scene repaint in this case (on Linux, removing the call of repaint on glass native code and some additional changes I'm working on). |
|
@hjohn Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add @beldenfox |
|
@hjohn |
|
I have noticed the tests for maximized stages fail on my system: This is on Windows 11, the Stage in fact does not change its background. I'm not sure what the problem could be, or if it's fixable. |
That's expected. This PR fixes the case where the window goes from iconified to restored. When looking at the code I realized the same fix needs to be applied when the window goes from iconified to maximized so I added that to the test. |
Do I need to add additional code for that case? The fix is now in Edit: I see that iconified state is explicitly set to |
At least on Windows you can go straight from an iconified state to a maximized state and the only event you'll see is WindowEvent.MAXIMIZE. That should be true on the Mac as well but there are bugs in the way glass handles maximized windows so I'm not entirely sure what the event sequence is. |
lukostyra
left a comment
lukostyra
left a comment
There was a problem hiding this comment.
Looks good, tests on Windows pass as they used to.
|
@hjohn you might want to add JDK-8146479 to the PR via |
|
Looks good on macOS 15.5 M1. /reviewers 3 |
| } | ||
| return null; | ||
| }); | ||
| forceRepaint(); |
There was a problem hiding this comment.
(github diff is confusing, this code is for WindowEvent.RESCALE case)
I suspect this might also fix an issue that I encountered in one of my applications which uses Canvas-based text editor. After the macOS is woken up after a sleep with an external monitor attached, the text on retina looked blurred. I could not get a reproducer in time, but I think it was because while I put it to sleep, the scene got re-rendered on the external monitor going from scale 2 to scale 1, but when woken up, the scene was correctly re-rendered for retina but the canvas was not resized, requiring it to be scaled.
I'll try to test this scenario, but it may take a few days.
There was a problem hiding this comment.
Sure, no hurry to integrate this
There was a problem hiding this comment.
actually, the reason I bumped the reviewer count is to let you add the issue and make sure we get the testing results for linux (which we got) and our CI run (which we did).
I may not be able to test my failing scenario before you integrate anyway. We are good to go.
|
@andy-goryachev-oracle |
|
I am going to launch a headful CI run, will report in about an hour. edit: @lukostyra already started one, thanks! edit2: headful CI run passes (with one unrelated failure with QPathTest on mac-13-aarch64 that we occasionally see). |
|
Tested on Ubuntu 22.04. Test failed in master branch and succeeded with this PR. |
|
/issue add JDK-8146479 |
|
@hjohn |
|
/reviewers 2 |
|
@andy-goryachev-oracle |
|
/integrate |
|
Going to push as commit 0270847.
Your commit was automatically rebased without conflicts. |
|
thank you @hjohn for fixing this bug! |
7 participants
Adds code to trigger a scene update when a Window is restored
This seems to solve https://bugs.openjdk.org/browse/JDK-8351867 and https://bugs.openjdk.org/browse/JDK-8146479
Progress
Issues
Reviewers
Contributors
<mfox@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1733/head:pull/1733$ git checkout pull/1733Update a local copy of the PR:
$ git checkout pull/1733$ git pull https://git.openjdk.org/jfx.git pull/1733/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1733View PR using the GUI difftool:
$ git pr show -t 1733Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1733.diff
Using Webrev
Link to Webrev Comment