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 can confirm on my system that this PR fixes the issues that I am seeing and some users reported. Here is a before and after comparison for context menus and tooltips:     The tests also pass for me (didn't build webkit though) - PR Comment: https://git.openjdk.org/jfx/pull/1437#issuecomment-2097459748
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes [v2]
On Mon, 6 May 2024 21:46:29 GMT, Andy Goryachev wrote:
>> drmarmac has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> javaDoc updates according to code review
>
> modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
> line 417:
>
>> 415: * integer values.
>> 416: *
>> 417: * If {@link #wrapAroundProperty wrapAround} is {@code true}, the
>> {@code IntegerSpinnerValueFactory} will step from
>
> wrapAround still does not work in Eclipse, but this does, while providing the
> same visuals:
>
>
> {@link SpinnerValueFactory#wrapAroundProperty wrapAround}
>
>
> does it work in intellij?
That one should work everywhere, since it is equivalent. Worth double-checking,
though.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591647816
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes [v2]
On Mon, 6 May 2024 21:35:40 GMT, drmarmac wrote:
>> `{@link #wrapAroundProperty wrapAround}` does not resolve **in Eclipse** (it
>> does in javadoc output).
>>
>> This might be Eclipse limitation, but it would be awfully nice to have it
>> resolved in Eclipse.
>
> I also fixed the min/max links to refer to minProperty/maxProperty. IntelliJ
> IDEA has the same problem as eclipse. I guess this has to do with the
> "javafx" option to the javadoc command (build.gradle:4286
> `options.addBooleanOption("javafx").setValue(true);`) that seems to allow to
> use the private fields' docs for the properties, by naming convention? Maybe
> we need to file issues in Eclipse/IntelliJ for this so they add the option to
> their javadoc renderer.
There are two issues here. The first is that you cannot just link to a `xxx`
property by name -- you have to link to the `xxxProperty` method. The former
doesn't work in the generated javadocs either (so it's not just an IDE
problem). The second issue seems specific to Eclipse, in that if you don't
prefix the method name with the name of the enclosing (outer) class, Eclipse
doesn't generate the link. That seems like an Eclipse bug to me, but as Andy
points out, it's a trivial thing to add the class name (and won't affect the
output).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591647400
Re: RFR: 8313138: Scrollbar Keyboard enhancement [v7]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/1393/files - new: https://git.openjdk.org/jfx/pull/1393/files/859f7bbd..eeaa8d91 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1393=06 - incr: https://webrevs.openjdk.org/?repo=jfx=1393=05-06 Stats: 156 lines in 4 files changed: 146 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jfx/pull/1393.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1393/head:pull/1393 PR: https://git.openjdk.org/jfx/pull/1393
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v6]
> Adding alt-ctrl-LEFT/RIGHT (option-command-LEFT/RIGHT) key bindings to > > - ListView > - TreeView > - TableView > - TreeTableView > > to support keyboard-only horizontal scrolling. The main reason for the > change is to improve accessibility. > > **NOTE: For controls in right-to-left orientation, the direction 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); > > > 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 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 16 additional commits since the last revision: - added vertical scrolling - Merge branch 'master' into 8313138.horizontal - tests - cleanup - node orientation - Merge remote-tracking branch 'origin/master' into 8313138.horizontal - table view behavior - tree view behavior - list view behavior - orientation - ... and 6 more: https://git.openjdk.org/jfx/compare/969d0b8e...859f7bbd - Changes: - all: https://git.openjdk.org/jfx/pull/1393/files - new: https://git.openjdk.org/jfx/pull/1393/files/5bae5e7a..859f7bbd Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1393=05 - incr: https://webrevs.openjdk.org/?repo=jfx=1393=04-05 Stats: 5171 lines in 211 files changed: 4184 ins; 503 del; 484 mod Patch: https://git.openjdk.org/jfx/pull/1393.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1393/head:pull/1393 PR: https://git.openjdk.org/jfx/pull/1393
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes [v2]
On Mon, 6 May 2024 21:32:07 GMT, drmarmac wrote:
>> This PR updates the javadoc for the SpinnerFactory wrap-around behavior
>> introduced in #1431.
>
> drmarmac has updated the pull request incrementally with one additional
> commit since the last revision:
>
> javaDoc updates according to code review
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 417:
> 415: * integer values.
> 416: *
> 417: * If {@link #wrapAroundProperty wrapAround} is {@code true}, the
> {@code IntegerSpinnerValueFactory} will step from
wrapAround still does not work in Eclipse, but this does, while providing the
same visuals:
{@link SpinnerValueFactory#wrapAroundProperty wrapAround}
does it work in intellij?
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 620:
> 618: * double values.
> 619: *
> 620: * If {@link #wrapAroundProperty wrapAround} is {@code true}, the
> {@code DoubleSpinnerValueFactory} will step
same here
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591590066
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591590894
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes [v2]
On Mon, 6 May 2024 17:59:14 GMT, Andy Goryachev wrote:
>> I don't think this will help. This is the same issue I mentioned in PR #1389
>> -- you need to link to the property _method_, so I think changing to the
>> following will do it:
>>
>>
>> * If {@link #wrapAroundProperty wrapAround}
>>
>>
>> (it shouldn't be necessary to qualify it with the enclosing class, but it
>> wouldn't hurt to do so)
>
> `{@link #wrapAroundProperty wrapAround}` does not resolve **in Eclipse** (it
> does in javadoc output).
>
> This might be Eclipse limitation, but it would be awfully nice to have it
> resolved in Eclipse.
I also fixed the min/max links to refer to minProperty/maxProperty. IntelliJ
IDEA has the same problem as eclipse. I guess this has to do with the "javafx"
option to the javadoc command (build.gradle:4286
`options.addBooleanOption("javafx").setValue(true);`) that seems to allow to
use the private fields' docs for the properties, by naming convention? Maybe we
need to file issues in Eclipse/IntelliJ for this so they add the option to
their javadoc renderer.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591581530
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes [v2]
On Mon, 6 May 2024 16:27:52 GMT, Andy Goryachev wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
>> line 420:
>>
>>> 418: * the minimum value to the maximum value (and vice versa). As a
>>> consequence of that, the number
>>> 419: * of steps required to wrap around to the same value is N+1,
>>> where N is the number of steps between
>>> 420: * {@link #min} (inclusive) and {@link #max} (inclusive).
>>
>> Suggestion: It might be helpful to show the equation:
>>
>>
>> val = (val + delta) % (max - min + 1)
>
> a good idea
agreed, I used "amountToStepBy" instead of "delta" here.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591576774
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes [v2]
> This PR updates the javadoc for the SpinnerFactory wrap-around behavior > introduced in #1431. drmarmac has updated the pull request incrementally with one additional commit since the last revision: javaDoc updates according to code review - Changes: - all: https://git.openjdk.org/jfx/pull/1450/files - new: https://git.openjdk.org/jfx/pull/1450/files/c491f8b8..0ecdfae1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1450=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1450=00-01 Stats: 10 lines in 1 file changed: 2 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1450.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1450/head:pull/1450 PR: https://git.openjdk.org/jfx/pull/1450
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen
On Sat, 4 May 2024 22:55:13 GMT, Martin Fox wrote: > This PR is based on a discussion that happened over in PR #1324. Some of this > explanation is copied from that thread. > > When `exitNestedEventLoop` is called on the innermost loop the > invokeLaterDispatcher suspends operation until the loop finishes. But if you > immediately start a new event loop the previous one won't finish and the > dispatcher will jam up and stop dispatching indefinitely. > > When the invokeLaterDispatcher is told that the innermost loop is exiting it > sets `leavingNestedEventLoop` to true expecting it to be set to false when > the loop actually exits. When the dispatcher is told that a new event loop > has started it is not clearing `leavingNestedEventLoop` which is causing the > jam. Basically it should follow the same logic used in glass; leaving the > innermost loop updates a boolean indicating that the loop should exit but if > a new loop is started the boolean is set back to a running state since it now > applies to the new loop, not the previous one. > > I suspect the invokeLaterDispatcher exists in part to deal with the specifics > of how deferred runnables are handled on the Mac. I investigated this a bit > and wrote up some comments in the Mac glass code. 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. - PR Comment: https://git.openjdk.org/jfx/pull/1449#issuecomment-2096926552
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 6 May 2024 18:57:57 GMT, eduardsdv wrote: >>> What about the `GridView` >> >> What's a GridView? ControlsFX is not a part of OpenJFX... > >> > What about the `GridView` >> >> What's a GridView? ControlsFX is not a part of OpenJFX... > > Oh, sorry, my mistake. You are right. It is part of the ControlsFX. As @eduardsdv mentioned in https://github.com/openjdk/jfx/pull/1326#issuecomment-209336 and @kevinrushforth asked about earlier, perhaps we should add similar capability for vertical scrolling using alt-ctrl-Up/DOWN (option-command-UP/DOWN) in the same PR rather than separately. - PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2096841533
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 1 Apr 2024 19:27:17 GMT, Andy Goryachev wrote: >> Adding alt-ctrl-LEFT/RIGHT (option-command-LEFT/RIGHT) key bindings to >> >> - ListView >> - TreeView >> - TableView >> - TreeTableView >> >> to support keyboard-only horizontal scrolling. The main reason for the >> change is to improve accessibility. >> >> **NOTE: For controls in right-to-left orientation, the direction 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); >> >> >> 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - tests > - cleanup > - node orientation > - Merge remote-tracking branch 'origin/master' into 8313138.horizontal > - table view behavior > - tree view behavior > - list view behavior > - orientation > - Merge remote-tracking branch 'origin/master' into 8313138.horizontal > - Merge branch 'master' into 8313138.horizontal > - ... and 4 more: https://git.openjdk.org/jfx/compare/3ef18774...5bae5e7a ... but nothing should prevent the maintainers of ControlsFx from adding this functionality to the GridView. - PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2096838107
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 6 May 2024 18:46:33 GMT, eduardsdv wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 14 additional >> commits since the last revision: >> >> - tests >> - cleanup >> - node orientation >> - Merge remote-tracking branch 'origin/master' into 8313138.horizontal >> - table view behavior >> - tree view behavior >> - list view behavior >> - orientation >> - Merge remote-tracking branch 'origin/master' into 8313138.horizontal >> - Merge branch 'master' into 8313138.horizontal >> - ... and 4 more: https://git.openjdk.org/jfx/compare/3483d28c...5bae5e7a > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java > line 686: > >> 684: right = !right; >> 685: } >> 686: ScrollBar sb = flow.getHbar(); > > The ListView has the orientation property > ``ListView.setOrientation(Orientation)``. > I think it should be > > ScrollBar sb = flow.isVertical() ? flow.getHbar() : flow.getVbar(); > > and the name of the method something like ``crossUnitScroll(boolean)``. I am afraid this suggestion is incorrect: the new key bindings affect horizontal scroll bar regardless of the orientation. The new behavior can be checked with the Monkey Tester https://github.com/andy-goryachev-oracle/MonkeyTest - PR Review Comment: https://git.openjdk.org/jfx/pull/1393#discussion_r1591492253
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 6 May 2024 18:52:10 GMT, Andy Goryachev wrote: > > What about the `GridView` > > What's a GridView? ControlsFX is not a part of OpenJFX... Oh, sorry, my mistake. You are right. It is part of the ControlsFX. - PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2096703376
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 6 May 2024 18:49:04 GMT, eduardsdv wrote: > What about the `GridView` What's a GridView? ControlsFX is not a part of OpenJFX... - PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2096694208
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 1 Apr 2024 19:27:17 GMT, Andy Goryachev wrote: >> Adding alt-ctrl-LEFT/RIGHT (option-command-LEFT/RIGHT) key bindings to >> >> - ListView >> - TreeView >> - TableView >> - TreeTableView >> >> to support keyboard-only horizontal scrolling. The main reason for the >> change is to improve accessibility. >> >> **NOTE: For controls in right-to-left orientation, the direction 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); >> >> >> 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - tests > - cleanup > - node orientation > - Merge remote-tracking branch 'origin/master' into 8313138.horizontal > - table view behavior > - tree view behavior > - list view behavior > - orientation > - Merge remote-tracking branch 'origin/master' into 8313138.horizontal > - Merge branch 'master' into 8313138.horizontal > - ... and 4 more: https://git.openjdk.org/jfx/compare/55618355...5bae5e7a What about the ``GridView``? It also uses the ``VirtualFlow``. Should it also support this kind of scrolling? - PR Comment: https://git.openjdk.org/jfx/pull/1393#issuecomment-2096689208
Re: RFR: 8313138: Horizontal Scrollbar Keyboard enhancement [v5]
On Mon, 1 Apr 2024 19:27:17 GMT, Andy Goryachev wrote: >> Adding alt-ctrl-LEFT/RIGHT (option-command-LEFT/RIGHT) key bindings to >> >> - ListView >> - TreeView >> - TableView >> - TreeTableView >> >> to support keyboard-only horizontal scrolling. The main reason for the >> change is to improve accessibility. >> >> **NOTE: For controls in right-to-left orientation, the direction 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); >> >> >> 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - tests > - cleanup > - node orientation > - Merge remote-tracking branch 'origin/master' into 8313138.horizontal > - table view behavior > - tree view behavior > - list view behavior > - orientation > - Merge remote-tracking branch 'origin/master' into 8313138.horizontal > - Merge branch 'master' into 8313138.horizontal > - ... and 4 more: https://git.openjdk.org/jfx/compare/07a85ee2...5bae5e7a modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java line 686: > 684: right = !right; > 685: } > 686: ScrollBar sb = flow.getHbar(); The ListView has the orientation property ``ListView.setOrientation(Orientation)``. I think it should be ScrollBar sb = flow.isVertical() ? flow.getHbar() : flow.getVbar(); and the name of the method something like ``crossUnitScroll(boolean)``. - PR Review Comment: https://git.openjdk.org/jfx/pull/1393#discussion_r1591418338
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 08:49:27 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: Fix scrolling when clicking the scrollbar track when only one > cell is visible It would be nice if it possible to scroll with the keyboard without moving the selection/focus. - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-209336
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen
On Sat, 4 May 2024 22:55:13 GMT, Martin Fox wrote: > This PR is based on a discussion that happened over in PR #1324. Some of this > explanation is copied from that thread. > > When `exitNestedEventLoop` is called on the innermost loop the > invokeLaterDispatcher suspends operation until the loop finishes. But if you > immediately start a new event loop the previous one won't finish and the > dispatcher will jam up and stop dispatching indefinitely. > > When the invokeLaterDispatcher is told that the innermost loop is exiting it > sets `leavingNestedEventLoop` to true expecting it to be set to false when > the loop actually exits. When the dispatcher is told that a new event loop > has started it is not clearing `leavingNestedEventLoop` which is causing the > jam. Basically it should follow the same logic used in glass; leaving the > innermost loop updates a boolean indicating that the loop should exit but if > a new loop is started the boolean is set back to a running state since it now > applies to the new loop, not the previous one. > > I suspect the invokeLaterDispatcher exists in part to deal with the specifics > of how deferred runnables are handled on the Mac. I investigated this a bit > and wrote up some comments in the Mac glass code. I get the following internal assertion failure when running the test on Linux: NestedEventLoopTest > testCanExitAndThenEnterNewLoop FAILED java.lang.AssertionError: Internal inconsistency - wrong EventLoop at javafx.graphics@23-ea/com.sun.glass.ui.EventLoop.enter(EventLoop.java:108) at javafx.graphics@23-ea/com.sun.javafx.tk.quantum.QuantumToolkit.enterNestedEventLoop(QuantumToolkit.java:656) at javafx.graphics@23-ea/javafx.application.Platform.enterNestedEventLoop(Platform.java:310) at test.javafx.stage.NestedEventLoopTest.lambda$testCanExitAndThenEnterNewLoop$29(NestedEventLoopTest.java:326 It ran cleanly on Mac and Windows. - PR Comment: https://git.openjdk.org/jfx/pull/1449#issuecomment-2096641134
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 17:54:44 GMT, eduardsdv wrote: > PageUp/PageDown not only scrolls the view, but also moves the > selection/focus, so it makes sense to align the cells up/down. You are right about focus/selection! This brings up another question: for accessibility reasons, do we want to add key bindings for the vertical scrolling in addition to the horizontal ones in #1393 ? For example, when the cell height exceeds the viewport height, there should be a way to scroll the content vertically using unit increment via keyboard only. What do you think? - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2096617895
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes
On Mon, 6 May 2024 17:39:33 GMT, Kevin Rushforth wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
>> line 417:
>>
>>> 415: * integer values.
>>> 416: *
>>> 417: * If {@link #wrapAround} is {@code true}, the {@code
>>> IntegerSpinnerValueFactory} will step from
>>
>> The link for wrapAround does not resolve in Eclipse.
>> I'd suggest to change it to
>>
>> * If {@link SpinnerValueFactory#wrapAround wrapAround}
>>
>>
>> Same comment applies to the min and max in line 420.
>
> I don't think this will help. This is the same issue I mentioned in PR #1389
> -- you need to link to the property _method_, so I think changing to the
> following will do it:
>
>
> * If {@link #wrapAroundProperty wrapAround}
>
>
> (it shouldn't be necessary to qualify it with the enclosing class, but it
> wouldn't hurt to do so)
`{@link #wrapAroundProperty wrapAround}` does not resolve **in Eclipse** (it
does in javadoc output).
This might be Eclipse limitation, but it would be awfully nice to have it
resolved in Eclipse.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591370127
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 08:49:27 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: Fix scrolling when clicking the scrollbar track when only one > cell is visible I agree with you, at least with scrolling when clicking on the scrollbar track. I would like to change the amount of scroll pixels to the viewport length. PageUp/PageDown not only scrolls the view, but also moves the selection/focus, so it makes sense to align the cells up/down. - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2096601060
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes
On Mon, 6 May 2024 16:26:42 GMT, Andy Goryachev wrote:
>> This PR updates the javadoc for the SpinnerFactory wrap-around behavior
>> introduced in #1431.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
> line 417:
>
>> 415: * integer values.
>> 416: *
>> 417: * If {@link #wrapAround} is {@code true}, the {@code
>> IntegerSpinnerValueFactory} will step from
>
> The link for wrapAround does not resolve in Eclipse.
> I'd suggest to change it to
>
> * If {@link SpinnerValueFactory#wrapAround wrapAround}
>
>
> Same comment applies to the min and max in line 420.
I don't think this will help. This is the same issue I mentioned in PR #1389 --
you need to link to the property _method_, so I think changing to the following
will do it:
* If {@link #wrapAroundProperty wrapAround}
(it shouldn't be necessary to qualify it with the enclosing class, but it
wouldn't hurt to do so)
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591345778
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 08:49:27 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: Fix scrolling when clicking the scrollbar track when only one > cell is visible I might have raised this question before. In my opinion, the scrolling in these virtualized views should be more or less equivalent to the scrolling in the ScrollPane. In other words, using thumb track to page down and then page up (or using PgUp/PgDn keys) should get the control to its original state, unless it hit the rail. This is how it's currently works in MS Word (which isn't exactly a pinnacle of usability, but something a lot of people across the globe use routinely). See here using Design -> Paragraph Spacing popup, notice how the initial state is not aligned exactly at the top of the cell, clicking on the track below the thumb to page down, then above the thumb to page up, we arrive at the same exact position:    diff 1 and 3:  - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2096570758
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 16:35:14 GMT, Andy Goryachev wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8323511: Fix scrolling when clicking the scrollbar track when only one
>> cell is visible
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 1810:
>
>> 1808: public double getBlockIncrement() {
>> 1809: // For reasons of backward compatibility, we use the cell
>> length of the empty cell (cell index -1)
>> 1810: return getCellLength(-1);
>
> How would that work when cell heights are different for each cell?
>
> What if the cell height is much larger than the viewport height (as in, 10
> times larger?)
I don't like that either, but as I said I didn't want to change it because of
backwards compatibility.
I would like to make it configurable or even make the complete scrolling
behavior interchangeable.
Because, the actual scrolling behavior is still not consistent.
If multiple cells are visible, the cells are currently aligned (when clicking
on the scrollbar) so that they are either exactly at the top or exactly at the
bottom. If only one cell is visible, the scrolling behavior changes to the
applying of pixel delta.
For example, if you have two large cells and a small ViewPort so that only one
cell is visible.
1) If you click on the thumb below the scroll bar, the ListView is scrolled by
a pixel delta so that two cells can become visible.
2) If you click below scroll thumb again, the ListView scrolls so that the
second cell appears at the top.
3) If you click above the scroll thumb, the ListView scrolls by pixel delta, as
the second cell is also larger than the viewport.
Now the ListView is in a different state than before the click (2).
These two behaviors are each useful for their own situation and should not be
mixed.
1) Align the cell at the top or bottom if you have cells of the same size that
are smaller than the viewport.
2) Scroll by pixel delta if you have cells of different sizes or if the cells
are larger than the viewport.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1591327052
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 16:34:22 GMT, Andy Goryachev wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8323511: Fix scrolling when clicking the scrollbar track when only one >> cell is visible > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 1804: > >> 1802: >> 1803: /** >> 1804: * The amount of pixels by which to adjust the position when the >> the scroll bar track is clicked. > > should it be the height of the viewport, so to speak? Yes, that would also be my favorite choice for the click on the scrollbar track as well as for the PageUp/PageDown. But for reasons of backwards compatibility, I didn't want to change it here. - PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1591295131
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 08:49:27 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: Fix scrolling when clicking the scrollbar track when only one
> cell is visible
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 1804:
> 1802:
> 1803: /**
> 1804: * The amount of pixels by which to adjust the position when the
> the scroll bar track is clicked.
should it be the height of the viewport, so to speak?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 1810:
> 1808: public double getBlockIncrement() {
> 1809: // For reasons of backward compatibility, we use the cell
> length of the empty cell (cell index -1)
> 1810: return getCellLength(-1);
How would that work when cell heights are different for each cell?
What if the cell height is much larger than the viewport height (as in, 10
times larger?)
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1591273143
PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1591274020
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes
On Mon, 6 May 2024 08:24:52 GMT, drmarmac wrote:
> This PR updates the javadoc for the SpinnerFactory wrap-around behavior
> introduced in #1431.
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 417:
> 415: * integer values.
> 416: *
> 417: * If {@link #wrapAround} is {@code true}, the {@code
> IntegerSpinnerValueFactory} will step from
The link for wrapAround does not resolve in Eclipse.
I'd suggest to change it to
* If {@link SpinnerValueFactory#wrapAround wrapAround}
Same comment applies to the min and max in line 420.
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 619:
> 617: * double values.
> 618: *
> 619: * If {@link #wrapAround} is {@code true}, the {@code
> DoubleSpinnerValueFactory} will step through
wrapAround link does not resolve in Eclipse, see my earlier comment
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591260910
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591264246
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes
On Mon, 6 May 2024 15:39:02 GMT, Kevin Rushforth wrote:
>> This PR updates the javadoc for the SpinnerFactory wrap-around behavior
>> introduced in #1431.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
> line 420:
>
>> 418: * the minimum value to the maximum value (and vice versa). As a
>> consequence of that, the number
>> 419: * of steps required to wrap around to the same value is N+1, where
>> N is the number of steps between
>> 420: * {@link #min} (inclusive) and {@link #max} (inclusive).
>
> Suggestion: It might be helpful to show the equation:
>
>
> val = (val + delta) % (max - min + 1)
a good idea
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591262735
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen
On Sat, 4 May 2024 22:55:13 GMT, Martin Fox wrote: > This PR is based on a discussion that happened over in PR #1324. Some of this > explanation is copied from that thread. > > When `exitNestedEventLoop` is called on the innermost loop the > invokeLaterDispatcher suspends operation until the loop finishes. But if you > immediately start a new event loop the previous one won't finish and the > dispatcher will jam up and stop dispatching indefinitely. > > When the invokeLaterDispatcher is told that the innermost loop is exiting it > sets `leavingNestedEventLoop` to true expecting it to be set to false when > the loop actually exits. When the dispatcher is told that a new event loop > has started it is not clearing `leavingNestedEventLoop` which is causing the > jam. Basically it should follow the same logic used in glass; leaving the > innermost loop updates a boolean indicating that the loop should exit but if > a new loop is started the boolean is set back to a running state since it now > applies to the new loop, not the previous one. > > I suspect the invokeLaterDispatcher exists in part to deal with the specifics > of how deferred runnables are handled on the Mac. I investigated this a bit > and wrote up some comments in the Mac glass code. This looks like a safe, targeted fix for the problem. It will need to be tested on all platforms with a full run of all system tests. - PR Comment: https://git.openjdk.org/jfx/pull/1449#issuecomment-2096441265
Re: RFR: 8329821: [Linux] When using i3 WM, menus are incorrectly sized
On Sat, 4 May 2024 23:03:25 GMT, Christopher Schnick wrote: > So I can reproduce this on my system now and could potentially test this PR. > How would one go about this, testing a specific PR? You will need to checkout the PR branch and build JavaFX from source. See the "Using git" instructions added by Skara in the "Reviewing" section of this PR's Description for how to fetch and checkout this PR branch. See the [build instructions Wiki page](https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX) for instructions on building JavaFX. - PR Comment: https://git.openjdk.org/jfx/pull/1437#issuecomment-2096416824
Re: RFR: 8092102: Labeled: textTruncated property [v9]
On Mon, 15 Apr 2024 09:57:10 GMT, Karthik P K wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 15 commits: >> >> - missing ) >> - review comments >> - Merge branch 'master' into 8092102.truncated >> - add exports >> - added unit tests >> - Merge remote-tracking branch 'origin/master' into 8092102.truncated >> - test >> - Merge remote-tracking branch 'origin/master' into 8092102.truncated >> - Merge branch 'master' into 8092102.truncated >> - labeled helper >> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e > > LGTM @karthikpandelu @arapte Can you re-review? - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2096392818
Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty
On Mon, 6 May 2024 14:14:05 GMT, eduardsdv wrote: > This is an alternative solution to the PR: > https://github.com/openjdk/jfx/pull/1310. > > This solution is based on the invariant that if a node is marked as dirty, > all ancestors must also be marked as dirty and that if an ancestor is marked > as clean, all descendants must also be marked as clean. > Therefore I removed the ``clearDirtyTree()`` method and put its content to > the ``clearDirty()`` method. > > Furthermore, since dirty flag is only used when rendering by ``ViewPainter``, > it should also be deleted by ``ViewPainter`` only. > This guarantees: > 1. that all dirty flags are removed after rendering, and > 2. that no dirty flags are removed when a node is rendered, e.g. by creating > a snapshot or printing. > Therefore I removed all calls of the methods ``clearDirty()`` and > ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``. > > The new version of the ``clearDirty()`` method together with calling it from > the ``ViewerPainter`` needs to visit far fewer nodes compared to the version > prior this PR. > > The supplied test checks that the nodes are updated even if they are > partially covered, which led to the error in the version before the PR. The > test can be started with ``gradlew -PFULL_TEST=true :systemTests:test --tests > NGNodeDirtyFlagTest`` Reviewers: @lukostyra @arapte This is a more intrusive fix than #1310 so we would only want to go this route if we can show that it is a correct fix, introduces no regressions (in either correctness or performance), and makes it easier to maintain in the future. - PR Comment: https://git.openjdk.org/jfx/pull/1451#issuecomment-2096370830
Re: RFR: 8285893: Hiding dialog and showing new one causes dialog to be frozen
On Sat, 4 May 2024 22:55:13 GMT, Martin Fox wrote: > This PR is based on a discussion that happened over in PR #1324. Some of this > explanation is copied from that thread. > > When `exitNestedEventLoop` is called on the innermost loop the > invokeLaterDispatcher suspends operation until the loop finishes. But if you > immediately start a new event loop the previous one won't finish and the > dispatcher will jam up and stop dispatching indefinitely. > > When the invokeLaterDispatcher is told that the innermost loop is exiting it > sets `leavingNestedEventLoop` to true expecting it to be set to false when > the loop actually exits. When the dispatcher is told that a new event loop > has started it is not clearing `leavingNestedEventLoop` which is causing the > jam. Basically it should follow the same logic used in glass; leaving the > innermost loop updates a boolean indicating that the loop should exit but if > a new loop is started the boolean is set back to a running state since it now > applies to the new loop, not the previous one. > > I suspect the invokeLaterDispatcher exists in part to deal with the specifics > of how deferred runnables are handled on the Mac. I investigated this a bit > and wrote up some comments in the Mac glass code. As with the earlier PR, this will need a lot of testing and careful review. @Maran23 I'd be interested in your thoughts on this approach. - PR Comment: https://git.openjdk.org/jfx/pull/1449#issuecomment-2096342944
Re: RFR: 8331214: Doc: update spec for SpinnerFactory classes
On Mon, 6 May 2024 08:24:52 GMT, drmarmac wrote:
> This PR updates the javadoc for the SpinnerFactory wrap-around behavior
> introduced in #1431.
Looks like a good start. I left a few minor suggestions.
@andy-goryachev-oracle Do you also want to take a look?
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 118:
> 116: * @param steps The number of decrements that should be performed on
> the value.
> 117: * If the number is negative, the call is equivalent to
> calling
> 118: * {@link #increment(int)} with the absolute number of
> steps.
Suggestion:
... with the absolute value of {@code steps}.
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 129:
> 127: * @param steps The number of increments that should be performed on
> the value.
> 128: * If the number is negative, the call is equivalent to
> calling
> 129: * {@link #decrement(int)} with the absolute number of
> steps.
Suggestion:
... with the absolute value of {@code steps}.
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 419:
> 417: * If {@link #wrapAround} is {@code true}, the {@code
> IntegerSpinnerValueFactory} will step from
> 418: * the minimum value to the maximum value (and vice versa). As a
> consequence of that, the number
> 419: * of steps required to wrap around to the same value is N+1, where
> N is the number of steps between
I recommend code case for `N` and `N+1`.
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 420:
> 418: * the minimum value to the maximum value (and vice versa). As a
> consequence of that, the number
> 419: * of steps required to wrap around to the same value is N+1, where
> N is the number of steps between
> 420: * {@link #min} (inclusive) and {@link #max} (inclusive).
Suggestion: It might be helpful to show the equation:
val = (val + delta) % (max - min + 1)
modules/javafx.controls/src/main/java/javafx/scene/control/SpinnerValueFactory.java
line 621:
> 619: * If {@link #wrapAround} is {@code true}, the {@code
> DoubleSpinnerValueFactory} will step through
> 620: * from the maximum value to the minimum value seamlessly; that is,
> any step up from the maximum value
> 621: * is equal to the same step up from the minimum value (and vice
> versa).
Suggestion: It might be helpful to show the equation:
val = (val + delta) % (max - min)
-
PR Review: https://git.openjdk.org/jfx/pull/1450#pullrequestreview-2041073507
PR Comment: https://git.openjdk.org/jfx/pull/1450#issuecomment-2096331641
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591187179
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591187802
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591189203
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591190823
PR Review Comment: https://git.openjdk.org/jfx/pull/1450#discussion_r1591191777
Re: RFR: 8092102: Labeled: textTruncated property [v15]
On Mon, 6 May 2024 15:26:18 GMT, Kevin Rushforth wrote: > Looks good. You'll need to change the PR title to match the updated JBS title. > Also, you can finalize the CSR. Thank you for your help, Kevin! - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2096304606
Re: RFR: 8092102: Labeled: textTruncated property [v15]
On Mon, 6 May 2024 15:23:20 GMT, Andy Goryachev wrote: >> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property is being set by the code which computes the actual text >> string to be displayed (and which inserts the ellipsis string) in >> `LabeledSkinBase.updateDisplayedText(double,double)`. >> >> >> **Alternative** >> >> None exists as this requires changes to the core (Utils). >> >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > comments Looks good. You'll need to change the PR title to match the updated JBS title. Also, you can finalize the CSR. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2041049872
Re: RFR: 8092102: Labeled: truncated property [v15]
> Adds **Labeled.textTruncated** property which indicates when the text is > visually truncated (and the ellipsis string is inserted) in order to fit the > available width. > > The new property is being set by the code which computes the actual text > string to be displayed (and which inserts the ellipsis string) in > `LabeledSkinBase.updateDisplayedText(double,double)`. > > > **Alternative** > > None exists as this requires changes to the core (Utils). > > > **See Also** > > * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow > for tooltip when cell text is truncated > * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show > Tooltip only when text is shown with ellipsis (...) Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: comments - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/7d267043..cce514ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=14 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=13-14 Stats: 34 lines in 2 files changed: 33 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Re: RFR: 8092102: Labeled: truncated property [v14]
On Mon, 6 May 2024 14:50:00 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java >> line 221: >> >>> 219: OverrunStyle type, >>> 220: String ellipsisString, >>> 221: AtomicBoolean textTruncated >> >> I recommend setting `textTruncated` to false as the first statement >> (alternatively, add a comment that the caller is expected to initialize it >> to false). > > setting to false would be incorrect in the case of multi-line text. > > I can add a comment, though it's not a public API and there was no > comment/javadoc before. Yes, I see. In that case, it's up to you whether to add a simple comment that this should be initialized by the caller (but maybe it doesn't need to be mentioned?) - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1591138400
Re: RFR: 8092102: Labeled: truncated property [v14]
On Sat, 4 May 2024 14:05:19 GMT, Kevin Rushforth wrote: >> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> whitespace > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java > line 221: > >> 219: OverrunStyle type, >> 220: String ellipsisString, >> 221: AtomicBoolean textTruncated > > I recommend setting `textTruncated` to false as the first statement > (alternatively, add a comment that the caller is expected to initialize it to > false). setting to false would be incorrect in the case of multi-line text. I can add a comment, though it's not a public API and there was no comment/javadoc before. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1591128736
RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty
This is an alternative solution to the PR: https://github.com/openjdk/jfx/pull/1310. This solution is based on the invariant that if a node is marked as dirty, all ancestors must also be marked as dirty and that if an ancestor is marked as clean, all descendants must also be marked as clean. Therefore I removed the ``clearDirtyTree()`` method and put its content to the ``clearDirty()`` method. Furthermore, since dirty flag is only used when rendering by ``ViewPainter``, it should also be deleted by ``ViewPainter`` only. This guarantees: 1. that all dirty flags are removed after rendering, and 2. that no dirty flags are removed when a node is rendered, e.g. by creating a snapshot or printing. Therefore I removed all calls of the methods ``clearDirty()`` and ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``. The new version of the ``clearDirty()`` method together with calling it from the ``ViewerPainter`` needs to visit far fewer nodes compared to the version prior this PR. The supplied test checks that the nodes are updated even if they are partially covered, which led to the error in the version before the PR. The test can be started with ``gradlew -PFULL_TEST=true :systemTests:test --tests NGNodeDirtyFlagTest`` - Commit messages: - JDK-8322619: Combine clearDirtyTree() and clearDirty() methods. Changes: https://git.openjdk.org/jfx/pull/1451/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1451=00 Issue: https://bugs.openjdk.org/browse/JDK-8322619 Stats: 284 lines in 11 files changed: 213 ins; 47 del; 24 mod Patch: https://git.openjdk.org/jfx/pull/1451.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1451/head:pull/1451 PR: https://git.openjdk.org/jfx/pull/1451
Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v5]
On Fri, 3 May 2024 10:23:27 GMT, Florian Kirmaier wrote: >> In some situations, a part of the SG is no longer rendered. >> I created a test program that showcases this problem. >> >> Explanation: >> >> This can happen, when a part of the SG, is covered by another Node. >> In this part, one node is totally covered, and the other node is visible. >> >> When the totally covered Node is changed, then it is marked dirty and it's >> parent, recursively until an already dirty node is found. >> Due to the Culling, this totally covered Node is not rendered - with the >> effect that the tree is never marked as Clean. >> >> In this state, a Node is Dirty but not It's parent. Based on my CodeReview, >> this is an invalid state which should never happen. >> >> In this invalid state, when the other Node is changed, which is visible, >> then the dirty state is no longer propagated upwards - because the recursive >> "NGNode.markTreeDirty" algorithm encounters a dirty node early. >> >> This has the effect, that any SG changes in the visible Node are no longer >> rendered. Sometimes the situation repairs itself. >> >> Useful parameters for further investigations: >> -Djavafx.pulseLogger=true >> -Dprism.printrendergraph=true >> -Djavafx.pulseLogger.threshold=0 >> >> PR: >> This PR ensures the dirty flag is set to false of the tree when the culling >> is used. >> It doesn't seem to break any existing tests - but I'm not sure whether this >> is the right way to fix it. >> It would be great to have some feedback on this solution - maybe guiding me >> to a better solution. >> >> I could write a test, that just does the same thing as the test application, >> but checks every frame that these nodes are not dirty - but maybe there is a >> better way to test this. > > Florian Kirmaier has updated the pull request incrementally with two > additional commits since the last revision: > > - JDK-8322619: Add test > - Revert "JDK-8322619: Clear dirty flag on the node and all its children if > they are skipped due to visible==false or opacity==0" > >This reverts commit 5f9f1574515c078c1fd0dccd476325090a0b284d. I created an alternative solution for this bug. See PR: https://github.com/openjdk/jfx/pull/1451 - PR Comment: https://git.openjdk.org/jfx/pull/1310#issuecomment-2096126581
Re: RFR: 8092102: Labeled: truncated property [v14]
On Fri, 3 May 2024 21:00:26 GMT, Andy Goryachev wrote:
>> Adds **Labeled.textTruncated** property which indicates when the text is
>> visually truncated (and the ellipsis string is inserted) in order to fit the
>> available width.
>>
>> The new property is being set by the code which computes the actual text
>> string to be displayed (and which inserts the ellipsis string) in
>> `LabeledSkinBase.updateDisplayedText(double,double)`.
>>
>>
>> **Alternative**
>>
>> None exists as this requires changes to the core (Utils).
>>
>>
>> **See Also**
>>
>> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow
>> for tooltip when cell text is truncated
>> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show
>> Tooltip only when text is shown with ellipsis (...)
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> whitespace
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
837:
> 835: * When truncated, the {@link #ellipsisStringProperty()
> ellipsisString}
> 836: * gets inserted in the place dictated by the
> 837: * {@link #textOverrun} property.
I just noticed that this link doesn't work. Looks like you'll need to change it
to point to the `textOverrunProperty()` method.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1591041626
Re: RFR: 8092102: Labeled: truncated property [v14]
On Sat, 4 May 2024 14:17:16 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> whitespace
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
> 855:
>
>> 853: }
>> 854:
>> 855: private ReadOnlyBooleanWrapper textTruncated() {
>
> ~Suggestion: rename this method to something like `textTruncatedImpl()` for
> clarity (as it is, the method, which is a writable property, shares the same
> name as the read-only property field, which could be confusing).~
Never mind. I missed that the types of the field and method are identical, so I
withdraw my suggestion.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1590957454
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
On Mon, 6 May 2024 08:49:27 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: Fix scrolling when clicking the scrollbar track when only one > cell is visible I added a new method ``VirtualFlow.getBlockIncrement()``, which returns the amount of pixels by which the position of the VirtualFlow should be adjusted when clicking on the scrollbar track. Now, if only one cell is visible, the ``VirtualScrollBar`` changes the position of the ``VirtualFlow`` simply by calling the VirtualFlow.scrollPixels(double)``. - PR Comment: https://git.openjdk.org/jfx/pull/1326#issuecomment-2095542279
Re: RFR: 8323511: Scrollbar Click jumps inconsistent amount of pixels [v3]
> 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: Fix scrolling when clicking the scrollbar track when only one cell is visible - Changes: - all: https://git.openjdk.org/jfx/pull/1326/files - new: https://git.openjdk.org/jfx/pull/1326/files/29d610fd..6a02f682 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1326=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1326=01-02 Stats: 37 lines in 2 files changed: 10 ins; 13 del; 14 mod Patch: https://git.openjdk.org/jfx/pull/1326.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1326/head:pull/1326 PR: https://git.openjdk.org/jfx/pull/1326
RFR: 8331214: Doc: update spec for SpinnerFactory classes
This PR updates the javadoc for the SpinnerFactory wrap-around behavior introduced in #1431. - Commit messages: - Update SpinnerValueFactory documentation Changes: https://git.openjdk.org/jfx/pull/1450/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1450=00 Issue: https://bugs.openjdk.org/browse/JDK-8331214 Stats: 21 lines in 1 file changed: 13 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1450.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1450/head:pull/1450 PR: https://git.openjdk.org/jfx/pull/1450
