Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values

[Kevin Rushforth]

On Sun, 24 Mar 2024 15:11:29 GMT, drmarmac  wrote:

> This PR adds the missing checks, as well as code documentation that an 
> IndexOutOfBoundsException may be thrown.

The fix and tests look good. I left a couple comments on the docs.

modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
 line 121:

> 119:  * @param index the index in this list
> 120:  * @return the index of the element's origin in the source list
> 121:  * @throws IndexOutOfBoundsException if the index is out of range 
> (index < 0 || index >= size())

Suggestion: consider using code case for the variables and equation?

modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
 line 134:

> 132:  * @param index the index of an element in this list
> 133:  * @return the index of the element's origin in the provided list
> 134:  * @throws IndexOutOfBoundsException if the index is out of range 
> (index < 0 || index >= list.getSource().size())

There is no`getSource` method in `ObservableList`. That should be `... index >= 
size())`

-

PR Review: https://git.openjdk.org/jfx/pull/1432#pullrequestreview-2029789757
PR Review Comment: https://git.openjdk.org/jfx/pull/1432#discussion_r1583998595
PR Review Comment: https://git.openjdk.org/jfx/pull/1432#discussion_r1583874591

Re: [External] : Re: Wayland

[Kevin Rushforth]

Thank you.

-- Kevin


On 4/29/2024 2:35 PM, Thiago Milczarek Sayão wrote:

I thought about possible legal conflicts.

The code is on my github - I'm exploring and testing before starting 
the real work.


wayland-scanner generates code from the protocol specs, which are xml 
files.
https://wayland.app/protocols/ 



I will write a new generator/scanner from scratch - it's not too much 
work.
The generator/scanner itself does not necessarily need to be part of 
the PR, but it might be a good idea to include it, since the protocol 
changes over time.


-- Thiago.



Em seg., 29 de abr. de 2024 às 18:10, Kevin Rushforth 
 escreveu:


As a reminder, contributors must not include 3rd-party code in any
openjdk repo. Per the terms of the OCA, all code that you
contribute to OpenJDK must be your own code. This includes code
you push to openjdk/jfx-sandbox and code in a branch of a personal
fork of openjdk/jfx from which you create a PR.

-- Kevin


On 4/28/2024 2:45 PM, Thiago Milczarek Sayão wrote:

Hi,

I managed to display a very basic wayland toplevel surface from java:
https://github.com/tsayao/glass-wayland



If you are using intellij, just run the "Test App" (with java 22).

generate.sh will jextract the code from wayland-client.

I rushed to get the window displayed - so it doesn't look good
yet (but I do accept suggestions).

It uses a java wayland-scanner (included) to read protocol xml
files and generate code that uses jextracted calls.

The sample also binds EGL and GL apis, but just because wayland
requires a buffer to display the surface. Maybe it was easier to
use a shared memory :)

Credits to (I adapted it to ouput jextract compatible code):
https://github.com/gfxstrand/wayland-java/tree/master/scanner



Cheers

Em ter., 23 de abr. de 2024 às 09:11, Thiago Milczarek Sayão
 escreveu:

I'm doing some work here:
https://github.com/tsayao/glass-wayland



So far it's been a good experience to use FFM / jextract.

The idea is to plug it as a glass wayland backend when it's
good enough.



Em seg., 22 de abr. de 2024 às 16:16, Nir Lisker
 escreveu:

Not sure it helps with warmup, but marking a foreign
function as critical can improve performance:

https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/foreign/Linker.Option.html#critical(boolean).

On Mon, Apr 22, 2024 at 10:02 PM Philip Race
 wrote:

No, it wasn't. I didn't even use jextracted code.
The startup cost is around initialisation of FFM -
around 70 ms (IIRC) overhead on my MacBook
Then creation of VarHandles and MethodHandles - 2-5
ms each is what I measured, so do these lazily if you
can.
And warmup cost is that it takes about 1
iterations to get code fully compiled.

java -XX:+PrintFlagsFinal -version 2>&1 | grep
CompileThreshold
 intx CompileThreshold =
1  {pd product}
{default}
    double CompileThresholdScaling  =
1.00 {product} {default}
    uintx IncreaseFirstTierCompileThresholdAt  =
50 {product} {default}
 intx Tier2CompileThreshold    =
0 {product} {default}
 intx Tier3CompileThreshold    =
2000 {product} {default}
 intx Tier4CompileThreshold    =
15000 {product} {default}

-phil.


On 4/22/24 11:45 AM, Thiago Milczarek Sayão wrote:

I think the startup time might be related to all
static symbol lookups.
So I'm manually including just what is needed:
jextract --output src -t com.sun.glass.wayland.extracted \
   --header-class-name GlassWayland

Re: RFR: 8325591: [Mac] DRAG_DONE reports null transferMode when destination is external

[Kevin Rushforth]

On Fri, 16 Feb 2024 22:35:49 GMT, Martin Fox  wrote:

