Re: [jfx22u] RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
On Thu, 16 May 2024 22:23:40 GMT, Kevin Rushforth wrote: > Clean backport of Linux GHA fix to jfx22u (so we can get clean GHA runs for > future 22u backports) @johanvos Since this is a clean backport, it doesn't need to be reviewed, but it does need maintainer approval. It's not an urgent fix, but until it is integrated, all GHA runs in this repo will fail on Linux. - PR Comment: https://git.openjdk.org/jfx22u/pull/29#issuecomment-2116311432
[jfx22u] RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
Clean backport of Linux GHA fix to jfx22u (so we can get clean GHA runs for future 22u backports) - Commit messages: - Backport d7ab55184f757a614f9fc8f191c3c5794a16cc88 Changes: https://git.openjdk.org/jfx22u/pull/29/files Webrev: https://webrevs.openjdk.org/?repo=jfx22u&pr=29&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332328 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx22u/pull/29.diff Fetch: git fetch https://git.openjdk.org/jfx22u.git pull/29/head:pull/29 PR: https://git.openjdk.org/jfx22u/pull/29
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v6]
On Tue, 14 May 2024 13:37:32 GMT, Florian Kirmaier wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8323511: Adjust javadoc of the getViewportLength() > > Ok, I've updated the CSR accrodingly together with Eduard. @FlorianKirmaier or @eduardsdv can one of you merge in the latest upstream master? The source branch for this PR is a few months (and 134 commits) behind master. - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2116066312
Re: RFR: 8313138: Scrollbar Keyboard enhancement [v7]
On Mon, 6 May 2024 23:14:11 GMT, Andy Goryachev wrote: >> Adding alt-ctrl-LEFT/RIGHT/UP/DOWN (option-command-LEFT/RIGHT/UP/DOWN) key >> bindings to >> >> - ListView >> - TreeView >> - TableView >> - TreeTableView >> >> to support keyboard-only horizontal and vertical scrolling. The main reason >> for the change is to improve accessibility. >> >> **NOTE: For controls in right-to-left orientation, the direction of >> horizontal scrolling is reversed.** >> >> As far as I can tell, these key combinations do not interfere with editing. >> >> The proposed solution can be further optimized by adding a public method to >> the VirtualFlow class, something like >> >> >> public void horizontalUnitScroll(boolean right); >> public void verticalUnitScroll(boolean down); >> >> >> Q: Does this change require a CSR to explain the change in the controls' >> behavior? We don't yet have the key bindings documented in >> /doc-files/behavior >> >> Note: >> Jenkins headful test passed on all mac configurations, failed on all linux >> configurations (master branch failed also, so it is test issue), while >> windows configuration is not yet available. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > vertical scrolling tests As a follow-up enhancement, we might consider extending this to TextArea and/or ScrollPane. - PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2116057386
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v6]
On Thu, 9 May 2024 14:49:26 GMT, Florian Kirmaier wrote:
>> As seen in the unit test of the PR, when we click on the area above/below
>> the scrollbar the position jumps - but the jump is now not always consistent.
>> In the current version on the last cell - the UI always jumps to the top. In
>> the other cases, the assumed default cell height is used.
>>
>> With this PR, always the default cell height is used, to determine how much
>> is scrolled.
>> This makes the behavior more consistent.
>>
>> Especially from the unit-test, it's clear that with this PR the behavior is
>> much more consistent.
>>
>> This is also related to the following PR:
>> https://github.com/openjdk/jfx/pull/1194
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8323511: Adjust javadoc of the getViewportLength()
The API changes look good. I left one minor wording suggestion on the API docs.
The new behavior feels more consistent to me, so I have no concerns. I'll leave
it to @johanvos and @andy-goryachev-oracle to formally review it.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 1915:
> 1913:
> 1914: /**
> 1915: * The length of the viewport portion of the {@code VirtualFlow} as
> computed during the layout pass.
Suggested change:
"Returns the length of the viewport ..."
-
PR Review: https://git.openjdk.org/jfx/pull/1326#pullrequestreview-2061722236
PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1603939589
Re: KeyCode in the javafx.scene.robot.Robot interface
Hello! On 15/05/2024 14:49, Martin Fox wrote: > 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. Very possible. I use this base set of keycodes: https://github.com/io7m-com/xoanon/blob/ac7c9c900c7908c5760a74d6cf4056fe3ffb8e92/com.io7m.xoanon.commander/src/main/java/com/io7m/xoanon/commander/internal/XCCommander.java#L235 I iterate over each key in that set, sending that keycode to a text field. I then do the same for each keycode but with the shift key held. I don't doubt that it could cause havoc in some setups! > 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. Right. > JavaFX needs a better framework for testing text entry and I’ve thought about > how that might look... In my case, I'm almost always testing whether some UI elements become enabled or disabled based on validation occurring on a text field of some kind. A recent case was checking to see if a text field correctly rejected input that didn't appear to be an email address. Typing '@' into the field was difficult to make reliable. :) -- Mark Raynsford | https://www.io7m.com
RFR: 8324327: ColorPicker shows a white rectangle on clicking on picker
Root cause: premature 'return' from initialization code leaves the hover square visible, even when it should be hidden, as the case is when the color value is not present in the palette. - added unit tests - Commit messages: - 8324327: ColorPicker shows a white rectangle on clicking on picker Changes: https://git.openjdk.org/jfx/pull/1458/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1458&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324327 Stats: 64 lines in 2 files changed: 52 ins; 6 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1458.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1458/head:pull/1458 PR: https://git.openjdk.org/jfx/pull/1458
Integrated: 8332251: javadoc: incorrect method references in Region and PopupControl
On Wed, 15 May 2024 23:01:30 GMT, Andy Goryachev wrote: > The javadoc for `Region.getPrefHeight() / getPrefWidth()` incorrectly refers > to `getPrefHeight(forWidth) / getPrefWidth(forHeight)` > > should be > > `prefHeight(forWidth) / prefWidth(forHeight)` > > - same issue is also fixed in `PopupControl`. This pull request has now been integrated. Changeset: ebe36893 Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/ebe36893dd609b3d9373a2cc2b294b2eb6d94523 Stats: 37 lines in 2 files changed: 0 ins; 0 del; 37 mod 8332251: javadoc: incorrect method references in Region and PopupControl Reviewed-by: arapte - PR: https://git.openjdk.org/jfx/pull/1456
Re: RFR: 8332251: javadoc: incorrect method references in Region and PopupControl [v2]
On Thu, 16 May 2024 16:23:26 GMT, Ambarish Rapte wrote: >> thank you for pointing this out, I am going to revert back to `` and >> update `PopupControl`. > > Looks good, > I recommend to update the summary, current summary is quite specific to > Region / prefWidth/Height. > > A suggestion-> > javadoc: incorrect method references in Region and PopupControl excellent suggestion! done. - PR Review Comment: https://git.openjdk.org/jfx/pull/1456#discussion_r1603721091
Re: RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth wrote: > It looks like the list of packages available in the Ubuntu 22.04 GitHub > Actions test runner no longer includes `gcc-13` as of yesterday. I didn't > find any documentation to indicate that this was intentional, but now that > Ubuntu 24.04 is available we can fix the problem by updating to that version. > We will need to do this at some point anyway, so we might as well do it now. > > With this fix, the GHA test build now passes on all platforms. See the test > results to verify this. > > Given that GHA builds are currently broken on Linux, and that this fix > doesn't affect the product at all (just our GHA tests), I will integrate this > as soon as it is approved by 1 Reviewer, as an exception to the usual > requirement to wait for 24 hours. it's working for me - https://github.com/openjdk/jfx/pull/1374/checks?check_run_id=25062333595 Thank you for fixing the setup! - PR Comment: https://git.openjdk.org/jfx/pull/1457#issuecomment-2115745629
Re: RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth wrote: > It looks like the list of packages available in the Ubuntu 22.04 GitHub > Actions test runner no longer includes `gcc-13` as of yesterday. I didn't > find any documentation to indicate that this was intentional, but now that > Ubuntu 24.04 is available we can fix the problem by updating to that version. > We will need to do this at some point anyway, so we might as well do it now. > > With this fix, the GHA test build now passes on all platforms. See the test > results to verify this. > > Given that GHA builds are currently broken on Linux, and that this fix > doesn't affect the product at all (just our GHA tests), I will integrate this > as soon as it is approved by 1 Reviewer, as an exception to the usual > requirement to wait for 24 hours. Hopefully we won't see any instability as a result. If we do, then we might need to figure out how to get gcc-13 back on Ubuntu 22.04. We should keep an eye out for any odd Linux failures. - PR Comment: https://git.openjdk.org/jfx/pull/1457#issuecomment-2115734798
Re: KeyCode in the javafx.scene.robot.Robot interface
In another thread someone asked whether JavaFX could report which keyboard layout is currently active. I don’t think there’s any useful way of doing this. The Mac organizes layouts using language codes. There are five variants for German and 15 for English. The German variants are ABC-QWERTZ, Austrian, German, German-DIN-2137, and SwissGerman. The English variants are ABC, USExtended, ABC-India, Australian, British, British-PC, Canadian, Colemak, Dvorak, Dvorak-Left, DVORAK-QWERTYCMD, Dvorak-Right, Irish, US, and USInternational-PC. Linux organizes layouts using country codes. When the user searches for a layout they see display names based on language but under the hood it’s all organized by country. On my Ubuntu 22 system there are 10 variants for Great Britain, 25 for the United States, and 20 for Germany. On Windows the user configures some number of input languages and then adds keyboard layouts to each language. On my system I have one language (English) and inside it are four layouts (U.S., French, German, and Spanish). It’s easy to find the user’s current input language but there’s no documented way to retrieve information about the current layout. Folks have reverse-engineered this but I’ve resisted going down that rabbit hole (it involves digging around in the registry). Setting aside the more specialized layouts like Dvorak most of the variants within a given language or country differ in the way they handle dead keys or the position of punctuation and symbol characters. So the variants matter but can’t be reported in any consistent way across platforms. > On May 15, 2024, at 7:49 AM, Martin Fox wrote: > > 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 th
Re: RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth wrote: > It looks like the list of packages available in the Ubuntu 22.04 GitHub > Actions test runner no longer includes `gcc-13` as of yesterday. I didn't > find any documentation to indicate that this was intentional, but now that > Ubuntu 24.04 is available we can fix the problem by updating to that version. > We will need to do this at some point anyway, so we might as well do it now. > > With this fix, the GHA test build now passes on all platforms. See the test > results to verify this. > > Given that GHA builds are currently broken on Linux, and that this fix > doesn't affect the product at all (just our GHA tests), I will integrate this > as soon as it is approved by 1 Reviewer, as an exception to the usual > requirement to wait for 24 hours. Note that the Ubuntu 24 runner is in beta: https://github.com/actions/runner-images#available-images. - PR Comment: https://git.openjdk.org/jfx/pull/1457#issuecomment-2115686652
Re: RFR: 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height [v2]
On Thu, 16 May 2024 14:54:47 GMT, Andy Goryachev wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line
>> 1212:
>>
>>> 1210: *
>>> 1211: * Defaults to the USE_COMPUTED_SIZE flag, which
>>> means that
>>> 1212: * {@link #prefHeight(forWidth)} will return the region's
>>> internally
>>
>> 1. In this file, the documentation for other properties like minWidth,
>> minHeight use ``
>> For similarity I think we should keep `` or change others as well
>> to `link`.
>>
>> 2. There are similar correction needed at four other places in this file. As
>> we are touching this file, I think these can be corrected too. If modified
>> then the issue summary would need a modification too.
>> 1252: * getMaxWidth(forHeight) will return the region's
>> internally
>> 1256: * getMaxWidth(forHeight) to return the region's
>> preferred width,
>> 1281: * getMaxHeight(forWidth) will return the region's
>> internally
>> 1285: * getMaxHeight(forWidth) to return the region's
>> preferred height
>>
>> 3. Similar mistakes are observed in the PopupControl.java file too. I leave
>> it to you to correct those here or handle separately
>
> thank you for pointing this out, I am going to revert back to `` and
> update `PopupControl`.
Looks good,
I recommend to update the summary, current summary is quite specific to Region
/ prefWidth/Height.
A suggestion->
javadoc: incorrect method references in Region and PopupControl
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1456#discussion_r1603683364
Re: RFR: 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height [v2]
On Thu, 16 May 2024 14:58:29 GMT, Andy Goryachev wrote: >> The javadoc for `Region.getPrefHeight() / getPrefWidth()` incorrectly refers >> to `getPrefHeight(forWidth) / getPrefWidth(forHeight)` >> >> should be >> >> `prefHeight(forWidth) / prefWidth(forHeight)` >> >> - same issue is also fixed in `PopupControl`. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > popup control Marked as reviewed by arapte (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1456#pullrequestreview-2061307814
Integrated: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth wrote: > It looks like the list of packages available in the Ubuntu 22.04 GitHub > Actions test runner no longer includes `gcc-13` as of yesterday. I didn't > find any documentation to indicate that this was intentional, but now that > Ubuntu 24.04 is available we can fix the problem by updating to that version. > We will need to do this at some point anyway, so we might as well do it now. > > With this fix, the GHA test build now passes on all platforms. See the test > results to verify this. > > Given that GHA builds are currently broken on Linux, and that this fix > doesn't affect the product at all (just our GHA tests), I will integrate this > as soon as it is approved by 1 Reviewer, as an exception to the usual > requirement to wait for 24 hours. This pull request has now been integrated. Changeset: d7ab5518 Author:Kevin Rushforth URL: https://git.openjdk.org/jfx/commit/d7ab55184f757a614f9fc8f191c3c5794a16cc88 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13 Upgrade GHA Linux build platform to Ubuntu 24.04 Reviewed-by: angorya - PR: https://git.openjdk.org/jfx/pull/1457
Re: RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth wrote: > It looks like the list of packages available in the Ubuntu 22.04 GitHub > Actions test runner no longer includes `gcc-13` as of yesterday. I didn't > find any documentation to indicate that this was intentional, but now that > Ubuntu 24.04 is available we can fix the problem by updating to that version. > We will need to do this at some point anyway, so we might as well do it now. > > With this fix, the GHA test build now passes on all platforms. See the test > results to verify this. > > Given that GHA builds are currently broken on Linux, and that this fix > doesn't affect the product at all (just our GHA tests), I will integrate this > as soon as it is approved by 1 Reviewer, as an exception to the usual > requirement to wait for 24 hours. looks good - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1457#pullrequestreview-2061224330
RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13
It looks like the list of packages available in the Ubuntu 22.04 GitHub Actions test runner no longer includes `gcc-13` as of yesterday. I didn't find any documentation to indicate that this was intentional, but now that Ubuntu 24.04 is available we can fix the problem by updating to that version. We will need to do this at some point anyway, so we might as well do it now. With this fix, the GHA test build now passes on all platforms. See the test results to verify this. Given that GHA builds are currently broken on Linux, and that this fix doesn't affect the product at all (just our GHA tests), I will integrate this as soon as it is approved by 1 Reviewer, as an exception to the usual requirement to wait for 24 hours. - Commit messages: - 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13 Changes: https://git.openjdk.org/jfx/pull/1457/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1457&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332328 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1457.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1457/head:pull/1457 PR: https://git.openjdk.org/jfx/pull/1457
Re: RFR: 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height [v2]
> The javadoc for `Region.getPrefHeight() / getPrefWidth()` incorrectly refers > to `getPrefHeight(forWidth) / getPrefWidth(forHeight)` > > should be > > `prefHeight(forWidth) / prefWidth(forHeight)` > > - same issue is also fixed in `PopupControl`. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: popup control - Changes: - all: https://git.openjdk.org/jfx/pull/1456/files - new: https://git.openjdk.org/jfx/pull/1456/files/003e6cff..251fefb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1456&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1456&range=00-01 Stats: 36 lines in 2 files changed: 0 ins; 0 del; 36 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
Re: RFR: 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height [v2]
On Thu, 16 May 2024 10:01:05 GMT, Ambarish Rapte wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> popup control
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line
> 1212:
>
>> 1210: *
>> 1211: * Defaults to the USE_COMPUTED_SIZE flag, which
>> means that
>> 1212: * {@link #prefHeight(forWidth)} will return the region's
>> internally
>
> 1. In this file, the documentation for other properties like minWidth,
> minHeight use ``
> For similarity I think we should keep `` or change others as well
> to `link`.
>
> 2. There are similar correction needed at four other places in this file. As
> we are touching this file, I think these can be corrected too. If modified
> then the issue summary would need a modification too.
> 1252: * getMaxWidth(forHeight) will return the region's
> internally
> 1256: * getMaxWidth(forHeight) to return the region's
> preferred width,
> 1281: * getMaxHeight(forWidth) will return the region's
> internally
> 1285: * getMaxHeight(forWidth) to return the region's
> preferred height
>
> 3. Similar mistakes are observed in the PopupControl.java file too. I leave
> it to you to correct those here or handle separately
thank you for pointing this out, I am going to revert back to `` and
update `PopupControl`.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1456#discussion_r1603530347
Re: RFR: 8332251: javadoc: incorrect reference in Region.getPrefWidth/Height
On Wed, 15 May 2024 23:01:30 GMT, Andy Goryachev wrote:
> 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.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 1212:
> 1210: *
> 1211: * Defaults to the USE_COMPUTED_SIZE flag, which means
> that
> 1212: * {@link #prefHeight(forWidth)} will return the region's internally
1. In this file, the documentation for other properties like minWidth,
minHeight use ``
For similarity I think we should keep `` or change others as well to
`link`.
2. There are similar correction needed at four other places in this file. As we
are touching this file, I think these can be corrected too. If modified then
the issue summary would need a modification too.
1252: * getMaxWidth(forHeight) will return the region's
internally
1256: * getMaxWidth(forHeight) to return the region's
preferred width,
1281: * getMaxHeight(forWidth) will return the region's
internally
1285: * getMaxHeight(forWidth) to return the region's
preferred height
3. Similar mistakes are observed in the PopupControl.java file too. I leave it
to you to correct those here or handle separately
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1456#discussion_r1603035540
Re: RFR: 8332313: Update code review guidelines
On Thu, 16 May 2024 07:30:31 GMT, Johan Vos wrote:
>> 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.
>
> I agree with the concern, but I still think it's much better to encourage
> developers to do formatting in a separate issue (or not at all) with all the
> required administration, than to sneak in a formatting change in a PR that
> has nothing to do with formatting. I prefer the noise to be completely
> isolated, so that it can be ignored easily, rather than being distracted by
> it in a PR.
I think this conflicts a bit with the previous statement ("don't worry too much
about import order"). In my experience, when merging complex changes, import
order changes are often the cause of conflicts. We also found that with many
different committer preferences, the whole import block could change from one
commit to the next, which will lead to a conflict if such a file is part of a
backport.
We solved this by simply mandating a single order for imports (alphabetical, no
gaps, no star imports) and made the build fail when it was violated. The
number of conflicts went down dramatically. This does require a one time
import fix of the code base, which will also be a source of conflicts, but at
least it will be in the usual area (imports) and should solve the problem once
and for all.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602895839
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 21:57:23 GMT, Kevin Rushforth wrote: >> 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. I agree with the concern, but I still think it's much better to encourage developers to do formatting in a separate issue (or not at all) with all the required administration, than to sneak in a formatting change in a PR that has nothing to do with formatting. I prefer the noise to be completely isolated, so that it can be ignored easily, rather than being distracted by it in a PR. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602767101
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:39:10 GMT, Kevin Rushforth wrote: >> 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. In the ideal world where we have tons of regression and compatibility tests, I would agree. Unfortunately, we are totally not there yet. Compared to other projects, the quality of tests in OpenJFX is good, but given the multitude of usage scenario's in client development, we would need much, much more tests before we can rely on them to detect regression. Therefore, the #1 point imho is that both the author (as Andy commented) and the reviewer have a real good understanding on what is happening. Every single change in a PR should be understood. The most dangerous things are "I don't understand why, but it seems to fail before and succeed after the patch". - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602762164
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen
On Mon, 6 May 2024 21:11:35 GMT, Martin Fox wrote: > I'll look into the Linux failure. The core EventLoop code passes an object > into the application's leaveNestedEventLoop and expects to see that object > returned from the applications's matching enterNestedEventLoop call. On Mac > and Windows this object is passed through to glass as an argument on the > stack. On Linux this value is handled as an application global. I suspect the > Linux bookkeeping isn't robust enough to handle this case but it will take a > bit to nail down the details. There is one additional change I made, which might be relevant for Linux: https://github.com/openjdk/jfx/pull/1324/files#diff-af779aafb50953f57cab2478dd220d0322592b60e92065cf658644866572b7e7R117 Worth to check. I remember that the code there was problematic to me. Otherwise this looks good. https://github.com/openjdk/jfx/pull/1324 also cleans up the unused return value which is allocated in the C++/Objective-C side, but never really used on the Java side. So might be worth to do at some point, but I would agree to do the minimal changes first. - PR Comment: https://git.openjdk.org/jfx/pull/1449#issuecomment-2114266915
