Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v3]

[Karthik P K]

On Mon, 6 Feb 2023 22:50:30 GMT, John Hendrikx  wrote:

>> Karthik P K has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Code optimization
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java
>  line 331:
> 
>> 329: 
>> 330: double width;
>> 331: if(isIgnoreGraphic()) {
> 
> minor: missed a space here after `if`

Updated code.

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java
>  line 393:
> 
>> 391: if (isIgnoreText()) {
>> 392: height = graphicHeight;
>> 393: } else if (contentDisplay == TOP || contentDisplay == 
>> BOTTOM){
> 
> minor: missing space before `{`

Updated code.

-

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

Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v4]

[Karthik P K]

> Ignore text condition was not considered in `computePrefHeight` method while 
> calculating the Label height.
> 
> Added `isIgnoreText` condition in `computePrefHeight` method while 
> calculating Label height.
> 
> Tests are present for all the ContentDisplay types. Modified tests which were 
> failing because of the new height value getting calculated.

Karthik P K has updated the pull request incrementally with one additional 
commit since the last revision:

  Addressing review comments

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/996/files
  - new: https://git.openjdk.org/jfx/pull/996/files/1d20f756..1b868a6f

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

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

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

Re: [jfx20] RFR: 8293587: Fix mistakes in FX API docs

[Nir Lisker]

On Mon, 6 Feb 2023 23:00:17 GMT, Nir Lisker  wrote:

> Fixes and cleanup in the areas in the linked issue.

Integrating can wait until a bit before the release to allow for more mistakes 
to be included.

-

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

[jfx20] RFR: 8293587: Fix mistakes in FX API docs

[Nir Lisker]

Fixes and cleanup in the areas in the linked issue.

-

Commit messages:
 - Initial commit

Changes: https://git.openjdk.org/jfx/pull/1025/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1025&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8293587
  Stats: 50 lines in 6 files changed: 3 ins; 4 del; 43 mod
  Patch: https://git.openjdk.org/jfx/pull/1025.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/1025/head:pull/1025

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

Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v3]

[John Hendrikx]

On Mon, 6 Feb 2023 13:33:20 GMT, Karthik P K  wrote:

>> Ignore text condition was not considered in `computePrefHeight` method while 
>> calculating the Label height.
>> 
>> Added `isIgnoreText` condition in `computePrefHeight` method while 
>> calculating Label height.
>> 
>> Tests are present for all the ContentDisplay types. Modified tests which 
>> were failing because of the new height value getting calculated.
>
> Karthik P K has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Code optimization

Marked as reviewed by jhendrikx (Committer).

modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java
 line 331:

> 329: 
> 330: double width;
> 331: if(isIgnoreGraphic()) {

minor: missed a space here after `if`

modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java
 line 393:

> 391: if (isIgnoreText()) {
> 392: height = graphicHeight;
> 393: } else if (contentDisplay == TOP || contentDisplay == 
> BOTTOM){

minor: missing space before `{`

-

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

Re: Enhance javafx Canvas API

[Bruce Johnson]

> On Feb 3, 2023, at 5:31 PM, Laurent Bourgès  wrote:
> 
> Hi,
> 
> While working with skia & jfree's skijaGraphics2D wrapper, I observed skia 
> offers nice methods in SkCanvas missing in javafx Canvas/GraphicsContext to 
> deal with save/restore state:
> 
> - int getSaveCount ()
>   Returns the number of saved states, each containing: SkMatrix and clip. 
>  
> - void restoreToCount (int saveCount)
>   Restores state to SkMatrix and clip values when save(), returned saveCount. 
> 
> As javafx canvas also maintains s stack of saved states, such methods are 
> trivial to implement to allow resetting completely the canvas state by 
> restoreToCount(0).
> 
> Moreover, clip(shape) + save/restore are really tricky to allows ensure clip 
> is reset to previous state ...
> 
> Would you agree to add such methods to have more predictive behaviour than 
> counting all save() calls to ensure to call restore() the same number of 
> times to reset the canvas to its initial state ?
> 
> See SkijaGraphics2d implementation that saves / restore skCanvas clip using 
> such index property:
> https://github.com/jfree/skijagraphics2d/blob/main/src/main/java/org/jfree/skija/SkijaGraphics2D.java
>  
> 
> 
> Cheers,
> Laurent

I’d love to see this.  I find keeping track of the state of clipping and 
getting it restored to a specific state can be painful. Swing let you clear the 
clip with setClip(null) which was definitely useful and I was somewhat 
surprised to see that I couldn’t do that in JavaFX.

Bruce

Re: [jfx20] RFR: 8301832: InputMethodEvents are not enabled for text input controls

[Kevin Rushforth]

On Mon, 6 Feb 2023 18:45:10 GMT, Andy Goryachev  wrote:

> **Targeting jfx20 branch**
> 
> A fix for regression introduced by 
> [JDK-8268877](https://bugs.openjdk.org/browse/JDK-8268877).
> 
> For IME to work, both `setOnInputMethodTextChanged()` and 
> `setInputMethodRequests()` needs to be set, see Scene:2242.
> 
> The fix involves changing the code back to setting the control properties and 
> doing so in the install() method.
> 
> Thank you Martin Fox  for noticing the issue!

Looks good. I confirm that this fixes the regression.

-

Marked as reviewed by kcr (Lead).

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

Re: [jfx20] RFR: 8301832: InputMethodEvents are not enabled for text input controls

[Kevin Rushforth]

On Mon, 6 Feb 2023 18:45:10 GMT, Andy Goryachev  wrote:

> **Targeting jfx20 branch**
> 
> A fix for regression introduced by 
> [JDK-8268877](https://bugs.openjdk.org/browse/JDK-8268877).
> 
> For IME to work, both `setOnInputMethodTextChanged()` and 
> `setInputMethodRequests()` needs to be set, see Scene:2242.
> 
> The fix involves changing the code back to setting the control properties and 
> doing so in the install() method.
> 
> Thank you Martin Fox  for noticing the issue!

Note that this will need approval (in JBS) to go into jfx20.

-

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

[jfx20] RFR: 8301832: InputMethodEvents are not enabled for text input controls

[Andy Goryachev]

A fix for regression introduced by 
[JDK-8268877](https://bugs.openjdk.org/browse/JDK-8268877).

For IME to work, both `setOnInputMethodTextChanged()` and 
`setInputMethodRequests()` needs to be set, see Scene:2242.

The fix involves changing the code back to setting the control properties and 
doing so in the install() method.

Thank you Martin Fox (@beldenfox ?) for noticing the issue!

-

Commit messages:
 - 8301832: InputMethodEvents are not enabled for text input controls

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

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

[jfx20] Integrated: 8301797: Pagination control has the wrong size

[Andy Goryachev]

On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev  wrote:

> Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 
> branch**.
> 
> The method PaginationSkin.resetIndexes(true) has been moved to the original 
> position in the constructor, fixing the initialization, while making sure no 
> properties of the control are touched in the constructor (only in install()).
> 
> Added a test case.
> 
> Tested with the PaginationDisappear.java and the LeakTest app
> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java

This pull request has now been integrated.

Changeset: 192ba4fc
Author:Andy Goryachev 
URL:   
https://git.openjdk.org/jfx/commit/192ba4fc71b585d9ff5a81c75499187944f548dc
Stats: 25 lines in 2 files changed: 22 ins; 1 del; 2 mod

8301797: Pagination control has the wrong size

Reviewed-by: kcr, aghaisas

-

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

Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]

[Karthik P K]

On Fri, 3 Feb 2023 18:04:09 GMT, Andy Goryachev  wrote:

>> Not sure if I didn't understand your comments properly but I'm guessing you 
>> are referring to `textHeight` and `graphicHeight` calculation in line 375 
>> and 380 because the line numbers you mentioned are in `computePrefWidth` 
>> method and changes are made to height calculation in `computePrefHeight` 
>> only.
>> Since `textHeight` and `graphicHeight` each are used in 3 conditions from 
>> line no 386 to 394 I have kept the calculation in the beginning itself. 
>> 
>> On the other hand `textWidth` declared in line 368 might change in line 371, 
>> so I can't use `width` directly instead of defining `textWidth` as `width` 
>> need to be used while calling other graphic related methods.
>
> Both `computePrefHeight()` and `computePrefWidth` can be restructured to 
> avoid unnecessary computation (especially since these are very popular 
> objects).
> 
> My point is that, for example, textWidth on line 375 (used to compute 
> textHeight:375) is not used if isIgnoreText() == true.  
> 
> Similarly, graphicHeight:380 is not used if isIgnoreGraphic() == true, line 
> 386.
> 
> Same optimization can be applied to computePrefWidth():315
> 
> Just a suggestion, really.

Understood.
I have optimized the code. Please take a look.
Did following optimizations in the code:

- Optimized `if else` conditions by moving the `graphicHeight` and 
`graphicWidth` value calculation inside the else statement in 
`computePrefHeight` and `computePrefWidth` methods respectively.
- Removed unused `graphic` variable.
- In `computePrefHeight` method, since a `if` condition was already present 
which was same as the one used for `padding` calculation, moved `padding` 
calculation code to the same location.
- In `computePrefWidth` method, replaced multiple return statements with single 
statement for better readability and did minor optimizations.

-

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

Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v2]

[JoachimSchriek]

On Mon, 6 Feb 2023 13:38:21 GMT, Kevin Rushforth  wrote:

>> It was not feasible for me -at least as i saw it- to integrate the tests in 
>> the existing tests without using a robot because I think the behavior  
>> reported in the bug report mostly relates to the reaction of the UI to mouse 
>> clicks.
>
> @JoachimSchriek In addition to restoring the mistakenly-deleted file, I 
> noticed that you don't have GitHub actions enabled (else you would have 
> discovered this problem right away). Also, your branch is fairly out of date 
> with respect to the upstream master.
> 
> To that end, I recommend doing the following:
> 
> 1. Merge the latest upstream master into your branch
> 2. Enable GitHub actions in your repo (follow the instructions listed above 
> in the details of "Pre-submit test status Skipped — Testing is not 
> configured" -- the short version is that you need to click the big green 
> button in the "Actions" tab of your repo)
> 3. Restore VirtualFlow.java such that it is identical to the upstream master.

@kevinrushforth the merge did the work. Now, there are only the two files that 
should be changed.

-

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

Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v6]

[JoachimSchriek]

On Mon, 6 Feb 2023 13:59:13 GMT, JoachimSchriek  wrote:

>> This is my (joachim.schr...@gmx.de) first contribution to openjfx. My 
>> Contributor Agreement is signed but still in review.
>> So please be patient with an absolute beginner as contributor ... .
>> The work of this pull request was fully done in my spare time.
>> 
>> I first filed the bug myself in 2017. I had begun working with JavaFX in 
>> 2014.
>> 
>> The two changes address the two problems mentioned in JDK-8173321:
>> - Using a JavaFX TableView, a click on the right trough has no effect when 
>> the cell height of the cell currently displayed is higher than viewport 
>> height
>> - The ScrollBar ist displayed with a minimal height.
>> 
>> The changes were tested and ran well with Java 17 and the current master 
>> branch of openjfx.
>
> JoachimSchriek has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   Restore original VirtualFlow.java

@ kevinrushforth I will need some time to correct the state of 
VirtualFlow.java. I will leave a notice when it is ready.

-

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

Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v7]