> At the end of a drag operation the Mac Glass code sends out a DRAG_DONE event 
> using the operation mask tracked in the GlassDragSource to determine the 
> final transfer mode. That mask is only updated when a window in the JavaFX 
> app is the drop destination. If the drag moves to an external destination the 
> mask is set to NONE. If the drag terminates in the external destination that 
> NONE forms the basis of the transfer mode sent via the DRAG_DONE event.
> 
> At the very end of the drag the OS calls the NSDraggingSource 
> (GlassDraggingSource) with the final drag operation. This PR issues the 
> DRAG_DONE from that callback so it can get the final transfer mode correct 
> for both internal and external destinations.

The code changes look good. I did a quick test and it seems fine.

Would you be able to provide a manual test program for this case, under 
`tests/manual/dnd`? Also, can you merge in the latest changes from `master`?

-

PR Review: https://git.openjdk.org/jfx/pull/1371#pullrequestreview-2029735350

Re: Wayland

[Thiago Milczarek Sayão]

I thought about possible legal conflicts.

The code is on my github - I'm exploring and testing before starting the
real work.

wayland-scanner generates code from the protocol specs, which are xml files.
https://wayland.app/protocols/

I will write a new generator/scanner from scratch - it's not too much work.
The generator/scanner itself does not necessarily need to be part of the
PR, but it might be a good idea to include it, since the protocol changes
over time.

-- Thiago.



Em seg., 29 de abr. de 2024 às 18:10, Kevin Rushforth <
kevin.rushfo...@oracle.com> escreveu:

> As a reminder, contributors must not include 3rd-party code in any openjdk
> repo. Per the terms of the OCA, all code that you contribute to OpenJDK
> must be your own code. This includes code you push to openjdk/jfx-sandbox
> and code in a branch of a personal fork of openjdk/jfx from which you
> create a PR.
>
> -- Kevin
>
>
> On 4/28/2024 2:45 PM, Thiago Milczarek Sayão wrote:
>
> Hi,
>
> I managed to display a very basic wayland toplevel surface from java:
> https://github.com/tsayao/glass-wayland
>
> If you are using intellij, just run the "Test App" (with java 22).
>
> generate.sh will jextract the code from wayland-client.
>
> I rushed to get the window displayed - so it doesn't look good yet (but I
> do accept suggestions).
>
> It uses a java wayland-scanner (included) to read protocol xml files and
> generate code that uses jextracted calls.
>
> The sample also binds EGL and GL apis, but just because wayland requires a
> buffer to display the surface. Maybe it was easier to use a shared memory :)
>
> Credits to (I adapted it to ouput jextract compatible code):
> https://github.com/gfxstrand/wayland-java/tree/master/scanner
>
> Cheers
>
> Em ter., 23 de abr. de 2024 às 09:11, Thiago Milczarek Sayão <
> thiago.sa...@gmail.com> escreveu:
>
>> I'm doing some work here:
>> https://github.com/tsayao/glass-wayland
>>
>> So far it's been a good experience to use FFM / jextract.
>>
>> The idea is to plug it as a glass wayland backend when it's good enough.
>>
>>
>>
>> Em seg., 22 de abr. de 2024 às 16:16, Nir Lisker 
>> escreveu:
>>
>>> Not sure it helps with warmup, but marking a foreign function as
>>> critical can improve performance:
>>> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/foreign/Linker.Option.html#critical(boolean)
>>> .
>>>
>>> On Mon, Apr 22, 2024 at 10:02 PM Philip Race 
>>> wrote:
>>>
 No, it wasn't. I didn't even use jextracted code.
 The startup cost is around initialisation of FFM - around 70 ms (IIRC)
 overhead on my MacBook
 Then creation of VarHandles and MethodHandles - 2-5 ms each is what I
 measured, so do these lazily if you can.
 And warmup cost is that it takes about 1 iterations to get code
 fully compiled.

 java -XX:+PrintFlagsFinal -version 2>&1 | grep CompileThreshold
  intx CompileThreshold =
 1  {pd product} {default}
 double CompileThresholdScaling  =
 1.00  {product} {default}
 uintx IncreaseFirstTierCompileThresholdAt  =
 50{product} {default}
  intx Tier2CompileThreshold=
 0 {product} {default}
  intx Tier3CompileThreshold=
 2000  {product} {default}
  intx Tier4CompileThreshold=
 15000 {product} {default}

 -phil.


 On 4/22/24 11:45 AM, Thiago Milczarek Sayão wrote:

 I think the startup time might be related to all static symbol lookups.
 So I'm manually including just what is needed:

 jextract --output src -t com.sun.glass.wayland.extracted \
   --header-class-name GlassWayland \
   `pkg-config --libs glib-2.0 gio-2.0 libportal wayland-client` \
   `pkg-config --cflags-only-I glib-2.0 gio-2.0 libportal wayland-client` \
glass-wayland.h \
--include-function xdp_portal_initable_new \
--include-function xdp_session_close \
--include-function xdp_portal_open_file \
--include-function xdp_portal_open_file_finish \
--include-function g_object_unref \
--include-function g_timeout_add \
--include-function g_add_idle \
--include-function g_main_loop_run \
--include-function g_main_loop_new \
--include-function g_main_loop_ref \
--include-function g_main_loop_unref \
--include-function g_main_loop_quit \
--include-function g_settings_new \
--include-function g_settings_get_int \
--include-function wl_display_connect \
--include-function wl_display_disconnect \
--include-function wl_display_roundtrip \
--include-function wl_display_dispatch_p

