RFR: 8325154: resizeColumnToFitContent is slower than it needs to be
The PR simply moves column and view-updates outside the loop. Since the column or view never changes within the for-loop it is not necessary to call these again and again. - Commit messages: - 8325154: resizeColumnToFitContent is slower than it needs to be Changes: https://git.openjdk.org/jfx/pull/1358/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1358&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325154 Stats: 8 lines in 1 file changed: 4 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1358.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1358/head:pull/1358 PR: https://git.openjdk.org/jfx/pull/1358
Re: CSS Performance regression inTableColumnHeader.resizeColumnToFitContent
Am 01.02.24 um 16:53 schrieb John Hendrikx: Is it still a CSS performance degradation then? I mean, it's the same speed, and the number of calls should be irrelevant? A performance degradation must be present, since after the expected deterioration of JFX 19 it has become twice as slow since. The assumption that is has something to do with CSS is based on the fact that we also have rendering performance problems in TableView in our production application where a column with a cell factory exists that has lots of nodes in a cell. However, if you want to narrow it down further (perhaps there is more performance to be gained), you could run your tests against the early access builds. You may be able to find a set of 10-20 isolated commits that could have led to the introduction of the extra 60.000 calls. For example, check if the problem is present in 20-ea+1 up to 20-ea-19, 20, 20.0.1 and 20.0.2 (early access versions available are 1, 2, 3, 4, 6, 7, 9, 11, 19), see here: https://mvnrepository.com/artifact/org.openjfx/javafx-base Good idea, I will try that. And I will also create an issue / PR for the "low hanging fruits" in resizeColumnToFitContent. Robert
Re: Integrated: 8324658: Allow animation play/start/stop/pause methods to be called on any thread
And thank you for raising the issue and answering the questions we had concerning the use case. This will be in the next promoted build, JavaFX 22+28 (as well as JavaFX 23+3). -- Kevin On 2/1/2024 12:42 AM, Jurgen Doll wrote: A big THANK YOU to everybody that was part of this process. It's very much appreciated ! Regards Jurgen On Tue, 30 Jan 2024 11:27:44 +0200, Nir Lisker wrote: On Fri, 26 Jan 2024 23:59:50 GMT, Nir Lisker wrote: Added a utility method to run code on the FX thread if it's not already, and changed the animation methods to use it. This pull request has now been integrated. Changeset: c5ab220b Author: Nir Lisker URL: https://git.openjdk.org/jfx/commit/c5ab220bbc885f2aa99d8c1d5ed8f1753e39251f Stats: 422 lines in 7 files changed: 225 ins; 148 del; 49 mod 8324658: Allow animation play/start/stop/pause methods to be called on any thread Reviewed-by: kcr, jpereda - PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8320912: IME should commit on focus change
On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox wrote:
> This is a Mac only bug. If the user was in the middle of IM text composition
> and clicked on a different node the partially composed text was left in the
> old node and the IM window wasn't dismissed. This PR implements the existing
> finishInputMethodComposition call so it can commit the text and dismiss the
> IM window before focus moves away from the node where composition was taking
> place.
>
> This PR changes the implementation of `unmarkText` to match what we want and
> what Apple says it should do ("The text view should accept the marked text as
> if it had been inserted normally"). With that said I haven't found an IME
> that calls this routine.
You are right - I should have checked a native app. It works as expected then.
Thank you for fixing this bug!
-
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1356#pullrequestreview-1856993847
Re: Binding properties to constant values
Hi John, If the issues with StyleOrigin are not as problematic as I presented, it comes out to the behavioral flexibility we have. Can we change the precedence order at this point? Do users rely on it or are surprised by it (I see the latter, but those who think it works correctly are not going to say that)? Perhaps Kevin can weigh in. The question of how to do it is simpler, and can be done with or without breaking changes to StyleOrigin. On Thu, Feb 1, 2024 at 2:27 AM John Hendrikx wrote: > Hi Nir, > > On 31/01/2024 22:36, Nir Lisker wrote: > > > > I would wait with the ramifications of setting competing values from > > different origins until the question of precedence is answered. > > Perhaps emitting warnings is better, though I can see some scenarios > > in which they will be annoying. > > > > The way I see the order: > > 1. Setting from code should always take precedence (including the > > current bindings over setter order of course). > > 2. INLINE origin (via setStyle). > > 3. Stylesheets according to their StyleOrigin as specified by the > > non-javafx css specifications [3]: AUTHOR > USER > USER_AGENT (see > > below for INLINE). > > > > 2 and 3 are already (more or less) specified in JavaFX's css as far as > > I can see. However, 1 is not css, hence I don't think StyleOrigin > > should be applicable here even. Even more, 2 isn't really css either, > > it mimics html tags and shouldn't count as a css StyleOrigin in my > > opinion. > StyleOrigin is more of an internal thing that got exposed, so I wouldn't > worry too much about what is CSS and what isn't. I agree that the above > order would make more sense. I suspect the reasoning why it wasn't done > is that setters are getting called to set up defaults, which if they by > default override all, would conflict with style sheets set by the user. > > Note also that a Stylesheet can set its origin [4], even to INLINE, so > > it takes precedence over java property setters and conflicts with > > 'setStyle' "real" INLINE. I'm not sure if this is a bug because the > > javafx css specs say that "Inline styles are specified via the Node > > setStyle AP". > > The CSS API is weird. It has many seemingly useful and public bits, but > they are isolated from the rest of FX. > > Take StyleSheet for example. You can convert them (using File to File) > from regular to binary, then load a binary file and get a StyleSheet > reference with a public API. You can then change its origin. However, > it stops there. You can't feed such a modified StyleSheet to FX via any > public API. > > It almost looks like a lot of the CSS public API is there for a possibly > previously integrated Scene Builder type application, which was later > split off. > > > > > So, if I were to able to do anything I wanted, I would have restricted > > Stylesheets to the options in 3, remove INLINE from a public > > perspective and apply it only behind the scenes to 'setStyle' calls, > > and stop treating java settings in the css hierarchy (which means > > removing the USER StyleOrigin from them). Obviously that breaks a lot > > of code, but this behavior would be my general goal. As for how to > > represent it, maybe a constant can be added to StyleOrigin to > > represent java code settings, but that's not a real css origin. I > > guess we could call INLINE and the hypothetical JAVA constants > > "pseudo-origins", because they don't apply to stylesheets, and are > > only used internally. Or just don't check StyleOrigin when the value > > is set from java. There are probably more ways. > > StyleOrigin really is only there for the CSS engine to distinguish if it > can replace a value or not with something else depending on precedence. > It probably shouldn't have been public at all, aside from the fact that > StyleableProperty being an interface forced it to be public. > > Oddly enough, the StyleableProperty interface is public, but it is again > never returned anywhere (didn't check extensively). So you can't > actually do "label.textAlignment().applyStyle( ... )" even though text > alignment is a styleable property. > > If `textAlignment()` did return a StyleableProperty, then you could do > `label.textAlignment().applyStyle(StyleOrigin.CODE, value)` for a > permanent override of the value which the CSS engine will respect. > > > > > I also wonder if StyleOrigin should implement Comparable for > > the precedence calculations of stylesheets. > > It sort of already does, they are defined in the order of least to > highest precedence, so an integer comparator on its ordinal can be used. > > --John > >
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Thu, 1 Feb 2024 08:06:18 GMT, Karthik P K wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
>> line 513:
>>
>>> 511: if ((x > run.getWidth() &&
>>> (!isMultiRunText || run.getStart() == curRunStart)) || textWidthPrevLine >
>>> 0) {
>>> 512: getBounds(run.getTextSpan(),
>>> textBounds);
>>> 513: x -= (runs[0].getLocation().x -
>>> textBounds.getMinX());
>>
>> suggestion: we are still in the for loop, so perhaps it makes sense to
>> extract
>> `(runs[0].getLocation().x - textBounds.getMinX());`
>> to a variable outside of the loop
>
> The idea is that outside the loop we don't know if we need to subtract the
> textBound min x value and the starting location of the first run or not. That
> is why this is present inside the loop. Once this is done we are breaking out
> of the loop so this will not get called multiple times.
> Let me know if you have any suggestions.
you are right: I missed the `break` in line 520
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1474745002
JavaFX 22 is in Rampdown Phase Two (RDP2)
To: JavaFX Developers As a reminder, JavaFX 22 is now in Rampdown Phase Two (RDP2). [1] P1-P2 bug fixes, and test or doc fixes of any priority, can be fixed during RDP2. Explicit approval is needed for all bug fixes and enhancements (except for doc and test fixes) to go in to the jfx22 branch [2]. The bar for approving bug fixes is appropriately high at this point. We do not anticipate approving any more enhancements. Now that we are in RDP2, the goal is to stabilize what is there, fixing bugs that are new in JavaFX 22. We need to be extremely careful about including anything that introduces risk. We will use the same rules for RDP2 that the JDK uses [3], with two modifications: 1. Approval is needed from one of the OpenJFX project leads (not the OpenJDK project lead) 2. Since we are not part of the JDK, we need to use labels that do not collide with the JDK 22 release. As an obvious choice, derived from the JBS fix version, we will use "jfx22-fix-request", "jfx22-fix-yes", "jfx22-fix-no" and "jfx22-fix-nmi", "jfx22-enhancement-request", "jfx22-enhancement-yes", "jfx22-enhancement-no" and "jfx22-enhancement-nmi" as corresponding labels. REMINDER: In this release we will integrate almost all stabilization changes via backports from the master branch [4]. * Almost all fixes intended for the jfx22 stabilization branch will also be applicable to the master branch. Integrate such a change into the master branch first. Then, after you have obtained any required approvals, backport it to the stabilization branch using the Skara `/backport` command or, if necessary, by manually opening a backport PR with the title `Backport $HASH`, where `$HASH` is the original commit hash. (The JDK Developers’ Guide contains more information on working with backports [5].) * Some fixes will be specific to the stabilization branch and not applicable to the master branch. Integrate such a change directly into the stabilization branch. IMPORTANT: Reviewers and Committers now have an extra responsibility to double-check the target branch of each PR that they review, integrate, or sponsor. By default a PR will be targeted to `master` which is the main development line (JavaFX 23 as of today). This is usually what we want. A backport PR should be targeted to `jfx22` if, and only if, it is intended for JavaFX 22 and meets the criteria for the current rampdown phase (we're in RDP2 as of today). Reviewers are advised to be extra cautious in approving potentially risky fixes targeted to `jfx22`. If there is a concern, then a reviewer can as part of the review indicate that the PR should be retargeted to `master` for 23 (or withdrawn if it is a backport of a fix already in `master`). Reviewers also need to be extra careful when reviewing PRs targeted to jfx22 that it doesn't mistakenly contain any commits from the master branch. You'll be able to tell because the diffs will contain changes that are not part of the fix being reviewed. Such a PR will either need to be closed and redone, or it will need to be rebased and force-pushed. This should be less of a problem for this release, since almost all PRs for jfx22 will be done as backport-style PRs, but it can still be a problem if the developer mistakenly merges master into their backport branch. Let me know if there are any questions. -- Kevin [1] https://mail.openjdk.org/pipermail/openjfx-dev/2023-September/042703.html [2] https://github.com/openjdk/jfx/tree/jfx22 [3] http://openjdk.org/jeps/3 [4] https://openjdk.org/jeps/3#Integrating-fixes-and-enhancements [5] https://openjdk.org/guide/#working-with-backports-in-jbs
Re: CSS Performance regression inTableColumnHeader.resizeColumnToFitContent
On 01/02/2024 13:00, Robert Lichtenberger wrote: Hi, We are seeing degraded performance in our production application concerning the "fit to content" function of TableViews. I've developed a little benchmark program that can be found at https://gist.github.com/effad/9eebee0c1e86a8e605cb55ced9485dd4 Here's the last lines of data from runs against different JavaFX versions: JFX 17.0.10+2 average run time: 848 JFX 18.0.1+2 average run time: 839 JFX 19.0.2+1 average run time: 1113 JFX 20.0.2+3 average run time: 1656 JFX 21.0.2+5 average run time: 2460 17 and 18 are almost the same. The performance penalty of 19 most likely is due to (my own) PR in https://github.com/openjdk/jfx/pull/757, which put's the cell used to measure the needed width of the column into a row. The penalties from 19 to 20 and 20 to 21 however are worse than that. To investigate further I let the benchmark run (with no warmup, just 3 iterations and only 5000 rows) in the profiler Visual VM 2.1.7. Looking at _the_ hotspot of the execution I can see that javafx.scene.CssStyleHelper.transitionToState is consuming most of the time. And I can see that this method is called: ~ 30.000 times in JFX 18 ~ 45.000 times in JFX 19 (which is 3 iterations * 5000 rows more than in JFX 18, as expected since we have an additional row whose css needs to be applied) ~ 105.000 times in JFX 20 ~ 105.000 times in JFX 21 In looking at my changes for PR 757, I noticed that there are calls to cell.updateTableColumn(tc) and cell.updateTableView(tv) that need not be within the loop iterating over the rows. When putting these two lines before the loop, I was able to measure: JFX 23-internal+0-2024-02-01-061822 average run time: 1119 which would be roughly at the performance lebel of JFX 19 again. Interestingly, when profiling with this optimization I still get 105k calls to transitionToState, but they seem to be much faster. Is it still a CSS performance degradation then? I mean, it's the same speed, and the number of calls should be irrelevant? However, if you want to narrow it down further (perhaps there is more performance to be gained), you could run your tests against the early access builds. You may be able to find a set of 10-20 isolated commits that could have led to the introduction of the extra 60.000 calls. For example, check if the problem is present in 20-ea+1 up to 20-ea-19, 20, 20.0.1 and 20.0.2 (early access versions available are 1, 2, 3, 4, 6, 7, 9, 11, 19), see here: https://mvnrepository.com/artifact/org.openjfx/javafx-base I checked the recent changes I did for CSS: * CSS performance regression up to 10x (https://bugs.openjdk.org/browse/JDK-8322795) * Quadratic layout time with nested nodes and pseudo-class in style sheet (https://bugs.openjdk.org/browse/JDK-8199216) * Public API in javafx.css.Match should not return private API class PseudoClassState (https://bugs.openjdk.org/browse/JDK-8304959) * Region#padding property rendering error (https://bugs.openjdk.org/browse/JDK-8245919) However, none of those were present in JavaFX 20. --John
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 1 Feb 2024 12:09:36 GMT, Ambarish Rapte wrote: > @Maran23 This is ready for integration. If you were waiting for me. Thanks, > It looks good, I don't have any comments. Thank you for checking as well! - PR Comment: https://git.openjdk.org/jfx/pull/1331#issuecomment-1921417204
Integrated: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl wrote:
> For some reason the `skinProperty` did not allow to set a new skin when it is
> the same class as the previous one.
> This leads to multiple issues:
> 1. When creating a new skin (same class as previous), the skin will likely
> install listener but is then rejected when setting it due to the
> `skinProperty` class constraint
> -> `PopupControl` is in a weird state as the current skin was not disposed
> since it is still set, although we already created and 'set' a new skin
> 2. A skin of the same class can behave differently, so it is not really valid
> to reject a skin just because it is the same class as the previous
> -> Just imagine we have the following skin class
>
> class MyCustomSkin extends Skin {
> public MyCustomSkin(C skinnable, boolean someFlag) { ... }
> }
>
> Now if we would change the skin of the `PopupControl` two times like this:
>
> popup.setSkin(new MyCustomSkin(popup, true));
> popup.setSkin(new MyCustomSkin(popup, false));
>
> The second time the skin will be rejected as it is the same class, although I
> may changed the skin behaviour, in this case demonstrated by a boolean flag
> for showing purposes.
>
> This is the same issue and fix as done for `Control` in
> [JDK-8276056](https://bugs.openjdk.org/browse/JDK-8276056) (PR:
> https://github.com/openjdk/jfx/pull/806)
This pull request has now been integrated.
Changeset: aac2df16
Author:Marius Hanl
URL:
https://git.openjdk.org/jfx/commit/aac2df168d524f97d663de6776962773702b360c
Stats: 42 lines in 2 files changed: 28 ins; 12 del; 2 mod
8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded
Skin
Reviewed-by: angorya, mstrauss
-
PR: https://git.openjdk.org/jfx/pull/1331
Re: RFR: JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin
On Thu, 11 Jan 2024 20:13:09 GMT, Marius Hanl wrote:
> For some reason the `skinProperty` did not allow to set a new skin when it is
> the same class as the previous one.
> This leads to multiple issues:
> 1. When creating a new skin (same class as previous), the skin will likely
> install listener but is then rejected when setting it due to the
> `skinProperty` class constraint
> -> `PopupControl` is in a weird state as the current skin was not disposed
> since it is still set, although we already created and 'set' a new skin
> 2. A skin of the same class can behave differently, so it is not really valid
> to reject a skin just because it is the same class as the previous
> -> Just imagine we have the following skin class
>
> class MyCustomSkin extends Skin {
> public MyCustomSkin(C skinnable, boolean someFlag) { ... }
> }
>
> Now if we would change the skin of the `PopupControl` two times like this:
>
> popup.setSkin(new MyCustomSkin(popup, true));
> popup.setSkin(new MyCustomSkin(popup, false));
>
> The second time the skin will be rejected as it is the same class, although I
> may changed the skin behaviour, in this case demonstrated by a boolean flag
> for showing purposes.
>
> This is the same issue and fix as done for `Control` in
> [JDK-8276056](https://bugs.openjdk.org/browse/JDK-8276056) (PR:
> https://github.com/openjdk/jfx/pull/806)
@Maran23
This is ready for integration. If you were waiting for me. Thanks, It looks
good, I don't have any comments.
-
PR Comment: https://git.openjdk.org/jfx/pull/1331#issuecomment-1921183306
CSS Performance regression inTableColumnHeader.resizeColumnToFitContent
Hi, We are seeing degraded performance in our production application concerning the "fit to content" function of TableViews. I've developed a little benchmark program that can be found at https://gist.github.com/effad/9eebee0c1e86a8e605cb55ced9485dd4 Here's the last lines of data from runs against different JavaFX versions: JFX 17.0.10+2 average run time: 848 JFX 18.0.1+2 average run time: 839 JFX 19.0.2+1 average run time: 1113 JFX 20.0.2+3 average run time: 1656 JFX 21.0.2+5 average run time: 2460 17 and 18 are almost the same. The performance penalty of 19 most likely is due to (my own) PR in https://github.com/openjdk/jfx/pull/757, which put's the cell used to measure the needed width of the column into a row. The penalties from 19 to 20 and 20 to 21 however are worse than that. To investigate further I let the benchmark run (with no warmup, just 3 iterations and only 5000 rows) in the profiler Visual VM 2.1.7. Looking at _the_ hotspot of the execution I can see that javafx.scene.CssStyleHelper.transitionToState is consuming most of the time. And I can see that this method is called: ~ 30.000 times in JFX 18 ~ 45.000 times in JFX 19 (which is 3 iterations * 5000 rows more than in JFX 18, as expected since we have an additional row whose css needs to be applied) ~ 105.000 times in JFX 20 ~ 105.000 times in JFX 21 In looking at my changes for PR 757, I noticed that there are calls to cell.updateTableColumn(tc) and cell.updateTableView(tv) that need not be within the loop iterating over the rows. When putting these two lines before the loop, I was able to measure: JFX 23-internal+0-2024-02-01-061822 average run time: 1119 which would be roughly at the performance lebel of JFX 19 again. Interestingly, when profiling with this optimization I still get 105k calls to transitionToState, but they seem to be much faster. So I see two issues here: a) A general CSS-related performance degradation b) A potential to optimize resizeColumnToFitContent() Is it ok to open two issues for that and provide a PR for b)? I have no idea what to do about a), maybe someone with more experience in the CSS stuff can look at it? Thanks, Robert
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v3]
On Mon, 29 Jan 2024 21:29:31 GMT, Laurent Bourgès wrote: >> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > fixed copyright years of modified files + fixed comments in > DMarlinPrismUtils - Provided a few comments in test, and - Copyright year needs to be updated in [modules/javafx.graphics/src/main/java/com/sun/marlin/DPathConsumer2D.java](https://github.com/openjdk/jfx/pull/1348/files/251d1d61534eafbfa59165a348aa02e47a0242bd#diff-5b6489e7b2e4a135ca5aa33367dd1cc7e704feb12cabed7259256603be13488c) - PR Comment: https://git.openjdk.org/jfx/pull/1348#issuecomment-1921014234
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v2]
On Mon, 29 Jan 2024 09:44:04 GMT, Karthik P K wrote: >> Laurent Bourgès has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fixed test package > > modules/javafx.graphics/src/main/java/com/sun/marlin/DPathConsumer2D.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2007, 2022, Oracle and/or its affiliates. All rights >> reserved. > > Shouldn't it be 2007, 2024? In general it is not required to update the copyright year along with the fix, though it is not a hard rule. We update the years for all files before release. But as you are modifying please change this one to 24. - PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1474210526
Re: RFR: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0 [v3]
On Mon, 29 Jan 2024 21:29:31 GMT, Laurent Bourgès wrote: >> Fixed scale=0 in DMarlinPrismUtils + added new test Scale0Test.java > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > fixed copyright years of modified files + fixed comments in > DMarlinPrismUtils tests/system/src/test/java/test/com/sun/marlin/Scale0Test.java line 253: > 251: } > 252: > 253: } I had few comments about the test like: 1. MyApp class can be removed 2. if (button instanceof Labeled) can be removed 3. preparePane() method could be shortened. 4. Variables leftPane and slider could be local 5. final keyword could be removed from some local variables. 6. Rename the test file and test function. I made these changes locally and sharing the file as attached. [ScaleX0Test.zip](https://github.com/openjdk/jfx/files/14123536/ScaleX0Test.zip) - PR Review Comment: https://git.openjdk.org/jfx/pull/1348#discussion_r1474190840
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v8]
> 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 incrementally with one additional commit since the last revision: Code review changes - Changes: - all: https://git.openjdk.org/jfx/pull/1323/files - new: https://git.openjdk.org/jfx/pull/1323/files/52ee61cc..3012fae8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1323&range=07 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1323&range=06-07 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1323.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1323/head:pull/1323 PR: https://git.openjdk.org/jfx/pull/1323
Re: Integrated: 8324658: Allow animation play/start/stop/pause methods to be called on any thread
A big THANK YOU to everybody that was part of this process. It's very much appreciated ! Regards Jurgen On Tue, 30 Jan 2024 11:27:44 +0200, Nir Lisker wrote: On Fri, 26 Jan 2024 23:59:50 GMT, Nir Lisker wrote: Added a utility method to run code on the FX thread if it's not already, and changed the animation methods to use it. This pull request has now been integrated. Changeset: c5ab220b Author:Nir Lisker URL: https://git.openjdk.org/jfx/commit/c5ab220bbc885f2aa99d8c1d5ed8f1753e39251f Stats: 422 lines in 7 files changed: 225 ins; 148 del; 49 mod 8324658: Allow animation play/start/stop/pause methods to be called on any thread Reviewed-by: kcr, jpereda - PR: https://git.openjdk.org/jfx/pull/1352
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Wed, 31 Jan 2024 21:09:53 GMT, Andy Goryachev wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix issue with multiline text
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
> line 513:
>
>> 511: if ((x > run.getWidth() && (!isMultiRunText
>> || run.getStart() == curRunStart)) || textWidthPrevLine > 0) {
>> 512: getBounds(run.getTextSpan(),
>> textBounds);
>> 513: x -= (runs[0].getLocation().x -
>> textBounds.getMinX());
>
> suggestion: we are still in the for loop, so perhaps it makes sense to
> extract
> `(runs[0].getLocation().x - textBounds.getMinX());`
> to a variable outside of the loop
The idea is that outside the loop we don't know if we need to subtract the
textBound min x value and the starting location of the first run or not. That
is why this is present inside the loop. Once this is done we are breaking out
of the loop so this will not get called multiple times.
Let me know if you have any suggestions.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1473962144
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v7]
On Wed, 31 Jan 2024 10:24:20 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 incrementally with one additional > commit since the last revision: > > Fix issue with multiline text I will work on the inline node issue and the issue in Rich text area. The rich text area issue is basically because of the repeated text node BOLD. Im not really sure if the increased scale is causing any issue, I will get into it. - PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1920730601
