Re: RFR: 8331748: Update libxml2 to 2.12.6
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
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]
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
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
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
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]
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]
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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? :-)  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
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]
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)
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
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
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
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
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
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
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
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? :-) > >  > > 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
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
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)
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
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