Re: RFR: 8330590: TextInputControl: previous word fails with Bhojpuri characters [v2]

[Andy Goryachev]

On Fri, 19 Apr 2024 20:36:42 GMT, Andy Goryachev  wrote:

>> This change replaces Character.isLetterOrDigit(char) which fails with 
>> surrogate characters with  Character.isLetterOrDigit(int).
>
> Andy Goryachev 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 two additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330590.prev.word
>  - 8330590 TextInputControl: previous word fails with Bhojpuri characters

Looking at the "next word" functionality across different applications on 
different platforms, it appears to be a wide variety of behaviors.

One vendor appears to be quite consistent - Microsoft.  Its word, word pad, 
notepad work exactly the same, with Word working the same across macOS and 
Win11.

JavaFX TextArea is inconsistent (by design) between macOS and Win11, but also 
is inconsistent with Swing's JTextArea.

If I were to fix the behavior (if we decide to fix the behavior of the nextWord 
function, that is), I would make it consistent with MS Word, but let's discuss.

For reference, here is the result of my testing.  Initially, the caret is 
placed at index 0 and the numbers in parentheses denote successive caret 
positions after ctrl-RIGHT (option-RIGHT) key presses.  An underline denotes a 
space, and a (nl) denotes a newline.


source
_english_english_eng:_end,_eng:_(nl)
(nl)
_eng


BreakIterator.getWordInstance()
_(1)english(2)_(3)english(4)_(5)eng(6):(7)_(8)end(9),(10)_(11)eng(12):(13)_(14)(nl)
(15)(nl)
(16)_(17)eng


text area (mac)
_english(1)_english(2)_eng(3):(4)_end(5),(6)_eng(7):(8)_(nl)
(9)(nl)
(10)_eng(11)


ms word (mac) 16.84 24041420 consistent with win11
_(1)english_(2)english_(3)eng(4):_(5)end(6),_(7)eng(8):_(9)(nl)
(10)(nl)
(11)_(12)eng(13)


text edit (mac)
_english(1)_english(2)_eng(3):_end(4),_eng(5):_(nl)
(nl)
(nl)_eng(6)


chrome (mac) 
 english(1)_english(2)_eng(3):(4)_end(5),(6)_eng(7):(8)_
(9)
_(10)eng(11)


eclipse (mac)
_(1)english_(2)english_(3)eng(4):_(5)end(6),_(7)eng(8):_(9)(nl)
(10)(nl)
(11)_(12)eng


JTextArea (mac)
_(1)english_(2)english_(3)eng(4):_(5)end(6),_(7)eng(8):_(9)(nl)
(nl)
_(10)eng


ms word 365 ver 2302 build 16.0.16130.20942 (win 11)
same as notepad (win 11)
same as wordpad (win 11)
_(1)english_(2)english_(3)eng(4):_(5)end(6),_(7)eng(8):_(9)(nl)
(10)(nl)
(11)_(12)eng


TextArea (win11)
_(1)english_(2)english_(3)eng(4):_(5)end(6),_(7)eng(8):_(9)(nl)
(10)(nl)
_(11)eng

@aghaisas would you please take a look at this also?

-

PR Comment: https://git.openjdk.org/jfx/pull/1444#issuecomment-2083707096
PR Comment: https://git.openjdk.org/jfx/pull/1444#issuecomment-2083707914

Re: Wayland

[Kevin Rushforth]

As a reminder, contributors must not include 3rd-party code in any 
openjdk repo. Per the terms of the OCA, all code that you contribute to 
OpenJDK must be your own code. This includes code you push to 
openjdk/jfx-sandbox and code in a branch of a personal fork of 
openjdk/jfx from which you create a PR.


-- Kevin


On 4/28/2024 2:45 PM, Thiago Milczarek Sayão wrote:

Hi,

I managed to display a very basic wayland toplevel surface from java:
https://github.com/tsayao/glass-wayland

If you are using intellij, just run the "Test App" (with java 22).

generate.sh will jextract the code from wayland-client.

I rushed to get the window displayed - so it doesn't look good yet 
(but I do accept suggestions).


It uses a java wayland-scanner (included) to read protocol xml files 
and generate code that uses jextracted calls.


The sample also binds EGL and GL apis, but just because wayland 
requires a buffer to display the surface. Maybe it was easier to use a 
shared memory :)


Credits to (I adapted it to ouput jextract compatible code):
https://github.com/gfxstrand/wayland-java/tree/master/scanner

Cheers

Em ter., 23 de abr. de 2024 às 09:11, Thiago Milczarek Sayão 
 escreveu:


I'm doing some work here:
https://github.com/tsayao/glass-wayland

So far it's been a good experience to use FFM / jextract.

The idea is to plug it as a glass wayland backend when it's good
enough.



Em seg., 22 de abr. de 2024 às 16:16, Nir Lisker
 escreveu:

Not sure it helps with warmup, but marking a foreign function
as critical can improve performance:

https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/foreign/Linker.Option.html#critical(boolean).

On Mon, Apr 22, 2024 at 10:02 PM Philip Race
 wrote:

