Re: RFR: 8331748: Update libxml2 to 2.12.6

2024-05-15 Thread Joeri Sykora
On Tue, 14 May 2024 05:21:37 GMT, Hima Bindu Meda  wrote:

> Updated libxml to v2.12.6. Verified build and sanity testing on all 
> platforms. No issue seen

All tests are green here too.

-

Marked as reviewed by sykora (Author).

PR Review: https://git.openjdk.org/jfx/pull/1453#pullrequestreview-2057279996


Re: KeyCode in the javafx.scene.robot.Robot interface

2024-05-15 Thread Martin Fox
Mark,

You may already know this but before JavaFX 21 the Mac and Windows Robot code 
had some long-standing bugs with non-US keyboards. Linux is in better shape but 
you can encounter problems if the user has installed multiple layouts (I have a 
PR pending to fix that). That might explain some of the flakey behavior you’ve 
been seeing. Your approach might also invoke an unexpected dead key depending 
on what modifiers you’re probing.

With one exception JavaFX doesn’t have any facility for querying the keyboard 
layout. It relies on the platform code to take in keyboard events and translate 
them to JavaFX events on the fly. The exception is the internal call 
Toolkit.getKeyCodeForChar which attempts to map from a character to a KeyCode 
and is used to match shortcuts specified as KeyCharacterCombinations. 
Unfortunately the current implementation on Mac and Linux can’t really be 
extended to cover the sort of testing you’re trying to do.

(On any OS the keyboard machinery is designed to take a series of physical 
keystrokes and produce a series of characters. Attempting to go in the opposite 
direction is fraught with complications particularly for punctuation and 
symbols. It’s faster and more reliable to wait for the user to type something 
and record what happens. That’s how getKeyCodeForChar is implemented on the 
Mac; until the user types something it yields no results. Linux is currently 
buggy but the PR to fix it adopts the same approach as the Mac.)

JavaFX needs a better framework for testing text entry and I’ve thought about 
how that might look. But my priorities might be different than yours. Based on 
the bugs I’ve seen the biggest issue is that developers only test on the 
platforms and layouts they use. Much of this is due to lack of awareness but 
it’s also a big hurdle to have to buy Mac, Windows, and Linux boxes and then 
manually switch between layouts to test. My ideal framework would be based on 
emulated keyboards so, say, a U.S. Windows developer could generate a key event 
stream that corresponds to a Spanish Mac layout.

That approach wouldn’t involve sending platform events through the OS. 
Extending the Robot to make it easier to, say, target punctuation and symbols 
via platform events would be nice to have but not essential (IMHO).

So, no, you’re not missing anything exposed or under the hood. The approach 
you’re taking is probably the best that can be done with the current API.

Martin

> On May 12, 2024, at 3:16 AM, Mark Raynsford  wrote:
> 
> Hello!
> 
> I maintain a test harness for JavaFX applications:
> 
> https://www.github.com/io7m-com/xoanon
> 
> I expose an interface that uses the javafx.scene.robot.Robot
> interface internally, but I expose a slightly higher level
> API that allows for (amongst other things) typing text as strings
> on components rather than sending individual key codes.
> 
> The problem I immediately run into is that KeyCodes are keyboard
> layout specific, and there doesn't appear to be any way to detect
> what keyboard layout is in use. If I, for example, send the KeyCode for
> the `@` symbol, I'll actually get `"` on some keyboard layouts. This
> can cause some types of tests to fail, even though the code is correct.
> 
> Consider a test where an email address is typed into a text field, and
> then the form containing the field is checked to see if the text field
> accepted/rejected the text as being a valid email address. On some
> keyboard layouts, a test that sends the string "some...@example.com"
> will fail because what ends up in the text field is actually
> "someone"example.com".
> 
> I provide a workaround for this: At the start of the test suite run,
> xoanon will send every KeyCode to a text field one at a time and see
> what character appeared in the text field as a result. It then builds a
> mapping between characters to KeyCodes, and it can then do the
> translations as necessary when running the rest of the test suite:
> 
> https://github.com/io7m-com/xoanon?tab=readme-ov-file#keymap-generation
> 
> Unfortunately, this is still rather flaky. This often seems to fail more
> or less at random for reasons that aren't clear to me.
> 
> Is there some way the Robot interface could be extended to allow for
> typing strings? Failing that, is there some way JavaFX could advertise
> the underlying keyboard map? I feel like some underlying component must
> know the information I'm trying to infer, but it doesn't seem to be
> exposed anywhere that I've been able to find.
> 
> -- 
> Mark Raynsford | https://www.io7m.com



Re: RFR: 8279140: ComboBox can lose selected value on item change via setAll [v2]

2024-05-15 Thread Andy Goryachev
On Thu, 9 May 2024 09:38:31 GMT, Karthik P K  wrote:

>> The `ComboBox` value was not set to previously selected value in the item 
>> list change listener when `setAll` method is used to change the items. Fixed 
>> the issue by restoring the selection in this case.
>> 
>> Added a unit test to validate the fix.
>> [JDK-8279139](https://bugs.openjdk.org/browse/JDK-8279139) is also fixed 
>> with this change and added a unit test for the same.
>
> Karthik P K has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix JDK-8279139

looks good.  thank you for adding the tests!

-

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1452#pullrequestreview-2058284727


Re: RFR: 8329821: [Linux] When using i3 WM, menus are incorrectly sized

2024-05-15 Thread Ambarish Rapte
On Sat, 6 Apr 2024 17:34:06 GMT, Thiago Milczarek Sayao  
wrote:

> Simple fix to only request `_NET_FRAME_EXTENTS` if window has decoration.
> 
> It seems i3 replies with decorated sizes, even if window is not decorated.
> 
> Won't hurt other WMs.

I could verify that this change fixes the issue with i3 window manager.
Shall do little more testing and update.

-

PR Comment: https://git.openjdk.org/jfx/pull/1437#issuecomment-2112824722


Integrated: 8329011: Update SQLite to 3.45.3

2024-05-15 Thread Hima Bindu Meda
On Tue, 14 May 2024 11:45:06 GMT, Hima Bindu Meda  wrote:

> Update SQLite to v3.45.3.Verified build on all platforms. Sanity testing 
> looks fine.

This pull request has now been integrated.

Changeset: 581e3a70
Author:Hima Bindu Meda 
URL:   
https://git.openjdk.org/jfx/commit/581e3a70a45a550a97e391e06735f4837336ada8
Stats: 26451 lines in 4 files changed: 17765 ins; 2993 del; 5693 mod

8329011: Update SQLite to 3.45.3

Reviewed-by: sykora, kcr

-

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


Integrated: 8331748: Update libxml2 to 2.12.6

2024-05-15 Thread Hima Bindu Meda
On Tue, 14 May 2024 05:21:37 GMT, Hima Bindu Meda  wrote:

> Updated libxml to v2.12.6. Verified build and sanity testing on all 
> platforms. No issue seen

This pull request has now been integrated.

Changeset: 97b14025
Author:Hima Bindu Meda 
URL:   
https://git.openjdk.org/jfx/commit/97b1402501983f121f75c24a510f466837fa2ecc
Stats: 24912 lines in 115 files changed: 8485 ins; 11207 del; 5220 mod

8331748: Update libxml2 to 2.12.6

Reviewed-by: kcr, sykora

-

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


Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]