[JoachimSchriek]

> This is my (joachim.schr...@gmx.de) first contribution to openjfx. My 
> Contributor Agreement is signed but still in review.
> So please be patient with an absolute beginner as contributor ... .
> The work of this pull request was fully done in my spare time.
> 
> I first filed the bug myself in 2017. I had begun working with JavaFX in 2014.
> 
> The two changes address the two problems mentioned in JDK-8173321:
> - Using a JavaFX TableView, a click on the right trough has no effect when 
> the cell height of the cell currently displayed is higher than viewport height
> - The ScrollBar ist displayed with a minimal height.
> 
> The changes were tested and ran well with Java 17 and the current master 
> branch of openjfx.

JoachimSchriek 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 seven additional 
commits since the last revision:

 - Merge branch 'openjdk:master' into master
 - Restore original VirtualFlow.java
 - Changes made following findings from review
 - Deleted trailing whitespace
 - Committed Test Case for [openjdk/jfx] 8173321: Click on trough has no
   effect when cell height > viewport (PR #985):
 - Replaces Tabs with Spaces
 - JDK-8173321: Click on trough has no effect when cell height > viewport
   height

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/985/files
  - new: https://git.openjdk.org/jfx/pull/985/files/a316200e..62606f41

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

  Stats: 202484 lines in 5685 files changed: 120624 ins; 43383 del; 38477 mod
  Patch: https://git.openjdk.org/jfx/pull/985.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/985/head:pull/985

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

Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v6]

[JoachimSchriek]

> This is my (joachim.schr...@gmx.de) first contribution to openjfx. My 
> Contributor Agreement is signed but still in review.
> So please be patient with an absolute beginner as contributor ... .
> The work of this pull request was fully done in my spare time.
> 
> I first filed the bug myself in 2017. I had begun working with JavaFX in 2014.
> 
> The two changes address the two problems mentioned in JDK-8173321:
> - Using a JavaFX TableView, a click on the right trough has no effect when 
> the cell height of the cell currently displayed is higher than viewport height
> - The ScrollBar ist displayed with a minimal height.
> 
> The changes were tested and ran well with Java 17 and the current master 
> branch of openjfx.