No, it wasn't. I didn't even use jextracted code.
The startup cost is around initialisation of FFM - around
70 ms (IIRC) overhead on my MacBook
Then creation of VarHandles and MethodHandles - 2-5 ms
each is what I measured, so do these lazily if you can.
And warmup cost is that it takes about 1 iterations to
get code fully compiled.

java -XX:+PrintFlagsFinal -version 2>&1 | grep
CompileThreshold
 intx CompileThreshold =
1  {pd product} {default}
    double CompileThresholdScaling = 1.00 {product}
{default}
    uintx IncreaseFirstTierCompileThresholdAt  =
50    {product} {default}
 intx Tier2CompileThreshold    =
0 {product} {default}
 intx Tier3CompileThreshold    =
2000  {product} {default}
 intx Tier4CompileThreshold    =
15000 {product} {default}

-phil.


On 4/22/24 11:45 AM, Thiago Milczarek Sayão wrote:

I think the startup time might be related to all static
symbol lookups.
So I'm manually including just what is needed:
jextract --output src -t com.sun.glass.wayland.extracted \
   --header-class-name GlassWayland \
   `pkg-config --libs glib-2.0 gio-2.0 libportal wayland-client`  \
   `pkg-config --cflags-only-I glib-2.0 gio-2.0 libportal 
wayland-client`  \
glass-wayland.h \
--include-function xdp_portal_initable_new \
--include-function xdp_session_close \
--include-function xdp_portal_open_file \
--include-function xdp_portal_open_file_finish \
--include-function g_object_unref \
--include-function g_timeout_add \
--include-function g_add_idle \
--include-function g_main_loop_run \
--include-function g_main_loop_new \
--include-function g_main_loop_ref \
--include-function g_main_loop_unref \
--include-function g_main_loop_quit \
--include-function g_settings_new \
--include-function g_settings_get_int \
--include-function wl_display_connect \
--include-function wl_display_disconnect \
--include-function wl_display_roundtrip \
--include-function wl_display_dispatch_pending \
--include-typedef GAsyncReadyCallback \
--include-typedef GSourceFunc \
--include-typedef GError

Em seg., 22 de abr. de 2024 às 13:24, Philip Race
 escreveu:

As a reminder, using FFM will require all FX
*applications* to speci

Re: RFR: 8092102: Labeled: truncated property [v9]

[Victor Dyakov]

On Wed, 10 Apr 2024 21:25:10 GMT, Andy Goryachev  wrote:

>> Adds **Labeled.textTruncated** property which indicates when the text is 
>> visually truncated (and the ellipsis string is inserted) in order to fit the 
>> available width.
>> 
>> The new property reacts to changes in the following properties:
>> - ellipsisString
>> - font
>> - height
>> - text
>> - width
>> - wrapText
>> 
>> I don't think it's worth creating a headful test (headless won't work) due 
>> to relative simplicity of the code.
>> 
>> **Alternative**
>> 
>> The desired functionality can be just as easily achieved on an application 
>> level, by adding a similar property to a subclass.  What is the benefit of 
>> adding this functionality to the core?
>> 
>> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as 
>> well) lives by different rules (TableCellSkinBase:152, 
>> TreeTableCellSkin:126).  The consequence of this is that the new 
>> functionality **cannot** be fully implemented with the public APIs alone.
>> 
>> **See Also**
>> 
>> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow 
>> for tooltip when cell text is truncated
>> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show 
>> Tooltip only when text is shown with ellipsis (...)
>
> Andy Goryachev has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - missing )
>  - review comments
>  - Merge branch 'master' into 8092102.truncated
>  - add exports
>  - added unit tests
>  - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>  - test
>  - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>  - Merge branch 'master' into 8092102.truncated
>  - labeled helper
>  - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e

@azuev-java please review

-

PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2083402516

Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]

[Victor Dyakov]

On Mon, 8 Apr 2024 18:19:22 GMT, Alexander Zuev  wrote:

>> Andy Goryachev 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 14 additional 
>> commits since the last revision:
>> 
>>  - tests
>>  - cleanup
>>  - node orientation
>>  - Merge remote-tracking branch 'origin/master' into 8313138.horizontal
>>  - table view behavior
>>  - tree view behavior
>>  - list view behavior
>>  - orientation
>>  - Merge remote-tracking branch 'origin/master' into 8313138.horizontal
>>  - Merge branch 'master' into 8313138.horizontal
>>  - ... and 4 more: https://git.openjdk.org/jfx/compare/66578503...5bae5e7a
>
> Looks good.

@azuev-java please review

-

PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2083402973

Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]

[Ambarish Rapte]

On Mon, 29 Apr 2024 15:47:54 GMT, Oliver Kopp  wrote:

>> modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java
>>  line 104:
>> 
>>> 102: int length = text.length();
>>> 103: start = Utils.clamp(0, start, length);
>>> 104: end = Utils.clamp(start, end, length);
>> 
>> This is only cleanup and not required for this fix.
>
> I applied the software engineering principle to leave the code cleaner than 
> seen. (Martin Fowler et all) 
> 
> Should I revert this?

Sorry for not being clear. I just wanted to point out that it is not required 
for the issue fix.
Please don't revert. This looks better.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583488561

Re: RFR: 8330590: TextInputControl: previous word fails with Bhojpuri characters [v2]

[Andy Goryachev]

