Re: RFR: 8313956: focusWithin on parents of a newly-added focused node is not updated [v2]
> This PR fixes an issue with the way `focusWithin` bits are adjusted in the > scene graph. Previously, the `focusWithin` counts of all parents of a removed > node would be decreased if the removed node has a non-zero `focusWithin` > count. This PR adds logic for the opposite scenario: the `focusWithin` count > is increased on all parents of a newly-added node that is already focused (or > contains other focused nodes). Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Removed unnecessary code - Changes: - all: https://git.openjdk.org/jfx/pull/1210/files - new: https://git.openjdk.org/jfx/pull/1210/files/d8d53631..fd85c902 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1210&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1210&range=00-01 Stats: 13 lines in 1 file changed: 0 ins; 4 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1210.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1210/head:pull/1210 PR: https://git.openjdk.org/jfx/pull/1210
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Tue, 8 Aug 2023 23:44:58 GMT, Jose Pereda wrote: >> So far, BorderPane does the calculation for the children min/pref >> width/height taken into account only the margin applied to them, if any, but >> not the total padding that could be applied as well to the BorderPane itself. >> >> However, this padding needs to be taken into account as well, and this PR >> modifies BorderPane to subtract its insets from its size while doing the >> children min/pref width/height calculations. >> >> A parameterized test has been included. >> >> It is a simplified version of the test case attached to >> https://bugs.openjdk.org/browse/JDK-8313709, but still shows how without >> this patch, two of the cases (padding with or without margin) fail, while >> pass with it. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Migrate old tests to JUnit 5 FYI: the test code shows 2x2 arrangement on windows at 100%, but 1x4 arrangement at 125%. Is this expected? In both cases scrolling and resizing seems ok. - PR Comment: https://git.openjdk.org/jfx/pull/1203#issuecomment-1681333792
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Tue, 8 Aug 2023 23:44:58 GMT, Jose Pereda wrote:
>> So far, BorderPane does the calculation for the children min/pref
>> width/height taken into account only the margin applied to them, if any, but
>> not the total padding that could be applied as well to the BorderPane itself.
>>
>> However, this padding needs to be taken into account as well, and this PR
>> modifies BorderPane to subtract its insets from its size while doing the
>> children min/pref width/height calculations.
>>
>> A parameterized test has been included.
>>
>> It is a simplified version of the test case attached to
>> https://bugs.openjdk.org/browse/JDK-8313709, but still shows how without
>> this patch, two of the cases (padding with or without margin) fail, while
>> pass with it.
>
> Jose Pereda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Migrate old tests to JUnit 5
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line
395:
> 393:
> 394: double middleAreaHeight = Math.max(0,
> 395: height - insets.getTop() - insets.getBottom() -
> topPrefHeight - bottomPrefHeight);
should these be snapped? snappedBottomInset() etc.?
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line
415:
> 413: if (width != -1) {
> 414: width -= (insets.getLeft() + insets.getRight());
> 415: }
ditto here
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java
line 938:
> 936: @ParameterizedTest
> 937: public void testFlowPaneCenterChildWithPaddingAndMargin(double
> width, double minHeight, boolean useMargin, boolean usePadding) {
> 938: FlowPane center = new FlowPane(10, 10);
I wonder if we should also test the layout-related code using fractional scale
+ snapping.
what do you think?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1296446580
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1296447169
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1296450320
Re: RFR: 8306083: Text.hitTest is incorrect when Text node is present in TextFlow [v5]
On Tue, 8 Aug 2023 13:57:31 GMT, Karthik P K wrote:
>> The text run selected in `PrismTextLayout::getHitInfo()` method for
>> character index calculation was not correct when Text node was embedded in
>> TextFlow. Hence wrong character index value was calculated for the same.
>>
>> Since only x, y coordinates were available in the above mentioned method,
>> sending the text as a parameter to this method is necessary so as to know if
>> the text run selected for character index calculation is correct. Along with
>> this change modified the `PrismTextLayout::getHitInfo()` method to calculate
>> the correct character index.
>>
>> Added tests to validate the changes.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix character index calculation issue when Text node content is wrapped
The updated code works correctly, as far as I can tell. Left some minor
comments.
This PR definitely needs another pair of eyes.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
line 202:
> 200: public boolean setTabSize(int spaces);
> 201:
> 202: public Hit getHitInfo(float x, float y, String text);
Perhaps we could add javadoc with an explanation for the 'text' parameter, that
it's expected to be null in the case of TextFlow and non-null in the case of
Text.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 505:
> 503: charIterator.setText(text);
> 504: } else {
> 505: charIterator.setText(new String(getText()));
getText() is possibly an expensive operation when the layout contains a lot of
text.
Since we are looking only for the following "character", is it possible to
optimize this and only use a subset of spans or limit the amount of text passed
to the break iterator in some other way?
What do you think?
(it's likely to be out of scope for this PR)
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 754:
> 752: int index = 0;
> 753: float bottom = 0;
> 754: boolean isTextPresent = text == null ? true : false;
this looks very confusing to me... what text is present?
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 775:
> 773: bottom += lines[index].getBounds().getHeight() + spacing;
> 774: if (index + 1 == lineCount) bottom -=
> lines[index].getLeading();
> 775: if (bottom > y && isTextPresent) break;
I would recommend adding { }'s to ifs here and on line 774.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java line 396:
> 394: for (int i = 0; i < glyphCount; i++) {
> 395: float advance = getAdvance(i);
> 396: if (runX + advance >= x) {
this is very interesting. how did it work before?
could there be any scenarios that might have failed before and are fixed with
this change?
tests/system/src/test/java/test/robot/javafx/scene/TextCharacterIndexTest.java
line 136:
> 134: private void mouseClick(double x, double y) {
> 135: Util.runAndWait(() -> {
> 136: robot.mouseMove((int) (scene.getWindow().getX() +
> scene.getX() + x),
minor: local Window variable eliminates multiple invocations of getWindow()
(I am too lazy to see if javac is smart enough to optimizes it away)
tests/system/src/test/java/test/robot/javafx/scene/TextCharacterIndexTest.java
line 153:
> 151: textTwo = new Text("This is Text");
> 152: textTwo.setFont(new Font(48));
> 153: textFlow.getChildren().clear();
is this clear() a necessary part of the test?
setAll() "Clears the ObservableList and adds all the elements..." according to
its javadoc
this comment applies here and in all other places (line 164, etc.)
tests/system/src/test/java/test/robot/javafx/scene/TextCharacterIndexTest.java
line 403:
> 401: // String testString = textOne.getText();
> 402: // testString += textTwo.getText();
> 403: // isSurrogatePair =
> Character.isSurrogate(testString.charAt(charIndex));
clean up?
-
PR Review: https://git.openjdk.org/jfx/pull/1157#pullrequestreview-1581344829
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296381587
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296389135
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296393704
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296379365
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296397150
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296399169
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296406718
PR Review Comment: https://git.openjdk.org/jfx/pull/1157#discussion_r1296408302
Re: RFR: 8309558: Create implementation of NSAccessibilityCheckBox protocol [v3]
On Mon, 14 Aug 2023 18:38:39 GMT, Alexander Zuev wrote: >> also >> 8309629: Create implementation of NSAccessibilityRadioButton protocol >> >> Create implementation of NSAccessibilityCheckBox and >> NSAccessibilityRadioButton protocols >> Add workaround for the wrong focus owner announcement with radio buttons >> Add workaround for wrong magnifier text on buttons > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Comment out TAB_ITEM until the TAB_GROUP is properly implemented in the new > API > Make isAccessibilityFocused to always return a proper value Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1184#pullrequestreview-1581170403
Integrated: 8310885: Width/height of window is not set after calling sizeToScene
On Thu, 3 Aug 2023 14:57:50 GMT, Guillaume Tâche wrote: > `setHeight()` / `setWidth()` were ignored if called after `sizeToScene()` and > before `show()`. > Now the `sizeToScene` flag is unset in these methods to ensure the right > values are set when the window is shown. This pull request has now been integrated. Changeset: 5c3e8329 Author:Guillaume Tâche Committer: Ambarish Rapte URL: https://git.openjdk.org/jfx/commit/5c3e83293eaf364d7288cd1e609d89c4b20bd91e Stats: 60 lines in 2 files changed: 58 ins; 2 del; 0 mod 8310885: Width/height of window is not set after calling sizeToScene Reviewed-by: mhanl, arapte - PR: https://git.openjdk.org/jfx/pull/1195
Re: RFR: 8309558: Create implementation of NSAccessibilityCheckBox protocol [v3]
On Mon, 14 Aug 2023 18:38:39 GMT, Alexander Zuev wrote: >> also >> 8309629: Create implementation of NSAccessibilityRadioButton protocol >> >> Create implementation of NSAccessibilityCheckBox and >> NSAccessibilityRadioButton protocols >> Add workaround for the wrong focus owner announcement with radio buttons >> Add workaround for wrong magnifier text on buttons > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Comment out TAB_ITEM until the TAB_GROUP is properly implemented in the new > API > Make isAccessibilityFocused to always return a proper value LGTM - Marked as reviewed by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1184#pullrequestreview-1580937856
Re: RFR: 8310885: Width/height of window is not set after calling sizeToScene [v3]
On Wed, 16 Aug 2023 09:28:57 GMT, Guillaume Tâche wrote: >> `setHeight()` / `setWidth()` were ignored if called after `sizeToScene()` >> and before `show()`. >> Now the `sizeToScene` flag is unset in these methods to ensure the right >> values are set when the window is shown. > > Guillaume Tâche has updated the pull request incrementally with one > additional commit since the last revision: > > 8310885: Uses exact comparison for tests Marked as reviewed by arapte (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1195#pullrequestreview-1580815105
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Wed, 16 Aug 2023 14:06:16 GMT, John Hendrikx wrote: >> modules/javafx.graphics/src/main/java/javafx/css/Match.java line 1: >> >>> 1: /* >> >> The compare method seems wrong. I think it should delegate to >> `Integer.compare`. >> >> `specificity` should also be made `private`. > > These were not changed as part of this PR, although I agree on both counts. I should add, even though there are some edge cases where the compare method could fail in `Match`, it will work correctly in practice as `specificity` is always a positive number. specificity = (idCount << 8) | (styleClassCount << 4) | nPseudoClasses; I do question how this is calculated though. It seems to make assumptions that `styleClassCount` and the number of pseudo classes never exceeds 16 -- I would agree that having 16 style classes / pseudo classes in a match is on the high side, but certainly not so high that it could never happen. It seems to be a bit of a premature optimization, since `Match` retains all the information that was used to calculate `specificity`, and so they could have instead have compared `idCount`, `styleClassCount` and `pseudoClasses.size()` in turn... - PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1296004826
Re: RFR: 8274932: Render scales in EmbeddedWindow are not properly updated [v10]
On Wed, 16 Aug 2023 06:01:45 GMT, Prasanta Sadhukhan wrote: >> When the JavaFX scene is set before it is really shown, then the scale >> factors are not properly propagated to the EmbeddedWindow, resulting in >> showing wrong scales. >> Fix is made to update scales to EmbeddedWindow > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Code optimize looks good, thanks! - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1171#pullrequestreview-1580762348
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Wed, 16 Aug 2023 13:16:19 GMT, Jose Pereda wrote:
>> You could check this yourself if you want. The `BitSet` class had problems
>> in many places, was largely untested and, to be very honest, should never
>> have passed code review. It violated the `Set` contract almost everywhere,
>> which is problematic since sets backed by this `BitSet` class were exposed
>> in several places where they could be accessed by users.
>>
>> A simple test shows the problem (it doesn't throw an exception though, I
>> misread that):
>>
>> public static void main(String[] args) {
>> for (int i = 0; i < 65; i++) {
>> PseudoClassState.getPseudoClass("" + i);
>> }
>>
>> System.out.println(Arrays.asList(new PseudoClassState(List.of("0",
>> "1", "64")).toArray()));
>> }
>>
>> Prints:
>>
>> [0, null, null]
>>
>> There is no point in fixing the existing code; it won't perform any better,
>> but would require writing additional test cases to verify that an
>> implementation we don't need is doing what we can get for free.
>
> Thanks for that test, actually it could be used as part of a test in
> PseudoClassTest, to verify that the old implementation failed and the new one
> worked?
>
> And this brings up another issue: the constructor `PseudoClassState(List)` is
> not used anywhere, is it? Should it be removed then (unless it is added to
> such test)?
Alright, I'll add the test.
As for the other issue: there are quite a few other issues **near** the code
I've changed, but in order for a PR to remain focused, and not burdened with
changes that don't contribute to the intended change (making it smaller and
easier to review), it's best to address these in another PR that focuses
specifically on such problems (unused constructors, unused fields, unused
methods, etc).
In this specific case, the constructor was not changed or added by me, it's
part of private API and it's not causing problems. Changing it would distract
from the goal of the PR, and so is IMHO out of scope.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295987373
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Wed, 16 Aug 2023 02:02:02 GMT, Nir Lisker wrote: >> John Hendrikx has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 16 commits: >> >> - Merge branch 'master' of https://git.openjdk.org/jfx into >>feature/immutable-pseudoclassstate >> - Merge remote-tracking branch 'upstream/master' into >> feature/immutable-pseudoclassstate >> - Avoid using Lambda in ImmutablePseudoClassSetsCache.of() >> - Merge branch 'master' of https://git.openjdk.org/jfx into >>feature/immutable-pseudoclassstate >> - Fix another edge case in BitSet equals >> >>When arrays are not the same size, but there are no set bits in the ones >>the other set doesn't have, two bit sets can still be considered equal >> - Take element type into account for BitSet.equals() >> - Base BitSet on AbstractSet to inherit correct equals/hashCode/toArray >> >>- Removed faulty toArray implementations in PseudoClassState and >>StyleClassSet >>- Added test that verifies equals/hashCode for PseudoClassState respect >>Set contract now >>- Made getBits package private so it can't be inherited >> - Remove unused code >> - Ensure Match doesn't allow modification >> - Simplify ImmutablePseudoClassSetsCache and avoid an unnecessary copy >> - ... and 6 more: https://git.openjdk.org/jfx/compare/9ad0e908...7975ae99 > > modules/javafx.graphics/src/main/java/javafx/css/Match.java line 1: > >> 1: /* > > The compare method seems wrong. I think it should delegate to > `Integer.compare`. > > `specificity` should also be made `private`. These were not changed as part of this PR, although I agree on both counts. - PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295973376
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Wed, 16 Aug 2023 05:02:36 GMT, Nir Lisker wrote:
>> Almost, but the key is also the copied set. In your version the key is the
>> original object, which may be a modifiable set. If the key is modified, the
>> cache will not function correctly anymore.
>>
>> I had a different version before, but @mstr2 pointed out that there was an
>> unnecessary allocation in that version which this version avoids.
>
> Then the map is just an optimization? If the set points to itself then a
> `Set` as a cache should be enough.
No, a `Set` wouldn't work here, for this reason:
if (CACHE.contains(mutableSet)) {
return ... // how do I get the immutable set from CACHE when it is
a Set?
}
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295967620
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Tue, 15 Aug 2023 23:05:38 GMT, John Hendrikx wrote:
>> The original ("broken") version has been working fine, and no bugs have been
>> reported so far, and there would be a reason to have a custom implementation
>> instead of the one in `AbstractSet` in the first place.
>>
>> I'm not against removing it, but only after we are certain that this
>> implementation is no longer needed.
>>
>> Also, have you tried fixing it instead of removing it? If you have, are
>> there any differences when you run the test with one or the other?
>
> You could check this yourself if you want. The `BitSet` class had problems
> in many places, was largely untested and, to be very honest, should never
> have passed code review. It violated the `Set` contract almost everywhere,
> which is problematic since sets backed by this `BitSet` class were exposed in
> several places where they could be accessed by users.
>
> A simple test shows the problem (it doesn't throw an exception though, I
> misread that):
>
> public static void main(String[] args) {
> for (int i = 0; i < 65; i++) {
> PseudoClassState.getPseudoClass("" + i);
> }
>
> System.out.println(Arrays.asList(new PseudoClassState(List.of("0",
> "1", "64")).toArray()));
> }
>
> Prints:
>
> [0, null, null]
>
> There is no point in fixing the existing code; it won't perform any better,
> but would require writing additional test cases to verify that an
> implementation we don't need is doing what we can get for free.
Thanks for that test, actually it could be used as part of a test in
PseudoClassTest, to verify that the old implementation failed and the new one
worked?
And this brings up another issue: the constructor `PseudoClassState(List)` is
not used anywhere, is it? Should it be removed then (unless it is added to such
test)?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295883206
Integrated: 8314266: Several test failures after fix for JDK-8159048
On Wed, 16 Aug 2023 12:11:54 GMT, Jose Pereda wrote: > This PR fixes some failing tests that started failing after the check on > animations being played on the JavaFX thread was enforced, by simply wrapping > the `play` call with `Platform.runLater` This pull request has now been integrated. Changeset: 3b105621 Author:Jose Pereda URL: https://git.openjdk.org/jfx/commit/3b105621ae9f0d0fd99a615db4c084e8ecb906a8 Stats: 7 lines in 3 files changed: 1 ins; 0 del; 6 mod 8314266: Several test failures after fix for JDK-8159048 Reviewed-by: kcr - PR: https://git.openjdk.org/jfx/pull/1211
Re: RFR: 8314266: Several test failures after fix for JDK-8159048
On Wed, 16 Aug 2023 12:11:54 GMT, Jose Pereda wrote: > This PR fixes some failing tests that started failing after the check on > animations being played on the JavaFX thread was enforced, by simply wrapping > the `play` call with `Platform.runLater` LGTM - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1211#pullrequestreview-1580480370
RFR: 8314266: Several test failures after fix for JDK-8159048
This PR fixes some failing tests that started failing after the check on animations being played on the JavaFX thread was enforced, by simply wrapping the `play` call with `Platform.runLater` - Commit messages: - Run timelines on JavaFX thread Changes: https://git.openjdk.org/jfx/pull/1211/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1211&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314266 Stats: 7 lines in 3 files changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1211.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1211/head:pull/1211 PR: https://git.openjdk.org/jfx/pull/1211
Re: RFR: 8310885: Width/height of window is not set after calling sizeToScene [v3]
On Wed, 16 Aug 2023 09:28:57 GMT, Guillaume Tâche wrote: >> `setHeight()` / `setWidth()` were ignored if called after `sizeToScene()` >> and before `show()`. >> Now the `sizeToScene` flag is unset in these methods to ensure the right >> values are set when the window is shown. > > Guillaume Tâche has updated the pull request incrementally with one > additional commit since the last revision: > > 8310885: Uses exact comparison for tests Looks good to me too. - Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1195#pullrequestreview-1580434274
Re: RFR: 8274932: Render scales in EmbeddedWindow are not properly updated [v10]
On Wed, 16 Aug 2023 06:01:45 GMT, Prasanta Sadhukhan wrote: >> When the JavaFX scene is set before it is really shown, then the scale >> factors are not properly propagated to the EmbeddedWindow, resulting in >> showing wrong scales. >> Fix is made to update scales to EmbeddedWindow > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Code optimize Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1171#pullrequestreview-1580397193
Re: RFR: 8310885: Width/height of window is not set after calling sizeToScene [v2]
On Wed, 16 Aug 2023 07:08:10 GMT, Marius Hanl wrote: >> Guillaume Tâche has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8310885: Fixes review comment > > modules/javafx.graphics/src/test/java/test/javafx/stage/WindowTest.java line > 184: > >> 182: testWindow.setWidth(800); >> 183: testWindow.show(); >> 184: assertEquals(800, testWindow.getWidth(), 1); > > I think you can remove the delta in this tests. We expect accurate numbers > here Done, indeed it also works with exact comparisons. - PR Review Comment: https://git.openjdk.org/jfx/pull/1195#discussion_r1295622629
Re: RFR: 8310885: Width/height of window is not set after calling sizeToScene [v3]
> `setHeight()` / `setWidth()` were ignored if called after `sizeToScene()` and > before `show()`. > Now the `sizeToScene` flag is unset in these methods to ensure the right > values are set when the window is shown. Guillaume Tâche has updated the pull request incrementally with one additional commit since the last revision: 8310885: Uses exact comparison for tests - Changes: - all: https://git.openjdk.org/jfx/pull/1195/files - new: https://git.openjdk.org/jfx/pull/1195/files/fd3fb463..1f1f943d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1195&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1195&range=01-02 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jfx/pull/1195.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1195/head:pull/1195 PR: https://git.openjdk.org/jfx/pull/1195
Re: RFR: 8310885: Width/height of window is not set after calling sizeToScene [v2]
On Tue, 8 Aug 2023 09:02:07 GMT, Guillaume Tâche wrote: >> `setHeight()` / `setWidth()` were ignored if called after `sizeToScene()` >> and before `show()`. >> Now the `sizeToScene` flag is unset in these methods to ensure the right >> values are set when the window is shown. > > Guillaume Tâche has updated the pull request incrementally with one > additional commit since the last revision: > > 8310885: Fixes review comment modules/javafx.graphics/src/test/java/test/javafx/stage/WindowTest.java line 184: > 182: testWindow.setWidth(800); > 183: testWindow.show(); > 184: assertEquals(800, testWindow.getWidth(), 1); I think you can remove the delta in this tests. We expect accurate numbers here - PR Review Comment: https://git.openjdk.org/jfx/pull/1195#discussion_r1295467845
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Tue, 15 Aug 2023 19:33:20 GMT, John Hendrikx wrote: >> modules/javafx.graphics/src/main/java/com/sun/javafx/css/ImmutablePseudoClassSetsCache.java >> line 61: >> >>> 59: CACHE.put(copy, copy); >>> 60: >>> 61: return copy; >> >> Isn't this just `return CACHE.computeIfAbsent(pseudoClasses, Set::copyOf);`? > > Almost, but the key is also the copied set. In your version the key is the > original object, which may be a modifiable set. If the key is modified, the > cache will not function correctly anymore. > > I had a different version before, but @mstr2 pointed out that there was an > unnecessary allocation in that version which this version avoids. Then the map is just an optimization? If the set points to itself then a `Set` as a cache should be enough. - PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295381418
Re: RFR: JDK-8199216: Quadratic layout time with nested nodes and pseudo-class in style sheet [v8]
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote: >> This fix introduces immutable sets of `PseudoClass` almost everywhere, as >> they are rarely modified. These are re-used by caching them in a new class >> `ImmutablePseudoClassSetsCache`. >> >> In order to make this work, `BitSet` had to be cleaned up. It made >> assumptions about the collections it is given (which may no longer always be >> another `BitSet`). I also added the appropriate null checks to ensure there >> weren't any other bugs lurking. >> >> Then there was a severe bug in `toArray` in both the subclasses that >> implement `BitSet`. >> >> The bug in `toArray` was incorrect use of the variable `index` which was >> used for both advancing the pointer in the array to be generated, as well as >> for the index to the correct `long` in the `BitSet`. This must have >> resulted in other hard to reproduce problems when dealing with >> `Set` or `Set` if directly or indirectly calling >> `toArray` (which is for example used by `List.of` and `Set.of`) -- I fixed >> this bug because I need to call `Set.copyOf` which uses `toArray` -- as the >> same bug was also present in `StyleClassSet`, I fixed it there as well. >> >> The net result of this change is that there are far fewer `PseudoClassState` >> objects created; the majority of these are never modified, and the few that >> are left are where you'd expect to see them modified. >> >> A test with 160 nested HBoxes which were given the hover state shows a >> 99.99% reduction in `PseudoClassState` instances and a 70% reduction in heap >> use (220 MB -> 68 MB), see the linked ticket for more details. >> >> Although the test case above was extreme, this change should have positive >> effects for most applications. > > John Hendrikx has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' of https://git.openjdk.org/jfx into >feature/immutable-pseudoclassstate > - Merge remote-tracking branch 'upstream/master' into > feature/immutable-pseudoclassstate > - Avoid using Lambda in ImmutablePseudoClassSetsCache.of() > - Merge branch 'master' of https://git.openjdk.org/jfx into >feature/immutable-pseudoclassstate > - Fix another edge case in BitSet equals > >When arrays are not the same size, but there are no set bits in the ones >the other set doesn't have, two bit sets can still be considered equal > - Take element type into account for BitSet.equals() > - Base BitSet on AbstractSet to inherit correct equals/hashCode/toArray > >- Removed faulty toArray implementations in PseudoClassState and >StyleClassSet >- Added test that verifies equals/hashCode for PseudoClassState respect >Set contract now >- Made getBits package private so it can't be inherited > - Remove unused code > - Ensure Match doesn't allow modification > - Simplify ImmutablePseudoClassSetsCache and avoid an unnecessary copy > - ... and 6 more: https://git.openjdk.org/jfx/compare/9ad0e908...7975ae99 modules/javafx.graphics/src/main/java/javafx/css/Match.java line 1: > 1: /* The compare method seems wrong. I think it should delegate to `Integer.compare`. `specificity` should also be made `private`. - PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1295306021
