Re: RFR: 8339068: [Linux] NPE: Cannot read field "firstFont" because "" is null
On Wed, 28 Aug 2024 22:33:47 GMT, Phil Race wrote: >> This PR adds a couple of null checks to `LogicalFont` and `FTFactory`, that >> make use of `FontConfigManager::getFontConfigFont`, which under certain >> cases, can return null. >> >> I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using >> `-Dprism.useFontConfig=false`. >> >> On Ubuntu, I've manually added some font files to the JDK path >> `$JAVA_HOME/lib/fonts` (which didn't exist), and the test passes now, though >> this message is still printed out: >> >> Error: JavaFX detected no fonts! Please refer to release notes for proper >> font configuration >> >> which is confusing to me, because fonts where found in the JDK path after >> all, and even in the case that there were no fonts found, "the release >> notes" is an ambiguous reference for the user. >> >> Also, instead of adding fonts to the JDK, I tested adding a >> `logicalfonts.properties` file with `-Dprism.fontdir` and without >> `fontConfig`, but this case was already working before the patch for this >> PR, and still works after it. >> >> Note that if there are no fonts in the JDK path, and `prism.fontdir` is not >> provided, without `fontConfig`, after this PR, there will still be an >> exception: >> >> >> Caused by: java.lang.NullPointerException: Cannot invoke >> "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return >> value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null >> at >> javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456) >> at >> javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461) >> at >> javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79) >> at >> javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139) >> at >> javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303) >> at >> javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258) >> at >> javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576) >> ... >> >> >> because `LogicalFont::getSlot0Resource` will return null. Adding null checks >> after this point will require many changes >> (`LogicalFont::getSlotResource(0)` is used in many places). Shouldn't we >> fail gracefully after `LogicalFont::getSlot0Resource` if `slot0FontResource` >> is finally null? > > I don't understand why this change is needed. > > Perhaps I'm misunderstanding, but if usefontconfig is false, that means "I > have supplied embedded fonts instead". > In which case the API calls where the null checks are added should not be > returning null. > > In other words, setting this to false, and not supplying fonts is a > misconfiguration. > Arguably we should disable this property and always look for fontconfig. > But one would only know how to misconfigure it by reading the sources to > discover this property. @prrace could you have another look at this PR, after my change and comments? - PR Comment: https://git.openjdk.org/jfx/pull/1546#issuecomment-2379775064
Re: RFR: 8339068: [Linux] NPE: Cannot read field "firstFont" because "" is null [v2]
> This PR adds a couple of null checks to `LogicalFont` and `FTFactory`, that > make use of `FontConfigManager::getFontConfigFont`, which under certain > cases, can return null. > > I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using > `-Dprism.useFontConfig=false`. > > On Ubuntu, I've manually added some font files to the JDK path > `$JAVA_HOME/lib/fonts` (which didn't exist), and the test passes now, though > this message is still printed out: > > Error: JavaFX detected no fonts! Please refer to release notes for proper > font configuration > > which is confusing to me, because fonts where found in the JDK path after > all, and even in the case that there were no fonts found, "the release notes" > is an ambiguous reference for the user. > > Also, instead of adding fonts to the JDK, I tested adding a > `logicalfonts.properties` file with `-Dprism.fontdir` and without > `fontConfig`, but this case was already working before the patch for this PR, > and still works after it. > > Note that if there are no fonts in the JDK path, and `prism.fontdir` is not > provided, without `fontConfig`, after this PR, there will still be an > exception: > > > Caused by: java.lang.NullPointerException: Cannot invoke > "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return > value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null > at > javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456) > at > javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461) > at > javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79) > at > javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139) > at > javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303) > at > javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258) > at > javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576) > ... > > > because `LogicalFont::getSlot0Resource` will return null. Adding null checks > after this point will require many changes (`LogicalFont::getSlotResource(0)` > is used in many places). Shouldn't we fail gracefully after > `LogicalFont::getSlot0Resource` if `slot0FontResource` is finally null? Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Use isLinux in FTFactory instead of null check - Changes: - all: https://git.openjdk.org/jfx/pull/1546/files - new: https://git.openjdk.org/jfx/pull/1546/files/e080a4a5..a6fe2e7d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1546&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1546&range=00-01 Stats: 11 lines in 1 file changed: 6 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1546.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1546/head:pull/1546 PR: https://git.openjdk.org/jfx/pull/1546
Re: RFR: 8339068: [Linux] NPE: Cannot read field "firstFont" because "" is null
On Tue, 27 Aug 2024 15:50:54 GMT, Jose Pereda wrote:
> This PR adds a couple of null checks to `LogicalFont` and `FTFactory`, that
> make use of `FontConfigManager::getFontConfigFont`, which under certain
> cases, can return null.
>
> I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using
> `-Dprism.useFontConfig=false`.
>
> On Ubuntu, I've manually added some font files to the JDK path
> `$JAVA_HOME/lib/fonts` (which didn't exist), and the test passes now, though
> this message is still printed out:
>
> Error: JavaFX detected no fonts! Please refer to release notes for proper
> font configuration
>
> which is confusing to me, because fonts where found in the JDK path after
> all, and even in the case that there were no fonts found, "the release notes"
> is an ambiguous reference for the user.
>
> Also, instead of adding fonts to the JDK, I tested adding a
> `logicalfonts.properties` file with `-Dprism.fontdir` and without
> `fontConfig`, but this case was already working before the patch for this PR,
> and still works after it.
>
> Note that if there are no fonts in the JDK path, and `prism.fontdir` is not
> provided, without `fontConfig`, after this PR, there will still be an
> exception:
>
>
> Caused by: java.lang.NullPointerException: Cannot invoke
> "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return
> value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null
> at
> javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456)
> at
> javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461)
> at
> javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79)
> at
> javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139)
> at
> javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303)
> at
> javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258)
> at
> javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576)
> ...
>
>
> because `LogicalFont::getSlot0Resource` will return null. Adding null checks
> after this point will require many changes (`LogicalFont::getSlotResource(0)`
> is used in many places). Shouldn't we fail gracefully after
> `LogicalFont::getSlot0Resource` if `slot0FontResource` is finally null?
So far, the documentation in
https://wiki.openjdk.org/display/OpenJFX/Font+Setup is still somehow valid in
this situation (no fontConfig and providing embedded fonts).
So I can revert the change in `LogicalFont`, and modify the message in
`FontConfigManager::initFontConfigLogFonts`:
System.err.println("Error: JavaFX detected no fonts! " +
"Please refer to release notes for proper font configuration");
```
to something more meaningful, like?
System.err.println("Error: JavaFX detected no fonts!\n " +
"For proper font configuration, please check
https://wiki.openjdk.org/display/OpenJFX/Font+Setup";);
```
The NPE will be still present though.
Thoughts?
-
PR Comment: https://git.openjdk.org/jfx/pull/1546#issuecomment-2320964768
Re: RFR: 8339068: [Linux] NPE: Cannot read field "firstFont" because "" is null
On Tue, 27 Aug 2024 15:50:54 GMT, Jose Pereda wrote: > This PR adds a couple of null checks to `LogicalFont` and `FTFactory`, that > make use of `FontConfigManager::getFontConfigFont`, which under certain > cases, can return null. > > I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using > `-Dprism.useFontConfig=false`. > > On Ubuntu, I've manually added some font files to the JDK path > `$JAVA_HOME/lib/fonts` (which didn't exist), and the test passes now, though > this message is still printed out: > > Error: JavaFX detected no fonts! Please refer to release notes for proper > font configuration > > which is confusing to me, because fonts where found in the JDK path after > all, and even in the case that there were no fonts found, "the release notes" > is an ambiguous reference for the user. > > Also, instead of adding fonts to the JDK, I tested adding a > `logicalfonts.properties` file with `-Dprism.fontdir` and without > `fontConfig`, but this case was already working before the patch for this PR, > and still works after it. > > Note that if there are no fonts in the JDK path, and `prism.fontdir` is not > provided, without `fontConfig`, after this PR, there will still be an > exception: > > > Caused by: java.lang.NullPointerException: Cannot invoke > "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return > value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null > at > javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456) > at > javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461) > at > javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79) > at > javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139) > at > javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303) > at > javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258) > at > javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576) > ... > > > because `LogicalFont::getSlot0Resource` will return null. Adding null checks > after this point will require many changes (`LogicalFont::getSlotResource(0)` > is used in many places). Shouldn't we fail gracefully after > `LogicalFont::getSlot0Resource` if `slot0FontResource` is finally null? On Linux, if you use `-Dprism.useFontConfig=false`, but then don't set valid embedded fonts, you get the reported NPE, instead of a different failure, with a more explicit message. But since this property is undocumented, and you indeed need to read the sources to find out about it, you might also discover that setting a fontDir with some fonts and a property file with some properties to use those fonts, will solve the issue and there won't be a NPE anymore. So I agree that setting this to false, and not supplying fonts is a misconfiguration, but should probably be handled better, with a more explicit message or fast failure. On Android, however, the situation is different: `fontpath_linux.c`(with the native implementation of `FontConfigManager::getFontConfig`) is not used, but `FTFactory::getFallbacks` calls it nonetheless. Using `-Dprism.useFontConfig=false` prevents the runtime issue (UnsatisfiedLinkError), but then since `FontConfigManager.getFontConfigFont` returns null (note there is an `AndroidFontFinder` class that should manage fonts on Android), then the NPE can't be avoided, unless with add this null check. I agree that a null check hides the underlying issue, so prabably we should use `isLinux` instead. `PrismFontFactory` has this gate before calling `FontConfigManager`. - PR Comment: https://git.openjdk.org/jfx/pull/1546#issuecomment-2320826055
Integrated: 8339236: Suppress removal warnings for Security Manager methods in iOS sources
On Thu, 29 Aug 2024 11:49:58 GMT, Jose Pereda wrote: > This PR adds the required annotations to the iOS sources in the JavaFX Web > module, to allow the compilation of such sources, now that `-Werror` is > enabled, in the same way that was done for > https://bugs.openjdk.java.net/browse/JDK-8264139 (where it was already > mentioned https://github.com/openjdk/jfx/pull/528#issue-910936512 the need of > a follow-up for iOS/Android). This pull request has now been integrated. Changeset: a53bc589 Author:Jose Pereda URL: https://git.openjdk.org/jfx/commit/a53bc589ac37d490b1406a2b977097a06bf5ac74 Stats: 14 lines in 4 files changed: 11 ins; 0 del; 3 mod 8339236: Suppress removal warnings for Security Manager methods in iOS sources Reviewed-by: kcr, jvos - PR: https://git.openjdk.org/jfx/pull/1550
RFR: 8339236: Suppress removal warnings for Security Manager methods in iOS sources
This PR adds the required annotations to the iOS sources in the JavaFX Web module, to allow the compilation of such sources, now that `-Werror` is enabled, in the same way that was done for https://bugs.openjdk.java.net/browse/JDK-8264139 (where it was already mentioned https://github.com/openjdk/jfx/pull/528#issue-910936512 the need of a follow-up for iOS/Android). - Commit messages: - Add suppress warnings to iOS sources Changes: https://git.openjdk.org/jfx/pull/1550/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1550&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8339236 Stats: 14 lines in 4 files changed: 11 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1550.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1550/head:pull/1550 PR: https://git.openjdk.org/jfx/pull/1550
RFR: 8339068: [Linux] NPE: Cannot read field "firstFont" because "" is null
This PR adds a couple of null checks to `LogicalFont` and `FTFactory`, that make use of `FontConfigManager::getFontConfigFont`, which under certain cases, can return null. I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using `-Dprism.useFontConfig=false`. On Ubuntu, I've manually added some font files to the JDK path `$JAVA_HOME/lib/fonts` (which didn't exist), and the test passes now, though this message is still printed out: Error: JavaFX detected no fonts! Please refer to release notes for proper font configuration which is confusing to me, because fonts where found in the JDK path after all, and even in the case that there were no fonts found, "the release notes" is an ambiguous reference for the user. Also, instead of adding fonts to the JDK, I tested adding a `logicalfonts.properties` file with `-Dprism.fontdir` and without `fontConfig`, but this case was already working before the patch for this PR, and still works after it. Note that if there are no fonts in the JDK path, and `prism.fontdir` is not provided, without `fontConfig`, after this PR, there will still be an exception: Caused by: java.lang.NullPointerException: Cannot invoke "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null at javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456) at javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461) at javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79) at javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139) at javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303) at javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258) at javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576) ... because `LogicalFont::getSlot0Resource` will return null. Adding null checks after this point will require many changes (`LogicalFont::getSlotResource(0)` is used in many places). Shouldn't we fail gracefully after `LogicalFont::getSlot0Resource` if `slot0FontResource` is finally null? - Commit messages: - prevent NPE if FontConfigManager.FcCompFont is null Changes: https://git.openjdk.org/jfx/pull/1546/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1546&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8339068 Stats: 12 lines in 2 files changed: 6 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1546.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1546/head:pull/1546 PR: https://git.openjdk.org/jfx/pull/1546
Re: [jfx17u] RFR: 8334657: Enable binary check
On Sun, 18 Aug 2024 20:37:23 GMT, Johan Vos wrote: > Hi all, > > This pull request contains a backport of commit 101e5175 from the openjdk/jfx > repository. > > The commit being backported was authored by Kevin Rushforth on 25 Jun 2024 > and was reviewed by Ambarish Rapte and Phil Race. > > Thanks! Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx17u/pull/198#pullrequestreview-2244401782
Re: [jfx21u] RFR: 8334657: Enable binary check
On Sun, 18 Aug 2024 20:46:48 GMT, Johan Vos wrote: > Hi all, > > This pull request contains a backport of commit 101e5175 from the openjdk/jfx > repository. > > The commit being backported was authored by Kevin Rushforth on 25 Jun 2024 > and was reviewed by Ambarish Rapte and Phil Race. > > Thanks! Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx21u/pull/68#pullrequestreview-2244401844
Re: RFR: 8333919: [macOS] dragViewOffsetX/dragViewOffsetY are ignored for the dragView image [v4]
On Mon, 12 Aug 2024 17:13:51 GMT, Lukasz Kostyra wrote: >> When fixing [JDK-8233955](https://bugs.openjdk.org/browse/JDK-8233955) >> offset calculation was left out, which made Dragboard offset API not work on >> macOS. >> >> This change fixes this mistake. Now drag image should be properly offset. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Merge two if image != nil statements together > > Those were unnecessarily split Looks good - Marked as reviewed by jpereda (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1532#pullrequestreview-2234191527
Re: RFR: 8333919: dragViewOffsetX/dragViewOffsetY are ignored for the dragView image [v2]
On Mon, 12 Aug 2024 16:22:17 GMT, Andy Goryachev wrote:
>> By the way, this is how you can disable the cancel animation:
>>
>>
>> diff --git
>> a/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
>> b/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
>> index 50a79cabf6..373537f24e 100644
>> --- a/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
>> +++ b/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
>> @@ -40,6 +40,7 @@
>>
>> - (NSDragOperation)draggingSession:(NSDraggingSession *)session
>> sourceOperationMaskForDraggingContext:(NSDraggingContext)context
>> {
>> + session.animatesToStartingPositionsOnCancelOrFail = NO;
>> // The Command key masks out every operation other than
>> NSDragOperationGeneric. We want
>> // internal Move events to get through this filtering so we copy the
>> Move bit into the
>> // Generic bit and treat Generic as a synonym for Move.
>>
>>
>> I'm still in favour of this native animation, as opposed to the Windows case
>> that doesn't have it. This can be a separated issue as well (unifying the
>> animation in all platforms).
>
> I wanted to chime in, thought it might be unrelated:
>
> In the context of a docking framework, it is impossible to disable the cancel
> animation. We should have a way to disable it. The use case is when the
> user drags and drops a tab/pane outside of the dockable windows. In this
> case a new window should appear at the drop location, without the drag image
> animated back to the start position.
>
> Another thing that is impossible is to control the drag image: sometimes one
> needs a scaled icon, sometimes one wants the full content.
>
> These two issues basically prevent implementing a docking framework via DnD,
> one needs to use other solution, for example
> https://github.com/andy-goryachev/FxDock
I'm not sure if I follow, but about the cancel animation, do you mean that is
not possible to disable it on macOS? Have you tested
`session.animatesToStartingPositionsOnCancelOrFail`?
As for the image, `DragBoard::setDragView` allows you setting any image that
you need for the drag gesture, or am I missing something?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1532#discussion_r1714139069
Re: RFR: 8333919: dragViewOffsetX/dragViewOffsetY are ignored for the dragView image [v2]
On Mon, 12 Aug 2024 12:39:52 GMT, Jose Pereda wrote:
>> Out of curiosity I removed offset clamping (macOS snaps the drag image
>> regardless) and technically it still works, but when you cancel the drag the
>> image snaps back to the offset position. With high enough (or negative)
>> values drag image can animate back even outside the application bounds,
>> which makes little sense and looks weird. I'll clamp it between 0,0 and
>> imageW,H as you mentioned.
>>
>> I also didn't find a way to disable above drag image "snapping". However, if
>> it makes sense to clamp it on macOS, maybe it would also make sense to
>> introduce similar limits to offsets on other platforms (probably as a
>> separate issue)?
>
> Right, there is a built-in native animation from the offset point to the
> mouse cursor (in and out).
>
> When the drag event is cancelled, the image returns to the offset location
> with this animation. We still want that, so I wouldn't remove the animation
> (if we could), but of course, with the original position of the source, no
> far away from it, so it makes sense to clamp between the image bounds.
>
> And doing the same (clamping) for Windows would be a separate issue, indeed.
By the way, this is how you can disable the cancel animation:
diff --git
a/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
b/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
index 50a79cabf6..373537f24e 100644
--- a/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
+++ b/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
@@ -40,6 +40,7 @@
- (NSDragOperation)draggingSession:(NSDraggingSession *)session
sourceOperationMaskForDraggingContext:(NSDraggingContext)context
{
+ session.animatesToStartingPositionsOnCancelOrFail = NO;
// The Command key masks out every operation other than
NSDragOperationGeneric. We want
// internal Move events to get through this filtering so we copy the Move
bit into the
// Generic bit and treat Generic as a synonym for Move.
I'm still in favour of this native animation, as opposed to the Windows case
that doesn't have it. This can be a separated issue as well (unifying the
animation in all platforms).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1532#discussion_r1714046402
Re: RFR: 8333919: dragViewOffsetX/dragViewOffsetY are ignored for the dragView image [v3]
On Mon, 12 Aug 2024 12:58:25 GMT, Lukasz Kostyra wrote:
>> When fixing [JDK-8233955](https://bugs.openjdk.org/browse/JDK-8233955)
>> offset calculation was left out, which made Dragboard offset API not work on
>> macOS.
>>
>> This change fixes this mistake. Now drag image should be properly offset.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adjust clamping values, remove old TODO
Looking good, I still have two minor comments
modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1049:
> 1047: if (image != nil)
> 1048: {
> 1049: // select the center of the image as the drag origin
There are two consecutive `if (image != nil)` blocks, so you can remove the 4
lines above this one.
-
PR Review: https://git.openjdk.org/jfx/pull/1532#pullrequestreview-2233475851
PR Review Comment: https://git.openjdk.org/jfx/pull/1532#discussion_r1714048165
Re: RFR: 8333919: dragViewOffsetX/dragViewOffsetY are ignored for the dragView image [v2]
On Mon, 12 Aug 2024 12:14:26 GMT, Lukasz Kostyra wrote:
>> modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line
>> 1068:
>>
>>> 1066:
>>> 1067: dragPoint.x -= offset.x;
>>> 1068: dragPoint.y -= offset.y;
>>
>> This works fine now, _if_ the offset values are smaller than the imageHalf
>> X,Y values, which doesn't make sense anymore:
>>
>> - If you drag from {0,0} (top-left), it works from that position
>> - But if you drag from {imageW, imageH} (bottom-right), the offset is
>> clamped to the image centre, which it is not expected.
>>
>> So now we need to change the above lines to clamp the offset between 0,0 and
>> imageW,H (though Windows doesn't clamp the offset at all).
>
> Out of curiosity I removed offset clamping (macOS snaps the drag image
> regardless) and technically it still works, but when you cancel the drag the
> image snaps back to the offset position. With high enough (or negative)
> values drag image can animate back even outside the application bounds, which
> makes little sense and looks weird. I'll clamp it between 0,0 and imageW,H as
> you mentioned.
>
> I also didn't find a way to disable above drag image "snapping". However, if
> it makes sense to clamp it on macOS, maybe it would also make sense to
> introduce similar limits to offsets on other platforms (probably as a
> separate issue)?
Right, there is a built-in native animation from the offset point to the mouse
cursor (in and out).
When the drag event is cancelled, the image returns to the offset location with
this animation. We still want that, so I wouldn't remove the animation (if we
could), but of course, with the original position of the source, no far away
from it, so it makes sense to clamp between the image bounds.
And doing the same (clamping) for Windows would be a separate issue, indeed.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1532#discussion_r1713684798
Re: RFR: 8333919: dragViewOffsetX/dragViewOffsetY are ignored for the dragView image [v2]
On Fri, 9 Aug 2024 08:13:16 GMT, Lukasz Kostyra wrote:
>> When fixing [JDK-8233955](https://bugs.openjdk.org/browse/JDK-8233955)
>> offset calculation was left out, which made Dragboard offset API not work on
>> macOS.
>>
>> This change fixes this mistake. Now drag image should be properly offset.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional
> commit since the last revision:
>
> GlassViewDelegate: Correctly set origin and offset in dragImage
>
> - Centering to dragImage was removed - origin is now top-left of dragImage
> - Offset direction was swapped
modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1049:
> 1047: if (image != nil)
> 1048: {
> 1049: // select the center of the image as the drag origin
The old comment about RT-17629 (https://bugs.openjdk.org/browse/JDK-8091965)
does not apply anymore: after this PR, the offset, which comes from the Java
layer, does adjust the drag image origin.
So I believe we can remove the whole comment as well (and possibly close
RT-17629).
modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1068:
> 1066:
> 1067: dragPoint.x -= offset.x;
> 1068: dragPoint.y -= offset.y;
This works fine now, _if_ the offset values are smaller than the imageHalf X,Y
values, which doesn't make sense anymore:
- If you drag from {0,0} (top-left), it works from that position
- But if you drag from {imageW, imageH} (bottom-right), the offset is clamped
to the image centre, which it is not expected.
So now we need to change the above lines to clamp the offset between 0,0 and
imageW,H (though Windows doesn't clamp the offset at all).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1532#discussion_r1713448689
PR Review Comment: https://git.openjdk.org/jfx/pull/1532#discussion_r1713460897
Re: RFR: 8333919: dragViewOffsetX/dragViewOffsetY are ignored for the dragView image
On Thu, 8 Aug 2024 12:35:59 GMT, Lukasz Kostyra wrote: > When fixing [JDK-8233955](https://bugs.openjdk.org/browse/JDK-8233955) offset > calculation was left out, which made Dragboard offset API not work on macOS. > > This change fixes this mistake. Now drag image should be properly offset. I'm testing this PR, and I see that on macOS the offsets are now taken into account. However, comparing to Windows the behaviour is different: - On Windows, offset 0,0 means the cursor is at the most top-left coordinates of the dragView image. Setting positive offsets in x, y goes to the right and down of the image (which goes in line with the usual JavaFX coordinate system) - On macOS, with this PR, offset 0, 0 is at the centre of the dragView image, and positive offsets go to the opposite direction >From the test attached to the issue, and with this PR: WritableImage snapshot = source.snapshot(null, null); db.setDragView(snapshot); db.setDragViewOffsetX(0); db.setDragViewOffsetY(0); on Windows -> cursor at the most top-left corner of the dragView on macOS -> cursor at the centre of the dragView. And: WritableImage snapshot = source.snapshot(null, null); db.setDragView(snapshot); db.setDragViewOffsetX(snapshot.getWidth() / 2); db.setDragViewOffsetY(snapshot.getHeight() / 2); on Windows -> cursor at the centre of the dragView on macOS -> cursor at the most top-left corner of the dragView. So I'd say macOS should do the same as Windows, and for that you could try: -dragPoint.x -= ([image size].width/2.0f); -dragPoint.y -= ([image size].height/2.0f); +// dragPoint.x -= ([image size].width/2.0f); +// dragPoint.y -= ([image size].height/2.0f); -dragPoint.x += offset.x; - dragPoint.y += offset.y; + dragPoint.x -= offset.x; + dragPoint.y -= offset.y; Does that make sense to you? - PR Comment: https://git.openjdk.org/jfx/pull/1532#issuecomment-2276175583
Re: [jfx21u] RFR: 8319779: SystemMenu: memory leak due to listener never being removed
On Thu, 1 Aug 2024 10:04:12 GMT, Johan Vos wrote: > almost clean backport of 8319779: SystemMenu: memory leak due to listener > never being removed > Didn't apply clean because the imports where shuffled. Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx21u/pull/66#pullrequestreview-2212342712
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v5]
On Mon, 29 Jul 2024 14:29:17 GMT, Thiago Milczarek Sayao wrote: >> Specific to KDE, in the case of the example provided, when an MODAL window >> pops, it calls `set_enabled` `false` on the child (or all other windows if >> APPLICATION_MODAL) which causes it to update the window constraints. When >> maximized, the constraints where applied anyways, causing the window to >> still be maximized but not show as maximized. The change is to not apply >> constraints when not floating (meaning floating on the screen - not >> maximized, fullscreen or iconified). >> >> It's not specific to DEBIAN, it depends on the window manager (kwin is >> affected). > > Thiago Milczarek Sayao has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert idea file > - Fix only the original bug I tested the patch on Debian 12 KDE plasma, and it fixes the issue (fails as described in JDK-833 before applying the patch, works after applying it). I've also tested it on Ubuntu 24.04, where there is no issue, and the test works the same before and after this patch. - Marked as reviewed by jpereda (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1460#pullrequestreview-2205441542
Integrated: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs
On Mon, 24 Jun 2024 18:05:09 GMT, Jose Pereda wrote: > This PR considers the horizontal scroll events that can be generated on a > trackpad, on an horizontally sided `TabPane` control (top or bottom), adding > to the existing vertical scroll events from mouse wheel and trackpad, to > scroll its tabs. > > Therefore, scrolling a `TabPane` will behave in the same way that `ScrollBar` > does regarding those same events and control orientation > (vertical/horizontal). This pull request has now been integrated. Changeset: 1bdb93c7 Author:Jose Pereda URL: https://git.openjdk.org/jfx/commit/1bdb93c792cf7c218c74ec5cacda8bac1242f73b Stats: 111 lines in 2 files changed: 107 ins; 0 del; 4 mod 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs Reviewed-by: angorya, kcr - PR: https://git.openjdk.org/jfx/pull/1486
Re: RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs [v4]
On Wed, 24 Jul 2024 17:10:15 GMT, Andy Goryachev wrote: >> (sorry I missed to reply) >> >> I believe the comment already states what are we taking into account, but >> this is a more detailed explanation: >> >> - a mouse wheel only has vertical scroll events (`dx == 0`, so `abs(dy) > >> abs(dx)`), then we just take `dy` to produce the horizontal scroll offset. >> - a trackpad has both scroll events, and we take whichever is bigger. >> Typically the user will do a horizontal scrolling if tabs are horizontally >> laid out, and therefore abs(dy) <<< abs(dx), so we take `dx` to produce the >> horizontal scroll offset, ignoring the smaller `dy` (that wouldn't have any >> effect in any case). >> >> I'm happy to reword the comment if needed. > > Thank you! > > A larger question is that ideally we would need a mechanism to differentiate > trackpad from mouse events, as they sometimes require different handling. We > can't fully rely on the fact that either dx or dy is exactly 0.0, since > theoretically we could get those events from the trackpad. > > What do you think? In this case, the bigger value from the scrolling gesture prevails, ignoring the other small one (it doesn't matter if it is exactly 0 or not). For the`TabPane` case, that is good enough, since we only want to scroll in one direction, and it really doesn't matter if the source is a mouse wheel or a trackpad. - PR Review Comment: https://git.openjdk.org/jfx/pull/1486#discussion_r1690570921
Re: RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs [v4]
> This PR considers the horizontal scroll events that can be generated on a > trackpad, on an horizontally sided `TabPane` control (top or bottom), adding > to the existing vertical scroll events from mouse wheel and trackpad, to > scroll its tabs. > > Therefore, scrolling a `TabPane` will behave in the same way that `ScrollBar` > does regarding those same events and control orientation > (vertical/horizontal). Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: remove extra new line - Changes: - all: https://git.openjdk.org/jfx/pull/1486/files - new: https://git.openjdk.org/jfx/pull/1486/files/2c4914ca..a8969ce8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1486.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1486/head:pull/1486 PR: https://git.openjdk.org/jfx/pull/1486
Re: RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs [v3]
On Wed, 24 Jul 2024 21:06:10 GMT, Andy Goryachev wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add tests for all four sides > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java > line 1345: > >> 1343: } >> 1344: } >> 1345: > > please remove unnecessary newline done - PR Review Comment: https://git.openjdk.org/jfx/pull/1486#discussion_r1690556795
Re: RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs [v3]
On Mon, 24 Jun 2024 23:29:07 GMT, Andy Goryachev wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add tests for all four sides > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java > line 947: > >> 945: case BOTTOM: >> 946: // Consider vertical scroll events (dy > dx) >> from mouse wheel and trackpad, >> 947: // and horizontal scroll events from a trackpad >> (dx > dy) > > Maybe we should explain why - to preserve the existing behavior with the > mouse (scrolling up/down)? > > The trackpad events can have both `dx` and `dy` of various magnitude, both > non-zero, so the logic seems to me a bit contrived - like filtering some > events, although at the end it still feels ok. > > What do you think? (sorry I missed to reply) I believe the comment already states what are we taking into account, but this is a more detailed explanation: - a mouse wheel only has vertical scroll events (`dx == 0`, so `abs(dy) > abs(dx)`), then we just take `dy` to produce the horizontal scroll offset. - a trackpad has both scroll events, and we take whichever is bigger. Typically the user will do a horizontal scrolling if tabs are horizontally laid out, and therefore abs(dy) <<< abs(dx), so we take `dx` to produce the horizontal scroll offset, ignoring the smaller `dy` (that wouldn't have any effect in any case). I'm happy to reword the comment if needed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1486#discussion_r1689397250
[jfx17u] Integrated: 8236689: macOS 10.15 Catalina: LCD text renders badly
On Tue, 2 Jul 2024 08:21:54 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a clean backport of commit > [a118d333](https://github.com/openjdk/jfx/commit/a118d33314a363336134d31c45b50329594e5a24) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Phil Race on 20 Oct 2021 and was > reviewed by Kevin Rushforth and Pankaj Bansal. > > Thanks! This pull request has now been integrated. Changeset: 59c4e81f Author:Jose Pereda URL: https://git.openjdk.org/jfx17u/commit/59c4e81f13dcf5b40cacc6983d219f76ba710f6c Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod 8236689: macOS 10.15 Catalina: LCD text renders badly Reviewed-by: jvos Backport-of: a118d33314a363336134d31c45b50329594e5a24 - PR: https://git.openjdk.org/jfx17u/pull/195
Re: [jfx17u] RFR: 8336730: Change JavaFX release version to 17.0.13 in jfx17u
On Thu, 18 Jul 2024 08:31:42 GMT, Johan Vos wrote: > Start work on 17.0.13 Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx17u/pull/196#pullrequestreview-2185280186
Re: [jfx21u] RFR: 8336733: Change JavaFX release version to 21.0.5 in jfx21u
On Thu, 18 Jul 2024 08:34:10 GMT, Johan Vos wrote: > Start work on 21.0.5 Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx21u/pull/62#pullrequestreview-2185280653
[jfx17u] RFR: 8236689: macOS 10.15 Catalina: LCD text renders badly
Hi all, This pull request contains a clean backport of commit [a118d333](https://github.com/openjdk/jfx/commit/a118d33314a363336134d31c45b50329594e5a24) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Phil Race on 20 Oct 2021 and was reviewed by Kevin Rushforth and Pankaj Bansal. Thanks! - Commit messages: - Backport a118d33314a363336134d31c45b50329594e5a24 Changes: https://git.openjdk.org/jfx17u/pull/195/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=195&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8236689 Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx17u/pull/195.diff Fetch: git fetch https://git.openjdk.org/jfx17u.git pull/195/head:pull/195 PR: https://git.openjdk.org/jfx17u/pull/195
Re: RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs [v3]
> This PR considers the horizontal scroll events that can be generated on a > trackpad, on an horizontally sided `TabPane` control (top or bottom), adding > to the existing vertical scroll events from mouse wheel and trackpad, to > scroll its tabs. > > Therefore, scrolling a `TabPane` will behave in the same way that `ScrollBar` > does regarding those same events and control orientation > (vertical/horizontal). Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Add tests for all four sides - Changes: - all: https://git.openjdk.org/jfx/pull/1486/files - new: https://git.openjdk.org/jfx/pull/1486/files/2046b04e..2c4914ca Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=01-02 Stats: 54 lines in 1 file changed: 31 ins; 5 del; 18 mod Patch: https://git.openjdk.org/jfx/pull/1486.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1486/head:pull/1486 PR: https://git.openjdk.org/jfx/pull/1486
Re: RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs [v2]
> This PR considers the horizontal scroll events that can be generated on a > trackpad, on an horizontally sided `TabPane` control (top or bottom), adding > to the existing vertical scroll events from mouse wheel and trackpad, to > scroll its tabs. > > Therefore, scrolling a `TabPane` will behave in the same way that `ScrollBar` > does regarding those same events and control orientation > (vertical/horizontal). Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Add unit tests - Changes: - all: https://git.openjdk.org/jfx/pull/1486/files - new: https://git.openjdk.org/jfx/pull/1486/files/9e6b89c6..2046b04e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=00-01 Stats: 77 lines in 1 file changed: 76 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1486.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1486/head:pull/1486 PR: https://git.openjdk.org/jfx/pull/1486
RFR: 8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs
This PR considers the horizontal scroll events that can be generated on a trackpad, on an horizontally sided `TabPane` control (top or bottom), adding to the existing vertical scroll events from mouse wheel and trackpad, to scroll its tabs. Therefore, scrolling a `TabPane` will behave in the same way that `ScrollBar` does regarding those same events and control orientation (vertical/horizontal). - Commit messages: - Consider horizontal scroll events on an horizontally sided tabPane control Changes: https://git.openjdk.org/jfx/pull/1486/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1486&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334874 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1486.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1486/head:pull/1486 PR: https://git.openjdk.org/jfx/pull/1486
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]
On Thu, 13 Jun 2024 20:07:32 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 two additional > commits since the last revision: > > - process more reviewer comments > - Process reviewer comments Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2117171377
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]
On Thu, 9 May 2024 19:48:19 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 more type info
I've tested on macOS, and tests pass now, and fail without the changes from
this PR.
I've left some comments, but once addressed, I'll re-approve.
modules/javafx.graphics/src/shims/java/com/sun/javafx/tk/quantum/GlassSystemMenuShim.java
line 37:
> 35:
> 36: private GlassSystemMenu gsm;
> 37: final ArrayList> uncollectedMenus = new
> ArrayList<>();
this can be private?
modules/javafx.graphics/src/shims/java/com/sun/javafx/tk/quantum/GlassSystemMenuShim.java
line 54:
> 52: protected void setMenuBindings(final Menu glassMenu, final MenuBase
> mb) {
> 53: super.setMenuBindings(glassMenu, mb);
> 54: uncollectedMenus.add(new WeakReference(glassMenu));
Add `<>` to avoid raw use of `WeakReference`
tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
line 289:
> 287: public void testJDK8309935() {
> 288: MenuBar menuBar = new MenuBar();
> 289: AtomicReference throwableRef = new AtomicReference();
Add missing `<>` to avoid raw use of `AtomicReference`
-
Marked as reviewed by jpereda (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2115396776
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637974242
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637973453
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637980139
Re: RFR: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
On Tue, 21 May 2024 14:25:51 GMT, Michael Strauß wrote: > This PR fixes a bug > ([JDK-8289115](https://bugs.openjdk.org/browse/JDK-8289115)) that was > introduced with #394, which changed the following line in the > `NotifyTouchInput` function (ViewContainer.cpp): > > - const bool isDirect = true; > + const bool isDirect = IsTouchEvent(); > > > `IsTouchEvent` is a small utility function that uses the Windows SDK function > `GetMessageExtraInfo` to distinguish between mouse events and pen events as > described in this article: > https://learn.microsoft.com/en-us/windows/win32/tablet/system-events-and-mouse-messages > > I think that using this function to distinguish between touchscreen events > and pen events is erroneous. The linked article states: > _"When your application receives a **mouse message** (such as WM_LBUTTONDOWN) > [...]"_ > > But we are not receiving a mouse message in `NotifyTouchInput`, we are > receiving a `WM_TOUCH` message. > I couldn't find any information on whether this API usage is also supported > for `WM_TOUCH` messages, but my testing shows that that's probably not the > case (I tested this with a XPS 15 touchscreen laptop, where > `GetMessageExtraInfo` returns 0 for `WM_TOUCH` messages). > > My suggestion is to check the `TOUCHEVENTF_PEN` flag of the `TOUCHINPUT` > structure instead: > > const bool isDirect = (ti->dwFlags & TOUCHEVENTF_PEN) == 0; I've tested this change with: - Wacom Intuos S pen tablet. Events generated with the pen cause `isDirect` to be false. There are no `TouchEvent`s generated. - HP E230t touch monitor. Events generated tapping the screen cause `isDirect` to be true. There are `TouchEvent`s generated. So I can confirm the proposed change matches the existing comment, and also that it fixes the issue since touch events are now dispatched on touch screens. - Marked as reviewed by jpereda (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1459#pullrequestreview-2081456486
Re: RFR: 8332222: Linux Debian: Maximized stage shrinks when opening another stage [v2]
On Thu, 23 May 2024 10:37:09 GMT, Thiago Milczarek Sayao wrote: >> Thiago Milczarek Sayao has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Should still report location > > The linux build seems to be failing because the lack of gcc-13 package. @tsayao the Ubuntu runner uses now 24.0.4. Your branch just needs to be synced with head. - PR Comment: https://git.openjdk.org/jfx/pull/1460#issuecomment-2126802774
Re: RFR: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
On Tue, 21 May 2024 14:25:51 GMT, Michael Strauß wrote: > This PR fixes a bug > ([JDK-8289115](https://bugs.openjdk.org/browse/JDK-8289115)) that was > introduced with #394, which changed the following line in the > `NotifyTouchInput` function (ViewContainer.cpp): > > - const bool isDirect = true; > + const bool isDirect = IsTouchEvent(); > > > `IsTouchEvent` is a small utility function that uses the Windows SDK function > `GetMessageExtraInfo` to distinguish between mouse events and pen events as > described in this article: > https://learn.microsoft.com/en-us/windows/win32/tablet/system-events-and-mouse-messages > > I think that using this function to distinguish between touchscreen events > and pen events is erroneous. The linked article states: > _"When your application receives a **mouse message** (such as WM_LBUTTONDOWN) > [...]"_ > > But we are not receiving a mouse message in `NotifyTouchInput`, we are > receiving a `WM_TOUCH` message. > I couldn't find any information on whether this API usage is also supported > for `WM_TOUCH` messages, but my testing shows that that's probably not the > case (I tested this with a XPS 15 touchscreen laptop, where > `GetMessageExtraInfo` returns 0 for `WM_TOUCH` messages). > > My suggestion is to check the `TOUCHEVENTF_PEN` flag of the `TOUCHINPUT` > structure instead: > > const bool isDirect = (ti->dwFlags & TOUCHEVENTF_PEN) == 0; Yes, I'll do that, I still have it around. - PR Comment: https://git.openjdk.org/jfx/pull/1459#issuecomment-2124142803
[jfx21u] Integrated: 8330462: StringIndexOutOfBoundException when typing anything into TextField
On Wed, 15 May 2024 16:20:04 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a backport of commit > [d3da033a](https://github.com/openjdk/jfx/commit/d3da033a2dd5c287733545935242a8d1f71c0554) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Oliver Kopp on 8 May 2024 and was > reviewed by Andy Goryachev, Kevin Rushforth and Ambarish Rapte. > > The backport is almost clean: While cherry picking the mainline commit, a > minor merge conflict was shown at the file > `tests/system/src/test/.classpath`: The mainline file has a few additional > exports compared to 21u, which are not needed here. > > Thanks! This pull request has now been integrated. Changeset: 136dd663 Author:Jose Pereda URL: https://git.openjdk.org/jfx21u/commit/136dd663dd94f0db6cc583181e2059bc09425f3a Stats: 147 lines in 5 files changed: 142 ins; 0 del; 5 mod 8330462: StringIndexOutOfBoundException when typing anything into TextField Reviewed-by: angorya Backport-of: d3da033a2dd5c287733545935242a8d1f71c0554 - PR: https://git.openjdk.org/jfx21u/pull/57
[jfx21u] RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField
Hi all, This pull request contains a backport of commit [d3da033a](https://github.com/openjdk/jfx/commit/d3da033a2dd5c287733545935242a8d1f71c0554) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Oliver Kopp on 8 May 2024 and was reviewed by Andy Goryachev, Kevin Rushforth and Ambarish Rapte. The backport is almost clean: While cherry picking the mainline commit, a minor merge conflict was shown at the file `tests/system/src/test/.classpath`: The mainline file has a few additional exports compared to 21u, which are not needed here. Thanks! - Commit messages: - Backport d3da033a2dd5c287733545935242a8d1f71c0554 Changes: https://git.openjdk.org/jfx21u/pull/57/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=57&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330462 Stats: 147 lines in 5 files changed: 142 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jfx21u/pull/57.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/57/head:pull/57 PR: https://git.openjdk.org/jfx21u/pull/57
Re: [jfx17u] RFR: 8330682: Change JavaFX release version to 17.0.12 in jfx17u
On Fri, 19 Apr 2024 13:32:14 GMT, Johan Vos wrote: > Increase JavaFX release version to 17.0.12 Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx17u/pull/189#pullrequestreview-2013084899
Re: [jfx21u] RFR: 8330683: Change JavaFX release version to 21.0.4 in jfx21u
On Fri, 19 Apr 2024 13:29:07 GMT, Johan Vos wrote: > Increase JavaFX release version Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx21u/pull/56#pullrequestreview-2011757631
Integrated: 8324939: Editable TableView loses focus after commit
On Wed, 20 Mar 2024 10:55:56 GMT, Jose Pereda wrote: > This PR fixes the issue that after committing an edit on a > ListView/TreeView/TableView/TreeTableView control, the control might lose the > focus unexpectedly. > > For that, it refactors the > `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, > in order to check if the control (`ListView`, `TreeView`, `TableView`, > `TreeTableView`) should request the focus _before_ the actual focus owner > (which could be the control added to the cell to edit its content, like a > `TextField`) is removed from the cell, so the `Control::requestFocus` call, > if needed, can be still invoked after the edit commit is done (as it was done > before). > > By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` > the `Cell::commitEdit` implementations can now query if the control should > have the focus, after `super.commitEdit(newValue);` but before firing the > `CellEditEvent` and calling `updateItem()`, and if the result is true, then > request focus after the edit commit ends (like it was done before). > > Two new tests per control have been included, to verify that the focus > remains at the control, one for edit cancel (this passes before and after the > proposed changes), one for edit commit (this fails before and passes after > including the proposed fix). This pull request has now been integrated. Changeset: 0d336063 Author:Jose Pereda URL: https://git.openjdk.org/jfx/commit/0d336063346879671e1c9fbdeed4926d69c6cf44 Stats: 443 lines in 9 files changed: 422 ins; 5 del; 16 mod 8324939: Editable TableView loses focus after commit Reviewed-by: angorya, mhanl - PR: https://git.openjdk.org/jfx/pull/1411
Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
> This PR fixes the issue that after committing an edit on a > ListView/TreeView/TableView/TreeTableView control, the control might lose the > focus unexpectedly. > > For that, it refactors the > `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, > in order to check if the control (`ListView`, `TreeView`, `TableView`, > `TreeTableView`) should request the focus _before_ the actual focus owner > (which could be the control added to the cell to edit its content, like a > `TextField`) is removed from the cell, so the `Control::requestFocus` call, > if needed, can be still invoked after the edit commit is done (as it was done > before). > > By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` > the `Cell::commitEdit` implementations can now query if the control should > have the focus, after `super.commitEdit(newValue);` but before firing the > `CellEditEvent` and calling `updateItem()`, and if the result is true, then > request focus after the edit commit ends (like it was done before). > > Two new tests per control have been included, to verify that the focus > remains at the control, one for edit cancel (this passes before and after the > proposed changes), one for edit commit (this fails before and passes after > including the proposed fix). Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Address feedback from reviewer - Changes: - all: https://git.openjdk.org/jfx/pull/1411/files - new: https://git.openjdk.org/jfx/pull/1411/files/8f312781..744562f0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1411&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1411&range=00-01 Stats: 16 lines in 4 files changed: 0 ins; 16 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1411.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1411/head:pull/1411 PR: https://git.openjdk.org/jfx/pull/1411
Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
On Thu, 28 Mar 2024 11:20:23 GMT, Marius Hanl wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address feedback from reviewer > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java > line 2362: > >> 2360: assertTrue(treeView.isFocused()); >> 2361: >> 2362: VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; > > This should not be needed since you are creating a `stageLoader` above. This > seems to be only used when no `stageLoader` was created before Thanks @Maran23, I've removed it now from the added tests. - PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1550359262
RFR: 8324939: Editable TableView loses focus after commit
This PR fixes the issue that after committing an edit on a ListView/TreeView/TableView/TreeTableView control, the control might lose the focus unexpectedly. For that, it refactors the `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, in order to check if the control (`ListView`, `TreeView`, `TableView`, `TreeTableView`) should request the focus _before_ the actual focus owner (which could be the control added to the cell to edit its content, like a `TextField`) is removed from the cell, so the `Control::requestFocus` call, if needed, can be still invoked after the edit commit is done (as it was done before). By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` the `Cell::commitEdit` implementations can now query if the control should have the focus, after `super.commitEdit(newValue);` but before firing the `CellEditEvent` and calling `updateItem()`, and if the result is true, then request focus after the edit commit ends (like it was done before). Two new tests per control have been included, to verify that the focus remains at the control, one for edit cancel (this passes before and after the proposed changes), one for edit commit (this fails before and passes after including the proposed fix). - Commit messages: - Refactor ControlUtils, check focus before owner is gone, and include tests Changes: https://git.openjdk.org/jfx/pull/1411/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1411&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324939 Stats: 459 lines in 9 files changed: 438 ins; 5 del; 16 mod Patch: https://git.openjdk.org/jfx/pull/1411.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1411/head:pull/1411 PR: https://git.openjdk.org/jfx/pull/1411
Re: RFR: 8326989 : Text selection issues on WebView after WebKit 617.1
On Wed, 6 Mar 2024 14:51:40 GMT, Hima Bindu Meda wrote: > Text Selection issue is observed due to a pending layout call. This issue is > resolved by a call to the method "updateSelectionAppearanceNow". > Verified Sanity testing and unit test verification looks fine. No issue seen. > > Note: Unit test in order to validate the fix, is in progress. Due to the > robustness of the implementation to verify on all platforms, the test would > be added in a followon > bug-[JDK-8327478](https://bugs.openjdk.org/browse/JDK-8327478) I've built and tested the proposed change successfully on macOS, Linux, and Windows, selecting text loaded from both URL or local html content, via mouse dragging and mouse double-clicking. - Marked as reviewed by jpereda (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1391#pullrequestreview-1920439949
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v10]
On Tue, 5 Mar 2024 07:34:32 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java
>> line 226:
>>
>>> 224: mb.textProperty().when(active).addListener(valueModel ->
>>> glassMenu.setTitle(parseText(mb)));
>>> 225: mb.disableProperty().when(active).addListener(valueModel ->
>>> glassMenu.setEnabled(!mb.isDisable()));
>>> 226:
>>> mb.mnemonicParsingProperty().when(active).addListener(valueModel ->
>>> glassMenu.setTitle(parseText(mb)));
>>
>> Since you are not calling `get()` from the `when` resulting observable (to
>> allow for invalidation), these need to use a `ChangeListener` instead of an
>> `InvalidationListener`, otherwise, while `active` doesn't change, any change
>> in those menuBase properties will trigger just one notification.
>>
>> Alternative, just replace `addListener` with `subscribe` (as it internally
>> uses a `ChangeListener`).
>>
>> @hjohn does this make sense to you?
>
> This could be a problem normally, but I think in this case you won't be able
> to get this to produce incorrect results.
>
> `parseText` is accessing the property (and it calls `isMnemonicParsing`), but
> only if the text is not empty. It's sort of "safe" as an empty text can't
> contain a mnemonic anyway, and you'd need to change the text at a minimum
> first to see the effect of turning mnemonic parsing on/off. Changing the
> text would trigger the `textProperty` listener, which will call `parseText`
> and read the mnemonic property.
>
> A change listener would be more obviously correct, but it is not strictly
> needed here.
>
> (not all `subscribe`s use change listener, the one that takes a `Runnable`
> uses invalidation)
There is an issue using `when()`, because you add the listener to a new
property, not to the original property. Therefore, using an invalidation
listener doesn't trigger notifications if you changing values more than once to
the original property.
Simply run this test:
BooleanProperty b = new SimpleBooleanProperty();
BooleanProperty active = new SimpleBooleanProperty(true);
b.addListener(o -> System.out.println("b: " + b.get()));
b.when(active).addListener(o -> System.out.println("when: " + b.get()));
b.set(true);
b.set(false);
It prints:
b: true
when: true
b: false
so with `when` we miss the changes! (I can go into details of why this happens,
but just notice we have a new property after all, and not calling `when.get()`
it doesn't get validated anymore).
This can happen in this SystemMenuBar case with the disabled property of any
given `Menu` or `MenuItem`, if you simply change it twice, while the
application is active.
My proposal of using `subscribe` implied using the `Consumer` (therefore the
"replacement").
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1512390742
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v10]
On Tue, 27 Feb 2024 09:30:14 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 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 11 additional commits since > the last revision: > > - Merge branch 'master' into 8319779-systemmenu > - Add additional test for IOOBE detection. >This test comes from JDK-8323787 > - Revert some of the conditional bindings. >Clear menu construction when an menuitem that is a menu needs to be removed >Add a test for the latter > - Merge remote-tracking branch 'upstream/master' into 8319779-systemmenu > - Cleanup test > - Add shim class so that we can access the references to > com.sun.glass.ui.Menu instances. >Add a test to make sure those references are gone. > - Revert WeakInvalidationListeners and use new listener resource management > approach. > - Fix more memoryleaks due to listeners never being unregistered. > - These changes are related to JBS-8318841 so we want to have that code in >as well. > >Merge branch 'master' into 8319779-systemmenu > - process reviewers comments > - ... and 1 more: https://git.openjdk.org/jfx/compare/a68c0a68...ec7308df modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java line 226: > 224: mb.textProperty().when(active).addListener(valueModel -> > glassMenu.setTitle(parseText(mb))); > 225: mb.disableProperty().when(active).addListener(valueModel -> > glassMenu.setEnabled(!mb.isDisable())); > 226: mb.mnemonicParsingProperty().when(active).addListener(valueModel > -> glassMenu.setTitle(parseText(mb))); Since you are not calling `get()` from the `when` resulting observable (to allow for invalidation), these need to use a `ChangeListener` instead of an `InvalidationListener`, otherwise, while `active` doesn't change, any change in those menuBase properties will trigger just one notification. Alternative, just replace `addListener` with `subscribe` (as it internally uses a `ChangeListener`). @hjohn does this make sense to you? - PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1511687632
[jfx17u] Integrated: 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS
On Thu, 29 Feb 2024 15:48:30 GMT, Jose Pereda wrote:
> Almost clean backport of 8322703: Intermittent crash in WebView in a JFXPanel
> from IME calls on macOS
>
> Reviewed-by: jbhaskar, arapte
>
> Very minor conflicts after cherry picking the commit:
>
> // InputMethodRequests implementation
> +<<<<<<< HEAD
> +===
> ...
> @Override
> +>>>>>>> 40809a3f84 (8322703: Intermittent crash in WebView in a JFXPanel
> from IME calls on macOS)
> public Point2D getTextLocation(int offset) {
>
>
> I fixed it manually, and pushed the change.
This pull request has now been integrated.
Changeset: c54a9ed3
Author:Jose Pereda
URL:
https://git.openjdk.org/jfx17u/commit/c54a9ed3e91dad2d0b06302d75387882310b3fe6
Stats: 63 lines in 1 file changed: 34 ins; 18 del; 11 mod
8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS
Reviewed-by: jvos
Backport-of: 40809a3f84d5f9f91b265f455a95d045e5b4f692
-
PR: https://git.openjdk.org/jfx17u/pull/179
[jfx17u] RFR: 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS
Almost clean backport of 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS Reviewed-by: jbhaskar, arapte - Commit messages: - 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS Changes: https://git.openjdk.org/jfx17u/pull/179/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=179&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322703 Stats: 63 lines in 1 file changed: 34 ins; 18 del; 11 mod Patch: https://git.openjdk.org/jfx17u/pull/179.diff Fetch: git fetch https://git.openjdk.org/jfx17u.git pull/179/head:pull/179 PR: https://git.openjdk.org/jfx17u/pull/179
[jfx21u] Integrated: 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS
On Thu, 29 Feb 2024 09:22:38 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a clean backport of commit > [40809a3f](https://github.com/openjdk/jfx/commit/40809a3f84d5f9f91b265f455a95d045e5b4f692) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Kevin Rushforth on 30 Jan 2024 > and was reviewed by Jay Bhaskar and Ambarish Rapte. > > Thanks! This pull request has now been integrated. Changeset: 61d9c36e Author:Jose Pereda URL: https://git.openjdk.org/jfx21u/commit/61d9c36ece644a4a17e018588833ef0aea8b6743 Stats: 62 lines in 1 file changed: 33 ins; 18 del; 11 mod 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS Reviewed-by: jvos Backport-of: 40809a3f84d5f9f91b265f455a95d045e5b4f692 - PR: https://git.openjdk.org/jfx21u/pull/45
[jfx21u] RFR: 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS
Hi all, This pull request contains a clean backport of commit [40809a3f](https://github.com/openjdk/jfx/commit/40809a3f84d5f9f91b265f455a95d045e5b4f692) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Kevin Rushforth on 30 Jan 2024 and was reviewed by Jay Bhaskar and Ambarish Rapte. Thanks! - Commit messages: - Backport 40809a3f84d5f9f91b265f455a95d045e5b4f692 Changes: https://git.openjdk.org/jfx21u/pull/45/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=45&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322703 Stats: 62 lines in 1 file changed: 33 ins; 18 del; 11 mod Patch: https://git.openjdk.org/jfx21u/pull/45.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/45/head:pull/45 PR: https://git.openjdk.org/jfx21u/pull/45
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: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Sat, 24 Feb 2024 17:34:54 GMT, Thiago Milczarek Sayao wrote: >> 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). @tsayao yes, I've double checked again. On Wayland and Xorg, I can scroll with touch events on a touch screen with this fix. The only issue I still see on Wayland: the mouse cursor location doesn't get updated from touch events, only from mouse events (I guess this can be a follow-up issue). - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1963041414
[jfx21u] Integrated: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView
On Thu, 22 Feb 2024 10:52:08 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a clean backport of commit > [ab68b716](https://github.com/openjdk/jfx/commit/ab68b716fbfd807918ca4a1bc096dcf40d9cfcbd) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Jose Pereda on 21 Dec 2023 and > was reviewed by Andy Goryachev. > > Thanks! This pull request has now been integrated. Changeset: f83dfdd4 Author:Jose Pereda URL: https://git.openjdk.org/jfx21u/commit/f83dfdd4ee6e7c87551a9be70dcdebbd6b3ad403 Stats: 24 lines in 2 files changed: 23 ins; 0 del; 1 mod 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView Reviewed-by: jvos Backport-of: ab68b716fbfd807918ca4a1bc096dcf40d9cfcbd - PR: https://git.openjdk.org/jfx21u/pull/44
[jfx21u] RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView
Hi all, This pull request contains a clean backport of commit [ab68b716](https://github.com/openjdk/jfx/commit/ab68b716fbfd807918ca4a1bc096dcf40d9cfcbd) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Jose Pereda on 21 Dec 2023 and was reviewed by Andy Goryachev. Thanks! - Commit messages: - Backport ab68b716fbfd807918ca4a1bc096dcf40d9cfcbd Changes: https://git.openjdk.org/jfx21u/pull/44/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=44&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321970 Stats: 24 lines in 2 files changed: 23 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx21u/pull/44.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/44/head:pull/44 PR: https://git.openjdk.org/jfx21u/pull/44
[jfx21u] Integrated: 8221261: Deadlock on macOS in JFXPanel app when handling IME calls
On Tue, 20 Feb 2024 10:18:36 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a clean backport of commit > [90cbc663](https://github.com/openjdk/jfx/commit/90cbc66305d0a1380cf3a8cd99ad40db240e554c) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Kevin Rushforth on 15 Jan 2024 > and was reviewed by Phil Race, Prasanta Sadhukhan and Sergey Bylokhov. > > Thanks! This pull request has now been integrated. Changeset: e00407b6 Author:Jose Pereda URL: https://git.openjdk.org/jfx21u/commit/e00407b6919cfe6fedefc74dba55c160640c5267 Stats: 30 lines in 1 file changed: 28 ins; 0 del; 2 mod 8221261: Deadlock on macOS in JFXPanel app when handling IME calls Reviewed-by: jvos Backport-of: 90cbc66305d0a1380cf3a8cd99ad40db240e554c - PR: https://git.openjdk.org/jfx21u/pull/43
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 11:19:18 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: > > 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. - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1954087066
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v4]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/1305/files - new: https://git.openjdk.org/jfx/pull/1305/files/f8ffe87f..ca5766fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=02-03 Stats: 105 lines in 5 files changed: 1 ins; 93 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1305.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1305/head:pull/1305 PR: https://git.openjdk.org/jfx/pull/1305
[jfx21u] RFR: 8221261: Deadlock on macOS in JFXPanel app when handling IME calls
Hi all, This pull request contains a clean backport of commit [90cbc663](https://github.com/openjdk/jfx/commit/90cbc66305d0a1380cf3a8cd99ad40db240e554c) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Kevin Rushforth on 15 Jan 2024 and was reviewed by Phil Race, Prasanta Sadhukhan and Sergey Bylokhov. Thanks! - Commit messages: - Backport 90cbc66305d0a1380cf3a8cd99ad40db240e554c Changes: https://git.openjdk.org/jfx21u/pull/43/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=43&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8221261 Stats: 30 lines in 1 file changed: 28 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx21u/pull/43.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/43/head:pull/43 PR: https://git.openjdk.org/jfx21u/pull/43
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 11:19:18 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: > > Add compile-time checks to GdkSeat I see... so removing all the changes from this PR, and using this diff from head instead: diff --git a/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp b/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp index e7a230b2b6..e89db2a55f 100644 --- a/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp +++ b/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp @@ -605,7 +605,8 @@ glass_gdk_mouse_devices_grab_with_cursor(GdkWindow *gdkWindow, GdkCursor *cursor | GDK_BUTTON2_MOTION_MASK | GDK_BUTTON3_MOTION_MASK | GDK_BUTTON_PRESS_MASK -| GDK_BUTTON_RELEASE_MASK), +| GDK_BUTTON_RELEASE_MASK + | GDK_TOUCH_MASK), NULL, cursor, GDK_CURRENT_TIME); return (status == GDK_GRAB_SUCCESS) ? TRUE : FALSE; it looks like dragging works on a touch enabled display and Wayland. (No changes needed in `GDK_FILTERED_EVENTS_MASK`). Tested this change on X11 as well, I don't see any issue. - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1952469103
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 19 Feb 2024 00:35:48 GMT, Thiago Milczarek Sayao wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add compile-time checks to GdkSeat > > A shot in the dark since I don't own a touch enabled monitor: > > Test 1: > > Add `GDK_SCROLL_MASK` on the original `gdk_pointer_grab` function; > > Test 2: > > This PR uses `GDK_SEAT_CAPABILITY_ALL_POINTING` which includes the touch > masks. > > So the equivalent would include `GDK_TOUCH_MASK` on `gdk_pointer_grab`. > > > I would bet on option 2. Thanks @tsayao, I can definitely test those suggestions. If I get it right, you are proposing this change: status = gdk_pointer_grab(gdkWindow, owner_events, (GdkEventMask) (GDK_POINTER_MOTION_MASK | GDK_POINTER_MOTION_HINT_MASK +| GDK_TOUCH_MASK // or GDK_SCROLL_MASK | GDK_BUTTON_MOTION_MASK | GDK_BUTTON1_MOTION_MASK | GDK_BUTTON2_MOTION_MASK | GDK_BUTTON3_MOTION_MASK | GDK_BUTTON_PRESS_MASK | GDK_BUTTON_RELEASE_MASK NULL, cursor, GDK_CURRENT_TIME); But I don't see how this would fix the issue of the failing test on Linux that happens also without a touch enabled display. I tried both, the test still fails (two out of 5 times or so). Also, this is called only if there is no gdk_seat_grab function (which happens before GTK 3.20). However, if I use `GDK_SEAT_CAPABILITY_NONE` instead of `GDK_SEAT_CAPABILITY_ALL_POINTING`, it passes 10 out of 10, but that would cancel pointer/touch events, wouldn't it? Is there any chance that the robot implementation (via XTest, that uses `XTestGrabControl`) might not work fine with the new seat_grab approach after GTK 3.20+? - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1952237235
[jfx22u] Integrated: 8325154: resizeColumnToFitContent is slower than it needs to be
On Sat, 17 Feb 2024 20:00:22 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a clean backport of commit > [e2f42c5b](https://github.com/openjdk/jfx/commit/e2f42c5b71ef061c614540508fbac3fb610b1ae3) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Robert Lichtenberger on 14 Feb > 2024 and was reviewed by Andy Goryachev and Marius Hanl. > > Thanks! This pull request has now been integrated. Changeset: 8d26e071 Author:Jose Pereda URL: https://git.openjdk.org/jfx22u/commit/8d26e071dafb5debe673868c7e704ba7ae54bf83 Stats: 8 lines in 1 file changed: 4 ins; 4 del; 0 mod 8325154: resizeColumnToFitContent is slower than it needs to be Backport-of: e2f42c5b71ef061c614540508fbac3fb610b1ae3 - PR: https://git.openjdk.org/jfx22u/pull/14
[jfx22u] RFR: 8325154: resizeColumnToFitContent is slower than it needs to be
Hi all, This pull request contains a clean backport of commit [e2f42c5b](https://github.com/openjdk/jfx/commit/e2f42c5b71ef061c614540508fbac3fb610b1ae3) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Robert Lichtenberger on 14 Feb 2024 and was reviewed by Andy Goryachev and Marius Hanl. Thanks! - Commit messages: - Backport e2f42c5b71ef061c614540508fbac3fb610b1ae3 Changes: https://git.openjdk.org/jfx22u/pull/14/files Webrev: https://webrevs.openjdk.org/?repo=jfx22u&pr=14&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/jfx22u/pull/14.diff Fetch: git fetch https://git.openjdk.org/jfx22u.git pull/14/head:pull/14 PR: https://git.openjdk.org/jfx22u/pull/14
[jfx21u] Integrated: 8325154: resizeColumnToFitContent is slower than it needs to be
On Fri, 16 Feb 2024 21:12:21 GMT, Jose Pereda wrote: > Hi all, > > This pull request contains a clean backport of commit > [e2f42c5b](https://github.com/openjdk/jfx/commit/e2f42c5b71ef061c614540508fbac3fb610b1ae3) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. > > The commit being backported was authored by Robert Lichtenberger on 14 Feb > 2024 and was reviewed by Andy Goryachev and Marius Hanl. > > Thanks! This pull request has now been integrated. Changeset: e6cc0715 Author:Jose Pereda URL: https://git.openjdk.org/jfx21u/commit/e6cc0715f293fbd3055180f284cb422e7ab47e1d Stats: 8 lines in 1 file changed: 4 ins; 4 del; 0 mod 8325154: resizeColumnToFitContent is slower than it needs to be Reviewed-by: jvos Backport-of: e2f42c5b71ef061c614540508fbac3fb610b1ae3 - PR: https://git.openjdk.org/jfx21u/pull/42
[jfx21u] RFR: 8325154: resizeColumnToFitContent is slower than it needs to be
Hi all, This pull request contains a clean backport of commit [e2f42c5b](https://github.com/openjdk/jfx/commit/e2f42c5b71ef061c614540508fbac3fb610b1ae3) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. The commit being backported was authored by Robert Lichtenberger on 14 Feb 2024 and was reviewed by Andy Goryachev and Marius Hanl. Thanks! - Commit messages: - Backport e2f42c5b71ef061c614540508fbac3fb610b1ae3 Changes: https://git.openjdk.org/jfx21u/pull/42/files Webrev: https://webrevs.openjdk.org/?repo=jfx21u&pr=42&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/jfx21u/pull/42.diff Fetch: git fetch https://git.openjdk.org/jfx21u.git pull/42/head:pull/42 PR: https://git.openjdk.org/jfx21u/pull/42
Re: RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v3]
On Fri, 2 Feb 2024 13:30:19 GMT, John Hendrikx wrote: >> The SimpleSelector and CompoundSelector classes are public classes in an >> exported package, javafx.css, but they are not intended to be used by >> applications. They are implementation details. They cannot be constructed >> directly and no other JavaFX API accepts or returns a SimpleSelector or >> CompoundSelector. >> >> We should deprecate them for removal so we can move them to a non-exported >> package, removing them from the public API. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Deprecate for 23 and add new method I've tested the change proposed in https://github.com/openjdk/jfx/pull/1340#issuecomment-1924508539 successfully: I've built locally this PR and Scene Builder with the proposed change. Then, showing the CSS analyzer, it calls `CSSInternal::getStyleClasses` using `Selector::getClasses`, which produces the same result as using the old implementation. - PR Comment: https://git.openjdk.org/jfx/pull/1340#issuecomment-1926007302
Re: RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v3]
On Fri, 2 Feb 2024 13:30:19 GMT, John Hendrikx wrote: >> The SimpleSelector and CompoundSelector classes are public classes in an >> exported package, javafx.css, but they are not intended to be used by >> applications. They are implementation details. They cannot be constructed >> directly and no other JavaFX API accepts or returns a SimpleSelector or >> CompoundSelector. >> >> We should deprecate them for removal so we can move them to a non-exported >> package, removing them from the public API. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Deprecate for 23 and add new method Sure, that was what I meant. I'll give it a try over the weekend. - PR Comment: https://git.openjdk.org/jfx/pull/1340#issuecomment-1924719862
Re: RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v3]
On Fri, 2 Feb 2024 13:30:19 GMT, John Hendrikx wrote: >> The SimpleSelector and CompoundSelector classes are public classes in an >> exported package, javafx.css, but they are not intended to be used by >> applications. They are implementation details. They cannot be constructed >> directly and no other JavaFX API accepts or returns a SimpleSelector or >> CompoundSelector. >> >> We should deprecate them for removal so we can move them to a non-exported >> package, removing them from the public API. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Deprecate for 23 and add new method Yes, I think so, simplifying the code without breaking the existing functionality should be fine. I guess it should be easy to do a build with this PR and test Scene Builder before and after the proposed change. - PR Comment: https://git.openjdk.org/jfx/pull/1340#issuecomment-1924554466
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v17]
On Tue, 30 Jan 2024 01:24:52 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. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests Looks good! - Marked as reviewed by jpereda (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1850577924
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v14]
On Mon, 29 Jan 2024 16:42:15 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. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update tests modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 909: > 907: * This method must be run on the JavaFX Application Thread. > 908: * > 909: * @see #playFromStartImpl(String) there is no such symbol. Do you mean `playFrom(String)`? modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 953: > 951: * This method must be run on the JavaFX Application Thread. > 952: * > 953: * @see #playFromStartImpl(Duration) there is no such symbol. Do you mean `playFrom(Duration)`? modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 989: > 987: * This method must be run on the JavaFX Application Thread. > 988: * > 989: * @see #playFromStartImpl() Do you mean `playFromStart()`? tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. Copyright header should keep 2023, even if it is almost completely changed tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. Same, copyright header should keep 2023, even if it is almost completely changed - PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470098558 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470099369 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470090981 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470088051 PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1470088333
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]
On Mon, 29 Jan 2024 03:28:51 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.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update tests
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java
line 1:
> 1: /*
Don't remove license
tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java
line 1:
> 1: /*
Don't remove license
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 1:
> 1: package test.com.sun.javafx.animation;
Add license
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 20:
> 18:
> 19: // Based on https://bugs.openjdk.org/browse/JDK-8159048
> 20: public class SynchronisityTest extends Application {
shouldn't it be `SynchronicityTest`?
tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java
line 53:
> 51: private AtomicBoolean failed = new AtomicBoolean(false);
> 52: private CountDownLatch waiter = new CountDownLatch(1);
> 53: private ExecutorService executor = Executors.newCachedThreadPool();
These can be `final`
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469281637
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469281296
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469278053
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469307640
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469295387
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Sat, 27 Jan 2024 21:56:59 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line
>> 903:
>>
>>> 901: }
>>> 902:
>>> 903: private void playFromOnFxThread(String cuePoint) {
>>
>> I take that adding these private `xxOnFxThread` methods is convenient so
>> they can be passed easily to the runnable in `Utils::runOnFxThread`, or
>> accessed from other subclasses.
>>
>> However, I'm not sure about the method naming `OnFxThread`, as it might
>> imply that what it does is already done on the FX thread, therefore not
>> needing to be wrapped up by `runOnFxThread`.
>>
>> Does `runXX` (i.e. `runPlayFrom`) make more sense? In any case, I'd be fine
>> with the current proposal as is.
>
>> However, I'm not sure about the method naming `OnFxThread`, as it might
>> imply that what it does is already done on the FX thread, therefore not
>> needing to be wrapped up by `runOnFxThread`.
>
> I understand what you mean, but the way I see it is that they do already run
> on the FX thread because that's the only reason they exist. I could use
> something like `runMustBeOnFxThread` or `runOnlyFromFxThread` if that's
> better.
> Note that there are already `do___` methods (bad names IMO), and adding
> `run___` might be confusing.
Probably you won't like `xxImpl` either?
I'm not sure we should start adding `runMustBeOnFxThread` to every method name
out there that should run on the FX thread. In this case, maybe we could simply
add a small javadoc with a comment, and that's it. Neither will enforce a real
check about that.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469290172
Re: RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v5]
On Sat, 27 Jan 2024 17:26:45 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.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update documentation on AnimationTimer methods
Changes look fine. I have two minor comments.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 1008:
> 1006: * @param runnable a {@code Runnable} encapsulating the code
> 1007: */
> 1008: public static void runOnFxThread(Runnable runnable) {
I understand that this is a private package, and not public API, but do we need
a null check of `runnable`? (Not needed for the current uses added from this
PR, but possibly for future uses?)
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 903:
> 901: }
> 902:
> 903: private void playFromOnFxThread(String cuePoint) {
I take that adding these private `xxOnFxThread` methods is convenient so they
can be passed easily to the runnable in `Utils::runOnFxThread`, or accessed
from other subclasses.
However, I'm not sure about the method naming `OnFxThread`, as it might imply
that what it does is already done on the FX thread, therefore not needing to be
wrapped up by `runOnFxThread`.
Does `runXX` (i.e. `runPlayFrom`) make more sense? In any case, I'd be fine
with the current proposal as is.
-
PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1847205003
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1468586394
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1468590831
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 11:19:18 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: > > Add compile-time checks to GdkSeat Yes, for some reason, after applying the changes from this PR, there are Leave events. When it works: loaded gdk_seat_grab loaded gdk_display_get_default_seat GlassApplication::process_events: GDK_ENTER_NOTIFY found schema 'org.gnome.desktop.interface' and key 'scaling-factor' GlassApplication::process_events: GDK_ENTER_NOTIFY GlassApplication::process_events: GDK_MOTION_NOTIFY GlassApplication::process_events: GDK_BUTTON_PRESS GlassApplication::process_mouse_button: pressed GlassApplication::process_events: GDK_BUTTON_RELEASE GlassApplication::process_mouse_button: not pressed ... When it fails: loaded gdk_seat_grab loaded gdk_display_get_default_seat GlassApplication::process_events: GDK_LEAVE_NOTIFY GlassApplication::process_events: GDK_ENTER_NOTIFY GlassApplication::process_events: GDK_LEAVE_NOTIFY GlassApplication::process_events: GDK_ENTER_NOTIFY found schema 'org.gnome.desktop.interface' and key 'scaling-factor' GlassApplication::process_events: GDK_LEAVE_NOTIFY GlassApplication::process_events: GDK_MOTION_NOTIFY GlassApplication::process_events: GDK_BUTTON_PRESS GlassApplication::process_mouse_button: pressed Java_com_sun_glass_ui_gtk_GtkWindow__1ungrabFocus() ungrab focus() ... - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1906355019
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 11:19:18 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: > > Add compile-time checks to GdkSeat So it seems definitely related to the changes in this PR. Running with `GDK_DEBUG=nograbs` works (10 out of 10). Adding some debug to the native code, and running with grabs, I can see: When it works: DatePickerTest STANDARD_OUT Glass GTK library to load is glassgtk3 loaded gdk_seat_grab loaded gdk_display_get_default_seat grab grab found schema 'org.gnome.desktop.interface' and key 'scaling-factor' loaded gdk_seat_ungrab ungrab grab ungrab found schema 'org.gnome.desktop.interface' and key 'scaling-factor' ungrab grab > Task :systemTests:test ... (so test starts after window is grabbed and focused, as it should be). When it fails: DatePickerTest STANDARD_OUT Glass GTK library to load is glassgtk3 loaded gdk_seat_grab loaded gdk_display_get_default_seat grab grab found schema 'org.gnome.desktop.interface' and key 'scaling-factor' loaded gdk_seat_ungrab ungrab > Task :systemTests:test ... so window is not grabbed and not focused, and therefore native mouse clicks go elsewhere. - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1905121653
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 11:19:18 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: > > Add compile-time checks to GdkSeat I'm testing on my local Linux Intel machine (Ubuntu 20.04), and running the test from head, it passes 5 out of 5 times. After applying this patch, it fails with your same stacktrace 2 out of 5 times. With some printouts, I can see that when the test fails, in `DatePickerTest::clickDatePickerCalendarPopup` the call to `mouseClick` processes correctly the mouse move at the correct coordinates and calls mouse press and mouse release, but what it should trigger the datePicker setOnAction event (line 168), fails to do so. For some reason, the event doesn't make it to the control. - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1904746485
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
On Mon, 18 Dec 2023 22:00:28 GMT, Kevin Rushforth wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add compile-time checks to GdkSeat > > The addition of the compile-time flags looks OK. > > I did a build with GTK 3.22 (so it compiles the new code, does the dlsym, and > does the runtime check) and verified that there are no regressions when > running on an older system (Ubuntu 16.04). > > I then did a full test run on our headful test systems, and there is one new > test failure -- it seems to be intermittent, although fails pretty > consistently on our Ubuntu 22.04 and Ubuntu 20.04 test systems. I can > reproduce it locally on a VM running Ubuntu 22.04, where it fails about 1/2 > the time with this patch applied (it fails more often on our physical test > systems). > > > DatePickerTest > testDatePickerSceneChange FAILED > java.lang.AssertionError: Timeout: Failed to receive onAction call. > at org.junit.Assert.fail(Assert.java:89) > at org.junit.Assert.assertTrue(Assert.java:42) > at test.util.Util.waitForLatch(Util.java:400) > at > test.robot.javafx.scene.DatePickerTest.clickDatePickerCalendarPopup(DatePickerTest.java:90) > at > test.robot.javafx.scene.DatePickerTest.testDatePickerSceneChange(DatePickerTest.java:123) > > > Not sure what to make of this. I am not aware of any problems with this test, > but it's possible that your fix has exposed a latent issue either in the test > or somewhere else. @kevinrushforth is there anything else to be done before we can move this PR forward? - PR Comment: https://git.openjdk.org/jfx/pull/1305#issuecomment-1902776414
Re: RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v2]
On Fri, 19 Jan 2024 16:00:49 GMT, John Hendrikx wrote: >> The SimpleSelector and CompoundSelector classes are public classes in an >> exported package, javafx.css, but they are not intended to be used by >> applications. They are implementation details. They cannot be constructed >> directly and no other JavaFX API accepts or returns a SimpleSelector or >> CompoundSelector. >> >> We should deprecate them for removal so we can move them to a non-exported >> package, removing them from the public API. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Add since parameter to deprecation annotation Scene Builder releases are always tight to the JavaFX version (i.e, SB 21 uses JavaFX 21), so once JavaFX 23 is out with this change (or if we start testing 23-ea before the release of SB 23), we'll just have to modify the `CSSInternal` class as needed. - PR Comment: https://git.openjdk.org/jfx/pull/1340#issuecomment-1900843857
Re: RFR: JDK-8323706 Move SimpleSelector and CompoundSelector to internal packages
On Fri, 19 Jan 2024 00:43:20 GMT, Andy Goryachev wrote: >> Moves `SimpleSelector` and `CompoundSelector` to internal packages. >> >> This can be done with only a minor API break, as `SimpleSelector` and >> `CompoundSelector` were public before. However, these classes could not be >> constructed by 3rd parties. The only way to access them was by doing a cast >> (generally they're accessed via `Selector` not by their sub types). The >> reason they were public at all was because the CSS engine needs to be able >> to access them from internal packages. >> >> This change fixes a mistake (or possibly something that couldn't be modelled >> at the time) when the CSS API was first made public. The intention was >> always to have a `Selector` interface/abstract class, with private >> implementations (`SimpleSelector` and `CompoundSelector`). >> >> This PR as said has a small API break. The other changes are (AFAICS) >> source and binary compatible: >> >> - Made `Selector` `sealed` only permitting `SimpleSelector` and >> `CompoundSelector` -- as `Selector` had a package private constructor, there >> are no concerns with pre-existing subclasses >> - `Selector` has a few more methods that are now `protected` -- given that >> the class is now sealed, these modified methods are not accessible (they may >> still require rudimentary documentation I suppose) >> - `Selector` now has a `public` default constructor -- as the class is >> sealed, it is inaccessible >> - `SimpleSelector` and `CompoundSelector` have a few more `public` methods, >> but they're internal now, so it is irrelevant >> - `createMatch` was implemented directly in `Selector` to avoid having to >> expose package private fields in `Match` for use by `CompoundSelector` >> - No need anymore for the `SimpleSelectorShim` > > Maybe @jperedadnr could check whether the ScenicView tools uses either > SimpleSelector or CompoundSelector? @andy-goryachev-oracle Scenic View source code doesn't use neither of them. - PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-1900085510
Re: [jfx17u] RFR: 8323829: Change javaFX release version to 17.0.11 in jfx17u
On Tue, 16 Jan 2024 20:26:40 GMT, Johan Vos wrote: > Increase the release version to JavaFX 17.0.11 Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx17u/pull/176#pullrequestreview-1831961532
Integrated: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView
On Mon, 18 Dec 2023 12:09:21 GMT, Jose Pereda wrote: > This PR fixes an issue when a new `TableColumn` is added to a `TableView` > control with fixed cell size set, where the `TableRowSkinBase` failed to add > the cells for the new column. > > A test is included that fails before applying this PR and passes with it. This pull request has now been integrated. Changeset: ab68b716 Author:Jose Pereda URL: https://git.openjdk.org/jfx/commit/ab68b716fbfd807918ca4a1bc096dcf40d9cfcbd Stats: 24 lines in 2 files changed: 23 ins; 0 del; 1 mod 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1308
Re: RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView [v2]
On Mon, 18 Dec 2023 14:13:51 GMT, Jeanette Winzenburg wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> address feedback > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java > line 87: > >> 85: tableView.setItems(items); >> 86: >> 87: stageLoader = new StageLoader(new Scene(tableView)); > > this changes the context of unrelated tests - no idea whether or not it > matters, but I would try not to :) Good catch, actually this changes is a leftover after some changes I did while getting the test to fail... but now I see it is not needed after all. - PR Review Comment: https://git.openjdk.org/jfx/pull/1308#discussion_r1430332070
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v3]
> 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: Add compile-time checks to GdkSeat - Changes: - all: https://git.openjdk.org/jfx/pull/1305/files - new: https://git.openjdk.org/jfx/pull/1305/files/99230ca6..f8ffe87f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=01-02 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1305.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1305/head:pull/1305 PR: https://git.openjdk.org/jfx/pull/1305
RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView
This PR fixes an issue when a new `TableColumn` is added to a `TableView` control with fixed cell size set, where the `TableRowSkinBase` failed to add the cells for the new column. A test is included that fails before applying this PR and passes with it. - Commit messages: - Add cells to tableRow after new columns are added to tableView Changes: https://git.openjdk.org/jfx/pull/1308/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1308&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321970 Stats: 26 lines in 2 files changed: 24 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1308.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1308/head:pull/1308 PR: https://git.openjdk.org/jfx/pull/1308
Re: RFR: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView [v2]
> This PR fixes an issue when a new `TableColumn` is added to a `TableView` > control with fixed cell size set, where the `TableRowSkinBase` failed to add > the cells for the new column. > > A test is included that fails before applying this PR and passes with it. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: address feedback - Changes: - all: https://git.openjdk.org/jfx/pull/1308/files - new: https://git.openjdk.org/jfx/pull/1308/files/9c138c49..04cc76b4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1308&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1308&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1308.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1308/head:pull/1308 PR: https://git.openjdk.org/jfx/pull/1308
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:34:02 GMT, Thiago Milczarek Sayao wrote: >> The check is to allow compilation and test on Ubuntu 16.04 - When shipping >> the final build it should be built on gtk 3.20+, so both dlsym and the check >> makes sense. >> >> So when running on Ubuntu 16.04: >> - For testing on the platform, the #ifdef will make it possible to compile; >> - For running the released built (which should be built on gtk 3.20+) dlsym >> will fail and fallback; > > That's only if we want to keep building working on 16.04. I think it makes > easier to test on it. > But, it's already failing for Platform preferences API code. In any case, if `GdkSeat` is available only since 3.20, then we need to add the compile-time checks anyway, since minimum supported is 3.8. - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429891858
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Mon, 18 Dec 2023 10:09:29 GMT, Thiago Milczarek Sayao
wrote:
>> I take `GdkSeat` is available since GTK 3.0?
>> https://docs.gtk.org/gdk3/class.Seat.html
>>
>> But don't we have a minimum set on 3.8.0?
>>
>> Would this work?
>>
>> #if GTK_CHECK_VERSION(3, 0, 0)
>> static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
>> GdkSeat * wrapped_gdk_display_get_default_seat (GdkDisplay *display)
>> {...}
>> #endif
>>
>> ...
>>
>> #if GTK_CHECK_VERSION(3, 0, 0)
>> GdkSeat* seat =
>> wrapped_gdk_display_get_default_seat(gdk_window_get_display(window));
>> if (seat != NULL && _gdk_seat_grab != NULL) {
>> *status = ...
>> return TRUE;
>> }
>> #endif
>
> I think the docs are wrong, I probably exists since 3.20, so maybe check for
> `GTK_CHECK_VERSION(3, 20, 0);`.
>
>
> This is the compilation error on Ubuntu 16.04:
> `/home/tsayao/jose/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c:200:34:
> error: unknown type name ‘GdkSeat’
> `
Okay, that is unfortunate (GTK docs inaccurate), but makes sense.
I'll add the compile-time checks to `wrapped.c`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429864963
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
On Sat, 16 Dec 2023 22:21:55 GMT, Thiago Milczarek Sayao
wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove compile-time if checks
>
> modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c line 197:
>
>> 195: return TRUE;
>> 196: }
>> 197: return FALSE;
>
> I did try to test on Ubuntu 16.04 and compilation failed (no surprise because
> `GdkSeat` does not exists there). Suggestion to keep `#ifdef` here and
> `return FALSE` on `#else` so it would still compile on Ubuntu 16.04 and older
> systems. Will need to `#ifdef` all `GdkSeat` usage.
I take `GdkSeat` is available since GTK 3.0?
https://docs.gtk.org/gdk3/class.Seat.html
But don't we have a minimum set on 3.8.0?
Would this work?
#if GTK_CHECK_VERSION(3, 0, 0)
static GdkSeat * (*_gdk_display_get_default_seat) (GdkDisplay *display);
GdkSeat * wrapped_gdk_display_get_default_seat (GdkDisplay *display)
{...}
#endif
...
#if GTK_CHECK_VERSION(3, 0, 0)
GdkSeat* seat =
wrapped_gdk_display_get_default_seat(gdk_window_get_display(window));
if (seat != NULL && _gdk_seat_grab != NULL) {
*status = ...
return TRUE;
}
#endif
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1429810712
[jfx17u] Integrated: 8251240: Menus inaccessible on Linux with i3 wm
On Sun, 17 Dec 2023 12:40:49 GMT, Jose Pereda wrote: > Clean backport of 8251240: Menus inaccessible on Linux with i3 wm > Reviewed-by: jpereda, jvos This pull request has now been integrated. Changeset: b7e3871c Author: Jose Pereda URL: https://git.openjdk.org/jfx17u/commit/b7e3871c5ac5126efd0ec4bab3aec9131f3fb3f1 Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod 8251240: Menus inaccessible on Linux with i3 wm Backport-of: f18597430d44f70086364170f7bb1e5d30e7ce56 - PR: https://git.openjdk.org/jfx17u/pull/173
[jfx17u] RFR: 8251240: Menus inaccessible on Linux with i3 wm
Clean backport of 8251240: Menus inaccessible on Linux with i3 wm Reviewed-by: jpereda, jvos - Commit messages: - 8251240: Menus inaccessible on Linux with i3 wm Changes: https://git.openjdk.org/jfx17u/pull/173/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=173&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8251240 Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod Patch: https://git.openjdk.org/jfx17u/pull/173.diff Fetch: git fetch https://git.openjdk.org/jfx17u.git pull/173/head:pull/173 PR: https://git.openjdk.org/jfx17u/pull/173
[jfx17u] Integrated: 8260528: Clean glass-gtk sizing and positioning code
On Sun, 17 Dec 2023 12:14:03 GMT, Jose Pereda wrote: > Clean backport of 8260528: Clean glass-gtk sizing and positioning code > Reviewed-by: jvos, kcr This pull request has now been integrated. Changeset: 84c2bd93 Author: Jose Pereda URL: https://git.openjdk.org/jfx17u/commit/84c2bd935f136b2d562d48cd135f6903f2d13051 Stats: 750 lines in 5 files changed: 175 ins; 434 del; 141 mod 8260528: Clean glass-gtk sizing and positioning code Backport-of: 0c03a411655047a393862eda937408aa90fc3fa9 - PR: https://git.openjdk.org/jfx17u/pull/172
[jfx17u] RFR: 8260528: Clean glass-gtk sizing and positioning code
Clean backport of 8260528: Clean glass-gtk sizing and positioning code Reviewed-by: jvos, kcr - Commit messages: - 8260528: Clean glass-gtk sizing and positioning code Changes: https://git.openjdk.org/jfx17u/pull/172/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=172&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8260528 Stats: 750 lines in 5 files changed: 175 ins; 434 del; 141 mod Patch: https://git.openjdk.org/jfx17u/pull/172.diff Fetch: git fetch https://git.openjdk.org/jfx17u.git pull/172/head:pull/172 PR: https://git.openjdk.org/jfx17u/pull/172
[jfx17u] Integrated: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
On Tue, 5 Dec 2023 12:03:10 GMT, Jose Pereda wrote: > Backport of 8299968: Second call to Stage.setScene() create sizing issue with > uiScale > 1.0 > > Reviewed-by: kcr, arapte > > The backport didn't pass the pre-submit tests: The added test in JDK-8299968 > (`SetSceneScalingTest`) was using some methods from > `/tests/system/src/test/java/test/util/Util.java` that weren't back ported > yet. > > Those methods were added in https://bugs.openjdk.org/browse/JDK-8206430, with > a commit that modified 101 files. In order to avoid backporting all those > files as well, this backport only cherry-picks the changes done to > `Util.java` in such commit. This pull request has now been integrated. Changeset: 7a4fff07 Author:Jose Pereda URL: https://git.openjdk.org/jfx17u/commit/7a4fff075f0c9e5767f1ff73977042b82e857561 Stats: 356 lines in 9 files changed: 349 ins; 4 del; 3 mod 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0 Reviewed-by: jvos Backport-of: 4051f1611646400b59ee871fb40399b933361ba2 - PR: https://git.openjdk.org/jfx17u/pull/171
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland [v2]
> 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: remove compile-time if checks - Changes: - all: https://git.openjdk.org/jfx/pull/1305/files - new: https://git.openjdk.org/jfx/pull/1305/files/f498b835..99230ca6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=00-01 Stats: 32 lines in 3 files changed: 5 ins; 8 del; 19 mod Patch: https://git.openjdk.org/jfx/pull/1305.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1305/head:pull/1305 PR: https://git.openjdk.org/jfx/pull/1305
Re: RFR: 8320965: Scrolling on a touch enabled display fails on Wayland
On Thu, 14 Dec 2023 16:02:48 GMT, Kevin Rushforth 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. > > modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp line 599: > >> 597: return TRUE; >> 598: } >> 599: #if GTK_CHECK_VERSION(3, 20, 0) > > I wouldn't have expected any compile-time `#if` checks as part of this PR. Okay, that makes sense. I've removed the compile-time `#if-#else` from `wrapped.c`. However, to do the same in `glass_general.cpp`, if the wrapped functions fail (when dlsym returns null), we still need to fall back to the old implementation, don't we? Therefore, I've changed the signature to return a `gboolean` (and removed the `GDK_GRAB_FAILED` enum value that was added in 3.16, as I just noticed). - PR Review Comment: https://git.openjdk.org/jfx/pull/1305#discussion_r1427224949
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v2]
On Mon, 20 Nov 2023 08:00:58 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: > > process reviewers comments About adding an automated test, the leak that PR tries to fix happens in `com.sun.javafx.tk.quantum.GlassSystemMenu`, which is package private, and adding new tests to check `javafx.scene.control.Menu`, or related public classes, won't catch that leak, will it? Unless we open it (and all the related classes/methods in `com.sun.glass.ui.*`), I don't think it can be done? - PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1853543288
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v2]
On Mon, 20 Nov 2023 08:00:58 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:
>
> process reviewers comments
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java
line 189:
> 187:
> 188: private ListChangeListener createListener(final Menu
> glassMenu) {
> 189: return (ListChangeListener.Change
> change) -> {
In this listener, when there are items removed that are `Menu`s, we should also
call `clearMenu`:
if (i >= 0 && menuItemList.size() > i) {
glassMenu.remove(i);
+Object item = menuItemList.get(i);
+if (item instanceof Menu menu) {
+clearMenu(menu);
+}
}
Otherwise, they won't be removed from the new maps.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1425062079
RFR: 8320965: Scrolling on a touch enabled display fails on Wayland
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. - Commit messages: - Add wrappers of gdk_seat_grab and gdk_seat_ungrab Changes: https://git.openjdk.org/jfx/pull/1305/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1305&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320965 Stats: 96 lines in 5 files changed: 88 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1305.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1305/head:pull/1305 PR: https://git.openjdk.org/jfx/pull/1305
Integrated: 8321722: Tab header flickering when dragging slowly other tabs and reordering uncompleted
On Mon, 11 Dec 2023 18:36:35 GMT, Jose Pereda wrote: > This PR prevents changes in direction when dragging a tab header over a > TabPane control, if delta value is zero, which can happen with slow drag > events, in order to prevent starting and stopping too frequently the > animation of the target tab header in opposite directions. This pull request has now been integrated. Changeset: a20f4bcd Author: Jose Pereda URL: https://git.openjdk.org/jfx/commit/a20f4bcd54c5ae2a3e19324e51ce1f4cc172e32f Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod 8321722: Tab header flickering when dragging slowly other tabs and reordering uncompleted Reviewed-by: angorya, mstrauss, kcr - PR: https://git.openjdk.org/jfx/pull/1304
Re: RFR: 8321722: Tab header flickering when dragging slowly other tabs and reordering uncompleted
On Mon, 11 Dec 2023 19:01:19 GMT, Andy Goryachev wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
>> line 2162:
>>
>>> 2160: }
>>> 2161: // Stop dropHeaderAnim if direction of drag is changed
>>> 2162: if (dragDirection != 0 && prevDragDirection != dragDirection)
>>> {
>>
>> Did you mean `dragDelta != 0`?
>
> it is sort of equivalent in this case...
> I think this code is correct.
It is the same as using `dragDelta != 0`, since `dragDelta = 0` implies
`dragDirection = 0`.
The if-else expression doesn't provide a value for the case `dragDelta == 0`,
so I've added an initial value 0 to `dragDirection`.
Since `prevDragDirection` initial value is -1, we need to verify that
`dragDirection` is not zero and also `prevDragDirection` is different to
`dragDirection` to detect a change of direction.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1304#discussion_r1423005976
RFR: 8321722: Tab header flickering when dragging slowly other tabs and reordering uncompleted
This PR prevents changes in direction when dragging a tab header over a TabPane control, if delta value is zero, which can happen with slow drag events, in order to prevent starting and stopping too frequently the animation of the target tab header in opposite directions. - Commit messages: - Prevent changes in direction when dragging if delta value is zero Changes: https://git.openjdk.org/jfx/pull/1304/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1304&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321722 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1304.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1304/head:pull/1304 PR: https://git.openjdk.org/jfx/pull/1304
