8154846: SwingNode does not resize when content size constraints are changed #15960
Conversation
|
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
/label add fx |
|
@prsadhuk
|
|
Fix is updated to register listener before component is added/removed. @andy-goryachev-oracle @aghaisas Can you please review this FX fix? There is some SwingNode rendering issue still due to JDK-8298796 but size update issue should be fixed.. |
|
@prsadhuk |
Not sure on this..The failure says which has nothing to do with the change.. |
|
could you restart the failing build? Let's make sure it's a transient build issue. |
|
While running the SCCE in the ticket with the jdk compiled with the fix from pull/15960 on macOS 13.5.2 M1, noticed two things:
|
|
I think OOME is expected. The earlier stack trace was for retina (scale=2), when displaying on a secondary monitor with the scale=1 I am getting OOME later: ~48M * 4 bytes (I assume) = 193Mbytes. |
|
What puzzles me is that I am getting inconsistent results. Of course, I might be doing something wrong. My command line:
|
Button appearing at the corner issue existed before the fix also as I see in this screenshot... (SInce the size was not updated before the fix, the label was truncated but it is moved to the corner as you can see)
disappear and reappear issue seems to be due to JDK-8298796 SwingNode rendering issue as I mentioned earlier in this PR...so I think we can ignore the unsolved rendering issue for now and see it as size update fix (although it may alleviate rendering issue to the user but maybe one fix at a time) I normally test with this command where run.args is |
|
Pre-submit tests - linux-x64 / test - Test (tier1) Failing after 80m — 1/10 failed |
|
With the latest change, it behaves as expected on the primary retina screen. However, something is wrong if I put it on the secondary monitor right after the launch: I am still getting inconsistent behavior, incorrect button location, and visual artifacts. For example, one time there is no button after the first click. The button appears when I resize the window. Another time, the button is in the corner again and some visual artifacts are present (see the screenshots below). Perhaps we need to update the scale somewhere?
|
|
just in case, my monitor setup: 
|
I am not able to reproduce these issues (apart from JDK-8298796) in windows primary and secondary monitor (primary 1.25 1920x1080, secondary 1.0 1920x1080, doesn't have FX setup in mac to test ...Also, I dont see how window can be painted red as the test doesn't do that so how did you get RED background) I think you are seeing the scale issue what you already mentioned in here |
|
let me test this on windows. in the mean time, do you know why the checks are consistently failing for this PR? |
No, this failure is in hotspot and it's ok to ignore this....nothing to do with the change in this PR |
These actions seems to fail one or the other which does not seem to affect integration...for ex. #16068 another action fail....many of earlier PRs also has one or more failure in these actions but never fail build/test after integration.. |
|
@prsadhuk could you please point me to a pre-built jdk with your fix? I am getting build errors - maybe because of git's autocrlf, but it's going to take too much time for me trying to resolve the issue. If I could download something already built it would be much easier. thanks! |
There was a problem hiding this comment.
So basically the code is out of order. A container listener is added which installs the property listeners but its all too late.
The content was already added so the event is missed. The fix is simply to move the block that adds the content to
after the listener is installed. Seems OK to me, but @kevinrushforth should test this.
|
/reviewers 2 |
|
@prsadhuk 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 401 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 |
I have DM-ed you the pre-build jdk path |
|
so tested on windows 11 with your pre-built jdk and the latest fx master branch. What I see is some kind of behavior that happens when the code is missing repaint() and/or validate(). When the window comes up first, it looks like this - with the button not painted (a solid black block):
a mouse over repaints the button (possibly due to hover?), or window resizing repaints the button. When I then click on the button, I see either 1/4 of the larger button where it is supposed to be, or at the corner, like so: resizing a window slightly places the button in the right spot and removes all the artifacts. All this happens on the primary screen with a 100% scale. |
Yes, known issue...please refer JDK-8298796 |
|
Testing this on a secondary screen with a 175% scale: the button is painted, works right most of the time, but occasionally the button is not placed at the center, or is 1/4 painted, or disappears completely (until I resize the window). So, basically, it exhibits the same symptoms as on the primary screen. It is very inconsistent, the results vary between launches.
|
|
|
|
just for fun, added after button.setSize(buttonSize); with not much effect. Which makes me think the problem might be in the way the swing damage gets propagated to fx maybe? |
As already mentioned above, this are known issues and not caused by this PR.. |
|
yes; i can see how JDK-8298796 might affect the initial state, but the issue seems to go beyond initial state. updating the preferred size is not working reliably - so perhaps it is JDK-8298796 or perhaps it is some other issue? |
|
/integrate |
|
Going to push as commit 37eb986.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
SwingNode does not update its internal cache of Swing pref/max/min height and widths when its JComponent content's corresponding size constraints are updated. As such, it isn't resized to honor those size constraints.
JLightweightFrame does install a PropertyChangeListener for "preferredSize", "maximumSize", and "minimumSize" properties, but this only happens via a ContainerListener which is not added until after the content has already been added to the content pane, and since the application cannot call this methods directly as per the documentation for the SwingNode.resize() method:
Applications should not invoke this method directly. If an application needs to directly set the size of the SwingNode, it should set the Swing component's minimum/preferred/maximum size constraints which will be propagated correspondingly to the SwingNode and it's parent will honor those settings during layout.so the fix is to add the listener as soon as the component is added to the JLightweightFrame's content.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15960/head:pull/15960$ git checkout pull/15960Update a local copy of the PR:
$ git checkout pull/15960$ git pull https://git.openjdk.org/jdk.git pull/15960/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15960View PR using the GUI difftool:
$ git pr show -t 15960Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15960.diff
Webrev
Link to Webrev Comment