JoachimSchriek has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  Restore original VirtualFlow.java

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/985/files
  - new: https://git.openjdk.org/jfx/pull/985/files/e7e9bc99..a316200e

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

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

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

Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v2]

[Kevin Rushforth]

On Sun, 22 Jan 2023 15:19:48 GMT, JoachimSchriek  wrote:

>> JoachimSchriek has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Committed Test Case for [openjdk/jfx] 8173321: Click on trough has no
>>   effect when cell height > viewport (PR #985):
>
> It was not feasible for me -at least as i saw it- to integrate the tests in 
> the existing tests without using a robot because I think the behavior  
> reported in the bug report mostly relates to the reaction of the UI to mouse 
> clicks.

@JoachimSchriek In addition to restoring the mistakenly-deleted file, I noticed 
that you don't have GitHub actions enabled (else you would have discovered this 
problem right away). Also, your branch is fairly out of date with respect to 
the upstream master.

To that end, I recommend doing the following:

1. Merge the latest upstream master into your branch
2. Enable GitHub actions in your repo (follow the instructions listed above in 
the details of "Pre-submit test status Skipped — Testing is not configured" -- 
the short version is that you need to click the big green button in the 
"Actions" tab of your repo)
3. Restore VirtualFlow.java such that it is identical to the upstream master.

-

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

Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v3]

[Karthik P K]

> Ignore text condition was not considered in `computePrefHeight` method while 
> calculating the Label height.
> 
> Added `isIgnoreText` condition in `computePrefHeight` method while 
> calculating Label height.
> 
> Tests are present for all the ContentDisplay types. Modified tests which were 
> failing because of the new height value getting calculated.

Karthik P K has updated the pull request incrementally with one additional 
commit since the last revision:

  Code optimization

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/996/files
  - new: https://git.openjdk.org/jfx/pull/996/files/c2a2afa6..1d20f756

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

  Stats: 55 lines in 1 file changed: 21 ins; 23 del; 11 mod
  Patch: https://git.openjdk.org/jfx/pull/996.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/996/head:pull/996

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

Re: RFR: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height [v5]

[Ajit Ghaisas]

On Sun, 5 Feb 2023 12:22:19 GMT, JoachimSchriek  wrote:

>> This is my (joachim.schr...@gmx.de) first contribution to openjfx. My 
>> Contributor Agreement is signed but still in review.
>> So please be patient with an absolute beginner as contributor ... .
>> The work of this pull request was fully done in my spare time.
>> 
>> I first filed the bug myself in 2017. I had begun working with JavaFX in 
>> 2014.
>> 
>> The two changes address the two problems mentioned in JDK-8173321:
>> - Using a JavaFX TableView, a click on the right trough has no effect when 
>> the cell height of the cell currently displayed is higher than viewport 
>> height
>> - The ScrollBar ist displayed with a minimal height.
>> 
>> The changes were tested and ran well with Java 17 and the current master 
>> branch of openjfx.
>
> JoachimSchriek has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Delete VirtualFlow.java
>   
>   Revert unintentional changes to VirtualFlow

In your last commit- instead of only reverting your changes to the 
file`VirtualFlow.java`, you have deleted the entire file itself. Please restore 
this file.

-

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

Re: [jfx20] RFR: 8301797: Pagination control has the wrong size [v2]

[Ajit Ghaisas]

On Fri, 3 Feb 2023 23:09:30 GMT, Andy Goryachev  wrote:

>> Fixed regression introduced by JDK-8295754, targeting upstream **jfx20 
>> branch**.
>> 
>> The method PaginationSkin.resetIndexes(true) has been moved to the original 
>> position in the constructor, fixing the initialization, while making sure no 
>> properties of the control are touched in the constructor (only in install()).
>> 
>> Added a test case.
>> 
>> Tested with the PaginationDisappear.java and the LeakTest app
>> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java
>
> Andy Goryachev has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8301797: review comments

The simplified fix looks good to me!
Thanks for fixing this regression.

-

Marked as reviewed by aghaisas (Reviewer).

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

Integrated: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Karthik P K]

On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K  wrote:

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

This pull request has now been integrated.

Changeset: ef9f0664
Author:Karthik P K 
Committer: Ajit Ghaisas 
URL:   
https://git.openjdk.org/jfx/commit/ef9f0664484f7a3c2a9ebfbd81e1d23ef65218ed
Stats: 37 lines in 4 files changed: 35 ins; 0 del; 2 mod

8138842: TableViewSelectionModel.selectIndices does not select index 0

Reviewed-by: angorya, aghaisas

-

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