Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v2]
> This fixes two bugs appointed on the JBS issue: > > 1) Sometimes window was moving to the top left corner - seems to be a bug > somewhere in `gdk_window_get_origin` when used before map (a X concept when > the window appears). The change is to ignore the configure events (happens > when location or size changes) until window is mapped. Before map java is > notified in the `set_bounds` function. > > This seems to happen on newer versions of linux distros. > > 2) Specific to KDE, in the case of the example provided, when an MODAL window > pops, it calls `set_enabled` `false` on the child (or all other windows if > APPLICATION_MODAL) which causes it to update the window constraints. When > maximized, the constraints where applied anyways, causing the window to still > be maximized but not show as maximized. The change is to not apply > constraints when not floating (meaning floating on the screen - not > maximized, fullscreen or iconified). Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Should still report location - Changes: - all: https://git.openjdk.org/jfx/pull/1460/files - new: https://git.openjdk.org/jfx/pull/1460/files/b3c7f407..7f6221a2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1460&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1460&range=00-01 Stats: 18 lines in 1 file changed: 5 ins; 7 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1460.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1460/head:pull/1460 PR: https://git.openjdk.org/jfx/pull/1460
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v2]
On Thu, 23 May 2024 10:29:33 GMT, Thiago Milczarek Sayao wrote: >> This fixes two bugs appointed on the JBS issue: >> >> 1) Sometimes window was moving to the top left corner - seems to be a bug >> somewhere in `gdk_window_get_origin` when used before map (a X concept when >> the window appears). The change is to ignore the configure events (happens >> when location or size changes) until window is mapped. Before map java is >> notified in the `set_bounds` function. >> >> This seems to happen on newer versions of linux distros. >> >> 2) Specific to KDE, in the case of the example provided, when an MODAL >> window pops, it calls `set_enabled` `false` on the child (or all other >> windows if APPLICATION_MODAL) which causes it to update the window >> constraints. When maximized, the constraints where applied anyways, causing >> the window to still be maximized but not show as maximized. The change is to >> not apply constraints when not floating (meaning floating on the screen - >> not maximized, fullscreen or iconified). > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Should still report location I think the mouse behavior is expected, since there is a pointer grab on popups. But the location was wrong. It's fixed now. - PR Comment: https://git.openjdk.org/jfx/pull/1460#issuecomment-2126771948
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v2]
On Thu, 23 May 2024 10:29:33 GMT, Thiago Milczarek Sayao wrote: >> This fixes two bugs appointed on the JBS issue: >> >> 1) Sometimes window was moving to the top left corner - seems to be a bug >> somewhere in `gdk_window_get_origin` when used before map (a X concept when >> the window appears). The change is to ignore the configure events (happens >> when location or size changes) until window is mapped. Before map java is >> notified in the `set_bounds` function. >> >> This seems to happen on newer versions of linux distros. >> >> 2) Specific to KDE, in the case of the example provided, when an MODAL >> window pops, it calls `set_enabled` `false` on the child (or all other >> windows if APPLICATION_MODAL) which causes it to update the window >> constraints. When maximized, the constraints where applied anyways, causing >> the window to still be maximized but not show as maximized. The change is to >> not apply constraints when not floating (meaning floating on the screen - >> not maximized, fullscreen or iconified). > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Should still report location The linux build seems to be failing because the lack of gcc-13 package. - PR Comment: https://git.openjdk.org/jfx/pull/1460#issuecomment-2126782606
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v3]
> This fixes two bugs appointed on the JBS issue: > > 1) Sometimes window was moving to the top left corner - seems to be a bug > somewhere in `gdk_window_get_origin` when used before map (a X concept when > the window appears). The change is to ignore the configure events (happens > when location or size changes) until window is mapped. Before map java is > notified in the `set_bounds` function. > > This seems to happen on newer versions of linux distros. > > 2) Specific to KDE, in the case of the example provided, when an MODAL window > pops, it calls `set_enabled` `false` on the child (or all other windows if > APPLICATION_MODAL) which causes it to update the window constraints. When > maximized, the constraints where applied anyways, causing the window to still > be maximized but not show as maximized. The change is to not apply > constraints when not floating (meaning floating on the screen - not > maximized, fullscreen or iconified). Thiago Milczarek Sayao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'refs/heads/master' into 833 - Should still report location - Fix - Teste - Teste - Teste - Fix 833 - Changes: - all: https://git.openjdk.org/jfx/pull/1460/files - new: https://git.openjdk.org/jfx/pull/1460/files/7f6221a2..bb9a90b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1460&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1460&range=01-02 Stats: 55624 lines in 196 files changed: 29519 ins; 14601 del; 11504 mod Patch: https://git.openjdk.org/jfx/pull/1460.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1460/head:pull/1460 PR: https://git.openjdk.org/jfx/pull/1460
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v2]
On Thu, 23 May 2024 10:37:09 GMT, Thiago Milczarek Sayao wrote: >> Thiago Milczarek Sayao has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Should still report location > > The linux build seems to be failing because the lack of gcc-13 package. @tsayao the Ubuntu runner uses now 24.0.4. Your branch just needs to be synced with head. - PR Comment: https://git.openjdk.org/jfx/pull/1460#issuecomment-2126802774
Toolkit Window Decorations
Hi, At some point we will need "Client Side" decorations on JavaFX to support modern gnome desktop on Linux as they moved to have client side decorations on everything. Mutter (the gnome compositor) does not even support "server side" decorations. On Wayland it's an extension for KDE only. Currently, even on Xorg it's a bit hacky when JavaFX needs to set total window size (with decorations). It caused too many problems. Having controls on the titlebar is a nice feature to have, as seen on many applications (tabs, hamburger menus, buttons). I'm not exactly sure on how it should be done. Maybe a Scene property that enables its contents to be wrapped in a decoration control? Would it break applications that walk through the scene graph? Should it look like Modena and have a JavaFX "identity", or should it try to look like the platform it's in? On Wayland, it would be possible to use a subcompositor to place the decoration behind, but I think it would be better if the decoration was part of the Scene. It would be less problematic on the variety of compositors (Mutter, Kwin, Sway, etc). -- Thiago
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v2]
On Thu, 23 May 2024 10:48:08 GMT, Jose Pereda wrote: >> The linux build seems to be failing because the lack of gcc-13 package. > > @tsayao the Ubuntu runner uses now 24.0.4. Your branch just needs to be > synced with head. @jperedadnr Thanks. @karthikpandelu I tested the sample with the Debian 12 VM and it seems to work now. Would you re-test? - PR Comment: https://git.openjdk.org/jfx/pull/1460#issuecomment-2126955548
Re: RFR: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
On Fri, 8 Mar 2024 16:39:56 GMT, Marius Hanl wrote:
>> This seems like a reasonable fix and spec change. Have you tested the case
>> of calling sizeToScene before setting full-screen or maximzed? Since the
>> pending flag will still be set in that case, I want to make sure that case
>> is tested as well.
>>
>> Also, if this fixed
>> [JDK-8316425](https://bugs.openjdk.org/browse/JDK-8316425), then that bug
>> should be closed as a duplicate of this one.
>>
>> @lukostyra @arapte can you also review this?
>
>> This seems like a reasonable fix and spec change. Have you tested the case
>> of calling sizeToScene before setting full-screen or maximzed? Since the
>> pending flag will still be set in that case, I want to make sure that case
>> is tested as well.
>
> I thought about this as well but could not find any problem at least on
> Windows.
> If we want to be perfectly safe, we may still want to set the flag when
> `sizeToScene()` is called. What do you think?
>
> I used the following code to test this, and it didn't matter when
> `sizeToScene()` was called:
>
>
>@Override
> public void start(Stage primaryStage) throws Exception {
> StackPane root = new StackPane();
> Button wss = new Button("Wss");
> wss.setPrefSize(50, 50);
> root.getChildren().add(wss);
>
> Scene scene = new Scene(root);
>
> Stage stage = new Stage();
> stage.setWidth(500);
> stage.setMaximized(true);
> stage.sizeToScene();
> stage.setScene(scene);
> stage.show();
> }
> @Maran23 I think this is pretty close to being ready to go in. At a minimum,
> you will need to merge master and then fix the test so that it will compile,
> and then create a CSR with the updated specification (I can help with that if
> needed). My only other suggestion was around additional tests that might be
> useful, but they could be done as a follow-on fix.
Yes, sure, I've just been very busy with my day job over the last few weeks. I
hopefully have more time now though :)
And I totally agree with writing more tests, always good to have and to ensure
quality. So no need for a follow-on ticket.
-
PR Comment: https://git.openjdk.org/jfx/pull/1382#issuecomment-2127013006
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen [v8]
> This PR fixes the dialog freeze problem once and for all. > > This one is a bit tricky to understand, here is how it works: > This bug happens on every platform, although the implementation of nested > event loops differs on every platform. > E.g. on Linux we use `gtk_main` and `gtk_main_quit`, on Windows and Mac we > have an own implementation of a nested event loop (while loop), controlled by > a boolean flag. > > Funny enough, the reason why this bug happens is always the same: Timing. > > 1. When we hide a dialog, `_leaveNestedEventLoop` is called. > 2. This will call native code to get out of the nested event loop, e.g. on > Windows we try to break out of the while loop with a boolean flag, on Linux > we call `gtk_main_quit`. > 3. Now, if we immediately open a new dialog, we enter a new nested event loop > via `_enterNestedEventLoop`, as a consequence we do not break out of the > while loop on Windows (the flag is set back again, the while loop is still > running), and we do not return from `gtk_main` on Linux. > 4. And this will result in the Java code never returning and calling > `notifyLeftNestedEventLoop`, which we need to recover the UI. > > So it is actually not trivial to fix this problem, and we can not really do > anything on the Java side. We may can try to wait until one more frame has > run so that things will hopefully be right, but that sounds rather hacky. > > I therefore analyzed, if we even need to return from `_enterNestedEventLoop`. > Turns out, we don't need to. > There is a return value which we actually do not use (it does not have any > meaning to us, other that it is used inside an assert statement). > ~Without the need of a return value, we also do not need to care when > `_enterNestedEventLoop` is returning - instead we cleanup and call > `notifyLeftNestedEventLoop` in `_leaveNestedEventLoop`, after the native code > was called.~ > See below for more information. To recover the UI (and in general nested > nested event loops ;) we need to set a flag for the `InvokeLaterDispatcher`. > > Lets see if this is the right approach (for all platforms). > Testing appreciated. > # > - [x] Tested on Windows > - [x] Tested on Linux > - [x] Tested on Mac > - [ ] Tested on iOS (although the nested event loop code is the same as for > Mac) (I would appreciate if someone can do this as I have no access to an iOS > device) > - [x] Adjust copyright > - [x] Write Systemtest > - [x] Document the new behaviour - in general, there should be more > information what to expect Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8285893-dialog-freezing-🥶 - Integrate changes as outline by beldenfox - test for multiple nested event loops - try leave outer loop after finishing inner loop - update copyright - trigger an outer nested event loop leave attempt - do not attempt to leave eventloop after leaving another eventloop - Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8285893-dialog-freezing-🥶 - Merge remote-tracking branch 'openjfx/master' into JDK-8285893-dialog-freezing-🥶 - JDK-8285893: Decrement nestedEventLoopCounter in leaveNestedEventLoop - ... and 1 more: https://git.openjdk.org/jfx/compare/ea0d1a15...ec43f2e3 - Changes: - all: https://git.openjdk.org/jfx/pull/1324/files - new: https://git.openjdk.org/jfx/pull/1324/files/bba7fc23..ec43f2e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=07 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=06-07 Stats: 86781 lines in 1031 files changed: 46446 ins; 20952 del; 19383 mod Patch: https://git.openjdk.org/jfx/pull/1324.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1324/head:pull/1324 PR: https://git.openjdk.org/jfx/pull/1324
Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling [v2]
> This PR fixes the border glitch/gap as explained in both linked tickets. > > I noted that the `ScrollPane` control does not suffer from this problem, > although the code is mostly the same as in `VirtualFlow`. The `ScrollPane` > snaps the content correctly, no matter which scale. I carefully checked the > code and it seems that the container structure in `ScrollPane` was explicitly > written to handle this perfectly. There was definitely some thought on that. > > So to finally fix this issue, I rewrote the `VirtualFlow` container/scrolling > code to be **exactly** the same as in `ScrollPane`(Skin). > And this also fixes the issue, while behaving the same as before. > > In the future it may makes sense to try to somewhat unify the code from > `ScrollPane` and `VirtualFlow`. I already have some ideas. > > Unfortunately though, one more container is introduced to `VirtualFlow`, so > the css needs to be changed since it is very strictly written in the first > place: > Before: `.list-view:focused > .virtual-flow > .clipped-container > .sheet > > .list-cell` > After: `.list-view:focused > .virtual-flow > .viewport > .clipped-container > > .sheet > .list-cell` > > To better understand this, the container structure (tree) is shown below: > > - ScrollPane > - viewRect -> `setClip` -> clipRect (viewContent size) > - viewContent -> `setLayoutX` > - Content > - vsb > - hsb > - corner > > --- > - VirtualFlow > - viewRect **(->NEW IN THIS PR<-)** -> `setClip` -> clipRect > (clippedContainer size) > - clippedContainer/clipView -> `setLayoutX` (when scrolling) > - sheet > - Cell > - vsb > - hsb > - corner Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' of https://github.com/openjdk/jfx into 8218745-snapping-x-y-tableview-scroll-3 # Conflicts: # modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java # modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java - JDK-8218745: TableView: visual glitch at borders on horizontal scrolling - Changes: https://git.openjdk.org/jfx/pull/1330/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1330&range=01 Stats: 306 lines in 14 files changed: 122 ins; 41 del; 143 mod Patch: https://git.openjdk.org/jfx/pull/1330.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1330/head:pull/1330 PR: https://git.openjdk.org/jfx/pull/1330
Re: RFR: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window [v3]
On Tue, 30 Apr 2024 23:20:59 GMT, Kevin Rushforth wrote:
> * I wanted to verify different orders of operation, so I wrote a (manual)
> test program
I'm retesting and writing tests right now and reproduced one usecase out of my
head that indeed 'fails' now.
Take the following code:
Button button = new Button();
button.setMinSize(440, 440);
Scene scene = new Scene(button);
stage.setTitle("JavaFX test stage!");
stage.setScene(scene);
stage.setWidth(50);
stage.setHeight(50);
stage.setFullScreen(true);
stage.sizeToScene();
stage.setFullScreen(false);
stage.show();
With my logic, the `sizeToScene()` flag is not remembered, so the scene is not
adjusted in the `sizeToScene` style after I 'go out' of fullscreen mode.
If I do instead:
stage.sizeToScene();
stage.setFullScreen(true);
stage.setFullScreen(false);
The flag is remembered and the scene has the size of the button. Not sure what
the expectation is here, but we could fix this problem by still remembering the
flag if called.
-
PR Comment: https://git.openjdk.org/jfx/pull/1382#issuecomment-2127160594
Re: RFR: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window [v4]
> This PR fixes the problem that maximizing/fullscreen a `Stage` or `Dialog` is > broken when `sizeToScene()` was called before or after. > > The approach here is to ignore the `sizeToScene()` request when the `Stage` > is maximized or set to fullscreen. > Otherwise the Window Manager of the OS will be confused and you will get > weird flickering or wrong Window buttons (e.g. on Windows, the 'Maximize' > button still shows the 'Restore' Icon, while we already resized the `Stage` > to not be maximized). Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Implement isSizeToSceneAllowed() method to determines whether the sizeToScene() request is allowed. Implement much more tests - Merge branch 'master' of https://github.com/openjdk/jfx into jdk-8326619-maximize-minimize-scene - improve tests - JDK-8326619: Improve tests - JDK-8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window - Changes: - all: https://git.openjdk.org/jfx/pull/1382/files - new: https://git.openjdk.org/jfx/pull/1382/files/e58a2fda..eeab0fa8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1382&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1382&range=02-03 Stats: 88862 lines in 1197 files changed: 47740 ins; 21303 del; 19819 mod Patch: https://git.openjdk.org/jfx/pull/1382.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1382/head:pull/1382 PR: https://git.openjdk.org/jfx/pull/1382
Re: prism.maxvram limitation
Hi all, Any ideas or suggestions? Thanks. Eduard Am 14. Mai 2024, 15:45, um 15:45, Eduard Sedov schrieb: >Hi all, > >There is a long-lived bug in JavaFX: JDK-8089112: Need to handle the >case of a failed texture load when rendering large images > >JavaFX manages a pool of resources that is limited to 512Mbytes by >default. This limit can be increased by setting the 'prism.maxvram' >system property. > >This limit is reasonable for the GraphicsPipelines (the D3DPipeline and >the ES2Pipeline) that are backed by a graphics device that has such a >limitation. > >But it does not make sense for pipelines that use only the main memory: >the J2DPipeline and perhaps the SWPipeline. > >The J2DPipeline is currently used for printing. For example, printing >an image on Letter paper using 600PPI printer requires 134_640_000 >bytes. When the printed image is redirected to a PDF printer, even >higher resolution is needed to get adequate quality because the PDF can >zoom in. This often exceeds the limitation and ends in a >NullPointerException if the allocated textures exceed the specified >maxvram value. > >There is no way to specify different values for each pipeline or to >remove the limitation for software pipelines. > >I would add this possibility? What do you think? > >Thanks, >Eduard > >
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen
On Tue, 14 May 2024 18:42:47 GMT, Martin Fox wrote: > On Linux leaveNestedEventLoop makes changes in glass that cannot be erased or > undone (it calls gtk_main_quit which updates internal GTK state that we don’t > have access to). In the end GTK will exit the loops in the correct order but > it will exit loop A before the core code has a chance to call > Application.leaveNestedEventLoop again. This is throwing off the bookkeeping > including some debug checks. That is exactly what I also figured out. If a second new nested event loop is started directly after the first one was requested to leave, the native code will never 'return'. That's why this code exists: https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/glass/ui/EventLoop.java#L118 which I slightly modified to not run `invokeLater` in my PR. Otherwise it would never return any value and code after `enterNestedEventLoop` is never called: Object o = Platform.enterNestedEventLoop(this); System.out.println(o); // <- never called And since you fixed the event jam so that we are not stuck anymore and Linux is implemented different with a global stateful variable, we will get that 'inconsistency', which I don't think is relevant or a problem here, hence I removed it in my PR. I also don't found any other way to deal with that situation, as you also stated, there is nothing else we can do, and while the other OS implementation have some kind of while loop where we can try to implement something, on the Linux side we completely rely on `gtk_main` and `gtk_main_quit`. - PR Comment: https://git.openjdk.org/jfx/pull/1449#issuecomment-2127252021
Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v4]
> This PR fixes a long standing issue where the `Tooltip` will always wait one > second until it appears the very first time, even if the > `-fx-show-delay` was set to another value. > > The culprit is, that the `cssForced` flag is not inside `Tooltip`, but inside > the `TooltipBehaviour`. So the very first `Tooltip` gets processed correctly, > but after no `Tooltip` will be processed by CSS before showing, resulting in > the set `-fx-show-delay` to not be applied immediately. > > Added a bunch of headful tests for the behaviour since there were none before. Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Implement applyStylesheetFromOwner(..) and use it instead to ensure correct CSS processing for the Tooltip Node. - Merge branch 'master' of https://github.com/openjdk/jfx into 8296387-tooltip-css - Allow Tooltip to process the owner styles first so that also global stylesheets are considered for the e.g. tooltip show-delay - JDK-8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed - Changes: - all: https://git.openjdk.org/jfx/pull/1394/files - new: https://git.openjdk.org/jfx/pull/1394/files/093b36f6..5af893c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1394&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1394&range=02-03 Stats: 88104 lines in 1155 files changed: 47257 ins; 21123 del; 19724 mod Patch: https://git.openjdk.org/jfx/pull/1394.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1394/head:pull/1394 PR: https://git.openjdk.org/jfx/pull/1394
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen [v9]
> This PR fixes the dialog freeze problem once and for all. > > This one is a bit tricky to understand, here is how it works: > This bug happens on every platform, although the implementation of nested > event loops differs on every platform. > E.g. on Linux we use `gtk_main` and `gtk_main_quit`, on Windows and Mac we > have an own implementation of a nested event loop (while loop), controlled by > a boolean flag. > > Funny enough, the reason why this bug happens is always the same: Timing. > > 1. When we hide a dialog, `_leaveNestedEventLoop` is called. > 2. This will call native code to get out of the nested event loop, e.g. on > Windows we try to break out of the while loop with a boolean flag, on Linux > we call `gtk_main_quit`. > 3. Now, if we immediately open a new dialog, we enter a new nested event loop > via `_enterNestedEventLoop`, as a consequence we do not break out of the > while loop on Windows (the flag is set back again, the while loop is still > running), and we do not return from `gtk_main` on Linux. > 4. And this will result in the Java code never returning and calling > `notifyLeftNestedEventLoop`, which we need to recover the UI. > > So it is actually not trivial to fix this problem, and we can not really do > anything on the Java side. We may can try to wait until one more frame has > run so that things will hopefully be right, but that sounds rather hacky. > > I therefore analyzed, if we even need to return from `_enterNestedEventLoop`. > Turns out, we don't need to. > There is a return value which we actually do not use (it does not have any > meaning to us, other that it is used inside an assert statement). > ~Without the need of a return value, we also do not need to care when > `_enterNestedEventLoop` is returning - instead we cleanup and call > `notifyLeftNestedEventLoop` in `_leaveNestedEventLoop`, after the native code > was called.~ > See below for more information. To recover the UI (and in general nested > nested event loops ;) we need to set a flag for the `InvokeLaterDispatcher`. > > Lets see if this is the right approach (for all platforms). > Testing appreciated. > # > - [x] Tested on Windows > - [x] Tested on Linux > - [x] Tested on Mac > - [ ] Tested on iOS (although the nested event loop code is the same as for > Mac) (I would appreciate if someone can do this as I have no access to an iOS > device) > - [x] Adjust copyright > - [x] Write Systemtest > - [x] Document the new behaviour - in general, there should be more > information what to expect Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: Fix javadoc - Changes: - all: https://git.openjdk.org/jfx/pull/1324/files - new: https://git.openjdk.org/jfx/pull/1324/files/ec43f2e3..ede84ef1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=08 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1324&range=07-08 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1324.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1324/head:pull/1324 PR: https://git.openjdk.org/jfx/pull/1324
Re: RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 15:22:38 GMT, Kevin Rushforth wrote:
> [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405) added markdown
> support to javadoc, using `///` to indicate markdown documentation comments.
> As a result, building JavaFX docs with JDK 23 produces new warnings, which
> causes the build to fail (since we treat warnings as errors).
>
> This PR was generated by running a program to automate the modification of
> each line that contains three or more slash chars in a row, `///` (except
> those in String literals). The replacement algorithm is as follows:
>
> 1. Replace each occurrence of exactly 3 slash chars `///` with 2 slash chars
> `//`
> 2. Replace each occurrence of 4 or more slash chars with an equal number of
> `-` chars, leaving the first two `//` chars of the first run of `` as is.
>
> This is similar to the proposed fix in java.base for
> [JDK-8331879](https://bugs.openjdk.org/browse/JDK-8331879) / PR
> openjdk/jdk#19130 (except for how I propose to handle the case of exactly
> three slashes).
>
> As an alternative approach, I could fix just the 5 javadoc comments causing
> the error, and leave the rest alone, but this will prevent problems from
> cropping up in the future, and is in keeping with changes going into the JDK
> source base.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/Locator.java
line 352:
> 350: // Only one '/' after the ':'.
> 351: if (protocol.equals("file")) {
> 352: // Map "file:/somepath" to "file:///somepath"
I did this one by hand, adding the `"` chars so that the automated program
wouldn't touch the `///` in `file:///`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1461#discussion_r1611906488
RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
[JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405) added markdown support to javadoc, using `///` to indicate markdown documentation comments. As a result, building JavaFX docs with JDK 23 produces new warnings, which causes the build to fail (since we treat warnings as errors). This PR was generated by running a program to automate the modification of each line that contains three or more slash chars in a row, `///` (except those in String literals). The replacement algorithm is as follows: 1. Replace each occurrence of exactly 3 slash chars `///` with 2 slash chars `//` 2. Replace each occurrence of 4 or more slash chars with an equal number of `-` chars, leaving the first two `//` chars of the first run of `` as is. This is similar to the proposed fix in java.base for [JDK-8331879](https://bugs.openjdk.org/browse/JDK-8331879) / PR openjdk/jdk#19130 (except for how I propose to handle the case of exactly three slashes). As an alternative approach, I could fix just the 5 javadoc comments causing the error, and leave the rest alone, but this will prevent problems from cropping up in the future, and is in keeping with changes going into the JDK source base. - Commit messages: - Replace all occurrences of '///' Changes: https://git.openjdk.org/jfx/pull/1461/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1461&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332732 Stats: 1309 lines in 50 files changed: 0 ins; 0 del; 1309 mod Patch: https://git.openjdk.org/jfx/pull/1461.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1461/head:pull/1461 PR: https://git.openjdk.org/jfx/pull/1461
Re: RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 15:22:38 GMT, Kevin Rushforth wrote: > [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405) added markdown > support to javadoc, using `///` to indicate markdown documentation comments. > As a result, building JavaFX docs with JDK 23 produces new warnings, which > causes the build to fail (since we treat warnings as errors). > > This PR was generated by running a program to automate the modification of > each line that contains three or more slash chars in a row, `///` (except > those in String literals). The replacement algorithm is as follows: > > 1. Replace each occurrence of exactly 3 slash chars `///` with 2 slash chars > `//` > 2. Replace each occurrence of 4 or more slash chars with an equal number of > `-` chars, leaving the first two `//` chars of the first run of `` as is. > > This is similar to the proposed fix in java.base for > [JDK-8331879](https://bugs.openjdk.org/browse/JDK-8331879) / PR > openjdk/jdk#19130 (except for how I propose to handle the case of exactly > three slashes). > > As an alternative approach, I could fix just the 5 javadoc comments causing > the error, and leave the rest alone, but this will prevent problems from > cropping up in the future, and is in keeping with changes going into the JDK > source base. went through all 50 files, all is good. ship it! - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1461#pullrequestreview-2074364579
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Fri, 17 May 2024 19:22:39 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8319555.manual
> - Merge remote-tracking branch 'origin/master' into 8319555.manual
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - removed example
> - cleanup
> - whitespace
> - extends application
> - works
> - .
> - sources moved back
> - ... and 5 more: https://git.openjdk.org/jfx/compare/681030f4...e4c8d29a
I run manual tests on the command line like so:
java --module-path=$env:JFX_SDK --add-modules=javafx.controls
./tests/manual/events/PlatformPreferencesChangedTest.java
This doesn't work with the new utility class, since it is outside of the class
file hierarchy.
Short of using a build system, how would I run a manual test with this new
class?
-
PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127511624
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Thu, 23 May 2024 16:01:29 GMT, Michael Strauß wrote: > Short of using a build system, how would I run a manual test with this new > class? I am running from within Eclipse. Here is the command line (remove newlines) it launches the EmojiTest. I think the key is to add the location of compiled classes via `-classpath`: /Library/Java/JavaVirtualMachines/jdk-21.jdk/Contents/Home/bin/java -XX:+ShowCodeDetailsInExceptionMessages -Dfile.encoding=UTF-8 -Dstdout.encoding=UTF-8 -Dstderr.encoding=UTF-8 -p /Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.base/bin:/Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.graphics/bin:/Users/angorya/Projects/jfx-1/jfx/rt/modules/javafx.controls/bin -classpath /Users/angorya/Projects/jfx-1/jfx/rt/tests/manual/util/bin:/Users/angorya/Projects/jfx-1/jfx/rt/tests/manual/text/bin --add-reads javafx.base=java.management --add-reads javafx.base=jdk.management --add-exports javafx.base/com.sun.javafx.property=javafx.graphics --add-exports javafx.base/test.javafx.collections=javafx.graphics --add-exports javafx.base/test.util.memory=javafx.graphics --add-exports java.base/sun.security.util=javafx.graphics --add-reads javafx.base=java.management --add-reads javafx.base=jdk.management --add-exports javafx.graphics/test.com.sun.javafx.pgstub=javafx.controls --add-exports javafx.base/test.com.sun.javafx.binding=javafx.controls --add-exports javafx.base/test.util.memory=javafx.controls --add-exports javafx.base/test.javafx.collections=javafx.controls --add-exports javafx.base/com.sun.javafx.property=javafx.graphics --add-exports javafx.base/test.javafx.collections=javafx.graphics --add-exports javafx.base/test.util.memory=javafx.graphics --add-exports java.base/sun.security.util=javafx.graphics --add-reads javafx.base=java.management --add-reads javafx.base=jdk.management --add-modules=javafx.base,javafx.graphics,javafx.controls --add-opens javafx.controls/test.com.sun.javafx.scene.control.test=javafx.base --add-exports javafx.base/com.sun.javafx=ALL-UNNAMED -Djava.library.path=../../../build/sdk/lib EmojiTest (also, there might be some unnecessary -add-exports here as well) - PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127537453
Re: prism.maxvram limitation
I wonder why there's a software-defined limit in the first place, given that there doesn't seem to be a way for JavaFX to gracefully handle failed texture loads. You say that the limit is reasonable for GPUs, but why is this any different from system memory? GPUs come in all different kinds of sizes, why is 512 MB any more significant than 256 MB or 8 GB? It's an arbitrary value that might be too small for some systems, and too large for other systems. Right now, JavaFX simply bugs out when the limit is reached. Unless a graceful degradation capability is added, I suggest to remove the limit entirely and let JavaFX bug out when the actual hardware limits are reached. On Tue, May 14, 2024 at 4:14 PM Eduard Sedov wrote: > > Hi all, > > There is a long-lived bug in JavaFX: JDK-8089112: Need to handle the case of > a failed texture load when rendering large images > > JavaFX manages a pool of resources that is limited to 512Mbytes by default. > This limit can be increased by setting the 'prism.maxvram' system property. > > This limit is reasonable for the GraphicsPipelines (the D3DPipeline and the > ES2Pipeline) that are backed by a graphics device that has such a limitation. > > But it does not make sense for pipelines that use only the main memory: the > J2DPipeline and perhaps the SWPipeline. > > The J2DPipeline is currently used for printing. For example, printing an > image on Letter paper using 600PPI printer requires 134_640_000 bytes. When > the printed image is redirected to a PDF printer, even higher resolution is > needed to get adequate quality because the PDF can zoom in. This often > exceeds the limitation and ends in a NullPointerException if the allocated > textures exceed the specified maxvram value. > > There is no way to specify different values for each pipeline or to remove > the limitation for software pipelines. > > I would add this possibility? What do you think? > > Thanks, > Eduard > >
Re: RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 15:22:38 GMT, Kevin Rushforth wrote: > [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405) added markdown > support to javadoc, using `///` to indicate markdown documentation comments. > As a result, building JavaFX docs with JDK 23 produces new warnings, which > causes the build to fail (since we treat warnings as errors). > > This PR was generated by running a program to automate the modification of > each line that contains three or more slash chars in a row, `///` (except > those in String literals). The replacement algorithm is as follows: > > 1. Replace each occurrence of exactly 3 slash chars `///` with 2 slash chars > `//` > 2. Replace each occurrence of 4 or more slash chars with an equal number of > `-` chars, leaving the first two `//` chars of the first run of `` as is. > > This is similar to the proposed fix in java.base for > [JDK-8331879](https://bugs.openjdk.org/browse/JDK-8331879) / PR > openjdk/jdk#19130 (except for how I propose to handle the case of exactly > three slashes). > > As an alternative approach, I could fix just the 5 javadoc comments causing > the error, and leave the rest alone, but this will prevent problems from > cropping up in the future, and is in keeping with changes going into the JDK > source base. Are these all the places where javadoc fails, or more files are coming separately? For example, see AccordionBehavior:151, ChoiceBoxSkin:327 - PR Comment: https://git.openjdk.org/jfx/pull/1461#issuecomment-2127565321
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Thu, 23 May 2024 16:11:42 GMT, Andy Goryachev wrote: > > Short of using a build system, how would I run a manual test with this new > > class? > > I am running from within Eclipse. Here is the command line (remove newlines) > it launches the EmojiTest. I think the key is to add the location of compiled > classes via `-classpath`: Yes this will work, but it requires me to compile `ManualTestWindow.java` before. That's quite a lot of heavy-lifting without tooling support (like you get from Eclipse). Maybe we should integrate the manual tests into the Gradle build, so that running a manual test might be as easy as invoking the application plugin's `:run` task for the manual test. - PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127576658
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Fri, 17 May 2024 19:22:39 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8319555.manual
> - Merge remote-tracking branch 'origin/master' into 8319555.manual
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - removed example
> - cleanup
> - whitespace
> - extends application
> - works
> - .
> - sources moved back
> - ... and 5 more: https://git.openjdk.org/jfx/compare/f8384dd1...e4c8d29a
But this is a good question: is there a better way to organize the tests to
make it easier to use common code?
-
PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127540895
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Fri, 17 May 2024 19:22:39 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8319555.manual
> - Merge remote-tracking branch 'origin/master' into 8319555.manual
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - removed example
> - cleanup
> - whitespace
> - extends application
> - works
> - .
> - sources moved back
> - ... and 5 more: https://git.openjdk.org/jfx/compare/1c6a743c...e4c8d29a
We need to provide a build script for any manual test that isn't self-contained
in a single directory (see my earlier comment). For example, we have a build
script for MonkeyTester and FXMediaPlayer.
As a separate step, we could make it available as part of the gradle build, as
long as we can do it without adding individual tests to build.gradle (sort of
like we do for apps), but the previous is the minimum.
This should probably be put on the back burner until the next test sprint.
-
PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127713126
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Fri, 17 May 2024 19:22:39 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8319555.manual
> - Merge remote-tracking branch 'origin/master' into 8319555.manual
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - removed example
> - cleanup
> - whitespace
> - extends application
> - works
> - .
> - sources moved back
> - ... and 5 more: https://git.openjdk.org/jfx/compare/399917f0...e4c8d29a
or possibly build the common test code into a jar as a part of the standard
build and include it along with `-Djava.library.path`?
-
PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2127727571
Re: RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 17:48:15 GMT, Kevin Rushforth wrote: > Those lines are modified by this PR You are right! I had cached java files in build/modulal-sdk/modules_src which Eclipse search picked up. Apparently, there is a way to mark the folder as "derived" so it will be ignored by the Search, but that hits an 18-year old Eclipse bug... https://bugs.eclipse.org/bugs/show_bug.cgi?id=154089 - PR Comment: https://git.openjdk.org/jfx/pull/1461#issuecomment-2127741418
Re: RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 16:21:06 GMT, Andy Goryachev wrote: > Are these all the places where javadoc fails, or more files are coming > separately? There are only 5 places where javadoc fails with the latest JDK 23. By eliminating all cases where we have more than three slashes, this PR fixes those 5 as well as avoids a pattern that could cause problems in the future. > For example, see AccordionBehavior:151, ChoiceBoxSkin:327 Not sure what you mean here. Those lines are modified by this PR such that they no longer have more than 2 slashes. - PR Comment: https://git.openjdk.org/jfx/pull/1461#issuecomment-2127729783
Re: RFR: 8332732: Clean up non-standard use of /// comments in JavaFX
On Thu, 23 May 2024 15:22:38 GMT, Kevin Rushforth wrote: > [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405) added markdown > support to javadoc, using `///` to indicate markdown documentation comments. > As a result, building JavaFX docs with JDK 23 produces new warnings, which > causes the build to fail (since we treat warnings as errors). > > This PR was generated by running a program to automate the modification of > each line that contains three or more slash chars in a row, `///` (except > those in String literals). The replacement algorithm is as follows: > > 1. Replace each occurrence of exactly 3 slash chars `///` with 2 slash chars > `//` > 2. Replace each occurrence of 4 or more slash chars with an equal number of > `-` chars, leaving the first two `//` chars of the first run of `` as is. > > This is similar to the proposed fix in java.base for > [JDK-8331879](https://bugs.openjdk.org/browse/JDK-8331879) / PR > openjdk/jdk#19130 (except for how I propose to handle the case of exactly > three slashes). > > As an alternative approach, I could fix just the 5 javadoc comments causing > the error, and leave the rest alone, but this will prevent problems from > cropping up in the future, and is in keeping with changes going into the JDK > source base. Thanks for checking. After this PR there should be no instances of `///` outside of String literals any `.java` file in the repo. - PR Comment: https://git.openjdk.org/jfx/pull/1461#issuecomment-2127752977
Re: RFR: 8319555: [TestBug] Utility for creating instruction window for manual tests [v9]
On Fri, 17 May 2024 19:22:39 GMT, Andy Goryachev wrote:
>> ## ManualTestWindow
>>
>> This facility provides the base class for manual tests which displays the
>> test instructions,
>> the UI under test, and the Pass/Fail buttons.
>>
>> Example:
>>
>>
>> public class ManualTestExample extends ManualTestWindow {
>> public ManualTestExample() {
>> super(
>> "Manual Test Example",
>> """
>> Instructions:
>> 1. you will see a button named "Test"
>> 2. press the button
>> 3. verify that the button can be pressed""",
>> 400, 250
>> );
>> }
>>
>> public static void main(String[] args) throws Exception {
>> launch(args);
>> }
>>
>> @Override
>> protected Node createContent() {
>> return new Button("Test");
>> }
>> }
>>
>>
>> Resulting application window:
>>
>> 
>>
>> Readme:
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8319555.manual/tests/manual/util/README.md
>>
>> @prrace 's test EmojiTest has been converted to use the new test window as a
>> demonstration (also fixed the Eclipse project so it works now).
>>
>> Q: What other features can be added to the test window?
>>
>> Edit: the sources are left in their original place at the root of the
>> project.
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 15 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8319555.manual
> - Merge remote-tracking branch 'origin/master' into 8319555.manual
> - Merge remote-tracking branch 'origin/8319555.manual' into 8319555.manual
> - removed example
> - cleanup
> - whitespace
> - extends application
> - works
> - .
> - sources moved back
> - ... and 5 more: https://git.openjdk.org/jfx/compare/c6b9c67e...e4c8d29a
moving to DRAFT until the next test sprint (after jfx23 ships).
-
PR Comment: https://git.openjdk.org/jfx/pull/1413#issuecomment-2128018657
Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v4]
On Thu, 23 May 2024 14:54:36 GMT, Marius Hanl wrote: >> This PR fixes a long standing issue where the `Tooltip` will always wait one >> second until it appears the very first time, even if the >> `-fx-show-delay` was set to another value. >> >> The culprit is, that the `cssForced` flag is not inside `Tooltip`, but >> inside the `TooltipBehaviour`. So the very first `Tooltip` gets processed >> correctly, but after no `Tooltip` will be processed by CSS before showing, >> resulting in the set `-fx-show-delay` to not be applied immediately. >> >> Added a bunch of headful tests for the behaviour since there were none >> before. > > Marius Hanl has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Implement applyStylesheetFromOwner(..) and use it instead to ensure > correct CSS processing for the Tooltip Node. > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8296387-tooltip-css > - Allow Tooltip to process the owner styles first so that also global > stylesheets are considered for the e.g. tooltip show-delay > - JDK-8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first > tooltip that is shown before it is displayed Strange: I still see the same issue described in https://github.com/openjdk/jfx/pull/1394#issuecomment-1986520680 (the very first tooltip after updating stylesheet uses the old value) - PR Comment: https://git.openjdk.org/jfx/pull/1394#issuecomment-2128039109
RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
Alternative PR to https://github.com/openjdk/jfx/pull/1330 which does not
modify the layout of `VirtualFlow`.
This PR fixes the glitching by removing the code in `NGNode.renderRectClip`,
which made many calculations leading to floating point errors.
Interestingly I found out, that `getClippedBounds(..)` is already returning the
correct bounds that just need to be intersected with the clip of the `Graphics`
object.
So the following code is effectively doing the same:
Old:
BaseBounds newClip = clipNode.getShape().getBounds();
if (!clipNode.getTransform().isIdentity()) {
newClip = clipNode.getTransform().transform(newClip, newClip);
}
final BaseTransform curXform = g.getTransformNoClone();
final Rectangle curClip = g.getClipRectNoClone();
newClip = curXform.transform(newClip, newClip); // <- The value of newClip
after the transform is what getClippedBounds(..) is returning
newClip.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
Rectangle clipRect = new Rectangle(newClip)
New:
BaseTransform curXform = g.getTransformNoClone();
BaseBounds clipBounds = getClippedBounds(new RectBounds(), curXform);
Rectangle clipRect = new Rectangle(clipBounds);
clipRect.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
As you can see, there are very similar, but `getClippedBounds` does a much
better job in calculating the bounds.
I also wrote a tests proving the bug. I took 100% of the setup and values from
a debugging session I did when reproducing this bug.
I checked several scenarios and code and could not find any regressions.
Still, since this is change affects all nodes with rectangular clips, we should
be careful.
Performance wise I could not spot any difference, I do not expect any
difference.
**So I would like to have at least 2 reviewers.**
Note that I will do more testing as well soon on all JavaFX applications I have
access to.
---
As written in the other PR, I have some interesting findings on this particular
problem.
Copy&Paste from the other PR:
--
Ok, so I found out the following:
When a Rectangle is used as clip without any effect or opacity modification,
the rendering goes another (probably faster) route with rendering the clip.
That's why setting the `opacity` to `0.99` fixes the issue - another route will
be used for the rendering.
This happens at the low level (`NGNode`) side of JavaFX.
...
I could track it down to be a typical floating point problem
...
The bug always appears when I scroll and the clip RectBounds are something like:
`RectBounds { minX:6.96, minY:37.0, maxX:289.3, maxY:194.0}
(w:282.3, h:157.0)`
...
while this does not happen when the value is something like:
`RectBounds { minX:7.04, minY:37.0, maxX:289.0, maxY:194.0} (w:282.0,
h:157.0`
Even more details:
---
- As briefly explained above, due to floating point arithmetic, we may do not
get the correct value, leading to an incorrect calculation where 1 pixel is
missing. That is why this issue happens only on display scales other than
integer values.
- And since only the `ClippedContainer` changes its `layoutX` AND the `layoutX`
of its clip, the bug only appears there
- JavaFX Nodes uses `double`, while the low level side (NG) uses `float`
mostly, which seems to make things less accurate, although not 100% sure if
doubles will avoid this particular problem completely, probably not.
I'm not sure why this decision was made.
-
Commit messages:
- 8218745: TableView: visual glitch at borders on horizontal scrolling
Changes: https://git.openjdk.org/jfx/pull/1462/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1462&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8218745
Stats: 255 lines in 11 files changed: 173 ins; 58 del; 24 mod
Patch: https://git.openjdk.org/jfx/pull/1462.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1462/head:pull/1462
PR: https://git.openjdk.org/jfx/pull/1462
Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
On Thu, 23 May 2024 21:51:55 GMT, Marius Hanl wrote:
> Alternative PR to https://github.com/openjdk/jfx/pull/1330 which does not
> modify the layout of `VirtualFlow`.
>
> This PR fixes the glitching by removing the code in `NGNode.renderRectClip`,
> which made many calculations leading to floating point errors.
> Interestingly I found out, that `getClippedBounds(..)` is already returning
> the correct bounds that just need to be intersected with the clip of the
> `Graphics` object.
>
> So the following code is effectively doing the same:
>
> Old:
>
> BaseBounds newClip = clipNode.getShape().getBounds();
> if (!clipNode.getTransform().isIdentity()) {
> newClip = clipNode.getTransform().transform(newClip, newClip);
> }
> final BaseTransform curXform = g.getTransformNoClone();
> final Rectangle curClip = g.getClipRectNoClone();
> newClip = curXform.transform(newClip, newClip); // <- The value of newClip
> after the transform is what getClippedBounds(..) is returning
> newClip.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
> Rectangle clipRect = new Rectangle(newClip)
>
>
> New:
>
> BaseTransform curXform = g.getTransformNoClone();
> BaseBounds clipBounds = getClippedBounds(new RectBounds(), curXform);
> Rectangle clipRect = new Rectangle(clipBounds);
> clipRect.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
>
>
> As you can see, there are very similar, but `getClippedBounds` does a much
> better job in calculating the bounds.
> I also wrote a tests proving the bug. I took 100% of the setup and values
> from a debugging session I did when reproducing this bug.
>
> I checked several scenarios and code and could not find any regressions.
> Still, since this is change affects all nodes with rectangular clips, we
> should be careful.
> Performance wise I could not spot any difference, I do not expect any
> difference.
> **So I would like to have at least 2 reviewers.**
> Note that I will do more testing as well soon on all JavaFX applications I
> have access to.
>
> ---
>
> As written in the other PR, I have some interesting findings on this
> particular problem.
>
> Copy&Paste from the other PR:
> --
>
> Ok, so I found out the following:
> When a Rectangle is used as clip without any effect or opacity modification,
> the rendering goes another (probably faster) route with rendering the clip.
> That's why setting the `opacity` to `0.99` fixes the issue - another route
> will be used for the rendering.
> This happens at the low level (`NGNode`) side of JavaFX.
> ...
> I could track it down to be a typical floating point problem
> ...
> The bug always appears when I scroll and the clip RectBounds are somethi...
I'm reasonably sure there was a good reason for the code in NGNode doing what
it did. This will need very careful review and testing before we would accept
it.
-
PR Comment: https://git.openjdk.org/jfx/pull/1462#issuecomment-2128092151
Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v5]
> This PR fixes a long standing issue where the `Tooltip` will always wait one > second until it appears the very first time, even if the > `-fx-show-delay` was set to another value. > > The culprit is, that the `cssForced` flag is not inside `Tooltip`, but inside > the `TooltipBehaviour`. So the very first `Tooltip` gets processed correctly, > but after no `Tooltip` will be processed by CSS before showing, resulting in > the set `-fx-show-delay` to not be applied immediately. > > Added a bunch of headful tests for the behaviour since there were none before. Marius Hanl has updated the pull request incrementally with one additional commit since the last revision: Add more documentation and improve css stylesheet test threshold - Changes: - all: https://git.openjdk.org/jfx/pull/1394/files - new: https://git.openjdk.org/jfx/pull/1394/files/5af893c1..a3f9f609 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1394&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1394&range=03-04 Stats: 18 lines in 4 files changed: 8 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1394.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1394/head:pull/1394 PR: https://git.openjdk.org/jfx/pull/1394
Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v4]
On Thu, 23 May 2024 14:54:36 GMT, Marius Hanl wrote: >> This PR fixes a long standing issue where the `Tooltip` will always wait one >> second until it appears the very first time, even if the >> `-fx-show-delay` was set to another value. >> >> The culprit is, that the `cssForced` flag is not inside `Tooltip`, but >> inside the `TooltipBehaviour`. So the very first `Tooltip` gets processed >> correctly, but after no `Tooltip` will be processed by CSS before showing, >> resulting in the set `-fx-show-delay` to not be applied immediately. >> >> Added a bunch of headful tests for the behaviour since there were none >> before. > > Marius Hanl has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Implement applyStylesheetFromOwner(..) and use it instead to ensure > correct CSS processing for the Tooltip Node. > - Merge branch 'master' of https://github.com/openjdk/jfx into > 8296387-tooltip-css > - Allow Tooltip to process the owner styles first so that also global > stylesheets are considered for the e.g. tooltip show-delay > - JDK-8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first > tooltip that is shown before it is displayed > Strange: I still see the same issue described in [#1394 > (comment)](https://github.com/openjdk/jfx/pull/1394#issuecomment-1986520680) > (the very first tooltip after updating stylesheet uses the old value) I don't know what is happening there, are you sure that there isn't a problem on your side? Testing `Tooltip` with a normal stylesheet added to the scene works perfectly fine for me. - PR Comment: https://git.openjdk.org/jfx/pull/1394#issuecomment-2128120302
Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
On Thu, 23 May 2024 21:51:55 GMT, Marius Hanl wrote:
> Alternative PR to https://github.com/openjdk/jfx/pull/1330 which does not
> modify the layout of `VirtualFlow`.
>
> This PR fixes the glitching by removing the code in `NGNode.renderRectClip`,
> which made many calculations leading to floating point errors.
> Interestingly I found out, that `getClippedBounds(..)` is already returning
> the correct bounds that just need to be intersected with the clip of the
> `Graphics` object.
>
> So the following code is effectively doing the same:
>
> Old:
>
> BaseBounds newClip = clipNode.getShape().getBounds();
> if (!clipNode.getTransform().isIdentity()) {
> newClip = clipNode.getTransform().transform(newClip, newClip);
> }
> final BaseTransform curXform = g.getTransformNoClone();
> final Rectangle curClip = g.getClipRectNoClone();
> newClip = curXform.transform(newClip, newClip); // <- The value of newClip
> after the transform is what getClippedBounds(..) is returning
> newClip.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
> Rectangle clipRect = new Rectangle(newClip)
>
>
> New:
>
> BaseTransform curXform = g.getTransformNoClone();
> BaseBounds clipBounds = getClippedBounds(new RectBounds(), curXform);
> Rectangle clipRect = new Rectangle(clipBounds);
> clipRect.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
>
>
> As you can see, there are very similar, but `getClippedBounds` does a much
> better job in calculating the bounds.
> I also wrote a tests proving the bug. I took 100% of the setup and values
> from a debugging session I did when reproducing this bug.
>
> I checked several scenarios and code and could not find any regressions.
> Still, since this is change affects all nodes with rectangular clips, we
> should be careful.
> Performance wise I could not spot any difference, I do not expect any
> difference.
> **So I would like to have at least 2 reviewers.**
> Note that I will do more testing as well soon on all JavaFX applications I
> have access to.
>
> ---
>
> As written in the other PR, I have some interesting findings on this
> particular problem.
>
> Copy&Paste from the other PR:
> --
>
> Ok, so I found out the following:
> When a Rectangle is used as clip without any effect or opacity modification,
> the rendering goes another (probably faster) route with rendering the clip.
> That's why setting the `opacity` to `0.99` fixes the issue - another route
> will be used for the rendering.
> This happens at the low level (`NGNode`) side of JavaFX.
> ...
> I could track it down to be a typical floating point problem
> ...
> The bug always appears when I scroll and the clip RectBounds are somethi...
Reviewers: @arapte @andy-goryachev-oracle
@Maran23 wait for either @arapte or me to review the proposed Prism changes in
this PR.
-
PR Comment: https://git.openjdk.org/jfx/pull/1462#issuecomment-2128133133
Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v5]
On Thu, 23 May 2024 22:17:36 GMT, Marius Hanl wrote: >> This PR fixes a long standing issue where the `Tooltip` will always wait one >> second until it appears the very first time, even if the >> `-fx-show-delay` was set to another value. >> >> The culprit is, that the `cssForced` flag is not inside `Tooltip`, but >> inside the `TooltipBehaviour`. So the very first `Tooltip` gets processed >> correctly, but after no `Tooltip` will be processed by CSS before showing, >> resulting in the set `-fx-show-delay` to not be applied immediately. >> >> Added a bunch of headful tests for the behaviour since there were none >> before. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Add more documentation and improve css stylesheet test threshold I am not sure what the difference might be. The code in the MonkeyTester generates a stylesheet, encodes it in base64 data url and adds it to all scenes in the application (it also removes the old version, if any). The stylesheet gets loaded - I can see it by slightly changed colors and the fact that the tooltip show delay gets changed on the second and any subsequent invocations. Can you try it? https://github.com/andy-goryachev-oracle/MonkeyTest - PR Comment: https://git.openjdk.org/jfx/pull/1394#issuecomment-2128136276
Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v5]
On Thu, 23 May 2024 22:17:36 GMT, Marius Hanl wrote: >> This PR fixes a long standing issue where the `Tooltip` will always wait one >> second until it appears the very first time, even if the >> `-fx-show-delay` was set to another value. >> >> The culprit is, that the `cssForced` flag is not inside `Tooltip`, but >> inside the `TooltipBehaviour`. So the very first `Tooltip` gets processed >> correctly, but after no `Tooltip` will be processed by CSS before showing, >> resulting in the set `-fx-show-delay` to not be applied immediately. >> >> Added a bunch of headful tests for the behaviour since there were none >> before. > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Add more documentation and improve css stylesheet test threshold with the latest monkey tester I see that updating the stylesheet does not update the showingDelay property immediately. When the tooltip gets shown, I see the following output from the change listener added to this property: showDelay=1000.0 ms showDelay=100.0 ms I suppose the second setting of 100ms (the value I actually set) happens too late or simply is ignored. - PR Comment: https://git.openjdk.org/jfx/pull/1394#issuecomment-2128142633
Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling
On Thu, 23 May 2024 22:02:41 GMT, Kevin Rushforth wrote: > I'm reasonably sure there was a good reason for the code in NGNode doing what > it did. This will need very careful review and testing before we would accept > it. 100% agree. Note that the code there is a shortcut for performance reasons. Removing it will also fix the bug since the code below is doing the right thing, but will probably result in a performance impact. Thats why I checked the other path and had a closer look what it does, since it still needs to the right for whatever clip is used. And there I saw that is does nearly the same thing, but with the `getClippedBounds` instead. With that in mind, we need to especially check if the fast path did something completely unexpected what the other 'slow' path did not and we may miss now. - PR Comment: https://git.openjdk.org/jfx/pull/1462#issuecomment-2128150585
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java line 99: > 97: /* > 98: * Copied from removed StyleClassSet to give StyleClasses a fixed > index when > 99: * first encountered. No longer needed once StyleClass is removed. Is this a possible idea for the future? - PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1612425514
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff Code looks good to me. As mentioned above, I tested it already and everything was good, so I'm certain that there is no regression here. I'm generally not a big fan of 'reimplementing' Collections, but I can see why it is needed here and it also makes sense, especially for something like CSS which needs to be fast. Something I saw as well in Swing (just check the `ArrayTable` class 😄 ). I wonder if we may want to add some tests for the `FixedCapacitySet`? - Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1316#pullrequestreview-2075265208
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff I'm in the process of testing this. I'll do that and report back. - PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2128170412
Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]
On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx wrote: >> Improves performance of selector matching in the CSS subsystem. This is done >> by using custom set implementation which are highly optimized for the most >> common cases where the number of selectors is small (most commonly 1 or 2). >> It also should be more memory efficient for medium sized and large >> applications which have many style names defined in various CSS files. >> >> Due to the optimization, the concept of `StyleClass`, which was only >> introduced to assign a fixed bit index for each unique style class name >> encountered, is no longer needed. This is because style classes are no >> longer stored in a `BitSet` which required a fixed index per encountered >> style class. >> >> The performance improvements are the result of several factors: >> - Less memory use when only very few style class names are used in selectors >> and styles from a large pool of potential styles (a `BitSet` for potentially >> 1000 different style names needed 1000 bits (worst case) as it was not >> sparse). >> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+) >> - Specialized sets are append only (reduces code paths) and can be made read >> only without requiring a wrapper >> - Iterator creation is avoided when doing `containsAll` check thanks to the >> inverse function `isSuperSetOf` >> - Avoids making a copy of the list of style class names to compare (to >> convert them to `StyleClass` and put them into a `Set`) -- this copy could >> not be cached and was always discarded immediately after... >> >> The overall performance was tested using the JFXCentral application which >> displays about 800 nodes on its start page with about 1000 styles in various >> style sheets (and which allows to refresh this page easily). >> >> On JavaFX 20, the fastest refresh speed was 121 ms on my machine. With the >> improvements in this PR, the fastest refresh had become 89 ms. The speed >> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up >> the bulk of the time to refresh the JFXCentral main page (about 100 ms >> before vs 70 ms after the change). > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Move getStyleClassNames to location it was introduced to reduce diff Code looks good. Testing looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1316#pullrequestreview-2075329950
RFR: 8332863: Crash in JPEG decoder if we enable MEM_STATS
In IJG library's jmemmgr.c file we can define MEM_STATS(by default this flag is not defined and we don't see any issue) to enable printing of memory statistics log. But if we enable it, we get crash while disposing IJG stored objects in jmemmgr->free-pool() function. # # A fatal error has been detected by the Java Runtime Environment: # # SIGSEGV (0xb) at pc=0x0001269d5164, pid=47784, tid=259 # # JRE version: Java(TM) SE Runtime Environment (21.0+35) (build 21+35-LTS-2513) # Java VM: Java HotSpot(TM) 64-Bit Server VM (21+35-LTS-2513, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64) # Problematic frame: # C [libjavafx_iio.dylib+0x49164] free_pool+0x88 # # No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again # # If you would like to submit a bug report, please visit: # https://bugreport.java.com/bugreport/crash.jsp # The crash happened outside the Java Virtual Machine in native code. # See problematic frame for where to report the bug. --- T H R E A D --- Current thread (0x000121a42c00): JavaThread "JavaFX Application Thread" [_thread_in_native, id=259, stack(0x00016d11c000,0x00016d918000) (8176K)] Stack: [0x00016d11c000,0x00016d918000], sp=0x00016d912780, free space=8153k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) C [libjavafx_iio.dylib+0x49164] free_pool+0x88 C [libjavafx_iio.dylib+0x49410] self_destruct+0x3c C [libjavafx_iio.dylib+0xe888] jpeg_destroy+0x3c C [libjavafx_iio.dylib+0x4bb1c] imageio_dispose+0x98 C [libjavafx_iio.dylib+0x4b178] disposeIIO+0x2c C [libjavafx_iio.dylib+0x4b140] Java_com_sun_javafx_iio_jpeg_JPEGImageLoader_disposeNative+0x2c This is happening because we delete the error handler before we actually start deleting IJG stored objects and while freeing the IJG objects we try to access cinfo->err->trace_level of error handler. This early deletion of error handler is happening in jpegloader.c->imageio_dispose() function. I have moved deletion of error handler logic after we destroy IJG stored objects in jpegloader.c->imageio_dispose(). This resolves this issue. There is no regression test case because we need to enable MEM_STATS flag to see this issue. Ran graphics unit tests also and i don't see any issues with this change. - Commit messages: - 8332863: Crash in JPEG decoder if we enable MEM_STATS Changes: https://git.openjdk.org/jfx/pull/1463/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1463&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332863 Stats: 4 lines in 1 file changed: 2 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1463.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1463/head:pull/1463 PR: https://git.openjdk.org/jfx/pull/1463