2024-05-15 Thread Johan Vos
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 think this PR is ready for review.

-

PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-2112928451


Re: RFR: 8279140: ComboBox can lose selected value on item change via setAll [v2]

2024-05-15 Thread Karthik P K
On Tue, 14 May 2024 22:27:25 GMT, Andy Goryachev  wrote:

>> Karthik P K has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix JDK-8279139
>
> It looks like we have quite a few bugs related to different aspects of 
> ComboBox.  This change is definitely an improvement, as it fixes the scenario 
> described in JDK-8279140.
> 
> This PR changes parts of the code that fixed JDK-8087838.  While I don't see 
> a regression in respect to JDK-8087838, I do see a different behavior with 
> and without this fix in JDK-8090221, using the test app there (to be 
> specific, with the fix in this PR, the tester in JDK-8090221 continues to 
> display the selected item in the combo box even though the output shows 
> `Selected Item After: null`
> 
> Do you think that JDK-8090221 scenario is a different case and should be 
> addressed separately?

Thanks for reviewing @andy-goryachev-oracle

-

PR Comment: https://git.openjdk.org/jfx/pull/1452#issuecomment-2112931888


Integrated: 8279140: ComboBox can lose selected value on item change via setAll

2024-05-15 Thread Karthik P K
On Tue, 7 May 2024 10:10:23 GMT, Karthik P K  wrote:

> The `ComboBox` value was not set to previously selected value in the item 
> list change listener when `setAll` method is used to change the items. Fixed 
> the issue by restoring the selection in this case.
> 
> Added a unit test to validate the fix.
> [JDK-8279139](https://bugs.openjdk.org/browse/JDK-8279139) is also fixed with 
> this change and added a unit test for the same.

This pull request has now been integrated.

Changeset: 88aad93d
Author:Karthik P K 
URL:   
https://git.openjdk.org/jfx/commit/88aad93d07b36c8b32afd701c9180753db975e10
Stats: 67 lines in 2 files changed: 63 ins; 3 del; 1 mod

8279140: ComboBox can lose selected value on item change via setAll

Reviewed-by: angorya

-

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


[jfx21u] RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField

2024-05-15 Thread Jose Pereda
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: [jfx21u] RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField

2024-05-15 Thread Andy Goryachev
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!

looking good

-

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx21u/pull/57#pullrequestreview-2058538870


Re: RFR: 8329821: [Linux] When using i3 WM, menus are incorrectly sized

2024-05-15 Thread Ambarish Rapte
On Sat, 6 Apr 2024 17:34:06 GMT, Thiago Milczarek Sayao  
wrote:

> Simple fix to only request `_NET_FRAME_EXTENTS` if window has decoration.
> 
> It seems i3 replies with decorated sizes, even if window is not decorated.
> 
> Won't hurt other WMs.

LGTM, tested with and without fix, with and without i3 window manager.

-

Marked as reviewed by arapte (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1437#pullrequestreview-2058602510


Integrated: 8329821: [Linux] When using i3 WM, menus are incorrectly sized

2024-05-15 Thread Thiago Milczarek Sayao
On Sat, 6 Apr 2024 17:34:06 GMT, Thiago Milczarek Sayao  
wrote:

> Simple fix to only request `_NET_FRAME_EXTENTS` if window has decoration.
> 
> It seems i3 replies with decorated sizes, even if window is not decorated.
> 
> Won't hurt other WMs.

This pull request has now been integrated.

Changeset: a7627fa8
Author:Thiago Milczarek Sayao 
URL:   
https://git.openjdk.org/jfx/commit/a7627fa8d4c0c2595d359a2656daf2f6005ee0f5
Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod

8329821: [Linux] When using i3 WM, menus are incorrectly sized

Reviewed-by: arapte

-

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


RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
Update the code review guidelines for JavaFX.

The JavaFX 
[CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
 guidelines includes guidance for creating, reviewing, and integrating changes 
to JavaFX, along with a pointer to a [Code Review 
Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.

This PR updates these guidelines to improve the quality of reviews, with a goal 
of improving JavaFX and decreasing the chance of introducing a serious 
regression or other critical bug.

The source branch has three commits:
1. Converts the Code Review Policies Wiki page to a 
[README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
 file in the repo and updates hyperlinks to the new location.
2. Update `README-code-reviews.md` with new guidelines
3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
changes to `README-code-reviews.md`)

Commit 1 is content neutral, so it might be helpful for reviewers to look at 
the changes starting from the second commit.

The updates are:

* In the Overview section, add a list of items for Reviewers, PR authors, and 
sponsoring Committers to verify prior to integration
* Create a "Guidelines for reviewing a PR" subsection of the Code review 
policies section
* Create a "Before you integrate or sponsor a PR" subsection of the Code review 
policies section
* Update the `CONTRIBUTING.md` page to highlight important requirements

-

Commit messages:
 - Update CONTRIBUTING.md and README-code-reviews.md
 - 8332313: Update code review guidelines
 - Convert Code Review Policies Wiki to README-code-reviews.md

Changes: https://git.openjdk.org/jfx/pull/1455/files
  Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1455&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332313
  Stats: 115 lines in 2 files changed: 106 ins; 0 del; 9 mod
  Patch: https://git.openjdk.org/jfx/pull/1455.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455

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


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

Reviewers: @johanvos @andy-goryachev-oracle @arapte @prrace 

I'd like at least 3 "R"eviewers to review this. Committers and Authors are also 
welcome to provide feedback.

-

PR Comment: https://git.openjdk.org/jfx/pull/1455#issuecomment-2113119065


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

CONTRIBUTING.md line 52:

> 50: 
> 51: * File a bug in JBS for every pull request
> 52: 

for line 53, would it make more sense to use this link
https://bugs.openjdk.org/projects/JDK/issues/
?

README-code-reviews.md line 10:

> 8: 
> 9: __Project Co-Lead__: Kevin Rushforth (kcr) 
> 10: __Project Co-Lead__: Johan Vos (jvos)

There are two sets of ids - one for OpenJFX/JBS and one for Github.  This might 
be confusing sometimes, should we list both?

README-code-reviews.md line 63:

> 61: Here is a list of things to keep in mind when reviewing a PR. This 
> applies to anyone doing a review, but especially a "R"eviewer:
> 62: 
> 63: * Make sure you understand why there was an issue to begin with, and 
> why/how the proposed PR solves the issue

Perhaps this should be noted as a required step for the PR author (maybe as a 
template text in the PR description?).

README-code-reviews.md line 64:

> 62: 
> 63: * Make sure you understand why there was an issue to begin with, and 
> why/how the proposed PR solves the issue
> 64: * Focus first on substantive comments rather than stylistic comments

this is not as important as the next two.

README-code-reviews.md line 66:

> 64: * Focus first on substantive comments rather than stylistic comments
> 65: * Consider the risk of regression
> 66: * Consider any compatibility concerns

regression and compatibility risks are probably the most important aspects of 
the code review from the "R"eviewer's perspective.  Perhaps this should be 
emphasized somehow?

README-code-reviews.md line 78:

> 76: * All substantive feedback has been addressed, especially any objections 
> from one with a Reviewer role.
> 77: * All Reviewers who have requested the chance to review have done so (or 
> indicated that they are OK with it going in without their review). In rare 
> cases a Project Lead may override this.
> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business 
> day (24 hours), not including weekends / or major holidays. This is to allow 
> sufficient time for those reviewers who might be in other time zones the 
> chance to review if they have concerns. This is measured from the time that 
> Skara has most recently added the "rfr" label (for example, for a PR that was 
> previously in Draft mode, wait for at least 24 hours after the PR has been 
> taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) 
> a Reviewer might give the OK to integrate without waiting for 24 hours.

nit pick: 24 hours does not always equal to one business day.  A long weekend 
may extend the waiting period to 96 hours, a "winter break" might span the 
whole week.

README-code-reviews.md line 79:

> 77: * All Reviewers who have requested the chance to review have done so (or 
> indicated that they are OK with it going in without their review). In rare 
> cases a Project Lead may override this.
> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business 
> day (24 hours), not including weekends / or major holidays. This is to allow 
> sufficient time for those reviewers who might be in other time zones the 
> chance to review if they have concerns. This is measured from the time that 
> Skara has most recently added the "rfr" label (for example, for a PR that was 
> previously in Draft mode, wait for at least 24 hours after the 

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 18:05:10 GMT, Andy Goryachev  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> CONTRIBUTING.md line 52:
> 
>> 50: 
>> 51: * File a bug in JBS for every pull request
>> 52: 
> 
> for line 53, would it make more sense to use this link
> https://bugs.openjdk.org/projects/JDK/issues/
> ?

Whether or not that might be a better link, it's unrelated to this PR. The 
scope of this PR is limited to guidelines surrounding reviewing, integrating, 
and sponsoring PRs.

> README-code-reviews.md line 10:
> 
>> 8: 
>> 9: __Project Co-Lead__: Kevin Rushforth (kcr) 
>> 10: __Project Co-Lead__: Johan Vos (jvos)
> 
> There are two sets of ids - one for OpenJFX/JBS and one for Github.  This 
> might be confusing sometimes, should we list both?

Not sure what you mean, but it doesn't seem related to this PR.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602112407
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602113564


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Nir Lisker
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

README-code-reviews.md line 48:

> 46: All code reviews must be done via a pull request submitted against this 
> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must 
> exist before the pull request will be reviewed. See 
> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
> request.
> 47: 
> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
> role (aka a "R"eviewer). We have a different code review threshold for 
> different types of changes. If there is disagreement as to whether a fix is 
> low-impact or high-impact, then it is considered high-impact. In other words 
> we will always err on the side of quality by "rounding up" to the next higher 
> category. The contributor can say whether they think something is low-impact 
> or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either 
> adds a comment indicating that they think a single review is sufficient, or 
> else issues the Skara `/reviewers 2` command requesting a second reviewer (a 
> Reviewer can request more than 2 reviewers in some cases where a fix might be 
> especially risky or cut across multiple functional areas).

"but **It** is" -> it

README-code-reviews.md line 48:

> 46: All code reviews must be done via a pull request submitted against this 
> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must 
> exist before the pull request will be reviewed. See 
> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
> request.
> 47: 
> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
> role (aka a "R"eviewer). We have a different code review threshold for 
> different types of changes. If there is disagreement as to whether a fix is 
> low-impact or high-impact, then it is considered high-impact. In other words 
> we will always err on the side of quality by "rounding up" to the next higher 
> category. The contributor can say whether they think something is low-impact 
> or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either 
> adds a comment indicating that they think a single review is sufficient, or 
> else issues the Skara `/reviewers 2` command requesting a second reviewer (a 
> Reviewer can request more than 2 reviewers in some cases where a fix might be 
> especially risky or cut across multiple functional areas).

About requesting reviews. I think that only some people can request reviews 
through GitHub, I never managed to do it on this repo, probably a matter of 
permissions. Might worth clarifying how this works.

README-code-reviews.md line 58:

> 56: * Determine whether this needs 2 reviewers and whether it needs a CSR; 
> issue the `/reviewers 2` or `/csr` command as needed
> 57: * If you want to indicate your approval, but still feel additional 
> reviewers are needed, you may increase the number of reviewers (e.g., from 2 
> to 3)
> 58: * If you want an area expert to review a PR, indicate this in a comment 
> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
> indicate whether or not they plan to review it

Should a list of experts per area be available somewhere? The Wiki has an old 
"code ownership" table that is out of date. Usually you get to know the experts 
only after they have reviewed y

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 18:20:22 GMT, Andy Goryachev  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 63:
> 
>> 61: Here is a list of things to keep in mind when reviewing a PR. This 
>> applies to anyone doing a review, but especially a "R"eviewer:
>> 62: 
>> 63: * Make sure you understand why there was an issue to begin with, and 
>> why/how the proposed PR solves the issue
> 
> Perhaps this should be noted as a required step for the PR author (maybe as a 
> template text in the PR description?).

I like the idea of adding some additional information on "here's what makes a 
good PR" to the CONTRIBUTING guidelines. I'll either take a stab at this or 
else file a follow-on issue.

> README-code-reviews.md line 64:
> 
>> 62: 
>> 63: * Make sure you understand why there was an issue to begin with, and 
>> why/how the proposed PR solves the issue
>> 64: * Focus first on substantive comments rather than stylistic comments
> 
> this is not as important as the next two.

Agreed. I'll move this bullet down.

> README-code-reviews.md line 66:
> 
>> 64: * Focus first on substantive comments rather than stylistic comments
>> 65: * Consider the risk of regression
>> 66: * Consider any compatibility concerns
> 
> regression and compatibility risks are probably the most important aspects of 
> the code review from the "R"eviewer's perspective.  Perhaps this should be 
> emphasized somehow?

That's a good idea.

> README-code-reviews.md line 83:
> 
>> 81:  A. Low-impact bug fixes.
>> 82: 
>> 83: These are typically isolated bug fixes with little or no impact beyond 
>> fixing the bug in question; included in this category are test fixes 
>> (including most new tests), doc fixes, and fixes to sample applications 
>> (including most new samples).
> 
> I think the main criteria is regression and compatibility impact.  Should we 
> expand upon that here?

That seems like a good idea.

> README-code-reviews.md line 99:
> 
>> 97: Feature requests come with a responsibility beyond just saying "here is 
>> the code for this cool new feature, please take it". There are many factors 
>> to consider for even small features. Larger features will need a significant 
>> contribution in terms of API design, coding, testing, maintainability, etc.
>> 98: 
>> 99: A feature should be discussed up-front on the openjfx-dev mailing list 
>> to get early feedback on the concept (is it a feature we are likely to 
>> accept) and the direction the API and/or implementation should take.
> 
> are there any special rules to gauging the consensus?  what happens when one 
> party in a discussion stops responding?

This is out of scope for this PR (this section is unchanged). It might be 
something to consider addressing in a follow-on enhancement, although I'm not 
sure I want to make it any more specific.

> README-code-reviews.md line 101:
> 
>> 99: A feature should be discussed up-front on the openjfx-dev mailing list 
>> to get early feedback on the concept (is it a feature we are likely to 
>> accept) and the direction the API and/or implementation should take.
>> 100: 
>> 101: To ensure that new features are consistent with the rest of the API and 
>> the desired direction of the Project, a CSR is required for a new Feature, 
>> API addition, or behavioral change. The CSR must be reviewed and approved by 
>> a "lead" of the Project. Currently this is either Kevin Ru

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

I replied to Andy's comments inline. Some of them are in unmodified sections 
and are out of scope for this, but I'll file a follow-on issue (P4).

I'll address the rest of the feedback in a subsequent commit.

-

PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2058843801


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 19:16:31 GMT, Nir Lisker  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 60:
> 
>> 58: * If you want an area expert to review a PR, indicate this in a comment 
>> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
>> indicate whether or not they plan to review it
>> 59: * If you want to ensure that you have the opportunity to review this PR 
>> yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this 
>> PR`, optionally add any concerns you might have
>> 60: 
> 
> I would add that it's a good idea to search for JBS issues similar/linked to 
> the one being targeted. There are many unfound duplicates in JBS that arise 
> only when searching, or at least closely linked ones (problems in 
> `TableTreeView` often have "pseudo-duplicates" for `TreeView` and 
> `TableView`, for example). 
> 
> I would explain (or link to where it is explained) when to close an issue a a 
> duplicated vs. when to use the /add Skara command to bundle issues into the 
> same fix.
> 
> We might also find that the targeted issue is a regression of another issue. 
> This is somewhat mentioned in the bullet point below "Consider the risk of 
> regression".

Second this, although I think the bulk of the work should fall on the author of 
the PR.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602168856


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 19:23:25 GMT, Nir Lisker  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 66:
> 
>> 64: * Focus first on substantive comments rather than stylistic comments
>> 65: * Consider the risk of regression
>> 66: * Consider any compatibility concerns
> 
> This might be included in this point already, but one thing I sometimes miss 
> is the inadvertent introduction of new API (that will have to be deprecated 
> if missed). This can happen with `protected` methods, default `public` 
> constructors (esp. if a non-API constructor is removed), or if a class is 
> moved from a non-exported to an exported package (something that you can't 
> see by looking at the area you're reviewing, you need to check the 
> `module-info`, or more practically, look at the package names).

I was wondering if there ought to be an unofficial checklist for things to look 
for?
As new people become "R"eviewers, I think there is a value in accumulating 
collective wisdom in a checklist.  I like checklists.

> README-code-reviews.md line 68:
> 
>> 66: * Consider any compatibility concerns
>> 67: * Check whether there is an automated test; if not, ask for one, if it 
>> is feasible
>> 68: * Make sure that the PR has executed the GHA tests and that they all 
>> pass; if they aren't being run, ask the PR author to enable GHA workflows
> 
> These tests sometimes fail and we integrate anyway. I noticed that sometimes 
> they need a few iterations to succeed. Are we really dependent on them?
> 
> Edit: currently the Linux test is failing, and this PR can't be the reason.

or is it?  :-)

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602171862
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602172232


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 18:56:29 GMT, Kevin Rushforth  wrote:

>> README-code-reviews.md line 10:
>> 
>>> 8: 
>>> 9: __Project Co-Lead__: Kevin Rushforth (kcr) 
>>> 10: __Project Co-Lead__: Johan Vos (jvos)
>> 
>> There are two sets of ids - one for OpenJFX/JBS and one for Github.  This 
>> might be confusing sometimes, should we list both?
>
> Not sure what you mean, but it doesn't seem related to this PR.

He means people reading reviews are usually on github and your github ID is 
kevinrusforth,
so they might be confused by "kcr" which is your openjdk id.

Since the context is identifying OpenJDK Project leads, I think the OpenJDK ID 
is the right and only one to include here. At most you could clarify  like 
(OpenJDK ID - kcr)

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602181574


[jfx21u] Integrated: 8330462: StringIndexOutOfBoundException when typing anything into TextField

2024-05-15 Thread Jose Pereda
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


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread John Hendrikx
On Wed, 15 May 2024 19:53:43 GMT, Andy Goryachev  wrote:

>> README-code-reviews.md line 68:
>> 
>>> 66: * Consider any compatibility concerns
>>> 67: * Check whether there is an automated test; if not, ask for one, if it 
>>> is feasible
>>> 68: * Make sure that the PR has executed the GHA tests and that they all 
>>> pass; if they aren't being run, ask the PR author to enable GHA workflows
>> 
>> These tests sometimes fail and we integrate anyway. I noticed that sometimes 
>> they need a few iterations to succeed. Are we really dependent on them?
>> 
>> Edit: currently the Linux test is failing, and this PR can't be the reason.
>
> or is it?  :-)

Isn't this automatic? Seems weird you could integrate without these passing.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602186713


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread John Hendrikx
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

Marked as reviewed by jhendrikx (Committer).

README-code-reviews.md line 79:

> 77: * All Reviewers who have requested the chance to review have done so (or 
> indicated that they are OK with it going in without their review). In rare 
> cases a Project Lead may override this.
> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business 
> day (24 hours), not including weekends or major holidays. This is to allow 
> sufficient time for those reviewers who might be in other time zones the 
> chance to review if they have concerns. This is measured from the time that 
> Skara has most recently added the "rfr" label (for example, for a PR that was 
> previously in Draft mode, wait for at least 24 hours after the PR has been 
> taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) 
> a Reviewer might give the OK to integrate without waiting for 24 hours.
> 79: * Verify the commit message. The Skara tooling adds a comment near the 
> top of the PR with the commit message that will be used. You can add a 
> summary to the commit message with the `/summary` command. You can add 
> additional contributors with the `/contributor` command. Commands are issued 
> by adding a comment to the PR that starts with a slash `/` character.

The last sentence feels a bit out of place, as several places earlier in this 
document already mention slash commands.

-

PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2058908894
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602186633


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

CONTRIBUTING.md line 233:

> 231: * Don't worry too much about import order. Try not to change it but 
> don't worry about fighting your IDE to stop it from doing so.
> 232: 
> 233: New code should be formatted consistently in accordance with the above 
> guidelines. However, please do not reformat existing code as part of a bug 
> fix. The makes more changes for code reviewers to track and review, and can 
> lead to merge conflicts. If you want to reformat a class, do that in a 
> separate pull request (which will need its own unique JBS bug ID).

"The makes more changes" ? I think you mean "This" not "The"

I'm not sure about the last sentence, it seems to encourage reformatting fixes 
which are just noise most of the time.

README-code-reviews.md line 14:

> 12: ### Reviewers
> 13: 
> 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on 
> the OpenJDK Census.

We use ".org" now, not ".java.net"

README-code-reviews.md line 40:

> 38: ### 1. The Reviewer role for the OpenJFX Project
> 39: 
> 40: We define a formal "Reviewer" role, similar to the JDK project. A 
> [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for 
> reviewing code changes and helping to determine whether a change is suitable 
> for including into OpenJFX. We expect Reviewers to feel responsible not just 
> for their piece, but for the quality of the JavaFX library as a whole. In 
> other words, the role of Reviewer is one of stewardship. See the following 
> section for what constitutes a good review.

(https://openjdk.java.net/census#openjfx)

.org please

BTW these very long source lines make it awkward to precisely identify the text 
I'm commenting on.

README-code-reviews.md line 77:

> 75: 
> 76: * All substantive feedback has been addressed, especially any objections 
> from one with a Reviewer role.
> 77: * All Reviewers who have requested the chance to review have done so (or 
> indicated that they are OK with it going in without their review). In rare 
> cases a Project Lead may override this.

One thing to add here (or hereabouts) is that if someone has commented on your 
review and requested changes that in almost all cases you should expect that 
they will want to return to review the results. So DO NOT push without letting 
earlier reviewers who made substantive comments re-review.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602185573
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602187001
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602188630
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602198401


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 19:00:57 GMT, Nir Lisker  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 48:
> 
>> 46: All code reviews must be done via a pull request submitted against this 
>> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID 
>> must exist before the pull request will be reviewed. See 
>> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
>> request.
>> 47: 
>> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
>> role (aka a "R"eviewer). We have a different code review threshold for 
>> different types of changes. If there is disagreement as to whether a fix is 
>> low-impact or high-impact, then it is considered high-impact. In other words 
>> we will always err on the side of quality by "rounding up" to the next 
>> higher category. The contributor can say whether they think something is 
>> low-impact or high-impact, but It is up to a Reviewer to confirm this. A 
>> Reviewer either adds a comment indicating that they think a single review is 
>> sufficient, or else issues the Skara `/reviewers 2` command requesting a 
>> second reviewer (a Reviewer can request more than 2 reviewers in some cases 
>> where a fix might be especially risky or cut across multiple functional 
>> areas).
> 
> "but **It** is" -> it

I think it worth noting that in skara syntax that isn't two people with the 
reviewer role.
And tell people what to use if that is what they intend - eg if I have it right

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602190743


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 19:32:04 GMT, Kevin Rushforth  wrote:

>> README-code-reviews.md line 103:
>> 
>>> 101: To ensure that new features are consistent with the rest of the API 
>>> and the desired direction of the Project, a CSR is required for a new 
>>> Feature, API addition, or behavioral change. The CSR must be reviewed and 
>>> approved by a "lead" of the Project. Currently this is either Kevin 
>>> Rushforth or Johan Vos as indicated above.
>>> 102: 
>>> 103: The review of the implementation follows the same "two reviewer" 
>>> standard for higher-impact changes as described in category B. The two code 
>>> reviewers for the implementation may or may not include the Lead who 
>>> reviewed the CSR. The review / approval of the CSR is an independent step 
>>> from the review / approval of the code change, although they can proceed in 
>>> parallel.
>> 
>> it is not clear (at least to me) the required order in respect to CSR 
>> approval.  for a new feature, does the CSR need to be approved before the 
>> work can be started, or the draft PR is published, or an PR enters its "rfr" 
>> stage, or just before the integration?
>> 
>> Or perhaps it should be clarified that the CSR is just a human-readable, 
>> specially formatted specification of the *change*, and it is an integral 
>> part of the new feature that needs to be approved before the feature can be 
>> integrated?
>
> This is out of scope for this PR, but it might be something to consider 
> addressing in a follow-on enhancement.

It should be made clear (somewhere, at some point) that it would be rare for a 
CSR to be completed and approved before starting work on the implementation 
because the spec. almost always evolves as the fix is developed.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602196255


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Nir Lisker
On Wed, 15 May 2024 20:08:10 GMT, John Hendrikx  wrote:

>> or is it?  :-)
>
> Isn't this automatic? Seems weird you could integrate without these passing.

> or is it? :-)

![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293)

Looks to me like it is...

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602201785


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 20:03:01 GMT, Phil Race  wrote:

>> Not sure what you mean, but it doesn't seem related to this PR.
>
> He means people reading reviews are usually on github and your github ID is 
> kevinrusforth,
> so they might be confused by "kcr" which is your openjdk id.
> 
> Since the context is identifying OpenJDK Project leads, I think the OpenJDK 
> ID is the right and only one to include here. At most you could clarify  like 
> (OpenJDK ID - kcr)

exactly my point.  when the majority of time is spent in the context of github 
(PR review etc) it might be confusing when one types `@jvos` and it's not 
found, or when within a PR there are references to both `jvos` and `johanvos`

(see the very first comment in https://github.com/openjdk/jfx/pull/1388 )

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602212035


Re: RFR: 8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout [v2]

2024-05-15 Thread Andy Goryachev
On Tue, 14 May 2024 22:29:18 GMT, Martin Fox  wrote:

>> On Linux getKeyCodeForChar does not consult the current keyboard layout. For 
>> example, it assumes the “+” character is on KeyCode.PLUS even on layouts 
>> which don’t generate KeyCode.PLUS. The result is that most 
>> KeyCharacterCombinations don’t work.
>> 
>> This PR fixes this using the same approach that Mac glass uses. When the 
>> user types a key we lookup all the characters that key might generate and 
>> put them in a map. getKeyCodeForChar then consults the map to find the Java 
>> key code. This ensures that we only pay attention to keys that are on the 
>> user’s physical keyboard.
>> 
>> (Some Linux layout maps are expansive and include entries for keys that may 
>> be on the user’s keyboard but probably aren’t, like “(“ and “)” on the 
>> numeric keypad. If we simply ask for all entries that generate a given 
>> character we can get multiple hits with no good way to determine which ones 
>> are physically present.)
>> 
>> This PR also contains fixes to the Robot to enable testing. When Glass 
>> consults the GdkKeymap to determine which keys might generate a keyval GDK 
>> returns a list covering every installed layout and shift level. The old 
>> Glass code simply picked the first entry in the list. This PR iterates 
>> through the list looking for one that works for the current layout and 
>> correct shift level.
>
> Martin Fox 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 ten additional commits since 
> the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into linuxcharcombo
>  - Comment fixes
>  - Consistency in naming conventions and comment cleanup.
>  - Merge remote-tracking branch 'upstream/master' into linuxcharcombo
>  - Expanded robot lookup table, general cleanup
>  - Merge remote-tracking branch 'upstream/master' into linuxcharcombo
>  - Resolving duplicates for Robot, fallback for getKeyCodeForChar
>  - Merge remote-tracking branch 'upstream/master' into linuxcharcombo
>  - KeyCharacterCombination fixes on Linux

In the context of the recent discussion on the mailing list, would it be 
possible to actually detect the keyboard layout?  There is a limited number of 
layouts, so in theory that should be possible.

Another question - I recall on Windows there is a way to enter certain key 
using Alt codes.  Does this method exist on Linux?

-

PR Comment: https://git.openjdk.org/jfx/pull/1373#issuecomment-2113454803


Re: Proposal: Public InputMap (v2)

2024-05-15 Thread John Hendrikx

I can only second this.

If anything, the proposal highlights several fundamental problems in 
current JavaFX by how it is trying to work around them. The work around 
works for one area (key events) but does not address similar problems 
for other events. The work around will also further cement these 
problems making them harder to address in the future.


I'll reiterate what I think are more fundamental issues that should be 
addressed:


1. Behaviors/Skins/Controls should not be hijacking events for their own 
purposes before the user has their say in their handling. If the user is 
not consuming the event (at any level), only then should it fall back to 
defaults. The reason why it currently works this way is because the 
public infrastructure for event handling is being used by Skin and 
Behavior implementations; this infrastructure however by its nature is a 
first come first serve system (unlike listeners), which makes it risky 
to share with users unless measures are taken to ensure there are no 
visible side effects of this sharing.  Effectively this also means Skins 
and Behaviors can *never* add a new event handler, or even a key mapping 
without risking breaking existing apps. Even consuming a new event in an 
existing handler, or changing its consumption pattern can cause breakage 
as it currently stands.


2. Separation of Behaviors and Skins. Even for complex controls this is 
possible, if there is a willingness to do so. This will open up far 
easier extension of the FX platform where the choice is not to either 
throw away all behavior and implement everything yourself, or hope you 
can sufficiently hack the existing skin (which were never designed for 
extension) to suit your needs.


3. Provide rock solid control primitives, and provide the infrastructure 
to build more complicated controls. I should be able to copy/paste any 
FX control, give it a new name, and then have it behave exactly as the 
original in all respects.  This may mean opening up (or more likely, 
evaluating and partially redesigning) certain private API's (like focus 
traversal).


I feel strongly that these would provide far more value currently, and, 
as these are all aspects that can't be addressed easily by 3rd parties 
or be worked around, only FX developers can solve these.


As far as the proposal is concerned, I feel that it is taking a giant 
leap towards a perceived solution for an X/Y type problem. I think we 
should be making far smaller incremental changes and fixes, and seeing 
what those bring.  This may eventually lead to an InputMap type system, 
but it could also lead to a place where the issue to provide it has 
become less urgent, giving us time to address other more fundamental 
problems.


--John

On 07/05/2024 10:44, Michael Strauß wrote:

Hi Andy!

The updated proposal seems to be a slight refinement of the original
proposal, and I think most of the points raised in the previous
discussion still stand.

As it is, I still think that this is an exceptionally large API
surface for what the feature can actually bring to the table. It's
overly specific in trying to solve problems for which general-purpose
APIs like event handlers can be extended to work just as well (and
open up many more use cases than just those of a bespoke control API).

The fact that the API tries to fix a problem with the event system
itself (unpredictable invocation order) is a red flag. We should fix
this problem where it originates, which is the event API; crafting
workaround APIs in other parts of JavaFX doesn't seem to be the right
approach to me. This would also help to break the problem down into
more manageable chunks: improve the event system first, then see how
that influences the remaining problems that need to be solved.

You provide several examples of what the InputMap API can do:


1. Adding a new key mapping

 // creates a new key binding mapped to an external function
 control.getInputMap().registerKey(KeyBinding.shortcut(KeyCode.W), () -> {
 externalFunction();
 });

I don't see why this is needed. It doesn't seem to be easier than
using a plain old event handler, it's just different.


2. Redefine an existing function

 // redefine function keeping existing key mappings
 getInputMap().registerFunction(Tag.COPY, (control) -> customCopy());

If I want to change the meaning of the copy() method, can I not just
override the method and provide my own implementation? Plain old Java
seems to do the job.


3. Map an existing function to a new binding

 // map a new key binding
 control.getInputMap().registerKey(KeyBinding.shortcut(KeyCode.W), 
Tag.COPY);

Again, an event handler would do the job here.


4. Obtain the default function

 //  obtains the default function handler
 FunctionHandler h = getInputMap().getDefaultFunction(Tag.COPY);

If I override the copy() method to provide my own implementation, I
could also add a defaultCopy() method that calls the base
implem

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

Thanks for the additional feedback. I'll update the PR in the next day or two. 
I'll double-check that I addressed all of the comments.

-

PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2059087200


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:07:06 GMT, Phil Race  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> CONTRIBUTING.md line 233:
> 
>> 231: * Don't worry too much about import order. Try not to change it but 
>> don't worry about fighting your IDE to stop it from doing so.
>> 232: 
>> 233: New code should be formatted consistently in accordance with the above 
>> guidelines. However, please do not reformat existing code as part of a bug 
>> fix. The makes more changes for code reviewers to track and review, and can 
>> lead to merge conflicts. If you want to reformat a class, do that in a 
>> separate pull request (which will need its own unique JBS bug ID).
> 
> "The makes more changes" ? I think you mean "This" not "The"
> 
> I'm not sure about the last sentence, it seems to encourage reformatting 
> fixes which are just noise most of the time.

Yeah, that was a typo (which I didn't notice when copying the block from the 
other doc). I'll fix it. And I agree with your concern, so I'll remove the last 
sentence.

> README-code-reviews.md line 14:
> 
>> 12: ### Reviewers
>> 13: 
>> 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on 
>> the OpenJDK Census.
> 
> We use ".org" now, not ".java.net"

Yes, I missed this. I'll update.

> README-code-reviews.md line 40:
> 
>> 38: ### 1. The Reviewer role for the OpenJFX Project
>> 39: 
>> 40: We define a formal "Reviewer" role, similar to the JDK project. A 
>> [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for 
>> reviewing code changes and helping to determine whether a change is suitable 
>> for including into OpenJFX. We expect Reviewers to feel responsible not just 
>> for their piece, but for the quality of the JavaFX library as a whole. In 
>> other words, the role of Reviewer is one of stewardship. See the following 
>> section for what constitutes a good review.
> 
> (https://openjdk.java.net/census#openjfx)
> 
> .org please
> 
> BTW these very long source lines make it awkward to precisely identify the 
> text I'm commenting on.

I'll fix it.

> README-code-reviews.md line 77:
> 
>> 75: 
>> 76: * All substantive feedback has been addressed, especially any objections 
>> from one with a Reviewer role.
>> 77: * All Reviewers who have requested the chance to review have done so (or 
>> indicated that they are OK with it going in without their review). In rare 
>> cases a Project Lead may override this.
> 
> One thing to add here (or hereabouts) is that if someone has commented on 
> your review and requested changes that in almost all cases you should expect 
> that they will want to return to review the results. So DO NOT push without 
> letting earlier reviewers who made substantive comments re-review.

I'll add something about this.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602296959
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602297934
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602298847
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602310947


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:33:37 GMT, Andy Goryachev  wrote:

>> He means people reading reviews are usually on github and your github ID is 
>> kevinrusforth,
>> so they might be confused by "kcr" which is your openjdk id.
>> 
>> Since the context is identifying OpenJDK Project leads, I think the OpenJDK 
>> ID is the right and only one to include here. At most you could clarify  
>> like (OpenJDK ID - kcr)
>
> exactly my point.  when the majority of time is spent in the context of 
> github (PR review etc) it might be confusing when one types `@jvos` and it's 
> not found, or when within a PR there are references to both `jvos` and 
> `johanvos`
> 
> (see the very first comment in https://github.com/openjdk/jfx/pull/1388 )

Ah, I misunderstood Andy's comment. Yes, I'll update this.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602297383


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 19:04:19 GMT, Nir Lisker  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 48:
> 
>> 46: All code reviews must be done via a pull request submitted against this 
>> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID 
>> must exist before the pull request will be reviewed. See 
>> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
>> request.
>> 47: 
>> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
>> role (aka a "R"eviewer). We have a different code review threshold for 
>> different types of changes. If there is disagreement as to whether a fix is 
>> low-impact or high-impact, then it is considered high-impact. In other words 
>> we will always err on the side of quality by "rounding up" to the next 
>> higher category. The contributor can say whether they think something is 
>> low-impact or high-impact, but It is up to a Reviewer to confirm this. A 
>> Reviewer either adds a comment indicating that they think a single review is 
>> sufficient, or else issues the Skara `/reviewers 2` command requesting a 
>> second reviewer (a Reviewer can request more than 2 reviewers in some cases 
>> where a fix might be especially risky or cut across multiple functional 
>> areas).
> 
> About requesting reviews. I think that only some people can request reviews 
> through GitHub, I never managed to do it on this repo, probably a matter of 
> permissions. Might worth clarifying how this works.

It's better to just add comments `@` mentioning who you want to do the review, 
since there isn't a way for most people to request a review from others.

> README-code-reviews.md line 58:
> 
>> 56: * Determine whether this needs 2 reviewers and whether it needs a CSR; 
>> issue the `/reviewers 2` or `/csr` command as needed
>> 57: * If you want to indicate your approval, but still feel additional 
>> reviewers are needed, you may increase the number of reviewers (e.g., from 2 
>> to 3)
>> 58: * If you want an area expert to review a PR, indicate this in a comment 
>> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
>> indicate whether or not they plan to review it
> 
> Should a list of experts per area be made available somewhere? The Wiki has 
> an old "code ownership" table that is out of date. Usually you get to know 
> the experts only after they have reviewed your code a couple of times.

That might be something to consider as a follow-on.

> README-code-reviews.md line 60:
> 
>> 58: * If you want an area expert to review a PR, indicate this in a comment 
>> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
>> indicate whether or not they plan to review it
>> 59: * If you want to ensure that you have the opportunity to review this PR 
>> yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this 
>> PR`, optionally add any concerns you might have
>> 60: 
> 
> Another thing to look out for is the targeting branch during rampdown. Should 
> they all target master and then be backported as needed, or can some target 
> the rampdown branch?

Good idea. I'll add the bit about checking the target branch -- if the PR isn't 
a backport it should almost always target the master branch (there might be 
_very_ rare exception where a bug is specific to the stabilization branch).

> README-code-rev

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:12:09 GMT, Phil Race  wrote:

>> README-code-reviews.md line 48:
>> 
>>> 46: All code reviews must be done via a pull request submitted against this 
>>> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID 
>>> must exist before the pull request will be reviewed. See 
>>> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
>>> request.
>>> 47: 
>>> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
>>> role (aka a "R"eviewer). We have a different code review threshold for 
>>> different types of changes. If there is disagreement as to whether a fix is 
>>> low-impact or high-impact, then it is considered high-impact. In other 
>>> words we will always err on the side of quality by "rounding up" to the 
>>> next higher category. The contributor can say whether they think something 
>>> is low-impact or high-impact, but It is up to a Reviewer to confirm this. A 
>>> Reviewer either adds a comment indicating that they think a single review 
>>> is sufficient, or else issues the Skara `/reviewers 2` command requesting a 
>>> second reviewer (a Reviewer can request more than 2 reviewers in some cases 
>>> where a fix might be especially risky or cut across multiple functional 
>>> areas).
>> 
>> "but **It** is" -> it
>
> I think it worth noting that in skara syntax that isn't two people with the 
> reviewer role.
> And tell people what to use if that is what they intend - eg if I have it 
> right

It's `/reviewers 2 reviewers`, so I'll add that as an example and clarify that 
`/reviewers 2` is 2 with at least one reviewer.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602299262


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 19:50:32 GMT, Andy Goryachev  wrote:

>> README-code-reviews.md line 60:
>> 
>>> 58: * If you want an area expert to review a PR, indicate this in a comment 
>>> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
>>> indicate whether or not they plan to review it
>>> 59: * If you want to ensure that you have the opportunity to review this PR 
>>> yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this 
>>> PR`, optionally add any concerns you might have
>>> 60: 
>> 
>> I would add that it's a good idea to search for JBS issues similar/linked to 
>> the one being targeted. There are many unfound duplicates in JBS that arise 
>> only when searching, or at least closely linked ones (problems in 
>> `TableTreeView` often have "pseudo-duplicates" for `TreeView` and 
>> `TableView`, for example). 
>> 
>> I would explain (or link to where it is explained) when to close an issue a 
>> a duplicated vs. when to use the /add Skara command to bundle issues into 
>> the same fix.
>> 
>> We might also find that the targeted issue is a regression of another issue. 
>> This is somewhat mentioned in the bullet point below "Consider the risk of 
>> regression".
>
> Second this, although I think the bulk of the work should fall on the author 
> of the PR.

This would probably be better in the CONTRIBUTING guidelines (for PR authors). 
Maybe as a follow-on, but I'll give it some thought.

>> README-code-reviews.md line 66:
>> 
>>> 64: * Focus first on substantive comments rather than stylistic comments
>>> 65: * Consider the risk of regression
>>> 66: * Consider any compatibility concerns
>> 
>> This might be included in this point already, but one thing I sometimes miss 
>> is the inadvertent introduction of new API (that will have to be deprecated 
>> if missed). This can happen with `protected` methods, default `public` 
>> constructors (esp. if a non-API constructor is removed), or if a class is 
>> moved from a non-exported to an exported package (something that you can't 
>> see by looking at the area you're reviewing, you need to check the 
>> `module-info`, or more practically, look at the package names).
>
> I was wondering if there ought to be an unofficial checklist for things to 
> look for?
> As new people become "R"eviewers, I think there is a value in accumulating 
> collective wisdom in a checklist.  I like checklists.

> ...inadvertent introduction of new API (that will have to be deprecated if 
> missed)

I think this is worth mentioning.

> I was wondering if there ought to be an unofficial checklist for things to 
> look for?

That's sort of what this section is meant to be. As long as it doesn't get too 
verbose we could add additional things to look for.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602302685
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602306295


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:23:37 GMT, Nir Lisker  wrote:

>> Isn't this automatic? Seems weird you could integrate without these passing.
>
>> or is it? :-)
> 
> ![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293)
> 
> Looks to me like it is...

A passing GHA test run is neither necessary nor sufficient. It is an 
interesting data point. A PR can be integrated with a failing run as long as we 
understand why it failed. It is something for a reviewer to check and ask about 
if they suspect it points to a real problem. I'll reword this a little bit to 
make it clear. I'm sort of glad that GitHub / Azure  chose today to break it. 
It is a great illustration of why we don't want it to be blocking.

Btw, I filed https://bugs.openjdk.org/browse/JDK-8332328 to track the GHA 
failure.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602309862


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:08:05 GMT, John Hendrikx  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> README-code-reviews.md line 79:
> 
>> 77: * All Reviewers who have requested the chance to review have done so (or 
>> indicated that they are OK with it going in without their review). In rare 
>> cases a Project Lead may override this.
>> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business 
>> day (24 hours), not including weekends or major holidays. This is to allow 
>> sufficient time for those reviewers who might be in other time zones the 
>> chance to review if they have concerns. This is measured from the time that 
>> Skara has most recently added the "rfr" label (for example, for a PR that 
>> was previously in Draft mode, wait for at least 24 hours after the PR has 
>> been taken out of Draft and marked "rfr"). In rare cases (e.g., a build 
>> breakage) a Reviewer might give the OK to integrate without waiting for 24 
>> hours.
>> 79: * Verify the commit message. The Skara tooling adds a comment near the 
>> top of the PR with the commit message that will be used. You can add a 
>> summary to the commit message with the `/summary` command. You can add 
>> additional contributors with the `/contributor` command. Commands are issued 
>> by adding a comment to the PR that starts with a slash `/` character.
> 
> The last sentence feels a bit out of place, as several places earlier in this 
> document already mention slash commands.

I'll remove it (I moved it from the CONTRIBUTING doc, and  it does seem out of 
place).

Actually as someone else commented, Skara will verify that the PR title and JBS 
bug match, so mostly this is about adding a summary and/or contributors if you 
want to.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602315921


Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:18:05 GMT, Phil Race  wrote:

>> This is out of scope for this PR, but it might be something to consider 
>> addressing in a follow-on enhancement.
>
> It should be made clear (somewhere, at some point) that it would be rare for 
> a CSR to be completed and approved before starting work on the implementation 
> because the spec. almost always evolves as the fix is developed.

Yes, that is a good point. I'll file a follow-up to do that (since this section 
is otherwise unmodified from the current guidelines on the Wiki).

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602318735


Re: Proposal: Public InputMap (v2)

2024-05-15 Thread Andy Goryachev
Dear John:

Thank you for a thoughtful response!

I am a bit surprised - the point #1 you are trying to make is explicitly 
supported by the new API.  The new API introduces a (fixed) number of 
priorities to the Controls' event handling specifically to address the issue 
you've raised earlier.  Yes, it is done at the controls' level rather than at 
the event dispatching level as it was proposed in 
https://github.com/openjdk/jfx/pull/1266 - for two reasons:

  1.  doing so at the event dispatching level is a major API change, maybe 
that's why #1266 went nowhere
  2.  the only place where fx has this problem is with Controls, since they 
have to deal with multiple actors, as I explained in my response to Michael's.

And the new API is not limited to key events, the priority scheme is extended 
to any event type.

#2. Separation of Behavior and Skins.  We went through this a number of times, 
so let me reiterate my points.  Behavior is tied to a particular skin; changing 
a skin in a drastic way or changing the behavior in a drastic way is possible, 
if a) there is a need and b) both Skin and its Behavior are made sufficiently 
public.  Alternatively, one must introduce an new API between Skin and its 
Behavior, which is possible for a particular skin, or a particular control, but 
I don't think is possible generally.  Different skins may require different 
control surfaces, different gestures, completely different event handling...  
In any case, this problem I feel is outside of the scope of my proposal.

#3. Rock solid primitives.  I am all for it!  Focus traversal - excellent idea! 
 Let's discuss maybe in a separate thread?  But as far as InputMap is 
concerned, it's a totally different area, out of scope for this proposal.

And finally, I disagree with you about incremental changes and fixes.  The 
value of the InputMap it brings to both application and skin developers cannot, 
I believe, be achieved by incremental steps.

Is there something that the proposed APIs that makes it impossible to move 
forward?

Thank you
-andy







From: openjfx-dev  on behalf of John Hendrikx 

Date: Wednesday, May 15, 2024 at 14:34
To: openjfx-dev@openjdk.org 
Subject: Re: Proposal: Public InputMap (v2)
I can only second this.

If anything, the proposal highlights several fundamental problems in
current JavaFX by how it is trying to work around them. The work around
works for one area (key events) but does not address similar problems
for other events. The work around will also further cement these
problems making them harder to address in the future.

I'll reiterate what I think are more fundamental issues that should be
addressed:

1. Behaviors/Skins/Controls should not be hijacking events for their own
purposes before the user has their say in their handling. If the user is
not consuming the event (at any level), only then should it fall back to
defaults. The reason why it currently works this way is because the
public infrastructure for event handling is being used by Skin and
Behavior implementations; this infrastructure however by its nature is a
first come first serve system (unlike listeners), which makes it risky
to share with users unless measures are taken to ensure there are no
visible side effects of this sharing.  Effectively this also means Skins
and Behaviors can *never* add a new event handler, or even a key mapping
without risking breaking existing apps. Even consuming a new event in an
existing handler, or changing its consumption pattern can cause breakage
as it currently stands.

2. Separation of Behaviors and Skins. Even for complex controls this is
possible, if there is a willingness to do so. This will open up far
easier extension of the FX platform where the choice is not to either
throw away all behavior and implement everything yourself, or hope you
can sufficiently hack the existing skin (which were never designed for
extension) to suit your needs.

3. Provide rock solid control primitives, and provide the infrastructure
to build more complicated controls. I should be able to copy/paste any
FX control, give it a new name, and then have it behave exactly as the
original in all respects.  This may mean opening up (or more likely,
evaluating and partially redesigning) certain private API's (like focus
traversal).

I feel strongly that these would provide far more value currently, and,
as these are all aspects that can't be addressed easily by 3rd parties
or be worked around, only FX developers can solve these.

As far as the proposal is concerned, I feel that it is taking a giant
leap towards a perceived solution for an X/Y type problem. I think we
should be making far smaller incremental changes and fixes, and seeing
what those bring.  This may eventually lead to an InputMap type system,
but it could also lead to a place where the issue to provide it has
become less urgent, giving us time to address other more fundamental
problems.

--John

On 07/05/2024 10:44, Michael Strauß wrote:
> Hi And

RFR: 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height

2024-05-15 Thread Andy Goryachev
The javadoc for `Region.getPrefHeight() / getPrefWidth()` incorrectly refers to 
`getPrefHeight(forWidth) / getPrefWidth(forHeight)`

should be

`prefHeight(forWidth) / prefWidth(forHeight)`

- also converted these references to `{@link}`s.

-

Commit messages:
 - 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height

Changes: https://git.openjdk.org/jfx/pull/1456/files
  Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1456&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332251
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jfx/pull/1456.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1456/head:pull/1456

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