On Fri, 19 Apr 2024 20:36:42 GMT, Andy Goryachev  wrote:

>> This change replaces Character.isLetterOrDigit(char) which fails with 
>> surrogate characters with  Character.isLetterOrDigit(int).
>
> Andy Goryachev 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 two additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330590.prev.word
>  - 8330590 TextInputControl: previous word fails with Bhojpuri characters

I think we need to fix endOf/nextWord as well, as the logic seems to be 
breaking with the surrogate pairs:

![Screenshot 2024-04-29 at 09 48 
33](https://github.com/openjdk/jfx/assets/107069028/02a33046-6d15-45c1-a294-57bf609bdc8d)


The issue can also be seen with Awadhi: अवधी/औधी

-

PR Comment: https://git.openjdk.org/jfx/pull/1444#issuecomment-2083210649

Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v4]

[eduardsdv]

On Mon, 29 Apr 2024 09:30:18 GMT, Florian Kirmaier  
wrote:

>> In some situations, a part of the SG is no longer rendered.
>> I created a test program that showcases this problem.
>> 
>> Explanation:
>> 
>> This can happen, when a part of the SG, is covered by another Node.
>> In this part, one node is totally covered, and the other node is visible.
>> 
>> When the totally covered Node is changed, then it is marked dirty and it's 
>> parent, recursively until an already dirty node is found.
>> Due to the Culling, this totally covered Node is not rendered - with the 
>> effect that the tree is never marked as Clean.
>> 
>> In this state, a Node is Dirty but not It's parent. Based on my CodeReview, 
>> this is an invalid state which should never happen.
>> 
>> In this invalid state, when the other Node is changed, which is visible, 
>> then the dirty state is no longer propagated upwards - because the recursive 
>> "NGNode.markTreeDirty" algorithm encounters a dirty node early.
>> 
>> This has the effect, that any SG changes in the visible Node are no longer 
>> rendered. Sometimes the situation repairs itself.
>> 
>> Useful parameters for further investigations:
>> -Djavafx.pulseLogger=true
>> -Dprism.printrendergraph=true
>> -Djavafx.pulseLogger.threshold=0
>> 
>> PR:
>> This PR ensures the dirty flag is set to false of the tree when the culling 
>> is used.
>> It doesn't seem to break any existing tests - but I'm not sure whether this 
>> is the right way to fix it.
>> It would be great to have some feedback on this solution - maybe guiding me 
>> to a better solution.
>> 
>> I could write a test, that just does the same thing as the test application, 
>> but checks every frame that these nodes are not dirty - but maybe there is a 
>> better way to test this.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8322619: Clear dirty flag on the node and all its children if they are 
> skipped due to visible==false or opacity==0

After further investigation and testing I would suggest to remove all 
``clearDirty()`` and ``clearDirtyTree()`` calls and put only one clear-dirty 
pass after rendering is done (at the end of ``ViewerPainter.paintImpl(final 
Graphics backBufferGraphics)``).

Since the ``markDirty()`` method does both, sets the dirty flag and propagates 
it to the ancestor nodes, it is better if there is only one method for deleting 
the dirty flag from the node and all its children. Therefore, I would remove 
the ``clearDirtyTree()`` method and move its content to the ``clearDirty()`` 
method.

This way we can guarantee that all dirty flags are removed after the rendering 
is completed.
We also guarantee that a clean parent with dirty children will never occur (the 
bug this PR addresses).

Furthermore, my quick test shows that fewer ``clearDirty()`` calls are required 
in the new version. Bonus: We are even faster.


public void clearDirty() {
if (dirty != DirtyFlag.CLEAN || childDirty) {
dirty = DirtyFlag.CLEAN;
childDirty = false;
dirtyBounds.makeEmpty();
dirtyChildrenAccumulated = 0;

if (this instanceof NGGroup) {
List children = ((NGGroup) this).getChildren();
for (NGNode child : children) {
child.clearDirtyTree_();
}
}
}

if (getClipNode() != null) {
getClipNode().clearDirty();
}
}

-

PR Comment: https://git.openjdk.org/jfx/pull/1310#issuecomment-2083153697

Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]

[Oliver Kopp]

On Mon, 29 Apr 2024 12:35:19 GMT, Ambarish Rapte  wrote:

>> Oliver Kopp has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Revert using utility method
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java
>  line 124:
> 
>> 122: // In case there was an overflow, return the maximum end index
>> 123: if (res < 0) return maxEndIndex;
>> 124: return res;
> 
> In this class,
> - The index **start** and **end** are guaranteed to be +ve values such that, 
> `0 <= start <= end <= length` (length is the total length of text in the text 
> component). : please check the method `validateRange()`
> - So we only need to validate that the variable `length` in this helper 
> method is valid i.e.
>1. `length > 0` and
>2. `length <= end - start`
> 
> 
> /**
>  * In the context of substrings, this is a helper method to get the end 
> offset index
>  * from the start index for a given length of a substring.
>  *
>  * @param startIndex The start index of the range in the string. Needs to 
> be 0 or more (not checked in the code).
>  * @param length The requested length of a substring in the range when 
> starting from "start".
>  *   Invalid values are treated as full length.
>  * @param endIndex The end index of the range in the string. Needs to be 
> equal or greater than startIndex
>  *(not checked in the code).
>  */
> static int getEndOffsetIndex(int startIndex, int length, int endIndex) {
> if (length < 0 || (endIndex - startIndex) <= length) {
> return endIndex;
> }
> return startIndex + length;
> }
> 
> - With this change
> 1. The start and end index values are always positive and hence negative 
> value tests should be removed.
> 2. The name of method is changed so, the Shim class would need a change 
> too.
> 3. The fix could be as simple as below one line correction in the ternary 
> statement in the GetText() method, but then we won't be able to add the test. 
> So keeping the helper method would be good idea.
> `int endOffset = (length < 0 || (endIndex - startIndex) <= length) ? end : 
> start + maxLength;`

