Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Mon, 26 Feb 2024 16:22:56 GMT, Andy Goryachev wrote: >> You are right. It fails when there are repeated text nodes. I will look into >> this > > yes, this bothered me from the start. I did have a test case in the MT with > two text nodes with the same text, and it seemed to work correctly. or did I > miss something? Actually most of the repeating text cases are handled. I saw failure in only one case where a Text node containing mix of LTR and RTL text is repeated. For example if we have a Text node with content like "شزذيArabic" repeated, then it fails. If the Text node contains only LTR or RTL text then no issues are seen. - PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1503644403
Re: RFR: JDK-8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
On Mon, 26 Feb 2024 20:51:56 GMT, Marius Hanl wrote: > This PR fixes the problem that maximizing/fullscreen a `Stage` or `Dialog` is > broken when `sizeToScene()` was called before or after. > > The approach here is to ignore the `sizeToScene()` request when the `Stage` > is maximized or set to fullscreen. > Otherwise the Window Manager of the OS will be confused and you will get > weird flickering or wrong Window buttons (e.g. on Windows, the 'Maximize' > button still shows the 'Restore' Icon, while we already resized the `Stage` > to not be maximized). Since this involved changing the specified behavior it will need a CSR. If we agree that this is the right behavior, then the CSR will be trivial. - PR Comment: https://git.openjdk.org/jfx/pull/1382#issuecomment-1965592210
RFR: JDK-8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
This PR fixes the problem that maximizing/fullscreen a `Stage` or `Dialog` is broken when `sizeToScene()` was called before or after. The approach here is to ignore the `sizeToScene()` request when the `Stage` is maximized or set to fullscreen. Otherwise the Window Manager of the OS will be confused and you will get weird flickering or wrong Window buttons (e.g. on Windows, the 'Maximize' button still shows the 'Restore' Icon, while we already resized the `Stage` to not be maximized). - Commit messages: - JDK-8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window Changes: https://git.openjdk.org/jfx/pull/1382/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1382=00 Issue: https://bugs.openjdk.org/browse/JDK-8326619 Stats: 179 lines in 3 files changed: 179 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1382.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1382/head:pull/1382 PR: https://git.openjdk.org/jfx/pull/1382
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
> 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: Fixed typo - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/83bf2eb1..60d45bc8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=05 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Mon, 26 Feb 2024 10:40:03 GMT, Karthik P K wrote:
>> 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.
>
> You are right. It fails when there are repeated text nodes. I will look into
> this
yes, this bothered me from the start. I did have a test case in the MT with
two text nodes with the same text, and it seemed to work correctly. or did I
miss something?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1502940731
Integrated: 8320965: Scrolling on a touch enabled display fails on Wayland
On Tue, 12 Dec 2023 11:19:23 GMT, Jose Pereda wrote: > This PR replaces the deprecated `gdk_pointer_grab` with `gdk_seat_grab`, and > `gdk_pointer_ungrab ` with `gdk_seat_ungrab`, using runtime checks and > wrapped functions for GTK 3.20+ (so systems without it still run with GTK > 3.8+), and fixes the dragging issue on Wayland. This pull request has now been integrated. Changeset: df3707d7 Author:Jose Pereda URL: https://git.openjdk.org/jfx/commit/df3707d7444c542ba55a8e76a8ed7e8f0637e874 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8320965: Scrolling on a touch enabled display fails on Wayland Reviewed-by: kcr, tsayao - PR: https://git.openjdk.org/jfx/pull/1305
Re: RFR: 8314147: Updated the PhongMaterial documentation [v5]
On Sat, 24 Feb 2024 14:40:25 GMT, Nir Lisker wrote: >> 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 This is a great docs update and exhaustive explanation of how Phong model works. I found one typo to iron out, otherwise LGTM (I will do a second pass once backgrounds are updated). modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java line 227: > 225: * the total specular component contribution is S * > CSM. > 226: * > 227: * Slef-Illumination Typo - `s/Slef/Self` - Changes requested by lkostyra (Committer). PR Review: https://git.openjdk.org/jfx/pull/1378#pullrequestreview-1901233188 PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1502832050
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v9]
On Sat, 20 Jan 2024 20:34:50 GMT, Johan Vos wrote: >> A listener was added but never removed. >> This patch removes the listener when the menu it links to is cleared. Fix >> for https://bugs.openjdk.org/browse/JDK-8319779 > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Add additional test for IOOBE detection. > This test comes from JDK-8323787 Getting back to this I ran the test on macOS and it now works as expected. However, one of the new tests fails on Linux: SystemMenuBarTest > testFocusMemoryLeak FAILED org.opentest4j.AssertionFailedError: expected: but was: at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) at app//org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:40) at app//org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:35) at app//org.junit.jupiter.api.Assertions.assertFalse(Assertions.java:227) at app//test.com.sun.javafx.tk.quantum.SystemMenuBarTest.testFocusMemoryLeak(SystemMenuBarTest.java:198) Btw, all of my tests were done in a local branch with the latest upstream master merged in. @johanvos since the source branch is now a bit out of date, can you merge in the latest upstream master? Also, I think there are still some unaddressed questions from John and Andy. Regarding the following: > All tests now pass, but I noticed that in some cases, the systemtests do not > correctly work with the application lifecycle management (see > https://mail.openjdk.org/pipermail/openjfx-dev/2024-January/044516.html). For > now, I consider this anomaly to be independent from JDK-8319779 Yes, this is unrelated to the memory leak being fixed by this PR. We should file a bug for the activation problem. - PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1964373792
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/579ce0f7...e3812732
Okay, I've been looking into this myself. First, I've limited myself to the
simple case of LTR text with standard English text. I used 5 Text Nodes
containing `The quick brown fox jumped over the lazy dog`. Using the "old"
`getHitInfo(float, float)` code from JavaFX 21. This code does not use the
extra 3 parameters it is given. This is what I got initially:
In the above image, there are two lines drawn below each character. The first
line is the `character index % 6` returned by `TextFlow` for each possible `x`
position. It's correct everywhere and returns the correct character offset.
The second line is the `character index` returned for the `Text` at the same
location. There are two things standing out here:
- The `Text` character index is based on the first line, even for each
subsequent line. The character widths therefore don't match properly.
- The `Text` character index does not reset to dark blue for the first
character (The `T` of the second `The` is colored Orange, while if the
character index resets it should have been Dark Blue).
The reason this is all incorrect is because the `TextLayout` used by the `Text`
is the one from its parent. So it must pass coordinates in the parents
coordinate system. If fix that, I get this:
Note that one of the two problems has disappeared. The returned character
index is still incorrect (The `T` of the second `The` is colored Orange, while
if the character index resets it should have been Dark Blue), but the widths
now match perfectly.
So, I now also fix the character index by subtracting `textRunStart` from the
returned `TextLayout.Hit`, and now it looks like this:
Note that now the character index for each `T` of `The` starts as Dark Blue.
So, I used the `getHitInfo` code from JavaFX 21 + this code in `Text`:
public final HitInfo hitTest(Point2D point) {
if (point == null) return null;
TextLayout layout = getTextLayout();
double x = point.getX() - getX();
double y = point.getY() - getY() + getYRendering();
GlyphList[] runs = getRuns();
int runIndex = findRunIndex(x, y, runs);
int textRunStart = 0;
int curRunStart = 0;
if (runs.length != 0) {
textRunStart = findFirstRunStart(runs);
curRunStart = ((TextRun) runs[runIndex]).getStart();
}
double px = x;
double py = y;
if(isSpan()) {
Point2D ppoint = localToParent(point);
px = ppoint.getX();
py = ppoint.getY();
}
TextLayout.Hit h = layout.getHitInfo((float)px, (float)py,
getTextInternal(), textRunStart, curRunStart);
return new HitInfo(h.getCharIndex() - textRunStart,
h.getInsertionIndex(), h.isLeading());
}
>From this "base" code I think the next step is to look how to fix mirrored
>text.
Here's the test program I used:
import javafx.animation.Animation;
import javafx.animation.KeyFrame;
import javafx.animation.Timeline;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.geometry.Point2D;
import javafx.geometry.Point3D;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.canvas.Canvas;
import javafx.scene.canvas.GraphicsContext;
import javafx.scene.control.Label;
import javafx.scene.input.MouseEvent;
import javafx.scene.input.PickResult;
import javafx.scene.layout.StackPane;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.scene.text.Font;
import
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v4]
On Tue, 20 Feb 2024 12:16:12 GMT, Jose Pereda wrote: >> This PR replaces the deprecated `gdk_pointer_grab` with `gdk_seat_grab`, and >> `gdk_pointer_ungrab ` with `gdk_seat_ungrab`, using runtime checks and >> wrapped functions for GTK 3.20+ (so systems without it still run with GTK >> 3.8+), and fixes the dragging issue on Wayland. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Revert changes and add touch mask to gdk pointer grab function All ok. - Marked as reviewed by tsayao (Committer). PR Review: https://git.openjdk.org/jfx/pull/1305#pullrequestreview-1900802532
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Sun, 25 Feb 2024 13:16:06 GMT, John Hendrikx wrote:
>> 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/993c5e2e...e3812732
>
> tests/system/src/test/java/test/robot/javafx/scene/RTLTextFlowCharacterIndexTest.java
> line 238:
>
>> 236:
>> 237: @Test
>> 238: public void testTextAndTextFlowHitInfoForRTLEnglishText() throws
>> Exception {
>
> I just replaced `PrismTextLayout#getHitInfo` with `return new Hit(0, 0,
> true);` and this test, and all the others still pass.
>
> I think the tests should check for a given range of x values if the correct
> character is being returned as the hit, perhaps more inspired by the tests in
> https://github.com/openjdk/jfx/pull/1236
I'm not sure if we could write headless test for this. Could you please point
me to a test which could be helpful for me?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1502469288
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]
On Sat, 24 Feb 2024 23:00:43 GMT, John Hendrikx wrote:
>> 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/05c327e9...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.
You are right. It fails when there are repeated text nodes. I will look into
this
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1502392096
Re: Proposal: RichTextArea Control (Incubator)
Having read the interesting posts following this announcement, let me add a few points from my perspective: I understand and agree we Johan Vos that there are a lot of open issues in JBS that should be addressed. At the same time I would very much appreciate if a RichTextArea were implemented in core JavaFX as opposed to a library. Having good controls implemented within JavaFX makes it more attractive to application developers. To that end I don't think it good to develop all kinds of controls outside of JavaFX. The current TextArea makes us laughing stock: If you load any kind of medium-sized text in it, you're application will stop to work, because there are too many elements in the scene graph. We're currently using RichTextFX in our application and it gives us some headaches (Can't wrap my head around the API, graphic errors under Windows, etc.). I haven't known about Gluon's RichTextArea before. May well be giving it a try (but will have to check if commercial licensing is viable for us). I have some editor building background myself: About 25 years ago, I built the editor for a commercial IDE (SNiFF+ by TakeFive, then Windriver, now Intel ...), which was written in C++ with ET++. This editor was then ported to Java / Swing (called RED Editor) which was already open sourced. About 6 years ago, I tried to "port" the RED Editor from Swing to JavaFX (https://github.com/effad/rtefx). The attempt failed for two reasons: I did not have the time to do it properly (Implementing a rich text control is a lot of work) and JavaFX was just too closed at that time. Robert
