Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Fri, 3 Feb 2023 22:27:26 GMT, Nir Lisker wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Initialize treeVisible field directly
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1917:
>
>> 1915: for (Node child : parent.getChildren()) {
>> 1916: child.updateDisabled();
>> 1917: }
>
> If you like this style, you could do
> `parent.getChildren().forEach(Node::updateDisabled);`. I find these more
> readable, but it's up to you.
Done.
> modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line
> 2024:
>
>> 2022:
>> 2023: }
>> 2024:
>
> Since you changed the copyright year on the other files, you might want to do
> this one too. There is a script that will do it anyway, so it doesn't matter.
Done.
> modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line
> 2039:
>
>> 2037: assertTrue(n2.isDisabled());
>> 2038: assertTrue(n2.isDisabled());
>> 2039: }
>
> I suggest to continue this test with a `g.setDisable(false)` to check both
> directions. The first group of assertions test the initial state, not a
> change to `false`.
>
> Same for the other test.
Done.
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > improved tests, changed foreach loop I missed that you can also change the foreach loop on the treeVisible method too, but it's not important. - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > improved tests, changed foreach loop I've extended the tests to check for the other direction as well. @hjohn @nlisker can you re-review? - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
> `Node` adds InvalidationListeners to its parent's `disabled` and > `treeVisible` properties and calls its own `updateDisabled()` and > `updateTreeVisible(boolean)` methods when the property values change. > > These listeners are not required, since `Node` can easily call the > `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, > saving the memory cost of maintaining listeners and bindings. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: improved tests, changed foreach loop - Changes: - all: https://git.openjdk.org/jfx/pull/841/files - new: https://git.openjdk.org/jfx/pull/841/files/02577bba..316ec939 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=841=04 - incr: https://webrevs.openjdk.org/?repo=jfx=841=03-04 Stats: 14 lines in 2 files changed: 10 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/841.diff Fetch: git fetch https://git.openjdk.org/jfx pull/841/head:pull/841 PR: https://git.openjdk.org/jfx/pull/841
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size [v2]
On Fri, 3 Feb 2023 23:03:28 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java >> line 197: >> >>> 195: nextStackPane.setVisible(false); >>> 196: >>> 197: resetIndexes(true); >> >> This will end up calling into the control in a couple places, which the PR >> description says you are trying to avoid. > > true, but since it sets to the same value it will be a no-op Right. - PR: https://git.openjdk.org/jfx/pull/1021
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size [v2]
On Fri, 3 Feb 2023 23:09:30 GMT, Andy Goryachev wrote: >> Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 >> branch**. >> >> The method PaginationSkin.resetIndexes(true) has been moved to the original >> position in the constructor, fixing the initialization, while making sure no >> properties of the control are touched in the constructor (only in install()). >> >> Added a test case. >> >> Tested with the PaginationDisappear.java and the LeakTest app >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > 8301797: review comments Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/1021
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size
On Fri, 3 Feb 2023 23:05:43 GMT, Kevin Rushforth wrote: >> Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 >> branch**. >> >> The method PaginationSkin.resetIndexes(true) has been moved to the original >> position in the constructor, fixing the initialization, while making sure no >> properties of the control are touched in the constructor (only in install()). >> >> Added a test case. >> >> Tested with the PaginationDisappear.java and the LeakTest app >> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java > > The changes you just made look good to me, since everything is now internally > consistent. I'll retest and then finish my review. Thank you, @kevinrushforth . - PR: https://git.openjdk.org/jfx/pull/1021
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size [v2]
On Fri, 3 Feb 2023 22:28:06 GMT, Kevin Rushforth wrote: >> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8301797: review comments > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java > line 197: > >> 195: nextStackPane.setVisible(false); >> 196: >> 197: resetIndexes(true); > > This will end up calling into the control in a couple places, which the PR > description says you are trying to avoid. true, but since it sets to the same value it will be a no-op - PR: https://git.openjdk.org/jfx/pull/1021
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size
On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev wrote: > Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 > branch**. > > The method PaginationSkin.resetIndexes(true) has been moved to the original > position in the constructor, fixing the initialization, while making sure no > properties of the control are touched in the constructor (only in install()). > > Added a test case. > > Tested with the PaginationDisappear.java and the LeakTest app > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java The changes you just made look good to me, since everything is now internally consistent. I'll retest and then finish my review. - PR: https://git.openjdk.org/jfx/pull/1021
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size [v2]
> Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 > branch**. > > The method PaginationSkin.resetIndexes(true) has been moved to the original > position in the constructor, fixing the initialization, while making sure no > properties of the control are touched in the constructor (only in install()). > > Added a test case. > > Tested with the PaginationDisappear.java and the LeakTest app > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: 8301797: review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1021/files - new: https://git.openjdk.org/jfx/pull/1021/files/3fbbfccb..a01348e6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1021=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1021=00-01 Stats: 7 lines in 1 file changed: 1 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1021.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1021/head:pull/1021 PR: https://git.openjdk.org/jfx/pull/1021
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß wrote: >> `Node` adds InvalidationListeners to its parent's `disabled` and >> `treeVisible` properties and calls its own `updateDisabled()` and >> `updateTreeVisible(boolean)` methods when the property values change. >> >> These listeners are not required, since `Node` can easily call the >> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children, >> saving the memory cost of maintaining listeners and bindings. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Initialize treeVisible field directly I took a quick look and it seems fine to me. I recommend to wait for a day or two to see if @arapte has any additional comments on the changes (since his review is stale). - PR: https://git.openjdk.org/jfx/pull/841
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size
On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev wrote: > Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 > branch**. > > The method PaginationSkin.resetIndexes(true) has been moved to the original > position in the constructor, fixing the initialization, while making sure no > properties of the control are touched in the constructor (only in install()). > > Added a test case. > > Tested with the PaginationDisappear.java and the LeakTest app > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java You are right! In fact, the added logic to bypass setting control properties in the constructor is unnecessary - it will be essentially a no-op, since it uses the control's current page index. - PR: https://git.openjdk.org/jfx/pull/1021
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size
On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev wrote:
> Fixed regression introduced by JDK-8295754, targeting upstream **jfx20
> branch**.
>
> The method PaginationSkin.resetIndexes(true) has been moved to the original
> position in the constructor, fixing the initialization, while making sure no
> properties of the control are touched in the constructor (only in install()).
>
> Added a test case.
>
> Tested with the PaginationDisappear.java and the LeakTest app
> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java
While the fix works for the specific case in the bug report, I don't think it's
quite right.
In your description you say:
> while making sure no properties of the control are touched in the constructor
> (only in install()).
However, the current fix _does_ touch properties of the control in the
constructor via the moved `resetIndexes` method. First, the "if" check you
added to avoid calling `setCurrentPageIndex` on the control is backwards,
meaning that it is being called in the case you were trying to avoid. Second,
createPage, which is also called from `resetIndexes` has a code path that also
calls `setCurrentPageIndex`.
If it really is important that the skin constructor not call any setter on the
control, then you will need to rework the solution. Possibly by moving the call
to `resetIndexes` back to install, and figuring out why calling it later isn't
causing the pref size to be recomputed.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java
line 197:
> 195: nextStackPane.setVisible(false);
> 196:
> 197: resetIndexes(true);
This will end up calling into the control in a couple places, which the PR
description says you are trying to avoid.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java
line 654:
> 652: nextStackPane.getChildren().clear();
> 653:
> 654: if (usePageIndex) {
Shouldn't this be `!usePageIndex`, given the stated intent? The `resetIndexes`
method is called twice: once from the constructor with `usePageIndex = true`
and once from `resetIndiciesAndNav` with `usePageIndex = false`.
-
Changes requested by kcr (Lead).
PR: https://git.openjdk.org/jfx/pull/1021
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß wrote:
>> `Node` adds InvalidationListeners to its parent's `disabled` and
>> `treeVisible` properties and calls its own `updateDisabled()` and
>> `updateTreeVisible(boolean)` methods when the property values change.
>>
>> These listeners are not required, since `Node` can easily call the
>> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children,
>> saving the memory cost of maintaining listeners and bindings.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Initialize treeVisible field directly
Looks good. I ran the tests and they pass. Left a few optional comments.
Maybe @kevinrushforth will want to have a look too before you merge.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1917:
> 1915: for (Node child : parent.getChildren()) {
> 1916: child.updateDisabled();
> 1917: }
If you like this style, you could do
`parent.getChildren().forEach(Node::updateDisabled);`. I find these more
readable, but it's up to you.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 2608:
> 2606: //PerformanceTracker.logEvent("Node.postinit " +
> 2607: //"for [{this}, id=\"{id}\"]
> finished");
> 2608: //}
Not sure if these comments are intended to be used for something. I doubt it
though.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 2024:
> 2022:
> 2023: }
> 2024:
Since you changed the copyright year on the other files, you might want to do
this one too. There is a script that will do it anyway, so it doesn't matter.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 2039:
> 2037: assertTrue(n2.isDisabled());
> 2038: assertTrue(n2.isDisabled());
> 2039: }
I suggest to continue this test with a `g.setDisable(false)` to check both
directions. The first group of assertions test the initial state, not a change
to `false`.
Same for the other test.
-
Marked as reviewed by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/841
Enhance javafx Canvas API
Hi, While working with skia & jfree's skijaGraphics2D wrapper, I observed skia offers nice methods in SkCanvas missing in javafx Canvas/GraphicsContext to deal with save/restore state: - int getSaveCount () Returns the number of saved states, each containing: SkMatrix and clip. - void restoreToCount (int saveCount) Restores state to SkMatrix and clip values when save(), returned saveCount. As javafx canvas also maintains s stack of saved states, such methods are trivial to implement to allow resetting completely the canvas state by restoreToCount(0). Moreover, clip(shape) + save/restore are really tricky to allows ensure clip is reset to previous state ... Would you agree to add such methods to have more predictive behaviour than counting all save() calls to ensure to call restore() the same number of times to reset the canvas to its initial state ? See SkijaGraphics2d implementation that saves / restore skCanvas clip using such index property: https://github.com/jfree/skijagraphics2d/blob/main/src/main/java/org/jfree/skija/SkijaGraphics2D.java Cheers, Laurent
Re: [jfx20] RFR: 8301797: Pagination control has the wrong size
On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev wrote: > Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 > branch**. > > The method PaginationSkin.resetIndexes(true) has been moved to the original > position in the constructor, fixing the initialization, while making sure no > properties of the control are touched in the constructor (only in install()). > > Added a test case. > > Tested with the PaginationDisappear.java and the LeakTest app > https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java A fix request in JBS is in progress (since we're in RDP2), which I'll review along with the code. @aghaisas Can I ask you to also review it? - PR: https://git.openjdk.org/jfx/pull/1021
Integrated: 8298382: JavaFX ChartArea Accessibility Reader
On Mon, 30 Jan 2023 21:56:45 GMT, Alexander Zuev wrote: > Change the underlying class XYChart to take into account labels for axes. > Make label patterns and default axes labels localized. This pull request has now been integrated. Changeset: 33f1f629 Author:Alexander Zuev Committer: Andy Goryachev URL: https://git.openjdk.org/jfx/commit/33f1f629c5df9f8e03e81e360730536cde0a8f53 Stats: 97 lines in 3 files changed: 90 ins; 0 del; 7 mod 8298382: JavaFX ChartArea Accessibility Reader Reviewed-by: kcr, angorya - PR: https://git.openjdk.org/jfx/pull/1016
[jfx20] RFR: 8301797: Pagination control has the wrong size
Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 branch**. The method PaginationSkin.resetIndexes(true) has been moved to the original position in the constructor, fixing the initialization, while making sure no properties of the control are touched in the constructor (only in install()). Added a test case. Tested with the PaginationDisappear.java and the LeakTest app https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java - Commit messages: - 8301797: fixed pagination initialization Changes: https://git.openjdk.org/jfx/pull/1021/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1021=00 Issue: https://bugs.openjdk.org/browse/JDK-8301797 Stats: 29 lines in 2 files changed: 25 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1021.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1021/head:pull/1021 PR: https://git.openjdk.org/jfx/pull/1021
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader [v5]
On Fri, 3 Feb 2023 14:09:08 GMT, Kevin Rushforth wrote: >> sorry to hold up the review - I'll speak to Naoto Sato tomorrow (2/3/23 @ >> 11:00) regarding the requirements for translation hints, and either approve >> or let you know what they expect us to do. > > OK. Presuming that the comment Alex already left is fine, or just needs a > slight format change, then that's good. If it's more than that, it would be > better to do it in a follow-up issue. The comment is good. The idea of giving translators hints in the form of comments like this was very warmly welcomed by our i18n lead Naoto Sato. So I think we should follow this approach for every new string we add in jfx (and jdk). The goal is to give the translators more context, by either giving an example of resulting text or by describing the parameters like Alex did in this case (or both). - PR: https://git.openjdk.org/jfx/pull/1016
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader [v5]
On Fri, 3 Feb 2023 08:08:35 GMT, Alexander Zuev wrote: >> Change the underlying class XYChart to take into account labels for axes. >> Make label patterns and default axes labels localized. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Removed getSeries() method very good, thank you. - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/1016
Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]
On Fri, 3 Feb 2023 05:56:32 GMT, Karthik P K wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java >> line 402: >> >>> 400: } >>> 401: >>> 402: return height + padding; >> >> My only comment is possibly reorganize the logic to avoid unnecessary >> computation. >> For example, there is no need to compute text width (line 329) if >> isIgnoreText() is true; >> Similarly, no need to compute graphicWidth on line 333 if isIgnoreGraphic() >> is true. >> >> (basically, same comment as @hjohn has made earlier) >> >> What do you think, @karthikpandelu ? > > Not sure if I didn't understand your comments properly but I'm guessing you > are referring to `textHeight` and `graphicHeight` calculation in line 375 and > 380 because the line numbers you mentioned are in `computePrefWidth` method > and changes are made to height calculation in `computePrefHeight` only. > Since `textHeight` and `graphicHeight` each are used in 3 conditions from > line no 386 to 394 I have kept the calculation in the beginning itself. > > On the other hand `textWidth` declared in line 368 might change in line 371, > so I can't use `width` directly instead of defining `textWidth` as `width` > need to be used while calling other graphic related methods. Both `computePrefHeight()` and `computePrefWidth` can be restructured to avoid unnecessary computation (especially since these are very popular objects). My point is that, for example, textWidth on line 375 (used to compute textHeight:375) is not used if isIgnoreText() == true. Similarly, graphicHeight:380 is not used if isIgnoreGraphic() == true, line 386. Same optimization can be applied to computePrefWidth():315 Just a suggestion, really. - PR: https://git.openjdk.org/jfx/pull/996
[jfx11u] Integrated: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
On Fri, 3 Feb 2023 15:34:09 GMT, Kevin Rushforth wrote: > Clean backport to jfx11u. This pull request has now been integrated. Changeset: 59fa5322 Author:Kevin Rushforth URL: https://git.openjdk.org/jfx11u/commit/59fa5322625b6f2a0867cf310bc855b183a84e7c Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13 Backport-of: 5b96d348ebcabb4b6d2e1d95937de3c82a1f6876 - PR: https://git.openjdk.org/jfx11u/pull/128
[jfx11u] Integrated: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
Clean backport to jfx11u. - Commit messages: - 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13 Changes: https://git.openjdk.org/jfx11u/pull/128/files Webrev: https://webrevs.openjdk.org/?repo=jfx11u=128=00 Issue: https://bugs.openjdk.org/browse/JDK-8296654 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx11u/pull/128.diff Fetch: git fetch https://git.openjdk.org/jfx11u pull/128/head:pull/128 PR: https://git.openjdk.org/jfx11u/pull/128
[jfx17u] Integrated: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
On Fri, 3 Feb 2023 15:31:58 GMT, Kevin Rushforth wrote: > Clean backport to jfx17u. This pull request has now been integrated. Changeset: a1c4d0a5 Author:Kevin Rushforth URL: https://git.openjdk.org/jfx17u/commit/a1c4d0a5857920b1d09615b4cd43b09b6b2189de Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13 Backport-of: 5b96d348ebcabb4b6d2e1d95937de3c82a1f6876 - PR: https://git.openjdk.org/jfx17u/pull/109
[jfx17u] Integrated: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
Clean backport to jfx17u. - Commit messages: - 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13 Changes: https://git.openjdk.org/jfx17u/pull/109/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u=109=00 Issue: https://bugs.openjdk.org/browse/JDK-8296654 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx17u/pull/109.diff Fetch: git fetch https://git.openjdk.org/jfx17u pull/109/head:pull/109 PR: https://git.openjdk.org/jfx17u/pull/109
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader [v5]
On Fri, 3 Feb 2023 00:37:56 GMT, Andy Goryachev wrote: >>> Still, I would recommend adding a comment (since now is the time when we >>> know all the context, and in the future we might forget or lose the >>> context), may be something like >> >> Good idea, let me do it for both strings with parameters > > sorry to hold up the review - I'll speak to Naoto Sato tomorrow (2/3/23 @ > 11:00) regarding the requirements for translation hints, and either approve > or let you know what they expect us to do. OK. Presuming that the comment Alex already left is fine, or just needs a slight format change, then that's good. If it's more than that, it would be better to do it in a follow-up issue. - PR: https://git.openjdk.org/jfx/pull/1016
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader [v5]
On Fri, 3 Feb 2023 08:08:35 GMT, Alexander Zuev wrote: >> Change the underlying class XYChart to take into account labels for axes. >> Make label patterns and default axes labels localized. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Removed getSeries() method Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.org/jfx/pull/1016
Re: [jfx17u] RFR: 8293214: Add support for Linux/LoongArch64
On Mon, 9 Jan 2023 13:28:51 GMT, Ao Qi wrote: > Clean backport. Verified on Linux/LoongArch64, Linux/x64 and Linux/aarch64. Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.org/jfx17u/pull/102
Re: Performance considerations of syncing FX Nodes with NG Nodes
On 03/02/2023 01:27, Kevin Rushforth wrote: A longer answer will take more time than I have at the moment, but here is a quick reply. 1. We don't have a lot of scene graph performance tests, so before making any changes here we would need to do some targeted testing. I completely agree here, if there are no tests that can be re-used then some will need to be created, that's okay. 2. FX scene graph node --> NGNode peer is intended to be be one-way Makes sense. 3. There are certainly use cases for reusing nodes (attach / detach). VirtualFlow will do this for ListView, TableView, etc., with many rows. I would expect some apps to make use of it as well, since it's cheaper than recreating the FX nodes. There are also cases where nodes are moved from one parent to another and might transiently be "removed" from the graph. Moving nodes is one of the cases that may degrade in performance. Keeping (unused) cell nodes as part of the children list but invisible/unmanaged should not be affected (I'm not 100% sure what VirtualFlow is doing at the moment, I only know that's how I do it in my own ListViewSkin -- I don't think it had a performance reason, more that it could result in visual artifacts in 1 rendered frame before correct colors were applied -- perhaps a bug in itself). 4. Setting the node's peer to null when detached will cause it to be recreated when / if the node is added back into the scene graph, which you touch on later in your email. We wouldn't want the case of a scrolling ListView / TableVIew to have to continually recreate peers as the cells are cycled through. Yeah, not sure if that is what would happen, it depends on how VirtualFlow is handling its cells. Anything done here would need to be done very carefully to ensure both correctness and performance. Your idea of splitting the peer data from the graph connectivity is interesting, but it also sounds pretty intrusive. Before going down that route I'd want to understand the problem better and see if there was a simpler solution to clear the parent / child relationship in the render graph. There are multiple problems it seems in this area. At first I thought it was only the `removed` list in Parent that is a problem, as it can contain old Nodes that don't get cleaned up when not part of a sync cycle. But any node that is not part of a sync cycle may refer to a peer, and that peer can refer to parent/children in turn, keeping an entire NGNode graph in memory. For the `removed` list in Parent, there is a simple solution I think. It already has a mechanism to mark itself as "fully" dirty when more than 20 children are removed. This mechanism could be re-used to also mark it as fully dirty when the Parent is no longer part of an active sync cycle. If ever re-attached, it would just fully render instead of trying to calculate a smaller dirty region based on removed children. There is also an odd piece of code in Node, in invalidateScenes. In this code it will call `peer.release()` which does nothing for any of the current NG implementations (even though there are comments that they should do "something"). But because it doesn't set `peer` to `null` it will later use the "released" peer again if re-attached to a scene. If this is not a mistake, it deserves an explanatory comment I think. --John -- Kevin On 2/2/2023 9:17 AM, John Hendrikx wrote: I have a few questions that maybe some Prism/SceneGraph experts could help me with. 1) Are there any performance tests related to syncing the NGNode peers? Specifically, I'm interested if there are tests that compare the performance of "fresh" FX graphs that have never been displayed before, versus ones that have their peers already created. 2) Does prism code ever refer back to FX Nodes? I've noticed that NGGroup imports javafx.scene.Node for one of its method signatures, but that seems to be a mistake; it can easily be changed to not require javafx.scene.Node. Aside from several enums, constants and data classes (like Color) being re-used from the javafx side, it seems the NG Prism nodes are well encapsulated and that references are one way only (FX Nodes refer to NG Nodes via `peer`, but never the other way around). 3) How common is it to re-use FX Nodes that are no longer part of an active scene? I've found myself that it is unwise to detach/recreate children in high performance controls that reuse cells -- it's often better to just hide cells that are not currently needed instead of removing them from the children list. 4) Are there any (major) performance implications to setting the NG peer to `null` when FX nodes are not part of an active sync cycle (ie, they have no Scene, or the Scene is not currently visible)? My observations on the sync cycle (syncPeer/doUpdatePeer) is that it is highly optimized, and tries to avoid new allocations as much as possible. However, this seems to come at a price: it
[jfx17u] Integrated: 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy.
On Fri, 3 Feb 2023 08:40:49 GMT, Johan Vos wrote: > clean backport of JDK-8087673 > [TableView] TableView and TreeTableView menu button overlaps columns when > using a constrained resize policy. > > Reviewed-by: angorya, aghaisas This pull request has now been integrated. Changeset: ad397700 Author:Johan Vos URL: https://git.openjdk.org/jfx17u/commit/ad397700b4c0744f5f87b25a46c3cd219dc53399 Stats: 256 lines in 4 files changed: 247 ins; 4 del; 5 mod 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy. Backport-of: 2baa10eed777574e40e5104278e8690941911018 - PR: https://git.openjdk.org/jfx17u/pull/108
Re: [jfx17u] RFR: 8293214: Add support for Linux/LoongArch64
On Fri, 3 Feb 2023 08:44:53 GMT, Johan Vos wrote: >> Clean backport. Verified on Linux/LoongArch64, Linux/x64 and Linux/aarch64. > > I'm ok with this. > Apart from the functionality, this BP also makes it easier to backport future > changes in build.gradle (because it backports a structural change in the > 64-bit "detection") Thanks, @johanvos ! Let's wait for a while to see if @kevinrushforth has further comments. - PR: https://git.openjdk.org/jfx17u/pull/102
Re: [jfx17u] RFR: 8293214: Add support for Linux/LoongArch64
On Mon, 9 Jan 2023 13:28:51 GMT, Ao Qi wrote: > Clean backport. Verified on Linux/LoongArch64, Linux/x64 and Linux/aarch64. Marked as reviewed by jvos (Reviewer). I'm ok with this. Apart from the functionality, this BP also makes it easier to backport future changes in build.gradle (because it backports a structural change in the 64-bit "detection") - PR: https://git.openjdk.org/jfx17u/pull/102
[jfx17u] RFR: 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy.
clean backport of JDK-8087673 [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy. Reviewed-by: angorya, aghaisas - Commit messages: - 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy. Changes: https://git.openjdk.org/jfx17u/pull/108/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u=108=00 Issue: https://bugs.openjdk.org/browse/JDK-8087673 Stats: 256 lines in 4 files changed: 247 ins; 4 del; 5 mod Patch: https://git.openjdk.org/jfx17u/pull/108.diff Fetch: git fetch https://git.openjdk.org/jfx17u pull/108/head:pull/108 PR: https://git.openjdk.org/jfx17u/pull/108
[jfx17u] Integrated: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON
On Mon, 9 Jan 2023 13:29:27 GMT, Ao Qi wrote: > Clean backport. Verified on (after #102): > - Linux/LoongArch64 (`USE_SYSTEM_MALLOC` is `on`, the platform triggering > this problem) > - Linux/x64 (`USE_SYSTEM_MALLOC` is `off`) > - Linux/aarch64 (`USE_SYSTEM_MALLOC` is `off`) This pull request has now been integrated. Changeset: 29f878fc Author:Ao Qi Committer: Johan Vos URL: https://git.openjdk.org/jfx17u/commit/29f878fcc9778798293a2d125a10593f1fe2d6dc Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON Reviewed-by: kcr, jvos Backport-of: 28f8fa9c20363ced9a94787ecfd45735b3e6b82e - PR: https://git.openjdk.org/jfx17u/pull/103
[jfx17u] Integrated: 8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar.
On Fri, 3 Feb 2023 08:29:41 GMT, Johan Vos wrote: > clean backport of JDK-8089009: TableView with CONSTRAINED_RESIZE_POLICY > incorrectly displays a horizontal scroll bar. > > > Reviewed-by: kcr, angorya, aghaisas This pull request has now been integrated. Changeset: 6e7def58 Author:Johan Vos URL: https://git.openjdk.org/jfx17u/commit/6e7def587177cc0d53794895f9292fe5ee0876bb Stats: 56 lines in 2 files changed: 53 ins; 0 del; 3 mod 8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar. Backport-of: d4ff45af9fbde29f96d80681c7fc3af3ec580d1a - PR: https://git.openjdk.org/jfx17u/pull/107
[jfx17u] Integrated: 8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar.
clean backport of JDK-8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar. Reviewed-by: kcr, angorya, aghaisas - Commit messages: - 8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar. Changes: https://git.openjdk.org/jfx17u/pull/107/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u=107=00 Issue: https://bugs.openjdk.org/browse/JDK-8089009 Stats: 56 lines in 2 files changed: 53 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx17u/pull/107.diff Fetch: git fetch https://git.openjdk.org/jfx17u pull/107/head:pull/107 PR: https://git.openjdk.org/jfx17u/pull/107
[jfx17u] Integrated: 8277853: With Touch enabled devices scrollbar disappears and the table is scrolled to the beginning
On Fri, 3 Feb 2023 07:48:19 GMT, Johan Vos wrote: > Clean backport for 8277853 > > Reviewed-by: kcr, mhanl This pull request has now been integrated. Changeset: d0579c35 Author:Johan Vos URL: https://git.openjdk.org/jfx17u/commit/d0579c3536b194ce771acc38d1bfb474b0b58c86 Stats: 36 lines in 2 files changed: 35 ins; 0 del; 1 mod 8277853: With Touch enabled devices scrollbar disappears and the table is scrolled to the beginning Backport-of: 78d922750747517553b101f5ff4af5361b1e7959 - PR: https://git.openjdk.org/jfx17u/pull/106
Re: [jfx17u] RFR: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON
On Mon, 9 Jan 2023 13:29:27 GMT, Ao Qi wrote: > Clean backport. Verified on (after #102): > - Linux/LoongArch64 (`USE_SYSTEM_MALLOC` is `on`, the platform triggering > this problem) > - Linux/x64 (`USE_SYSTEM_MALLOC` is `off`) > - Linux/aarch64 (`USE_SYSTEM_MALLOC` is `off`) good to go now - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.org/jfx17u/pull/103
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader [v2]
On Thu, 2 Feb 2023 23:46:27 GMT, Kevin Rushforth wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed debug output.
>
> modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java line
> 1271:
>
>> 1269:
>> 1270: Series getSeries() {
>> 1271: return seriesProperty.get();
>
> This method is unused, so should probably be removed.
Done.
-
PR: https://git.openjdk.org/jfx/pull/1016
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader [v5]
> Change the underlying class XYChart to take into account labels for axes. > Make label patterns and default axes labels localized. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Removed getSeries() method - Changes: - all: https://git.openjdk.org/jfx/pull/1016/files - new: https://git.openjdk.org/jfx/pull/1016/files/ce66bfbf..1358b989 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1016=04 - incr: https://webrevs.openjdk.org/?repo=jfx=1016=03-04 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1016.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1016/head:pull/1016 PR: https://git.openjdk.org/jfx/pull/1016