Smalltalk: I could go back to my first commit 
https://github.com/openjdk/jfx/commit/e81712ca12035e6607c9a31379fa0180be8be7fd 
to be consistent with the other style in the class 🤣🤣🤣. Only joking, the new 
code is more readable to Java newcomers IMHO. 😅✌️

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583344489

Re: RFR: 8330590: TextInputControl: previous word fails with Bhojpuri characters [v2]

[Andy Goryachev]

On Mon, 29 Apr 2024 08:55:58 GMT, Karthik P K  wrote:

> Is this expected?

I think it might be a bug - even though it's unclear how many words the text 
"𑂦𑂷𑂔𑂣𑂳𑂩𑂲" contains, I would not expect it to go to the beginning of that 
segment.

I suspect the code in `TextInputControl.endOfNextWord(boolean)` is incorrect, 
and it needs a deeper re-write than the naive replacement with 
`isLetterOrDigit()`.

-

PR Comment: https://git.openjdk.org/jfx/pull/1444#issuecomment-2083089021

Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]

[Oliver Kopp]

On Mon, 29 Apr 2024 14:09:52 GMT, Ambarish Rapte  wrote:

>> Oliver Kopp has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Revert using utility method
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java
>  line 104:
> 
>> 102: int length = text.length();
>> 103: start = Utils.clamp(0, start, length);
>> 104: end = Utils.clamp(start, end, length);
> 
> This is only cleanup and not required for this fix.

I applied the software engineering principle to leave the code cleaner than 
seen. (Martin Fowler et all) 

Should I revert this?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583314511

Re: RFR: 8320912: IME should commit on focus change

[Andy Goryachev]

On Mon, 29 Apr 2024 14:09:03 GMT, yosbits  wrote:

> I found a serious problem with this change.

Thank you!  I've created https://bugs.openjdk.org/browse/JDK-8331319 feel free 
to update / clarify the information in the ticket.

-

PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-2082942526

Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]

[Ambarish Rapte]

On Fri, 26 Apr 2024 22:58:37 GMT, Oliver Kopp  wrote:

>> Fixes https://bugs.openjdk.org/browse/JDK-8330462.
>> 
>> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, 
>> then an addition of `start` to it leads to a negative value. This is "fixed" 
>> by using `Math.max` comparing the `maxLength` and `maxLength + start`.
>
> Oliver Kopp has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revert using utility method

Additionally I tried to figure out the reason as to why DeepL makes the 
`GetText` call with `INT_MAX` value by reading through all APIs of 
[ITextRangeProvider 
interface](https://learn.microsoft.com/en-us/windows/win32/api/uiautomationcore/nn-uiautomationcore-itextrangeprovider).
Tried to understand if the client application reads any attributes of the text 
range that can be used to determine the `maxLength` value.
If we find a dependency then the right way would have to fix that dependency. 
But I could not identify any attribute or any API that could be related to the 
`maxLength` value.

-

PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2082876837

Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v24]

[Ambarish Rapte]

On Fri, 26 Apr 2024 22:58:37 GMT, Oliver Kopp  wrote:

>> Fixes https://bugs.openjdk.org/browse/JDK-8330462.
>> 
>> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, 
>> then an addition of `start` to it leads to a negative value. This is "fixed" 
>> by using `Math.max` comparing the `maxLength` and `maxLength + start`.
>
> Oliver Kopp has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revert using utility method

