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