Integrated: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Fri, 23 Feb 2024 17:46:20 GMT, Nir Lisker wrote: > Fixes for the `AnchorPane` docs, as well as for the `NodeOrientation` docs in > `Node` and `Scene`. > > Note that the default value for a `Scene`'s `NodeOrientation` depends on a > system property, while for `Node` it isn't (which means `SubScene` will be > different from `Scene`). Not sure if this is intended. This pull request has now been integrated. Changeset: b43c4edf Author:Nir Lisker URL: https://git.openjdk.org/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b Stats: 66 lines in 3 files changed: 19 ins; 25 del; 22 mod 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc 8318624: API docs specify incorrect default value for nodeOrientation property Reviewed-by: angorya, kcr - PR: https://git.openjdk.org/jfx/pull/1379
[jfx22] RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
Backport of commit [b43c4edf](https://github.com/openjdk/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. - Commit messages: - Backport b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b Changes: https://git.openjdk.org/jfx/pull/1380/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1380&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325550 Stats: 66 lines in 3 files changed: 19 ins; 25 del; 22 mod Patch: https://git.openjdk.org/jfx/pull/1380.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1380/head:pull/1380 PR: https://git.openjdk.org/jfx/pull/1380
Re: RFR: 8314147: Updated the PhongMaterial documentation [v4]
> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass > `Material`). Except for the introduction, I divided the documentation into 3 > sections: qualitative explanation, mathematical model (I wouldn't think it > necessary, but the current doc explains it), and examples. > > The reason for the verbosity of the doc is that I envisioned 2 target > audiences for this class. One is a Java developer who wants to understand the > terminology and workings of computer graphics or of the artists who are > already familiar with this domain. (How many Java developers know what > diffuse, specular and normal maps are?) The other is an artist who is already > familiar with the domain, but wants to see how this class compares with other > renderers. For this reason, I looked at the terminology used by engines like > Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump > vs. height vs. normal maps, or specular vs. roughness/smoothness). > > The examples I chose and some of the schematics are not the best, looking at > it retroactively, but I want to give enough time for reviewers and get this > into 22. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Clarifications for the diffuse component - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/0130d0f5..e367c882 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1378&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1378&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: [jfx22] RFR: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Sat, 24 Feb 2024 09:53:01 GMT, Nir Lisker wrote: > Backport of commit > [b43c4edf](https://github.com/openjdk/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. I note that this is a clean backport of a doc-only fix. Approved to go into jfx22. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1380#pullrequestreview-1899492670
Re: RFR: 8314147: Updated the PhongMaterial documentation [v5]
> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass > `Material`). Except for the introduction, I divided the documentation into 3 > sections: qualitative explanation, mathematical model (I wouldn't think it > necessary, but the current doc explains it), and examples. > > The reason for the verbosity of the doc is that I envisioned 2 target > audiences for this class. One is a Java developer who wants to understand the > terminology and workings of computer graphics or of the artists who are > already familiar with this domain. (How many Java developers know what > diffuse, specular and normal maps are?) The other is an artist who is already > familiar with the domain, but wants to see how this class compares with other > renderers. For this reason, I looked at the terminology used by engines like > Blender, Maya, UE4 and Unity and tried to mention the comparisons (like bump > vs. height vs. normal maps, or specular vs. roughness/smoothness). > > The examples I chose and some of the schematics are not the best, looking at > it retroactively, but I want to give enough time for reviewers and get this > into 22. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Remove redundant image copies - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/e367c882..83bf2eb1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1378&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1378&range=03-04 Stats: 65 lines in 3 files changed: 0 ins; 65 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
[jfx22] Integrated: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
On Sat, 24 Feb 2024 09:53:01 GMT, Nir Lisker wrote: > Backport of commit > [b43c4edf](https://github.com/openjdk/jfx/commit/b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. This pull request has now been integrated. Changeset: 6e1246e8 Author:Nir Lisker URL: https://git.openjdk.org/jfx/commit/6e1246e870aa6dafe05a3f544bb283aa90a0c078 Stats: 66 lines in 3 files changed: 19 ins; 25 del; 22 mod 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc 8318624: API docs specify incorrect default value for nodeOrientation property Reviewed-by: kcr Backport-of: b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b - PR: https://git.openjdk.org/jfx/pull/1380
Re: RFR: JDK-8285893: Hiding dialog and showing new one causes dialog to be frozen [v2]
On Fri, 23 Feb 2024 17:39:44 GMT, Kevin Rushforth wrote: > This changes one of the fundamental aspects of entering and exiting a nested > event loop. This will need a lot more analysis to show why this is the right > approach. Yes, this is right and I also thought about documenting that as soon as we think that this is the right approach. I completely agree that we need to analyse this. This is not a simple change but will affect Dialogs and every custom code that uses nested event loops (e.g. I used it for blocking the UI without freezing it for data loading purposes, where we want to wait until the code returns in a non UI blicking way). Maybe there is a better way as well, I just don't figured out one. As described, right now we rely that `enterNestedEventLoop` returns always, which is not happening immediately for ALL platforms, even though Platform has another implementation. Which makes me think the current approach has a flaw, resulting in this behaviour. > I see the same failure with the most recent patch as I did yesterday. I > looked over the changes, and I'm not convinced that this is the right > approach. Thanks for testing. As soon as my colleague is available, I will debug this issue. I don't get why only MacOS is failing, since the low level code is pretty much the same, we just continue right after we left the nested event loop. I agree that we should find out the root cause why MacOS behaves different. > I don't know of any supported way to run macOS in a VM on a Windows host, but > perhaps others will be able to help. That was also my understanding the last time I checked. AFAIK, think there is no official way from Apple, but VirtualBox may offer that. Will check as soon as I have more time. - PR Comment: https://git.openjdk.org/jfx/pull/1324#issuecomment-1962428672
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Tue, 20 Feb 2024 12:13:31 GMT, Jose Pereda wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add compile-time checks to GdkSeat > > I've just reverted the previous changes, and just applied the touch mask to > the `gdk_pointer_grab` function. Tests should be green now. @jperedadnr Would you confirm that scroll on a touch screen still works on Xorg ? My tests looks all good (manual and system/robot). - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1962431721
Re: RFR: JDK-8325402: TreeTableRow updateItem() does not check item with isItemChanged(..) unlike all other cell implementations [v2]
> `TreeTableRow` does not check the item with `isItemChanged(..)`, unlike all > other implementations of the cell. > > This also means that the `TreeTableRow` always updates the item, although it > should not, resulting in a performance loss as a `TreeTableRow` will always > call `updateItem(..)`. > > It looks like that this was forgotten in > [JDK-8092593](https://bugs.openjdk.org/browse/JDK-8092593). > > Checking the whole history, it looks like the following was happening: > 1. There was a check if the item is the same in all cell implementations > (with `.equals(..)`) > 2. Check was removed as it caused bugs > 3. Check was readded, but instead we first check the index (same index) and > then if we also have the same item (this time with `.isItemChanged(..)`, to > allow developers to subclass it) > 4. Forgotten for `TreeTableRow` inbetween this chaos. > > Added many tests for the case where an item should be changed and where it > should not. > Improved existing tests as well. Using `stageLoader`, as before the tests > only created a stage when doing the assert. > > NOTE: The update logic is now the same as for all other 5 cell > implementations. Especially `TreeCell` (since it is for a tree structure as > well). Marius Hanl has updated the pull request incrementally with two additional commits since the last revision: - JDK-8325402: remove labeled if - JDK-8325402: Unit test improvements - Changes: - all: https://git.openjdk.org/jfx/pull/1360/files - new: https://git.openjdk.org/jfx/pull/1360/files/947b4153..a7584d81 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1360&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1360&range=00-01 Stats: 29 lines in 10 files changed: 11 ins; 12 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1360.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1360/head:pull/1360 PR: https://git.openjdk.org/jfx/pull/1360
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Wed, 21 Feb 2024 10:01:37 GMT, Karthik P K wrote:
>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation
>> conditions were not considered, hence hit test values such as character
>> index and insertion index values were incorrect.
>>
>> Added checks for RTL orientation of nodes and fixed the issue in
>> `getHitInfo()` to calculate correct hit test values.
>>
>> Added system tests to validate the changes.
>
> Karthik P K 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 13 additional
> commits since the last revision:
>
> - Merge branch 'master' into rtl_hittest_issue
> - Code refactoring
> - Review comments
> - Fix emoji issue
> - Inline node issue fix
> - Code review changes
> - Fix issue with multiline text
> - Fix issue with RTL text within LTR text
> - Review changes
> - Fix multiline text insertion index calculation issue
> - ... and 3 more: https://git.openjdk.org/jfx/compare/02e8b6ba...e3812732
I'm confused by this part:
> we will not have information to decide if the character index should be
> calculated relative to Text node or TextFlow
The character index, even in your example image above, seems to be relative to
the `Text`, not to the `TextFlow` (it is 0, even though there are clearly 10
characters before it belonging to another `Text`), so what exactly do you mean
with this?
What I find it confusing further more is that if I request the `HitInfo` of the
`TextFlow` container (which I think would make more sense to do) that these
values are subtly different. Take this modification to your test program:
if (n instanceof Text t) {
Point3D p3 = pick.getIntersectedPoint();
Point2D p = new Point2D(p3.getX(), p3.getY());
HitInfo h = t.getParent() instanceof TextFlow tf ?
tf.hitTest(p) : t.hitTest(p);
hitInfo2.setText("TextFlow: " + h + "\nText: " + t.hitTest(p));
}
When used with these two texts:
t("Arabic**"),
t("ABCD")
You'll find that the values perfectly match for the first `Text` in the flow,
but for the 2nd `Text` in the flow they're subtly different. Somehow I think
that's already very strange. The reported character indices are always
relative to the `Text`s though, which is why I'm confused about your earlier
statement. Your fix however does mean that the `HitInfo` for `Text` seems to
be more accurate, but it doesn't match with what `TextFlow` reports (in other
words, the `hitTest` of `TextFlow` was and is broken still, your fix didn't
change that).
I'm still looking into why `PrismTextLayout` needs to be "aware" of texts being
nested into `TextFlow` -- this seems like such an odd thing to me that I find
it tough to let it go. I think that if we know why `hitTest` of `TextFlow` is
broken, then that it will also make clear why it currently must be aware of the
nesting; and I think once fixed, it won't need to know this anymore.
In its current form, the `hitTest` method on `TextFlow` seems completely
useless. The reported `HitInfo` can't be made sense of without knowing which
child we're talking about. I think it may make sense to extend `HitInfo` with
the `Node` involved.
-
PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1962749265
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Wed, 21 Feb 2024 10:01:37 GMT, Karthik P K wrote:
>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation
>> conditions were not considered, hence hit test values such as character
>> index and insertion index values were incorrect.
>>
>> Added checks for RTL orientation of nodes and fixed the issue in
>> `getHitInfo()` to calculate correct hit test values.
>>
>> Added system tests to validate the changes.
>
> Karthik P K 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 13 additional
> commits since the last revision:
>
> - Merge branch 'master' into rtl_hittest_issue
> - Code refactoring
> - Review comments
> - Fix emoji issue
> - Inline node issue fix
> - Code review changes
> - Fix issue with multiline text
> - Fix issue with RTL text within LTR text
> - Review changes
> - Fix multiline text insertion index calculation issue
> - ... and 3 more: https://git.openjdk.org/jfx/compare/98fd3ec3...e3812732
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 606:
> 604: boolean addLtrIdx = run.getTextSpan().getText().length() !=
> run.length;
> 605: if (r.getStart() != curRunStart && !r.isLinebreak() &&
> addLtrIdx
> 606: && r.getTextSpan().getText().equals(text)) {
I'm concerned about this check: `r.getTextSpan().getText().equals(text)` -- it
seems to me that it either must be irrelevant, or it if is relevant, what if I
have many `Text` nodes in a `TextFlow` that happen to have the same text? Let's
say I have an English text, where I give each word its own `Text` node in a
`TextFlow`. There would be many duplicates... so I find it hard to believe
this check could accomplish anything.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1501710956