In addition to the evaluation added by @jperedadnr to 
[JDK-8330462](https://bugs.openjdk.org/browse/JDK-8330462)
- The UI Automation framework offers two sets of APIs.
1. Provider APIs : to be implemented by UI providers ( JavaFX )
2. Client APIs : to be implemented by the Client applications like (Screen 
readers, DeepL)
- Both providers(JavaFX) and client application should follow their respective 
specification.
- For this scenario
1. Client Application should follow : [IUIAutomationTextRange::GetText 
method](https://learn.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-gettext#parameters)
2. JavaFX should follow : 
[ITextRangeProvider::GetText](https://learn.microsoft.com/en-us/windows/win32/api/uiautomationcore/nf-uiautomationcore-itextrangeprovider-gettext#parameters)
- As per both above spec, the length variable could be either -1 or a valid 
value.
- The check we had earlier: `length != -1` was based on the spec, but seems 
like we need to validate against out of range check i.e. `length <= end - 
start`.
- It seems in this scenario DeepL invokes the GetText call with INT_MAX value, 
which is the root cause. The check `length <= end - start` should handle that 
scenario.
- and `length < 0` would future protect us from any negative value though as 
per spec `length != -1` should suffice

Testing:
- I could reproduce the issue with DeepL
- Issue gets fixed with this fix and also with the change that I suggesting
- A couple tests of negative values for start and end should be removed.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java
 line 104:

> 102: int length = text.length();
> 103: start = Utils.clamp(0, start, length);
> 104: end = Utils.clamp(start, end, length);

This is only cleanup and not required for this fix.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java
 line 124:

> 122: // In case there was an overflow, return the maximum end index
> 123: if (res < 0) return maxEndIndex;
> 124: return res;

In this class,
- The index **start** and **end** are guaranteed to be +ve values such that, `0 
<= start <= end <= length` (length is the total length of text in the text 
component). : please check the method `validateRange()`
- So we only need to validate that the variable `length` in this helper method 
is valid i.e.
   1. `length > 0` and
   2. `length <= end - start`


/**
 * In the context of substrings, this is a helper method to get the end 
offset index
 * from the start index for a given length of a substring.
 *
 * @param startIndex The start index of the range in the string. Needs to 
be 0 or more (not checked in the code).
 * @param length The requested length of a substring in the range when 
starting from "start".
 *   Invalid values are treated as full length.
 * @param endIndex The end index of the range in the string. Needs to be 
equal or greater than startIndex
 *(not checked in the code).
 */
static int getEndOffsetIndex(int startIndex, int length, int endIndex) {
if (length < 0 || (endIndex - startIndex) <= length) {
return endIndex;
}
return startIndex + length;
}

- With this change
1. The start and end index values are always positive and hence negative 
value tests should be removed.
2. The name of method is changed so, the Shim class would need a change too.
3. The fix could be as simple as below one line correction in the ternary 
statement in the GetText() method, but then we won't be able to add the test. 
So keeping the helper method would be good idea.
`int endOffset = (length < 0 || (endIndex - startIndex) <= length) ? end : 
start + maxLength;`

tests/system/src/test/java/test/com/sun/glass/ui/win/WinTextRangeProviderTest.java
 line 93:

> 91: 
> 92: // No range check for maxEndIndex
> 93: Arguments.of(-1, -1, 2, -1),

negative valued tests for `start` or `end` are not required for this particular 
scenario as `0 <= start <= end <= total length of text` is guaranteed.

-

Changes requested by arapte (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1442#pullrequestreview-2028389977
PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583158450
PR Review Comment: https://git.openjdk.org/jfx/pull/1

Re: RFR: 8320912: IME should commit on focus change

[yosbits]

On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox  wrote:

> This is a Mac only bug. If the user was in the middle of IM text composition 
> and clicked on a different node the partially composed text was left in the 
> old node and the IM window wasn't dismissed. This PR implements the existing 
> finishInputMethodComposition call so it can commit the text and dismiss the 
> IM window before focus moves away from the node where composition was taking 
> place.
> 
> This PR changes the implementation of `unmarkText` to match what we want and 
> what Apple says it should do ("The text view should accept the marked text as 
> if it had been inserted normally"). With that said I haven't found an IME 
> that calls this routine.

**I found a serious problem with this change.**

After applying this change, the IME cannot be typed after the small window is 
displayed.
Even basic applications that use ContextMenu, ChoiceBox, ComboBox, MenuButton, 
etc. are affected.
The impact of this problem will be perceived as block level in the IME usage 
environment.

This problem is no longer reproduced by reverting the changes in JDK-8320912.
However, since JDK-8301900 depends on the changes in JDK-8320912, 8301900 was 
also reverted.

This problem can be reproduced with the following application

``` Java
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.ChoiceBox;
import javafx.scene.control.ComboBox;
import javafx.scene.control.Label;
import javafx.scene.control.MenuButton;
import javafx.scene.control.MenuItem;
import javafx.scene.control.TextArea;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class ImeTest extends Application{

enum Items {
A,
B,
C
}

@Override
public void start(Stage primaryStage) throws Exception {
ChoiceBox choiceBox = new ChoiceBox<>();
ComboBox comboBox = new ComboBox<>();
MenuButton menuButton = new MenuButton();

menuButton.getItems().addAll(new MenuItem("1"), new 
MenuItem("2"));
choiceBox.getItems().addAll(Items.values());
comboBox.getItems().addAll(Items.values());

VBox root = new VBox(
new Label("1. Select any of the following control 
items."),
new HBox( 8, choiceBox, comboBox, menuButton),
new Label("2. Entering text with the IME."),
new HBox(8, new TextField(), new TextArea()));

Scene scene = new Scene(root, 600, 600);
primaryStage.setScene(scene);
primaryStage.show();

}

public static void main(String[] args) {
Application.launch(args);
}
}

-

PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-2082858747

Re: RFR: 8329820: [Linux] Prefer EGL over GLX [v6]

[Thiago Milczarek Sayao]

> Wayland implementation will require EGL. 
> 
> EGL works with Xorg as well. The idea is to be EGL first and if it fails, 
> fallback to GLX. A force flag `prism.es2.forceGLX=true` is available.
> 
> 
> See:
> [Switching the Linux graphics stack from GLX to 
> EGL](https://mozillagfx.wordpress.com/2021/10/30/switching-the-linux-graphics-stack-from-glx-to-egl/)
> [Prefer EGL to GLX for the GL support on 
> X11](https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3540)

Thiago Milczarek Sayao has updated the pull request incrementally with one 
additional commit since the last revision:

  Revert "Fix cast"
  
  This reverts commit 4d62744aeedd23090be369e576d492d671fff930.

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1381/files
  - new: https://git.openjdk.org/jfx/pull/1381/files/4d62744a..f8c61efc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1381&range=05
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1381&range=04-05

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/1381.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1381/head:pull/1381

PR: https://git.openjdk.org/jfx/pull/1381

Re: RFR: 8329820: [Linux] Prefer EGL over GLX [v5]

[Thiago Milczarek Sayao]

> Wayland implementation will require EGL. 
> 
> EGL works with Xorg as well. The idea is to be EGL first and if it fails, 
> fallback to GLX. A force flag `prism.es2.forceGLX=true` is available.
> 
> 
> See:
> [Switching the Linux graphics stack from GLX to 
> EGL](https://mozillagfx.wordpress.com/2021/10/30/switching-the-linux-graphics-stack-from-glx-to-egl/)
> [Prefer EGL to GLX for the GL support on 
> X11](https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3540)

Thiago Milczarek Sayao has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix cast

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1381/files
  - new: https://git.openjdk.org/jfx/pull/1381/files/aa321efb..4d62744a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1381&range=04
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1381&range=03-04

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/1381.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1381/head:pull/1381

PR: https://git.openjdk.org/jfx/pull/1381

Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v4]

[Florian Kirmaier]

> In some situations, a part of the SG is no longer rendered.
> I created a test program that showcases this problem.
> 
> Explanation:
> 
> This can happen, when a part of the SG, is covered by another Node.
> In this part, one node is totally covered, and the other node is visible.
> 
> When the totally covered Node is changed, then it is marked dirty and it's 
> parent, recursively until an already dirty node is found.
> Due to the Culling, this totally covered Node is not rendered - with the 
> effect that the tree is never marked as Clean.
> 
> In this state, a Node is Dirty but not It's parent. Based on my CodeReview, 
> this is an invalid state which should never happen.
> 
> In this invalid state, when the other Node is changed, which is visible, then 
> the dirty state is no longer propagated upwards - because the recursive 
> "NGNode.markTreeDirty" algorithm encounters a dirty node early.
> 
> This has the effect, that any SG changes in the visible Node are no longer 
> rendered. Sometimes the situation repairs itself.
> 
> Useful parameters for further investigations:
> -Djavafx.pulseLogger=true
> -Dprism.printrendergraph=true
> -Djavafx.pulseLogger.threshold=0
> 
> PR:
> This PR ensures the dirty flag is set to false of the tree when the culling 
> is used.
> It doesn't seem to break any existing tests - but I'm not sure whether this 
> is the right way to fix it.
> It would be great to have some feedback on this solution - maybe guiding me 
> to a better solution.
> 
> I could write a test, that just does the same thing as the test application, 
> but checks every frame that these nodes are not dirty - but maybe there is a 
> better way to test this.

Florian Kirmaier has updated the pull request incrementally with one additional 
commit since the last revision:

  JDK-8322619: Clear dirty flag on the node and all its children if they are 
skipped due to visible==false or opacity==0

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1310/files
  - new: https://git.openjdk.org/jfx/pull/1310/files/530adc10..5f9f1574

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1310&range=03
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1310&range=02-03

  Stats: 16 lines in 1 file changed: 8 ins; 2 del; 6 mod
  Patch: https://git.openjdk.org/jfx/pull/1310.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1310/head:pull/1310

PR: https://git.openjdk.org/jfx/pull/1310

Re: RFR: 8330590: TextInputControl: previous word fails with Bhojpuri characters [v2]

[Karthik P K]

On Fri, 19 Apr 2024 20:36:42 GMT, Andy Goryachev  wrote:

>> This change replaces Character.isLetterOrDigit(char) which fails with 
>> surrogate characters with  Character.isLetterOrDigit(int).
>
> Andy Goryachev 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 two additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8330590.prev.word
>  - 8330590 TextInputControl: previous word fails with Bhojpuri characters

LGTM
Validated the changes manually and using the unit tests.
The new unit test fails without the fix and passes with the fix provided in 
this PR as expected.

I have one query though, while moving from left to write by word (using option 
+ RIGHT) for the same text, after the first word "Bhojpuri", it goes to the 
beginning of the Bhojpuri text and then to the end of the Bhojpuri text. But if 
it was all English text, then it directly goes to the end of the text and skips 
space. Is this expected?

-

Marked as reviewed by kpk (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1444#pullrequestreview-2027897621

Integrated: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Lukasz Kostyra]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

This pull request has now been integrated.

Changeset: 7294849c
Author:Lukasz Kostyra 
URL:   
https://git.openjdk.org/jfx/commit/7294849caefe1c986fdf7764f4c41b5047ed7765
Stats: 142 lines in 19 files changed: 0 ins; 62 del; 80 mod

8320563: Remove D3D9 code paths in favor of D3D9Ex

Reviewed-by: nlisker, mstrauss

-

PR: https://git.openjdk.org/jfx/pull/1445