Integrated: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)

[Robert Lichtenberger]

On Thu, 21 Apr 2022 08:38:20 GMT, Robert Lichtenberger  
wrote:

> Separate test class added for TreeTableView case.
> Fix is analogous to JDK-8251480.

This pull request has now been integrated.

Changeset: 18b2366f
Author:Robert Lichtenberger 
Committer: Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jfx/commit/18b2366f3d0520479f0d9c4af48acf9495d15a72
Stats: 180 lines in 2 files changed: 176 ins; 1 del; 3 mod

8285197: TableColumnHeader: calc of cell width must respect row styling 
(TreeTableView)

Reviewed-by: mhanl, aghaisas

-

PR: https://git.openjdk.java.net/jfx/pull/779

Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]

[Robert Lichtenberger]

On Thu, 12 May 2022 05:22:38 GMT, Ajit Ghaisas  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - 8285197: TableColumnHeader: calc of cell width must respect row styling 
>> (TreeTableView)
>>
>>Test class cosmetic cleanups #2.
>>  - 8285197: TableColumnHeader: calc of cell width must respect row styling 
>> (TreeTableView)
>>
>>Test class cosmetic cleanups.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java
>  line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights 
>> reserved.
> 
> Replace "2018, 2021" with just "2022" as this is newly introduced test file.

done

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableColumnHeaderTest.java
>  line 51:
> 
>> 49: import java.util.List;
>> 50: 
>> 51: import static org.junit.Assert.*;
> 
> Replace generic import statement with specific ones.

done

-

PR: https://git.openjdk.java.net/jfx/pull/779

Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)

[Robert Lichtenberger]

On Thu, 21 Apr 2022 08:38:20 GMT, Robert Lichtenberger  
wrote:

> Separate test class added for TreeTableView case.
> Fix is analogous to JDK-8251480.

Finally found the time to cleanup the test class...

-

PR: https://git.openjdk.java.net/jfx/pull/779

Re: RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView) [v2]

[Robert Lichtenberger]

> Separate test class added for TreeTableView case.
> Fix is analogous to JDK-8251480.

Robert Lichtenberger has updated the pull request incrementally with two 
additional commits since the last revision:

 - 8285197: TableColumnHeader: calc of cell width must respect row styling 
(TreeTableView)
   
   Test class cosmetic cleanups #2.
 - 8285197: TableColumnHeader: calc of cell width must respect row styling 
(TreeTableView)
   
   Test class cosmetic cleanups.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/779/files
  - new: https://git.openjdk.java.net/jfx/pull/779/files/38d930a8..68f7c806

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=779=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=779=00-01

  Stats: 13 lines in 1 file changed: 3 ins; 6 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/779.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/779/head:pull/779

PR: https://git.openjdk.java.net/jfx/pull/779

RFR: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)

[Robert Lichtenberger]

Separate test class added for TreeTableView case.
Fix is analogous to JDK-8251480.

-

Commit messages:
 - 8285197: TableColumnHeader: calc of cell width must respect row styling 
(TreeTableView)

Changes: https://git.openjdk.java.net/jfx/pull/779/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=779=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285197
  Stats: 183 lines in 2 files changed: 179 ins; 1 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/779.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/779/head:pull/779

PR: https://git.openjdk.java.net/jfx/pull/779

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Tue, 19 Apr 2022 09:57:58 GMT, Marius Hanl  wrote:

>>> @effad Do you have time and interest to take a look at this for the 
>>> `TreeTableView` as well? Just asking as otherwise I will have a look :)
>> 
>> Sorry for late response (extended easter holidays ...). Yes I could also 
>> take a look at this for TreeTableView. Is there a pre-existing issue for 
>> this? Otherwise I'll try and come up with an example and create a new 
>> issue...
>
>> Is there a pre-existing issue for this? Otherwise I'll try and come up with 
>> an example and create a new issue...
> 
> I don't think so, you may need to create a new issue. I think it's pretty 
> much the same problem and solution as this ticket was, so you can probably 
> copy some information.

@Maran23 https://bugs.openjdk.java.net/browse/JDK-8285197 created.

-

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Wed, 16 Mar 2022 13:01:45 GMT, Robert Lichtenberger  
wrote:

>> Might make sense to also adjust the TreeTableView sizing implementation?
>
>> Might make sense to also adjust the TreeTableView sizing implementation?
> 
> Yes I think that may be a good idea. True to the idea of specific, narrow 
> commits I've not integrated this into this PR but would be willing to open a 
> new issue / produce a separate PR for TreeTableView.

> @effad Do you have time and interest to take a look at this for the 
> `TreeTableView` as well? Just asking as otherwise I will have a look :)

Sorry for late response (extended easter holidays ...). Yes I could also take a 
look at this for TreeTableView. Is there a pre-existing issue for this? 
Otherwise I'll try and come up with an example and create a new issue...

-

PR: https://git.openjdk.java.net/jfx/pull/757

Integrated: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Wed, 16 Mar 2022 08:20:59 GMT, Robert Lichtenberger  
wrote:

> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below 
> the row.
> In that way the row's style will be used.

This pull request has now been integrated.

Changeset: 18b9e941
Author:Robert Lichtenberger 
Committer: Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jfx/commit/18b9e94131dea0569d0df14ec96aa368a40fcbd0
Stats: 98 lines in 2 files changed: 93 ins; 1 del; 4 mod

8251480: TableColumnHeader: calc of cell width must respect row styling

Reviewed-by: mhanl, aghaisas

-

PR: https://git.openjdk.java.net/jfx/pull/757

Integrated: 8283509: Invisible menus can lead to IndexOutOfBoundsException

[Robert Lichtenberger]

On Tue, 22 Mar 2022 14:42:07 GMT, Robert Lichtenberger  
wrote:

> findSibling adapted to only use visible menus when calculating the
> index.

This pull request has now been integrated.

Changeset: cff84e24
Author:Robert Lichtenberger 
Committer: Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jfx/commit/cff84e24b2de83a57163462fe275ab067034766c
Stats: 34 lines in 2 files changed: 22 ins; 4 del; 8 mod

8283509: Invisible menus can lead to IndexOutOfBoundsException

Reviewed-by: aghaisas

-

PR: https://git.openjdk.java.net/jfx/pull/759

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]

[Robert Lichtenberger]

> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below 
> the row.
> In that way the row's style will be used.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8251480: TableColumnHeader: calc of cell width must respect row styling
  
  Assertion removed.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/757/files
  - new: https://git.openjdk.java.net/jfx/pull/757/files/960e5012..f463ab09

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=757=03
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=757=02-03

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/757.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]

[Robert Lichtenberger]

On Wed, 30 Mar 2022 12:10:19 GMT, Kevin Rushforth  wrote:

>> I found assertions in various other parts of the code (e.g. 
>> `javafx.scene.control.skin.VirtualFlow`) so I assumed this is ok.
>
> Some old code has assertions, but library code should not use them. Please 
> delete them from any new / modified code.

Deleted the assertion.

-

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v3]

[Robert Lichtenberger]

> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below 
> the row.
> In that way the row's style will be used.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8251480: TableColumnHeader: calc of cell width must respect row styling
  
  Test readability improved.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/757/files
  - new: https://git.openjdk.java.net/jfx/pull/757/files/af582570..960e5012

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=757=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=757=01-02

  Stats: 13 lines in 1 file changed: 6 ins; 6 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/757.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]

[Robert Lichtenberger]

On Tue, 29 Mar 2022 20:35:10 GMT, Marius Hanl  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8251480: TableColumnHeader: calc of cell width must respect row styling
>>   
>>   Old behaviour restored for table rows with custom skins.
>>   Unnecessary call to cell.applyCss() removed.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 715:
> 
>> 713: tableRow = createMeasureRow(tv, tableSkin, null);
>> 714: }
>> 715: assert tableRow.getSkin() instanceof SkinBase;
> 
> Not sure if there is some convention about code asserts, maybe some else can 
> tell. At least I never saw one in a PR here.

I found assertions in various other parts of the code (e.g. 
`javafx.scene.control.skin.VirtualFlow`) so I assumed this is ok.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 276:
> 
>> 274: }
>> 275: 
>> 276: private TableRow createSmallRow(TableView 
>> tableView) {
> 
> Minor: Move this method to the other row method below?

You're right. I'll move the method.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
>  line 295:
> 
>> 293: private TableRow createCustomRow(TableView 
>> tableView) {
>> 294: TableRow row = new TableRow<>() {
>> 295: protected javafx.scene.control.Skin createDefaultSkin() {
> 
> Very minor: `javafx.scene.control.` is not needed

You're right. I'll remove this.

-

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException [v2]

[Robert Lichtenberger]

> findSibling adapted to only use visible menus when calculating the
> index.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8251480: TableColumnHeader: calc of cell width must respect row styling
  
  Comment improved.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/759/files
  - new: https://git.openjdk.java.net/jfx/pull/759/files/2198d6bf..6d511a3f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=759=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=759=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/759.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/759/head:pull/759

PR: https://git.openjdk.java.net/jfx/pull/759

Re: RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException

[Robert Lichtenberger]

On Wed, 30 Mar 2022 04:48:30 GMT, Ajit Ghaisas  wrote:

> 



> Only one minor correction needed is in comment.

Right. Just pushed a correction.

-

PR: https://git.openjdk.java.net/jfx/pull/759

Re: RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException

[Robert Lichtenberger]

On Tue, 29 Mar 2022 09:23:24 GMT, Ajit Ghaisas  wrote:

> Have you considered keeping the same while loop in findSibling() method and 
> skipping invisible Menus in it? This is to avoid creating and traversing a 
> new list.

This would not give the correct result. findSibling must return the index in 
the list of **visible** menus, since this is what setFocusedMenuIndex() really 
expects. If we only skip invisible menus we will possibly return an index that 
is out of bounds, which is exactly what this bug is about.

-

PR: https://git.openjdk.java.net/jfx/pull/759

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]

[Robert Lichtenberger]

> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below 
> the row.
> In that way the row's style will be used.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8251480: TableColumnHeader: calc of cell width must respect row styling
  
  Old behaviour restored for table rows with custom skins.
  Unnecessary call to cell.applyCss() removed.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/757/files
  - new: https://git.openjdk.java.net/jfx/pull/757/files/08851726..af582570

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=757=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=757=00-01

  Stats: 74 lines in 2 files changed: 65 ins; 6 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/757.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Wed, 23 Mar 2022 08:15:47 GMT, Marius Hanl  wrote:

>> This fix respects a row factory, if present.
>> It will put the cell that is used to measure the column width as child below 
>> the row.
>> In that way the row's style will be used.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 667:
> 
>> 665: if ((cell.getText() != null && !cell.getText().isEmpty()) 
>> || cell.getGraphic() != null) {
>> 666: tableRow.applyCss();
>> 667: cell.applyCss();
> 
> Just wondering: Is `cell.applyCss();` still needed here?

Right, this is no longer needed. I'll remove it.

-

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Wed, 23 Mar 2022 08:17:38 GMT, Marius Hanl  wrote:

>> This fix respects a row factory, if present.
>> It will put the cell that is used to measure the column width as child below 
>> the row.
>> In that way the row's style will be used.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 653:
> 
>> 651: tableSkin.getChildren().add(tableRow);
>> 652: tableRow.applyCss();
>> 653: ((SkinBase) tableRow.getSkin()).getChildren().add(cell);
> 
> I don't think this is a safe cast. Instead, you probably should do an 
> `instanceof SkinBase` check before

You're right. A tablerow may have been created that returns a custom skin which 
is not necessarily derived from SkinBase. 
However, since we do not have a getChildren() method in such a custom skin we 
will be unable to proceed. We could now abandon altogether (another return) but 
that mean that resizeColumnToFitContent no longer works at all in the presence 
of custom rows. So I think the best solution in that case would be to ignore 
the rowFactory and use a default `new TableRow<>` again to preserve at least 
the old behaviour in such cases.
I'll try and improve the patch.

-

PR: https://git.openjdk.java.net/jfx/pull/757

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Wed, 23 Mar 2022 08:19:41 GMT, Marius Hanl  wrote:

>> This fix respects a row factory, if present.
>> It will put the cell that is used to measure the column width as child below 
>> the row.
>> In that way the row's style will be used.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>  line 650:
> 
>> 648: }
>> 649: Callback, TableRow> rowFactory = 
>> tv.getRowFactory();
>> 650: TableRow tableRow = rowFactory != null ? rowFactory.call(tv) 
>> : new TableRow<>();
> 
> When there is no row factory, we probably should just return like in line 
> 632-633?

Unlike cell factory, which always has to be present, the row factory is 
optional and in fact most tables will not have a row factory. If we return in 
that case, the algorithm will no longer work for these tables.
Compare with `javafx.scene.control.skin.TableViewSkin.createCell()`.

-

PR: https://git.openjdk.java.net/jfx/pull/757

RFR: 8283509: Invisible menus can lead to IndexOutOfBoundsException

[Robert Lichtenberger]

findSibling adapted to only use visible menus when calculating the
index.

-

Commit messages:
 - 8283509: Invisible menus can lead to IndexOutOfBoundsException

Changes: https://git.openjdk.java.net/jfx/pull/759/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=759=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8283509
  Stats: 33 lines in 2 files changed: 22 ins; 4 del; 7 mod
  Patch: https://git.openjdk.java.net/jfx/pull/759.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/759/head:pull/759

PR: https://git.openjdk.java.net/jfx/pull/759

Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

On Wed, 16 Mar 2022 11:46:07 GMT, Marius Hanl  wrote:

> Might make sense to also adjust the TreeTableView sizing implementation?

Yes I think that may be a good idea. True to the idea of specific, narrow 
commits I've not integrated this into this PR but would be willing to open a 
new issue / produce a separate PR for TreeTableView.

-

PR: https://git.openjdk.java.net/jfx/pull/757

RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling

[Robert Lichtenberger]

This fix respects a row factory, if present.
It will put the cell that is used to measure the column width as child below 
the row.
In that way the row's style will be used.

-

Commit messages:
 - 8251480: TableColumnHeader: calc of cell width must respect row styling

Changes: https://git.openjdk.java.net/jfx/pull/757/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=757=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8251480
  Stats: 38 lines in 2 files changed: 34 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/757.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757

PR: https://git.openjdk.java.net/jfx/pull/757

Integrated: 8274137: TableView scrollbar/header misaligned when reloading data

[Robert Lichtenberger]

On Thu, 23 Sep 2021 13:48:44 GMT, Robert Lichtenberger  
wrote:

> This PR fixes JDK-8274137 by removing the optimization from updateHbar() that 
> will no-op the method in case the VirtualFlow is invisible or currently has 
> no scene.
> Since changes to the hBar's value can happen even if the VirtualFlow is not 
> currently visible, the synchronisation between hBar and clipX must happen all 
> the time.
> 
> A test agains VirtualFlow has been added that will fail before the change and 
> pass afterwards.

This pull request has now been integrated.

Changeset: b591912c
Author:Robert Lichtenberger 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/b591912c749edef1e6c1b8509a8ea10e9fe9000f
Stats: 34 lines in 3 files changed: 34 ins; 0 del; 0 mod

8274137: TableView scrollbar/header misaligned when reloading data

Reviewed-by: kcr, aghaisas

-

PR: https://git.openjdk.java.net/jfx/pull/629

Re: RFR: 8274137: TableView scrollbar/header misaligned when reloading data [v3]

[Robert Lichtenberger]

On Mon, 27 Sep 2021 05:27:25 GMT, Robert Lichtenberger  
wrote:

>> This PR fixes JDK-8274137 by removing the optimization from updateHbar() 
>> that will no-op the method in case the VirtualFlow is invisible or currently 
>> has no scene.
>> Since changes to the hBar's value can happen even if the VirtualFlow is not 
>> currently visible, the synchronisation between hBar and clipX must happen 
>> all the time.
>> 
>> A test agains VirtualFlow has been added that will fail before the change 
>> and pass afterwards.
>
> Robert Lichtenberger has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274137: TableView scrollbar/header misaligned when reloading data
>   
>   Remove @Override annotations unrelated to fix.

Ist there anything left for me to do? IIRC someone must now /sponsor this 
change and then I can /integrate it, right?

-

PR: https://git.openjdk.java.net/jfx/pull/629

Re: RFR: 8274137: TableView scrollbar/header misaligned when reloading data [v3]

[Robert Lichtenberger]

> This PR fixes JDK-8274137 by removing the optimization from updateHbar() that 
> will no-op the method in case the VirtualFlow is invisible or currently has 
> no scene.
> Since changes to the hBar's value can happen even if the VirtualFlow is not 
> currently visible, the synchronisation between hBar and clipX must happen all 
> the time.
> 
> A test agains VirtualFlow has been added that will fail before the change and 
> pass afterwards.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8274137: TableView scrollbar/header misaligned when reloading data
  
  Remove @Override annotations unrelated to fix.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/629/files
  - new: https://git.openjdk.java.net/jfx/pull/629/files/03bcd0f3..20156784

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=629=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=629=01-02

  Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/629.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/629/head:pull/629

PR: https://git.openjdk.java.net/jfx/pull/629

Re: RFR: 8274137: TableView scrollbar/header misaligned when reloading data [v2]

[Robert Lichtenberger]

On Fri, 24 Sep 2021 11:22:22 GMT, Kevin Rushforth  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8274137: TableView scrollbar/header misaligned when reloading data
>>   
>>   Alternative fix using additional listeners instead of removing
>>   optimization.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
>  line 3194:
> 
>> 3192: }
>> 3193: 
>> 3194: @Override
> 
> The added `@Override` annotations are unrelated to your fix. Can you please 
> revert them?

By trying to make eclipse remove trailing whitespace I've accidentally 
activated the "Add missing Annotations" option :-(.
Will remove them and update the PR ...

-

PR: https://git.openjdk.java.net/jfx/pull/629

Re: RFR: 8274137: TableView scrollbar/header misaligned when reloading data [v2]

[Robert Lichtenberger]

> This PR fixes JDK-8274137 by removing the optimization from updateHbar() that 
> will no-op the method in case the VirtualFlow is invisible or currently has 
> no scene.
> Since changes to the hBar's value can happen even if the VirtualFlow is not 
> currently visible, the synchronisation between hBar and clipX must happen all 
> the time.
> 
> A test agains VirtualFlow has been added that will fail before the change and 
> pass afterwards.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8274137: TableView scrollbar/header misaligned when reloading data
  
  Alternative fix using additional listeners instead of removing
  optimization.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/629/files
  - new: https://git.openjdk.java.net/jfx/pull/629/files/7d04993f..03bcd0f3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=629=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=629=00-01

  Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/629.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/629/head:pull/629

PR: https://git.openjdk.java.net/jfx/pull/629

RFR: 8274137: TableView scrollbar/header misaligned when reloading data

[Robert Lichtenberger]

This PR fixes JDK-8274137 by removing the optimization from updateHbar() that 
will no-op the method in case the VirtualFlow is invisible or currently has no 
scene.
Since changes to the hBar's value can happen even if the VirtualFlow is not 
currently visible, the synchronisation between hBar and clipX must happen all 
the time.

A test agains VirtualFlow has been added that will fail before the change and 
pass afterwards.

-

Commit messages:
 - 8274137: TableView scrollbar/header misaligned when reloading data.
 - 8274137: TableView scrollbar/header misaligned when reloading data.
 - 8274137: TableView scrollbar/header misaligned when reloading data

Changes: https://git.openjdk.java.net/jfx/pull/629/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=629=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8274137
  Stats: 33 lines in 3 files changed: 32 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/629.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/629/head:pull/629

PR: https://git.openjdk.java.net/jfx/pull/629

Re: Cannot load jpeg images on Fedora 34

[Robert Lichtenberger]

I've analysed the problem described below and found out that:

* When building JavaFX from master-branch, the problem is gone

* When building from tags/16+8, the problem is present

And finally I found https://bugs.openjdk.java.net/browse/JDK-8211362 
(Restrict export of libjpeg symbols from libjavafx_iio.so) together with 
https://git.openjdk.java.net/jfx/pull/442 from Johan Vos.


When I apply the changes in buildSrc/linux.gradle to tags/16+8, the 
problem goes away.


I also found out that Fedora 34 is not the only Linux distribution 
affected by the problem.


So the good news is, the problem will probably vanish with JavaFX 17. 
But the question remains whether the fix should be backported, since a 
lot of JavaFX applications will not work correctly under various modern 
Linux distributions.



Best regards,

Robert


Am 6/2/21 um 12:43 PM schrieb Robert Lichtenberger:

Using this testapplication:

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.image.Image;
import javafx.scene.image.ImageView;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;

public class ImageTest extends Application {

    @Override
    public void start(Stage stage) throws Exception {

        Image img = new 
Image("https://www.synedra.com/_thumbnails_/606_11_robert_lichtenberger.jpg;);

        if (img.isError()) {
            System.out.println(img.getException());
        }
        ImageView imgView = new ImageView(img);

        StackPane stack = new StackPane();
        stack.getChildren().add(imgView);
        Scene scene = new Scene(stack, 640, 480);
        stage.setScene(scene);
        stage.show();
    }

    public static void main(String[] args) {
        launch();
    }
}

And running it with regular JavaFX-16 (downloaded from gluonhq), I get:

[rli@rlimbus javafx]$ java --module-path 
/home/rli/Downloads/javafx-sdk-16/lib --add-modules 
javafx.controls,javafx.fxml ImageTest
java.io.IOException: Wrong JPEG library version: library is 62, caller 
expects 90


and the image will not be loaded.

The same happens for all other regular JavaFX SDKs (even tried LTS 
11.0.12).


When I compile JavaFX myself, it works as expected:

[rli@rlimbus javafx]$ java --module-path 
/home/rli/PWEs/jfx/build/sdk/lib --add-modules 
javafx.controls,javafx.fxml ImageTest


The problem seems to be related to the OS version, because on Fedora 
33 everything works fine.


Other applications are also impacted, e.g. AsciidocFX cannot show 
previews of jpeg images on Fedora 34.


Any hints / ideas appreciated.

Robert

Cannot load jpeg images on Fedora 34

[Robert Lichtenberger]

Using this testapplication:

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.image.Image;
import javafx.scene.image.ImageView;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;

public class ImageTest extends Application {

    @Override
    public void start(Stage stage) throws Exception {

        Image img = new 
Image("https://www.synedra.com/_thumbnails_/606_11_robert_lichtenberger.jpg;);

        if (img.isError()) {
            System.out.println(img.getException());
        }
        ImageView imgView = new ImageView(img);

        StackPane stack = new StackPane();
        stack.getChildren().add(imgView);
        Scene scene = new Scene(stack, 640, 480);
        stage.setScene(scene);
        stage.show();
    }

    public static void main(String[] args) {
        launch();
    }
}

And running it with regular JavaFX-16 (downloaded from gluonhq), I get:

[rli@rlimbus javafx]$ java --module-path 
/home/rli/Downloads/javafx-sdk-16/lib --add-modules 
javafx.controls,javafx.fxml ImageTest
java.io.IOException: Wrong JPEG library version: library is 62, caller 
expects 90


and the image will not be loaded.

The same happens for all other regular JavaFX SDKs (even tried LTS 11.0.12).

When I compile JavaFX myself, it works as expected:

[rli@rlimbus javafx]$ java --module-path 
/home/rli/PWEs/jfx/build/sdk/lib --add-modules 
javafx.controls,javafx.fxml ImageTest


The problem seems to be related to the OS version, because on Fedora 33 
everything works fine.


Other applications are also impacted, e.g. AsciidocFX cannot show 
previews of jpeg images on Fedora 34.


Any hints / ideas appreciated.

Robert

Re: Convenience factories for Border and Background

[Robert Lichtenberger]

I would like this, I am not a big fan of CSS everywhere and this would make
small examples easier to read/write.


Am Do., 22. Apr. 2021 um 15:47 Uhr schrieb Nir Lisker :

> Hi,
>
> Many times when I want to create a simple solid Background or Border, it is
> quite a hassle because of the configurability these classes have:
>
> new Border(new BorderStroke(Color. BLACK, BorderStrokeStyle.SOLID, null,
> null));
> new Background(new BackgroundFill(Color.BLACK, null, null));
>
> I was thinking of adding convenience factory methods
>
> Border.of(Paint stroke), or Border.stroke(Paint stroke)
> Background.of((Paint fill), or Background.fill(Paint fill)
>
> I was wondering if others would like this, or is everyone using CSS anyway?
>
> - Nir
>

Re: Unable to import OpenJFX Build into Eclipse

[Robert Lichtenberger]

I have tried to import OpenJFX to eclipse under Windows today for the first
time and having the exakt same problem.


Am Mo., 17. Aug. 2020 um 13:52 Uhr schrieb Nir Lisker :

> They are not required if you use the command line interface, but I find
> that they do make things easier since I can run the gradle command from
> Eclipse directly.
>
> The instructions mention that if you import through gradle, you will need
> to replace the Eclipse files with the ones in the repo because at this
> point gradle still messes them up.
>
> On Mon, Aug 17, 2020 at 10:43 AM Tom Schindl 
> wrote:
>
> > Hi,
> >
> > Do we really use the Eclipse-Gradle-Tooling now? I think the reasons we
> > checked in all .product/.classpath files is that this did not work in
> > the past.
> >
> > At least my Eclipse install I use for OpenJFX-Development does not have
> > the gradle-tooling installed and things work there just fine.
> >
> > Tom
> >
> > Am 16.08.20 um 21:49 schrieb Nir Lisker:
> > > Hi Andrew,
> > >
> > > I did the same setup only with Ubuntu 18, which shouldn't matter, and I
> > > don't remember having this issue. I can try redoing it next time I boot
> > the
> > > Ubuntu partition.
> > >
> > > What looks odd is that the error references the build directory. What
> > > happens if you clean the project first?
> > >
> > > - Nir
> > >
> > > On Sat, Aug 15, 2020 at 1:55 PM Andrew Waters 
> wrote:
> > >
> > >> Hi All,
> > >>
> > >>
> > >> I'm trying to diagnose a bug in OpenJFX that I've been struggling with
> > >> on and off for almost a year(!) and I decided to build OpenJFX from
> > >> source and use Eclipse to help.  I've built a brand new Ubuntu
> > >> 20.04.1-Desktop system with OpenJDK 14.0.1 and the latest stable
> OpenJFX
> > >> 14.  I've run the Gradle build and run the tests and all looks 100%.
> > >>
> > >>
> > >> When importing the root directory (home/jdkdev/dev/jfx) into Eclipse
> > >> using the gradle import tool (using the wrapper option as
> recommended) I
> > >> get this build path error:
> > >>
> > >>
> > >> Cannot nest
> > >>
> >
> 'home/jdkdev/dev/jfx/modules/javafx.base/build/classes/java/main/javafx.base'
> > >>
> > >> inside library
> > >> 'home/jdkdev/dev/jfx/modules/javafx.base/build/classes/java/main'
> > >>
> > >>
> > >> It seems to be trying to set up a looped import to itself somehow.
> I've
> > >> tried to edit the build path in eclipse but the fields appear to be
> all
> > >> non-editable.  Has anyone any idea as to what the problem is and how
> to
> > >> fix it?  Has anyone recently done a successful import with these
> latest
> > >> levels?
> > >>
> > >>
> > >> As this is the base module none of the other modules compile of course
> > >> so they too may have other problems once the base module is fixed.
> > >>
> > >>
> > >> I tried the "existing projects" import too but that just appears to
> have
> > >> even more problems.
> > >>
> > >>
> > >> Thanks,
> > >>
> > >> Andrew Waters
> > >>
> > >> UK.
> > >>
> > >>
> >
>

Integrated: 8261840: Submenus close to screen borders are no longer repositioned

[Robert Lichtenberger]

On Tue, 23 Feb 2021 15:32:17 GMT, Robert Lichtenberger  
wrote:

> Reverting to the old way of showing the context menu but with application
> of CSS prior to calling prefHeight(-1) / prefWidth(-1) to ensure correct
> size measurement of the menu.

This pull request has now been integrated.

Changeset: 67828aeb
Author:    Robert Lichtenberger 
Committer: Ajit Ghaisas 
URL:   https://git.openjdk.java.net/jfx/commit/67828aeb
Stats: 58 lines in 1 file changed: 5 ins; 40 del; 13 mod

8261840: Submenus close to screen borders are no longer repositioned

Reviewed-by: aghaisas, kcr

-

PR: https://git.openjdk.java.net/jfx/pull/410

Re: RFR: 8261840: Submenus close to screen borders are no longer repositioned

[Robert Lichtenberger]

On Tue, 23 Feb 2021 15:32:17 GMT, Robert Lichtenberger  
wrote:

> Reverting to the old way of showing the context menu but with application
> of CSS prior to calling prefHeight(-1) / prefWidth(-1) to ensure correct
> size measurement of the menu.

I've finally managed to build JavaFX under Windows and tried out 
MenuShowBug.java from JDK-8228363 under different setups:

**JavaFX 16**: Does not contain any fix for JDK-8228363 and thus will always 
show the menu at the wrong position when opened the first time.

**PR-383**: This was the version that used AnchorLocation. When using 125% DPI, 
it works well, but not perfect, i.e. when only one of my two screens is set to 
125% DPI, gaps still appear sometimes. However this solution is unacceptable 
anyway, due to the issue described in JDK-8261840 (for which this PR is the 
fix).

**PR-410** (i.e.: this PR): I can reproduce the problem of @kevinrushforth 
(i.e. gap on first click). Since up to JavaFX 16 the first opening of the menu 
never worked under any DPI setting, this is not a regression but rather another 
issue that we need to look into. 

To be on the safe side, I also checked MenuShowBug without any stylesheets:
* JavaFX 16: Works perfectly with 100% DPI, menu slightly too low with 125% DPI
* PR-410: Works perfectly with 100% DPI, menu slightly too low with 125% DPI
i.e. no difference.

-

PR: https://git.openjdk.java.net/jfx/pull/410

Re: RFR: 8261840: Submenus close to screen borders are no longer repositioned

[Robert Lichtenberger]

On Mon, 15 Mar 2021 09:11:41 GMT, Robert Lichtenberger  
wrote:

>> Reverting to the old way of showing the context menu but with application
>> of CSS prior to calling prefHeight(-1) / prefWidth(-1) to ensure correct
>> size measurement of the menu.
>
> Yes I can try to look into this.
> 
> On 3/12/21 2:59 PM, Kevin Rushforth wrote:
>>
>> This fixes the bug in question, although I see a slight regression in 
>> behavior on Windows with 125% pixel scaling (it doesn't reproduce with 
>> any other scaling value that I tried). With the original test case for 
>> JDK-8228363 <https://bugs.openjdk.java.net/browse/JDK-8228363>, and 
>> |TOP| as the value of |side|, the initial menu is positioned slightly 
>> lower (by a few pixels) than it should be. See the attached image. It 
>> is correct the second and subsequent times the context menu is opened.
>>
>> Given that this new bug only happens with 125% scaling, it seems 
>> likely that it is a preexisting bug, and this fix merely exposes it. 
>> Can you take a look at this? If it is preexisting, then we should file 
>> a follow-on bug for this.
>>
>> ContextMenu-125 
>> <https://user-images.githubusercontent.com/34689748/110947924-9991ce00-82f5-11eb-82d2-6959ef24293f.png>
>>
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub 
>> <https://github.com/openjdk/jfx/pull/410#issuecomment-797506928>, or 
>> unsubscribe 
>> <https://github.com/notifications/unsubscribe-auth/ABCKAQ667DN5I4VPLF37ACTTDIF23ANCNFSM4YCWEUJQ>.
>>

> @effad have you had a chance to look at the rendering issues with 125% HiDPI 
> scaling?

Unfortunately no, not yet. I had some troubles setting up the Windows build and 
a lot of other stuff to do.
But I will try to get things going this wednesday.

-

PR: https://git.openjdk.java.net/jfx/pull/410

Re: RFR: 8261840: Submenus close to screen borders are no longer repositioned

[Robert Lichtenberger]

On Fri, 12 Mar 2021 13:59:07 GMT, Kevin Rushforth  wrote:

>> Marked as reviewed by aghaisas (Reviewer).
>
> This fixes the bug in question, although I see a slight regression in 
> behavior on Windows with 125% pixel scaling (it doesn't reproduce with any 
> other scaling value that I tried). With the original test case for 
> [JDK-8228363](https://bugs.openjdk.java.net/browse/JDK-8228363), and `TOP` as 
> the value of `side`, the initial menu is positioned slightly lower (by a few 
> pixels) than it should be. See the attached image. It is correct the second 
> and subsequent times the context menu is opened.
> 
> Given that this new bug only happens with 125% scaling, it seems likely that 
> it is a preexisting bug, and this fix merely exposes it. Can you take a look 
> at this? If it is preexisting, then we should file a follow-on bug for this.
> 
> ![ContextMenu-125](https://user-images.githubusercontent.com/34689748/110947924-9991ce00-82f5-11eb-82d2-6959ef24293f.png)

Yes I can try to look into this.

On 3/12/21 2:59 PM, Kevin Rushforth wrote:
>
> This fixes the bug in question, although I see a slight regression in 
> behavior on Windows with 125% pixel scaling (it doesn't reproduce with 
> any other scaling value that I tried). With the original test case for 
> JDK-8228363 , and 
> |TOP| as the value of |side|, the initial menu is positioned slightly 
> lower (by a few pixels) than it should be. See the attached image. It 
> is correct the second and subsequent times the context menu is opened.
>
> Given that this new bug only happens with 125% scaling, it seems 
> likely that it is a preexisting bug, and this fix merely exposes it. 
> Can you take a look at this? If it is preexisting, then we should file 
> a follow-on bug for this.
>
> ContextMenu-125 
> 
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub 
> , or 
> unsubscribe 
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/410

RFR: 8261840: Submenus close to screen borders are no longer repositioned

[Robert Lichtenberger]

Reverting to the old way of showing the context menu but with application
of CSS prior to calling prefHeight(-1) / prefWidth(-1) to ensure correct
size measurement of the menu.

-

Commit messages:
 - 8261840: Submenus close to screen borders are no longer repositioned

Changes: https://git.openjdk.java.net/jfx/pull/410/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=410=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261840
  Stats: 58 lines in 1 file changed: 5 ins; 40 del; 13 mod
  Patch: https://git.openjdk.java.net/jfx/pull/410.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/410/head:pull/410

PR: https://git.openjdk.java.net/jfx/pull/410

Re: Make javafx.controls open and community-driven

[Robert Lichtenberger]

I think I can comment from the perspective of a successful user of JavaFX
(we have been running an administration application for our medical
software for over 5 years now) as well as a part-time contributor to the
JavaFX project (with the help of Kevin Rushforth and Ajit Ghaisas my pull
request https://github.com/openjdk/jfx/pull/383 was integrated into JavaFX
a few days ago):
* Yes, the closed bug tracker of OpenJFX is a bit of a nuisance but no, it
is not the biggest obstacle when trying to contribute.
* Contributing is hard because JavaFX is quite complex and there is a very
stringent process in place. As a contributor this can sometimes be
tiresome. As an example, when Kevin Rushforth asked for a better API
comment in the aforementioned pull request, it felt like exxagerated
attention to details. However, when trying to write that better API comment
I discovered another bug in my pull request which would have broken things.
So it might be a difficult process but it pays off in terms of quality. So
as a user of JavaFX I absolutely want the quality and stability it
provides. As a contributor I would sometimes like to have my pull request
integrated more easily. Since I can't have both, I'd rather have reviewers
nitpick at my next pull request (and thus ensuring quality) for a dozen
times than having my application break down from regressions after I update
to the next JavaFX version.
* As a long-time user of JavaFX I would not like for javafx.controls to be
split off and modified in a way that is less stringent than now. Because
one users fix is the regression of another :-).
* I think contributing to JavaFX is about as difficult as contributing to
other projects of the same scale. You can't contribute to Qt easily and
certainly not to the Linux kernel.



Am Mi., 3. Feb. 2021 um 13:24 Uhr schrieb :

> What do you think about decoupling javafx.controls from other modules
> private API? Is it possible? In current situation
> it may be the best solution, because this would allow to fork it. So,
> those who interested could play with fork(s) and
> backport changes when neccessary. It would also allow to develop other
> controls libs, those aren't based of
> javafx.controls, e.g. just for mobile.
>
> Best regards
> Mike
>
> On Tue, 2021-02-02 at 13:40 -0800, Kevin Rushforth wrote:
> > There are two separate issues here. I won't address the first point from
> > the initial poster, other than to say that I understand that the lack of
> > direct write access to JBS for occasional contributors is a pain point.
> > I don't think it is stopping anyone from contributing, though. As for
> > the other part of this point, JavaFX is already on GitHub along with the
> > rest of the JDK, and it's easy enough for someone who is motivated to
> > provide a pull request for a bug fix.
> >
> > The more interesting question is the one about extensibility that others
> > have also responded to. This raises the question of which areas are most
> > in need of new public API to facilitate this. "Just make every internal
> > detail of your implementation public or protected" isn't the right way
> > to frame the discussion. Anything that makes it into the public API is
> > something that needs to be documented, tested, and supported. It
> > requires us to consider what it means to support that API forever; we
> > take compatibility seriously. The main idea is to think in terms of
> > "stewardship" when evolving the JavaFX API.
> >
> > Keeping that in mind, there are certainly areas that could be made
> > easier to extend. Two things in the UI Controls area that were brought
> > up are skins, which are public API with not all of the fields and
> > methods of the skins are public/protected, and behaviors. The former
> > isn't too difficult to address. It requires some amount of thought,
> > discussion, API documentation, implementation, and testing, but doesn't
> > need a whole new design. VirtualFlow has been extended in this fashion
> > to provide missing pieces for third-party controls. If there are others,
> > developers can propose them and we can discuss them. This is definitely
> > an area we would welcome well-thought-out contributions in.
> >
> > The harder area is that of behaviors. For a little background, a public
> > behaviors API was originally part of the proposed new public API for FX
> > in JDK 9, which was done via JEP 253 [1]. It was decoupled from that
> > JEP, but still remains an open enhancement request tracked by
> > JDK-8091189 [2]. That JBS issue has a very long discussion thread about
> > possible approaches for the API. This would be a very large effort, and
> > would require someone with a very good understanding of both UI control
> > design in general and the design and implementation of JavaFX UI
> > Controls in particular to lead the effort. It isn't something that we
> > would accept from an application developer who is just wanting to make
> > their life easier. Again, think of it in terms of API 

Integrated: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Thu, 21 Jan 2021 06:42:19 GMT, Robert Lichtenberger  
wrote:

> By using the anchor location facility of PopupWindows we can avoid 
> miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_ cases serve as "proof" that no regressions are 
> introduces.
> They work before and after the fix is introduced.

This pull request has now been integrated.

Changeset: 6c1a0ca7
Author:Robert Lichtenberger 
Committer: Kevin Rushforth 
URL:   https://git.openjdk.java.net/jfx/commit/6c1a0ca7
Stats: 192 lines in 3 files changed: 164 ins; 14 del; 14 mod

8228363: ContextMenu.show with side=TOP does not work the first time in the 
presence of CSS

Reviewed-by: kcr, aghaisas

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v4]

[Robert Lichtenberger]

> By using the anchor location facility of PopupWindows we can avoid 
> miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_ cases serve as "proof" that no regressions are 
> introduces.
> They work before and after the fix is introduced.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8228363: ContextMenu.show with side=TOP does not work the first time in the 
presence of CSS
  
  Removed unnecessary imports.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/383/files
  - new: https://git.openjdk.java.net/jfx/pull/383/files/b02dcb02..ba803abf

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=383=03
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=02-03

  Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/383.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/383/head:pull/383

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]

[Robert Lichtenberger]

On Wed, 27 Jan 2021 11:21:19 GMT, Ajit Ghaisas  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8228363: ContextMenu.show with side=TOP does not work the first time in 
>> the presence of CSS
>>   
>>   Removed node orientation from API doc.
>>   Removed extra blank lines.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java 
> line 312:
> 
>> 310: if (getItems().size() == 0) return;
>> 311: 
>> getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation());
>> 312: setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT);
> 
> Do we need to handle RTL case here?

I don't think so.
At least we didn't handle the RTL case here until now. The call to 
setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); is only neccessary to 
"reset" the anchor location in case we called show with a Side before.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]

[Robert Lichtenberger]

On Wed, 27 Jan 2021 10:41:25 GMT, Ajit Ghaisas  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8228363: ContextMenu.show with side=TOP does not work the first time in 
>> the presence of CSS
>>   
>>   Removed node orientation from API doc.
>>   Removed extra blank lines.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
>  line 49:
> 
>> 47: import static com.sun.javafx.scene.control.ContextMenuContentShim.*;
>> 48: 
>> 49: import java.io.File;
> 
> These 3 imports are unused. Remove them.

Good catch. I removed the (now unused) imports.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]

[Robert Lichtenberger]

> By using the anchor location facility of PopupWindows we can avoid 
> miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_ cases serve as "proof" that no regressions are 
> introduces.
> They work before and after the fix is introduced.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8228363: ContextMenu.show with side=TOP does not work the first time in the 
presence of CSS
  
  Removed node orientation from API doc.
  Removed extra blank lines.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/383/files
  - new: https://git.openjdk.java.net/jfx/pull/383/files/67a7da14..b02dcb02

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=383=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=01-02

  Stats: 8 lines in 1 file changed: 0 ins; 6 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/383.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/383/head:pull/383

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]

[Robert Lichtenberger]

On Fri, 22 Jan 2021 18:54:35 GMT, Kevin Rushforth  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8228363: ContextMenu.show with side=TOP does not work the first time in 
>> the presence of CSS
>>   
>>   Corrections as per Kevin Rushforth's comments.
>>   Also added two more test cases that test right-to-left node orientation.
>>   Fixed the implementation and the API documentation.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java 
> line 237:
> 
>> 235:  * NodeOrientation.RIGHT_TO_LEFT is set.
>> 236:  * Using NodeOrientation.RIGHT_TO_LEFT will also "mirror" the 
>> meaning of Side.LEFT and
>> 237:  * Side.RIGHT respectively.
> 
> We don't document the effect of node orientation in other controls or in 
> charts, so I wouldn't want to mention it here. Instead you can document the 
> behavior assuming the default effective orientation of `LEFT_TO_RIGHT` 
> (without mentioning it).
> 
> You could make the case that we should document more precisely the effect or 
> NodeOrientation, but that would be a large task, and not something I would 
> want to do for an isolated control in the course of a bug fix (and it would 
> require a CSR).
> 
> The rest of the doc changes look good and don't need a CSR.

Thanks for clarifying. I have adapted the documentation accordingly.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]

[Robert Lichtenberger]

On Fri, 22 Jan 2021 19:01:13 GMT, Kevin Rushforth  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8228363: ContextMenu.show with side=TOP does not work the first time in 
>> the presence of CSS
>>   
>>   Corrections as per Kevin Rushforth's comments.
>>   Also added two more test cases that test right-to-left node orientation.
>>   Fixed the implementation and the API documentation.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java 
> line 302:
> 
>> 300: 
>> 301: 
>> 302: 
> 
> Minor: there should only be one blank line here.

Agreed. Additional lines removed.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]

[Robert Lichtenberger]

> By using the anchor location facility of PopupWindows we can avoid 
> miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_ cases serve as "proof" that no regressions are 
> introduces.
> They work before and after the fix is introduced.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8228363: ContextMenu.show with side=TOP does not work the first time in the 
presence of CSS
  
  Corrections as per Kevin Rushforth's comments.
  Also added two more test cases that test right-to-left node orientation.
  Fixed the implementation and the API documentation.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/383/files
  - new: https://git.openjdk.java.net/jfx/pull/383/files/48d6453a..67a7da14

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=383=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=00-01

  Stats: 123 lines in 3 files changed: 87 ins; 29 del; 7 mod
  Patch: https://git.openjdk.java.net/jfx/pull/383.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/383/head:pull/383

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]

[Robert Lichtenberger]

On Fri, 22 Jan 2021 11:02:29 GMT, Robert Lichtenberger  
wrote:

>> While trying to come up with a good documentation I've detected a real 
>> change in behaviour in connection with the NodeOrientation of the anchor 
>> node.
>> Although this has never been documented, when NodeOrientation.RIGHT_TO_LEFT 
>> is set, the context menu was right-aligned for Side=TOP and Side=BOTTOM. 
>> Since I assume we don't want to change this behaviour I would now document 
>> it and adapt my patch accordingly. I've already written a test case like 
>> this:
>> @Test public void test_position_withOrientation() throws 
>> InterruptedException {
>> ContextMenu cm = createContextMenu(false);
>> anchorBtn.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT);
>> cm.show(anchorBtn, Side.TOP, 0, 0);
>> 
>> Bounds anchorBounds = 
>> anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
>> Node cmNode = cm.getScene().getRoot();
>> Bounds cmBounds = 
>> cm.getScene().getRoot().localToScreen(cmNode.getLayoutBounds());
>> 
>> assertEquals(anchorBounds.getMaxX(), cmBounds.getMaxX(), 0.0);
>> assertEquals(anchorBounds.getMinY(), cmBounds.getMaxY(), 0.0);
>> anchorBtn.setNodeOrientation(NodeOrientation.LEFT_TO_RIGHT);
>> }
>> which passes with the old implementation but (currently) fails with the new 
>> implementation.
>
> Oh wow. Further experimentation has shown, that if 
> NodeOrientation.RIGHT_TO_LEFT is used on the anchor, then Side.LEFT used to 
> make the menu appear on the **right** hand side and Side.RIGHT used to make 
> the menu appear on the **left** hand side of the button, which really 
> surprises me, since I didn't expect the side parameter to be relative.

Ok, I just pushed all the changes necessary to keep the old behaviour for 
right-to-left node orientation.
If you would rather have the behaviour changed (it doesn't seem very intuitive 
to me...) just tell me and I can change the implementation / tests.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Fri, 22 Jan 2021 10:53:00 GMT, Robert Lichtenberger  
wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java 
>> line 241:
>> 
>>> 239:  * the {@code ContextMenu} such that its top-left (0,0) position 
>>> would be attached
>>> 240:  * to the top-right position of the anchor.
>>> 241:  * 
>> 
>> I think it is worth adding a short replacement for this, explaining how the 
>> side parameter and delta values affect the location of the popup.
>
> While trying to come up with a good documentation I've detected a real change 
> in behaviour in connection with the NodeOrientation of the anchor node.
> Although this has never been documented, when NodeOrientation.RIGHT_TO_LEFT 
> is set, the context menu was right-aligned for Side=TOP and Side=BOTTOM. 
> Since I assume we don't want to change this behaviour I would now document it 
> and adapt my patch accordingly. I've already written a test case like this:
> @Test public void test_position_withOrientation() throws 
> InterruptedException {
> ContextMenu cm = createContextMenu(false);
> anchorBtn.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT);
> cm.show(anchorBtn, Side.TOP, 0, 0);
> 
> Bounds anchorBounds = 
> anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
> Node cmNode = cm.getScene().getRoot();
> Bounds cmBounds = 
> cm.getScene().getRoot().localToScreen(cmNode.getLayoutBounds());
> 
> assertEquals(anchorBounds.getMaxX(), cmBounds.getMaxX(), 0.0);
> assertEquals(anchorBounds.getMinY(), cmBounds.getMaxY(), 0.0);
> anchorBtn.setNodeOrientation(NodeOrientation.LEFT_TO_RIGHT);
> }
> which passes with the old implementation but (currently) fails with the new 
> implementation.

Oh wow. Further experimentation has shown, that if 
NodeOrientation.RIGHT_TO_LEFT is used on the anchor, then Side.LEFT used to 
make the menu appear on the **right** hand side and Side.RIGHT used to make the 
menu appear on the **left** hand side of the button, which really suprises me, 
since I didn't expect the side parameter to be relative.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Thu, 21 Jan 2021 23:07:23 GMT, Kevin Rushforth  wrote:

>> By using the anchor location facility of PopupWindows we can avoid 
>> miscalculation of the
>> menu's height entirely.
>> This fix also cleans up some documentation issues.
>> This fix introduces tests that check the correct positioning (test_position_)
>> test_position_withCSS reproduces the problem that is fixed with this patch.
>> The other test_position_ cases serve as "proof" that no regressions are 
>> introduces.
>> They work before and after the fix is introduced.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java 
> line 241:
> 
>> 239:  * the {@code ContextMenu} such that its top-left (0,0) position 
>> would be attached
>> 240:  * to the top-right position of the anchor.
>> 241:  * 
> 
> I think it is worth adding a short replacement for this, explaining how the 
> side parameter and delta values affect the location of the popup.

While trying to come up with a good documentation I've detected a real change 
in behaviour in connection with the NodeOrientation of the anchor node.
Although this has never been documented, when NodeOrientation.RIGHT_TO_LEFT is 
set, the context menu was right-aligned for Side=TOP and Side=BOTTOM. 
Since I assume we don't want to change this behaviour I would now document it 
and adapt my patch accordingly. I've already written a test case like this:
@Test public void test_position_withOrientation() throws 
InterruptedException {
ContextMenu cm = createContextMenu(false);
anchorBtn.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT);
cm.show(anchorBtn, Side.TOP, 0, 0);

Bounds anchorBounds = 
anchorBtn.localToScreen(anchorBtn.getLayoutBounds());
Node cmNode = cm.getScene().getRoot();
Bounds cmBounds = 
cm.getScene().getRoot().localToScreen(cmNode.getLayoutBounds());

assertEquals(anchorBounds.getMaxX(), cmBounds.getMaxX(), 0.0);
assertEquals(anchorBounds.getMinY(), cmBounds.getMaxY(), 0.0);
anchorBtn.setNodeOrientation(NodeOrientation.LEFT_TO_RIGHT);
}
which passes with the old implementation but (currently) fails with the new 
implementation.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Thu, 21 Jan 2021 23:15:09 GMT, Kevin Rushforth  wrote:

>> By using the anchor location facility of PopupWindows we can avoid 
>> miscalculation of the
>> menu's height entirely.
>> This fix also cleans up some documentation issues.
>> This fix introduces tests that check the correct positioning (test_position_)
>> test_position_withCSS reproduces the problem that is fixed with this patch.
>> The other test_position_ cases serve as "proof" that no regressions are 
>> introduces.
>> They work before and after the fix is introduced.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
>  line 686:
> 
>> 684: private String createStylesheet() {
>> 685: try {
>> 686: File f = 
>> File.createTempFile("test_position_showOnTopWithCSS", ".css");
> 
> There is no need to create a temporary file for this css resource, since its 
> contents are fixed. Instead, you should add 
> `test_position_showOnTopWithCSS.css` to the repo under 
> [`modules/javafx.controls/src/test/resources/test/javafx/scene/control/`](https://github.com/openjdk/jfx/tree/master/modules/javafx.controls/src/test/resources/test/javafx/scene/control).
>  Then you can just do this:
> 
> return 
> ContextMenuTest.class.getResource("test_position_showOnTopWithCSS.css").toExternalForm();

Thanks for pointing this out. I took the example from the bug entry (where it 
was nice to have everything in one file). But in the test it is of course much 
more elegant to use a .css file from the resources.
Adapted the test accordingly.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Thu, 21 Jan 2021 23:11:44 GMT, Kevin Rushforth  wrote:

>> By using the anchor location facility of PopupWindows we can avoid 
>> miscalculation of the
>> menu's height entirely.
>> This fix also cleans up some documentation issues.
>> This fix introduces tests that check the correct positioning (test_position_)
>> test_position_withCSS reproduces the problem that is fixed with this patch.
>> The other test_position_ cases serve as "proof" that no regressions are 
>> introduces.
>> They work before and after the fix is introduced.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
>  line 646:
> 
>> 644: @Test public void test_position_showOnScreen() {
>> 645: ContextMenu cm = createContextMenu(false);
>> 646: cm.show(anchorBtn,100, 100);
> 
> Minor: missing space after the `,`

Space inserted.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java
>  line 649:
> 
>> 647: 
>> 648: assertEquals(cm.getAnchorX(), 100, 0.0);
>> 649: assertEquals(cm.getAnchorY(), 100, 0.0);
> 
> The expected and actual values are backwards (expected should be first).

Swapped positions.

-

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Thu, 21 Jan 2021 06:37:59 GMT, Robert Lichtenberger  
wrote:

>> Regarding the spec change, I was thinking of this section, which you removed:
>> 
>>  * To clarify the purpose of the {@code hpos} and {@code vpos} 
>> parameters,
>>  * consider that they are relative to the anchor node. As such, a {@code 
>> hpos}
>>  * and {@code vpos} of {@code CENTER} would mean that the ContextMenu 
>> appears
>>  * on top of the anchor, with the (0,0) position of the {@code 
>> ContextMenu}
>>  * positioned at (0,0) of the anchor. A {@code hpos} of right would then 
>> shift
>>  * the {@code ContextMenu} such that its top-left (0,0) position would 
>> be attached
>>  * to the top-right position of the anchor.
>> 
>> As you pointed out, the reference to `hpos` and `vpos` is incorrect, but 
>> unless I'm missing something, it still seems like the concept still needs to 
>> be described.
>
>> Regarding the spec change, I was thinking of this section, which you removed:
>> 
>> ```
>>  * To clarify the purpose of the {@code hpos} and {@code vpos} 
>> parameters,
>>  * consider that they are relative to the anchor node. As such, a {@code 
>> hpos}
>>  * and {@code vpos} of {@code CENTER} would mean that the ContextMenu 
>> appears
>>  * on top of the anchor, with the (0,0) position of the {@code 
>> ContextMenu}
>>  * positioned at (0,0) of the anchor. A {@code hpos} of right would then 
>> shift
>>  * the {@code ContextMenu} such that its top-left (0,0) position would 
>> be attached
>>  * to the top-right position of the anchor.
>> ```
>> 
>> As you pointed out, the reference to `hpos` and `vpos` is incorrect, but 
>> unless I'm missing something, it still seems like the concept still needs to 
>> be described.
> 
> I assume that a very long time ago, there were hpos and vpos parameters for 
> this method. And that really required an explanation, because then you could 
> e.g. specify hpos=RIGHT and vpos=BOTTOM, which would give a very strange 
> position for the context menu (the top left corner of the menu would be in 
> the bottom right corner of the button).
> It also was very counter-intuitive that giving CENTER/CENTER for hpos/vpos 
> would put the top left corner of the menu at the top left corner of the 
> button.
> However all this confusion was ended when Side was used instead of HPos/VPos. 
> At that very moment all the confusing cases went away. No more BOTTOM/RIGHT 
> or CENTER/CENTER placement.
> So I think we can really just remove the (obsolete) explanation of hPos/vPos.

I've just "recreated" this PR as https://github.com/openjdk/jfx/pull/383 which 
is based on a separate branch as suggested.

-

PR: https://git.openjdk.java.net/jfx/pull/381

RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

By using the anchor location facility of PopupWindows we can avoid 
miscalculation of the
menu's height entirely.
This fix also cleans up some documentation issues.
This fix introduces tests that check the correct positioning (test_position_)
test_position_withCSS reproduces the problem that is fixed with this patch.
The other test_position_ cases serve as "proof" that no regressions are 
introduces.
They work before and after the fix is introduced.

-

Commit messages:
 - 8228363: ContextMenu.show with side=TOP does not work the first time in the 
presence of CSS
 - Merge pull request #1 from openjdk/master
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - 8232524: Test cleanup: terminate background thread upon failure.
 - 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - ... and 1 more: https://git.openjdk.java.net/jfx/compare/d8bb41d1...48d6453a

Changes: https://git.openjdk.java.net/jfx/pull/383/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=383=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8228363
  Stats: 144 lines in 2 files changed: 115 ins; 14 del; 15 mod
  Patch: https://git.openjdk.java.net/jfx/pull/383.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/383/head:pull/383

PR: https://git.openjdk.java.net/jfx/pull/383

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Wed, 20 Jan 2021 16:49:35 GMT, Kevin Rushforth  wrote:

> Regarding the spec change, I was thinking of this section, which you removed:
> 
> ```
>  * To clarify the purpose of the {@code hpos} and {@code vpos} parameters,
>  * consider that they are relative to the anchor node. As such, a {@code 
> hpos}
>  * and {@code vpos} of {@code CENTER} would mean that the ContextMenu 
> appears
>  * on top of the anchor, with the (0,0) position of the {@code 
> ContextMenu}
>  * positioned at (0,0) of the anchor. A {@code hpos} of right would then 
> shift
>  * the {@code ContextMenu} such that its top-left (0,0) position would be 
> attached
>  * to the top-right position of the anchor.
> ```
> 
> As you pointed out, the reference to `hpos` and `vpos` is incorrect, but 
> unless I'm missing something, it still seems like the concept still needs to 
> be described.

I assume that a very long time ago, there were hpos and vpos parameters for 
this method. And that really required an explanation, because then you could 
e.g. specify hpos=RIGHT and vpos=BOTTOM, which would give a very strange 
position for the context menu (the top left corner of the menu would be in the 
bottom right corner of the button).
It also was very counter-intuitive that giving CENTER/CENTER for hpos/vpos 
would put the top left corner of the menu at the top left corner of the button.
However all this confusion was ended when Side was used instead of HPos/VPos. 
At that very moment all the confusing cases went away. No more BOTTOM/RIGHT or 
CENTER/CENTER placement.
So I think we can really just remove the (obsolete) explanation of hPos/vPos.

-

PR: https://git.openjdk.java.net/jfx/pull/381

Integrated: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Wed, 20 Jan 2021 11:57:04 GMT, Robert Lichtenberger  
wrote:

> By using the anchor location facility of PopupWindows we can avoid 
> miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_*)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_* cases serve as "proof" that no regressions are 
> introduces.
> They work before and after the fix is introduced.

This pull request has now been integrated.

Changeset: d8bb41d1
Author:Jose Pereda 
URL:   https://git.openjdk.java.net/jfx/commit/d8bb41d1
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod

8165749: java.lang.RuntimeException: dndGesture.dragboard is null in dragDrop

Reviewed-by: kcr, arapte

-

PR: https://git.openjdk.java.net/jfx/pull/381

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v2]

[Robert Lichtenberger]

> By using the anchor location facility of PopupWindows we can avoid 
> miscalculation of the
> menu's height entirely.
> This fix also cleans up some documentation issues.
> This fix introduces tests that check the correct positioning (test_position_*)
> test_position_withCSS reproduces the problem that is fixed with this patch.
> The other test_position_* cases serve as "proof" that no regressions are 
> introduces.
> They work before and after the fix is introduced.

Robert Lichtenberger 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.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/381/files
  - new: https://git.openjdk.java.net/jfx/pull/381/files/48d6453a..d8bb41d1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=381=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=381=00-01

  Stats: 144 lines in 2 files changed: 14 ins; 115 del; 15 mod
  Patch: https://git.openjdk.java.net/jfx/pull/381.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/381/head:pull/381

PR: https://git.openjdk.java.net/jfx/pull/381

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Wed, 20 Jan 2021 16:37:04 GMT, Kevin Rushforth  wrote:

> This changes the specification in a way that will require prior discussion,. 
> It also will need a CSR.
My hope is that it really doesn't change the specification in any way. All it 
should do is fix the bug.
What part of the spec do you think this would change?

-

PR: https://git.openjdk.java.net/jfx/pull/381

Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

On Wed, 20 Jan 2021 16:39:45 GMT, Kevin Rushforth  wrote:

> I recommend that you follow the instructions in the earlier comment about 
> pushing these changes to a new branch, resetting your master branch, and 
> creating a new PR from your new branch.

Ah yes, will try to do so tomorrow.

-

PR: https://git.openjdk.java.net/jfx/pull/381

RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS

[Robert Lichtenberger]

By using the anchor location facility of PopupWindows we can avoid 
miscalculation of the
menu's height entirely.
This fix also cleans up some documentation issues.
This fix introduces tests that check the correct positioning (test_position_*)
test_position_withCSS reproduces the problem that is fixed with this patch.
The other test_position_* cases serve as "proof" that no regressions are 
introduces.
They work before and after the fix is introduced.

-

Commit messages:
 - 8228363: ContextMenu.show with side=TOP does not work the first time in the 
presence of CSS
 - Merge pull request #1 from openjdk/master
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - Merge remote-tracking branch 'upstream/master'
 - 8232524: Test cleanup: terminate background thread upon failure.
 - 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - ... and 1 more: https://git.openjdk.java.net/jfx/compare/d8bb41d1...48d6453a

Changes: https://git.openjdk.java.net/jfx/pull/381/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=381=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8228363
  Stats: 144 lines in 2 files changed: 115 ins; 14 del; 15 mod
  Patch: https://git.openjdk.java.net/jfx/pull/381.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/381/head:pull/381

PR: https://git.openjdk.java.net/jfx/pull/381

[jfx15] Integrated: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Thu, 5 Mar 2020 16:01:10 GMT, Robert Lichtenberger  
wrote:

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

This pull request has now been integrated.

Changeset: e2d1c021
Author:Robert Lichtenberger 
Committer: Kevin Rushforth 
URL:   https://git.openjdk.java.net/jfx/commit/e2d1c021
Stats: 219 lines in 4 files changed: 18 ins; 172 del; 29 mod

8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException

Reviewed-by: aghaisas, kcr

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [jfx15] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Thu, 2 Jul 2020 23:39:58 GMT, Kevin Rushforth  wrote:

>> seeing that you are working at it (and still without too close a look, sry 
>> ;) - we need more tests about the
>> notifications of all properties involved: text, selectedText, indexRange 
>> (anything else?).  The things to test are
>> count and old/new value. F.i. something like:
>> List values = new ArrayList();
>> textField.selectedTextProperty().addListener((src, ov, nv) -> {
>>  list.addAll(ov, nv);
>> }
>> // do stuff
>> assertEquals(2, values.size());
>> assertEquals(expectedOldValue, values.get(0));
>> assertEquals(expectedNewValue, values.get(1));
>
> I need to do some more testing, but this looks like the right approach.
> 
> This fix might be a candidate for JavaFX 15, so I recommend to _not_ merge 
> the master branch. If I don't spot anything
> of concern during the review, then I will ask you to retarget your PR to the 
> `jfx15` branch.

I changed the base branch of this PR by editing it in the github GUI. Is this 
all I need to do to retarget it for jfx15?

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [jfx15] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException [v8]

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  Fix check condition in RTest.
  Replace underscore based variable names.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/bf9e08d7..3dd80c78

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.07
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.06-07

  Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [jfx15] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException [v7]

[Robert Lichtenberger]

On Thu, 9 Jul 2020 20:29:51 GMT, Kevin Rushforth  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
>> StringOutOfBoundsException
>>   
>>   Clamping is no longer needed, removed it.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TextFieldTest.java
>  line 475:
> 
>> 474: assertEquals(4, txtField.getSelection().getStart());
>> 475: assertEquals(4, txtField.getSelection().getStart());
>> 476: }
> 
> Did you mean to check `getStart()` twice?

No I didn't. Good catch. Fixed in next commit.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java
>  line 1924:
> 
>> 1923: textInput.selectionProperty().addListener((__, ___, selection) 
>> -> selectionLog.append("|" +
>> selection.getStart() + "," + selection.getEnd())); 1924: 
>> textInput.textProperty().addListener((__, ___,
>> text) -> textLog.append("|" + text)); 1925:
> 
> The pattern of using multiple underscores for unused variables isn't one we 
> use. I recommend using something more
> common like `obs` and `o` (or similar).

Replaced the variable names in next commit. It looks like "observable" / 
"oldValue" / "newValue" are almost exclusively
used in the current code base so I changed them to that.

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException [v6]

[Robert Lichtenberger]

On Tue, 30 Jun 2020 23:05:40 GMT, Kevin Rushforth  wrote:

>> Robert Lichtenberger has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
>> StringOutOfBoundsException
>>   
>>   Move replaceSelectionAtEndWithListener test to general test class.
>>   Add a more general test for selection/text properties and 
>> replace/undo/redo operations.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java
>  line 186:
> 
>> 185: int length = txt.length();
>> 186: if (end > start + length) end = length;
>> 187: if (start > length-1) start = end = 0;
> 
> As I mentioned in [this comment in PR 
> #73](https://github.com/openjdk/jfx/pull/73#discussion_r376528686), I think 
> this
> test is wrong. The value of `start` has no impact on whether `end` is out of 
> range. So... shouldn't this simply be `if
> (end > length) end = length;` ?

You're right. The whole point of the fix was to remove the need for any 
clamping by preventing the "in-between" updates
of the selected text property. I've checked that the condition in line 186 is 
never fulfilled now anymore and removed
it. I've also removed the clamping in the line below, because the only case 
where start > length-1 is now that start ==
length in which case also end == length and txt.substring(length, length) will 
be just as empty as txt.substring(0, 0).

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException [v7]

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  Clamping is no longer needed, removed it.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/660335bf..bf9e08d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.06
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.05-06

  Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Re: Test style questions

[Robert Lichtenberger]

Thanks for the guidance. Since my additional tests happen to test against a
regression that was actually happening I'll keep them in my PR. If the
reviewer finds them superfluous I can still remove them.


Am Di., 16. Juni 2020 um 16:17 Uhr schrieb Kevin Rushforth <
kevin.rushfo...@oracle.com>:

> 1. It's a judgment call. I tend to like fairly fine-grained tests as
> long as they are (at least mostly) independent. If the test is "do A",
> "verify state", "do B", "verify state" and B is something that you need
> to do after A, then having them in the same test is better.
>
> 2. It depends. If the additional tests are related to the area you are
> fixing and you are worried that the missing tests could lead to bugs if
> additional changes were made in the area you are fixing, then I think
> it's fine to add the new tests. If it's just a case where the test
> coverage is poor, it might make sense for a follow-on test bug to add
> new tests.
>
> I know that this isn't a definitive answer, but hopefully it will
> provide some general guidance.
>
> -- Kevin
>
>
> On 6/15/2020 10:21 PM, Robert Lichtenberger wrote:
> > While discussing my Pull Request ([1]) for JDK-8176270 ([2]) with
> Jeanette
> > Winzenburg I came across two questions that I would like to ask the
> > community regarding test cases:
> >
> > 1) How fine-grained to we want tests to be? Is it ok to test two
> (somewhat
> > similiar) things at once or should a separate test case be written? e.g.
> > this test case of mine:
> >  @Test public void replaceSelectionAtEndWithListener() {
> >  StringBuilder selectedTextLog = new StringBuilder();
> >  StringBuilder selectionLog = new StringBuilder();
> >  textInput.setText("x xxx");
> >  textInput.selectRange(2, 5);
> >  textInput.selectedTextProperty().addListener((__, ___,
> selection)
> > -> selectedTextLog.append("|" + selection));
> >  textInput.selectionProperty().addListener((__, ___, selection)
> ->
> > selectionLog.append("|" + selection.getStart() + "," +
> selection.getEnd()));
> >  textInput.replaceSelection("a");
> >  assertEquals("|", selectedTextLog.toString());
> >  assertEquals("|3,3", selectionLog.toString());
> >  assertEquals("x a", textInput.getText());
> >  }
> > Will test the selectedTextProperty AND the selectionProperty at the same
> > time. Is this acceptable/desireable or should the test case be split into
> > two separate tests?
> >
> > 2) When fixing bugs, is it ok to not only provide a test case that will
> > fail before the fix and run successfully after the fix but also provide
> > additional test cases with the intention of preventing regressions?
> Ideally
> > of course such tests should already exist but what if they are not.
> > In my case I wanted to add a test case to "prove" that redo() will work
> in
> > the presence of a shortened text and I would also like to have a general
> > test case about selection properties and text property. What's the
> general
> > rule here?
> >
> > Best regards,
> > Robert
> >
> > [1] https://github.com/openjdk/jfx/pull/138
> > [2] https://bugs.openjdk.java.net/browse/JDK-8176270
>
>

Re: [Rev 05] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  Move replaceSelectionAtEndWithListener test to general test class.
  Add a more general test for selection/text properties and replace/undo/redo 
operations.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/533774fe..660335bf

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.05
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.04-05

  Stats: 71 lines in 3 files changed: 44 ins; 27 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Test style questions

[Robert Lichtenberger]

While discussing my Pull Request ([1]) for JDK-8176270 ([2]) with Jeanette
Winzenburg I came across two questions that I would like to ask the
community regarding test cases:

1) How fine-grained to we want tests to be? Is it ok to test two (somewhat
similiar) things at once or should a separate test case be written? e.g.
this test case of mine:
@Test public void replaceSelectionAtEndWithListener() {
StringBuilder selectedTextLog = new StringBuilder();
StringBuilder selectionLog = new StringBuilder();
textInput.setText("x xxx");
textInput.selectRange(2, 5);
textInput.selectedTextProperty().addListener((__, ___, selection)
-> selectedTextLog.append("|" + selection));
textInput.selectionProperty().addListener((__, ___, selection) ->
selectionLog.append("|" + selection.getStart() + "," + selection.getEnd()));
textInput.replaceSelection("a");
assertEquals("|", selectedTextLog.toString());
assertEquals("|3,3", selectionLog.toString());
assertEquals("x a", textInput.getText());
}
Will test the selectedTextProperty AND the selectionProperty at the same
time. Is this acceptable/desireable or should the test case be split into
two separate tests?

2) When fixing bugs, is it ok to not only provide a test case that will
fail before the fix and run successfully after the fix but also provide
additional test cases with the intention of preventing regressions? Ideally
of course such tests should already exist but what if they are not.
In my case I wanted to add a test case to "prove" that redo() will work in
the presence of a shortened text and I would also like to have a general
test case about selection properties and text property. What's the general
rule here?

Best regards,
Robert

[1] https://github.com/openjdk/jfx/pull/138
[2] https://bugs.openjdk.java.net/browse/JDK-8176270

Re: Build Problems on Linux

[Robert Lichtenberger]

Please ignore my previous mail. I forgot to update my fork of OpenJFX so I
was operating on a Mid-March version. After really updating everything
worked fine.

Am Mo., 15. Juni 2020 um 10:12 Uhr schrieb Robert Lichtenberger <
r.lichtenber...@gmail.com>:

> I haven't built in a while but today I pulled the current master branch
> and tried to build OpenJFX.
> Unfortunately, the task :graphics:ccLinuxGlassGlassgtk2 fails like this:
>
> :graphics:ccLinuxGlassGlassgtk2 (Thread[Daemon worker Thread 2,5,main])
> started.
> Starting process 'command 'gcc''. Working directory:
> /home/rli/PWEs/jfx/modules/javafx.graphics Command: gcc
> -fno-strict-aliasing -fPIC -fno-omit-frame-pointer -fstack-protector
> -Wextra -Wall -Wformat-security -Wno-unused -Wno-parentheses
> -Werror=implicit-function-declaration -Werror=trampolines
> -I/usr/java/jdk-12.0.2+10/include -I/usr/java/jdk-12.0.2+10/include/linux
> -c -ffunction-sections -fdata-sections -O2 -DNDEBUG -I/usr/include/gtk-2.0
> -I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0
> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
> -I/usr/include/harfbuzz -I/usr/include/fribidi -I/usr/include/freetype2
> -I/usr/include/libpng16 -I/usr/include/cairo -I/usr/include/pixman-1
> -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libmount -I/usr/include/blkid
> -I/usr/include/atk-1.0 -pthread
>  
> -I/home/rli/PWEs/jfx/modules/javafx.graphics/build/gensrc/headers/javafx.graphics
> -o
> /home/rli/PWEs/jfx/modules/javafx.graphics/build/native/glass/linux/glassgtk2/wrapped.obj
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c
> Successfully started process 'command 'gcc''
> gcc: error: : No such file or directory
> ... repeats 17 times for different files ...
>
> > Task :graphics:ccLinuxGlassGlassgtk2 FAILED
> Caching disabled for task ':graphics:ccLinuxGlassGlassgtk2' because:
>   Build cache is disabled
> Task ':graphics:ccLinuxGlassGlassgtk2' is not up-to-date because:
>   Task has failed previously.
> Compiling native files:
> [/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassView.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassCommonDialogs.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassRobot.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassPixels.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassDnDClipboard.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassCursor.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassWindow.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassSystemClipboard.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_evloop.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassTimer.cpp,
> /home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_screen.cpp]
> :graphics:ccLinuxGlassGlassgtk2 (Thread[Daemon worker Thread 2,5,main])
> completed. Took 0.06 secs.
>
> FAILURE: Build failed with an exception.
>
> * What went wrong:
> Execution failed for task ':graphics:ccLinuxGlassGlassgtk2'.
> > java.util.concurrent.ExecutionException:
> org.gradle.process.internal.ExecException: Process 'command 'gcc'' finished
> with non-zero exit value 1
>
> * Try:
> Run with --stacktrace option to get the stack trace. Run with --debug
> option to get more log output. Run with --scan to get full insights.
>
> * Get more help at https://help.gradle.org
>
> Deprecated Gradle features were used in this build, making it incompatible
> with Gradle 7.0.
> Use '--warning-mode all' to show the individual deprecation warnings.
> See
> https://docs.gradle.org/6.0/userguide/command_line_interface.html#sec:command_line_warnings
>
> BUILD FAILED in 1s
>
> When I execute the command manually:
> gcc -fno-strict-aliasing -fPIC -fno-omit-frame-pointer -fstack-protector
> -Wextra -Wall -Wformat-security -Wno-unused -Wno-parentheses
> -Werror=implicit-function-declaration -Werro

Re: [Rev 04] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  Let Exceptions fail TextInputControlTest
  Fix undo/redo by delaying selectedText update
  Add a test for redo

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/6f10355d..533774fe

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.04
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.03-04

  Stats: 65 lines in 2 files changed: 47 ins; 0 del; 18 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Build Problems on Linux

[Robert Lichtenberger]

I haven't built in a while but today I pulled the current master branch and
tried to build OpenJFX.
Unfortunately, the task :graphics:ccLinuxGlassGlassgtk2 fails like this:

:graphics:ccLinuxGlassGlassgtk2 (Thread[Daemon worker Thread 2,5,main])
started.
Starting process 'command 'gcc''. Working directory:
/home/rli/PWEs/jfx/modules/javafx.graphics Command: gcc
-fno-strict-aliasing -fPIC -fno-omit-frame-pointer -fstack-protector
-Wextra -Wall -Wformat-security -Wno-unused -Wno-parentheses
-Werror=implicit-function-declaration -Werror=trampolines
-I/usr/java/jdk-12.0.2+10/include -I/usr/java/jdk-12.0.2+10/include/linux
-c -ffunction-sections -fdata-sections -O2 -DNDEBUG -I/usr/include/gtk-2.0
-I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
-I/usr/include/harfbuzz -I/usr/include/fribidi -I/usr/include/freetype2
-I/usr/include/libpng16 -I/usr/include/cairo -I/usr/include/pixman-1
-I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libmount -I/usr/include/blkid
-I/usr/include/atk-1.0 -pthread
 
-I/home/rli/PWEs/jfx/modules/javafx.graphics/build/gensrc/headers/javafx.graphics
-o
/home/rli/PWEs/jfx/modules/javafx.graphics/build/native/glass/linux/glassgtk2/wrapped.obj
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c
Successfully started process 'command 'gcc''
gcc: error: : No such file or directory
... repeats 17 times for different files ...

> Task :graphics:ccLinuxGlassGlassgtk2 FAILED
Caching disabled for task ':graphics:ccLinuxGlassGlassgtk2' because:
  Build cache is disabled
Task ':graphics:ccLinuxGlassGlassgtk2' is not up-to-date because:
  Task has failed previously.
Compiling native files:
[/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_key.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassView.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassCommonDialogs.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassRobot.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassPixels.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassDnDClipboard.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassCursor.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassWindow.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassSystemClipboard.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_evloop.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_window_ime.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/GlassTimer.cpp,
/home/rli/PWEs/jfx/modules/javafx.graphics/src/main/native-glass/gtk/glass_screen.cpp]
:graphics:ccLinuxGlassGlassgtk2 (Thread[Daemon worker Thread 2,5,main])
completed. Took 0.06 secs.

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':graphics:ccLinuxGlassGlassgtk2'.
> java.util.concurrent.ExecutionException:
org.gradle.process.internal.ExecException: Process 'command 'gcc'' finished
with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --debug
option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible
with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See
https://docs.gradle.org/6.0/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 1s

When I execute the command manually:
gcc -fno-strict-aliasing -fPIC -fno-omit-frame-pointer -fstack-protector
-Wextra -Wall -Wformat-security -Wno-unused -Wno-parentheses
-Werror=implicit-function-declaration -Werror=trampolines
-I/usr/java/jdk-12.0.2+10/include -I/usr/java/jdk-12.0.2+10/include/linux
-c -ffunction-sections -fdata-sections -O2 -DNDEBUG -I/usr/include/gtk-2.0
-I/usr/lib64/gtk-2.0/include -I/usr/include/pango-1.0
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
-I/usr/include/harfbuzz -I/usr/include/fribidi -I/usr/include/freetype2
-I/usr/include/libpng16 -I/usr/include/cairo -I/usr/include/pixman-1
-I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libmount -I/usr/include/blkid
-I/usr/include/atk-1.0 -pthread
 
-I/home/rli/PWEs/jfx/modules/javafx.graphics/build/gensrc/headers/javafx.graphics
-o

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Thu, 11 Jun 2020 14:39:08 GMT, Jeanette Winzenburg  
wrote:

> good direction, I think :)
> 
> Didn't look too closely, just added your changes and run the tests - getting 
> a StringIndexOutofBounds at
> TextInputControlTest.test_jdk_8171229_replaceText(TextInputControlTest.java:1862)
>  (no failing test because the
> uncaughtException handler not redirected). Could you check please?

Will check this week.

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Wed, 27 May 2020 12:11:48 GMT, Robert Lichtenberger  
wrote:

>> you are hacking around ;)
>> 
>> doSelect _must not_ be called somewhere "in-between" changing the text: the 
>> api doc clearly states that the
>> caret/anchor coordinates are the _new_ coordinates, that is after all 
>> changes have been applied. When re-calculating
>> them on-the-fly might have unexpected side-effects, f.i. if a TextFormatter 
>> had something special in mind (no failing
>> test handy though).
>
> Most of the time, value in 
> javafx.scene.control.TextInputControl.replaceText(int, int, String, int, int) 
> will already
> be filtered (e.g. linebreaks in TextField) In that case, adjustmentAmount 
> will be zero and one could just do the
> selection before actually inserting the text.
> But a case can be constructed with a TextFormatter that will produce 
> characters that will be filtered by the TextField
> itself:
> 
> @Test public void replaceSelectionWithFilteredCharacters() {
> txtField.setText("x xxxyyy");
> txtField.selectRange(2, 5);
> txtField.setTextFormatter(new TextFormatter<>(this::noDigits));
> txtField.replaceSelection("a1234a");
> assertEquals("x aayyy", txtField.getText());
> assertEquals(4, txtField.getSelection().getStart());
> assertEquals(4, txtField.getSelection().getStart());
> }
> 
> private Change noDigits(Change change) {
> Change filtered = change.clone();
> filtered.setText(change.getText().replaceAll("[0-9]","\n"));
> return filtered;
> }

The last commit on this branch seems like the "best" way to fix this problem. 
It prevents in-between changes in
selectedText and does not introduce (to the best of my knowledge) any new 
corner cases. It eliminiates the "root cause"
of the problem by postponing selectedText update to the end of whole 
replace-operation. So, this is RFR now (I hope).

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [Rev 03] RFR: WIP: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  This fix changes the selectedText property from a direct binding to a property
  that can be updated manually after text AND selection change have been 
processed.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/54eacb41..6f10355d

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.03
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.02-03

  Stats: 70 lines in 2 files changed: 42 ins; 15 del; 13 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: WIP: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Wed, 27 May 2020 10:09:52 GMT, Jeanette Winzenburg  
wrote:

>> Clearing the selection temporarily works fine to prevent the 
>> StringOutOfBoundsException but will also change
>> selectionProperty to reflect the selection being 0/0 for a short time.
>
> you are hacking around ;)
> 
> doSelect _must not_ be called somewhere "in-between" changing the text: the 
> api doc clearly states that the
> caret/anchor coordinates are the _new_ coordinates, that is after all changes 
> have been applied. When re-calculating
> them on-the-fly might have unexpected side-effects, f.i. if a TextFormatter 
> had something special in mind (no failing
> test handy though).

Most of the time, value in 
javafx.scene.control.TextInputControl.replaceText(int, int, String, int, int) 
will already
be filtered (e.g. linebreaks in TextField) In that case, adjustmentAmount will 
be zero and one could just do the
selection before actually inserting the text.

But a case can be constructed with a TextFormatter that will produce characters 
that will be filtered by the TextField
itself:

@Test public void replaceSelectionWithFilteredCharacters() {
txtField.setText("x xxxyyy");
txtField.selectRange(2, 5);
txtField.setTextFormatter(new TextFormatter<>(this::noDigits));
txtField.replaceSelection("a1234a");
assertEquals("x aayyy", txtField.getText());
assertEquals(4, txtField.getSelection().getStart());
assertEquals(4, txtField.getSelection().getStart());
}

private Change noDigits(Change change) {
Change filtered = change.clone();
filtered.setText(change.getText().replaceAll("[0-9]","\n"));
return filtered;
}

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [Rev 02] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  This fix will execute the selection change before the insertion of new text.
  Only if adjustmentAmount is not 0 a second selection change will be made.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/f5209e05..54eacb41

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.01-02

  Stats: 20 lines in 3 files changed: 10 ins; 2 del; 8 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Wed, 27 May 2020 07:37:13 GMT, Robert Lichtenberger  
wrote:

>> We're in the process of finishing our products' release. If I find time
>> I will try and give this (rather nasty) Problem another shot.
>> 
>> On 2020-05-08 20:51, Kevin Rushforth wrote:
>>>
>>> @effad <https://github.com/effad> I just closed PR #73
>>> <https://github.com/openjdk/jfx/pull/73> due to inactivity. If you are
>>> interested interested in pursuing this, go ahead and reopen this
>>> (although you would need to address the feedback that clamping is
>>> insufficient).
>>>
>>> —
>>> You are receiving this because you were mentioned.
>>> Reply to this email directly, view it on GitHub
>>> <https://github.com/openjdk/jfx/pull/138#issuecomment-625963496>, or
>>> unsubscribe
>>> <https://github.com/notifications/unsubscribe-auth/ABCKAQ6JRLHJSGPI3U2YTNLRQRIB7ANCNFSM4LCNBSIA>.
>>>
>
> Reopening the pull request, since PR #73 has been closed.

Clearing the selection temporarily works fine to prevent the 
StringOutOfBoundsException but will also change
selectionProperty to reflect the selection being 0/0 for a short time.

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [Rev 01] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

Robert Lichtenberger has updated the pull request incrementally with one 
additional commit since the last revision:

  8176270: Adding ChangeListener to TextField.selectedTextProperty causes 
StringOutOfBoundsException
  
  This fix will prevent the StringOutOfBoundsException by clearing the 
selection temporarily.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/138/files
  - new: https://git.openjdk.java.net/jfx/pull/138/files/e78e8793..f5209e05

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/138/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/138/webrev.00-01

  Stats: 46 lines in 3 files changed: 40 ins; 1 del; 5 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Mon, 11 May 2020 04:54:31 GMT, Robert Lichtenberger  
wrote:

>> @effad I just closed PR #73 due to inactivity. If you are interested 
>> interested in pursuing this, go ahead and reopen
>> this (although you would need to address the feedback that clamping is 
>> insufficient).
>
> We're in the process of finishing our products' release. If I find time
> I will try and give this (rather nasty) Problem another shot.
> 
> On 2020-05-08 20:51, Kevin Rushforth wrote:
>>
>> @effad <https://github.com/effad> I just closed PR #73
>> <https://github.com/openjdk/jfx/pull/73> due to inactivity. If you are
>> interested interested in pursuing this, go ahead and reopen this
>> (although you would need to address the feedback that clamping is
>> insufficient).
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/openjdk/jfx/pull/138#issuecomment-625963496>, or
>> unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/ABCKAQ6JRLHJSGPI3U2YTNLRQRIB7ANCNFSM4LCNBSIA>.
>>

Reopening the pull request, since PR #73 has been closed.

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Fri, 8 May 2020 18:50:57 GMT, Kevin Rushforth  wrote:

>> I wasn't aware of PR #73, I only saw the (very old) issue in the JDK Bug 
>> System and started to look into the issue.
>> The fact that two different people start to look into the same issue shows 
>> it is important however :-).
>> Should I close this PR and participate in PR #73?
>
> @effad I just closed PR #73 due to inactivity. If you are interested 
> interested in pursuing this, go ahead and reopen
> this (although you would need to address the feedback that clamping is 
> insufficient).

We're in the process of finishing our products' release. If I find time
I will try and give this (rather nasty) Problem another shot.

On 2020-05-08 20:51, Kevin Rushforth wrote:
>
> @effad  I just closed PR #73
>  due to inactivity. If you are
> interested interested in pursuing this, go ahead and reopen this
> (although you would need to address the feedback that clamping is
> insufficient).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [Closed] RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Thu, 5 Mar 2020 16:01:10 GMT, Robert Lichtenberger  
wrote:

> This PR fixes JDK-8176270 by clamping the end index of the selected text to 
> the length of the text.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jfx/pull/138

Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

On Thu, 5 Mar 2020 18:00:25 GMT, Kevin Rushforth  wrote:

>> why a second pr for the same issue (see 
>> https://github.com/openjdk/jfx/pull/73)? particularly one that doesn't do 
>> much more/else/better (than clamping, which isn't good enough)?
>
> I have exactly the same question.
> 
> In general, it's better to work with the author of an existing PR instead of 
> creating a new one. If the original PR #73 is completely stalled, then it 
> might make sense, but not until then.

I wasn't aware of PR #73, I only saw the (very old) issue in the JDK Bug System 
and started to look into the issue. 
The fact that two different people start to look into the same issue shows it 
is important however :-).
Should I close this PR and participate in PR #73?

-

PR: https://git.openjdk.java.net/jfx/pull/138

RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException

[Robert Lichtenberger]

This PR fixes JDK-8176270 by clamping the end index of the selected text to the 
length of the text.

-

Commits:
 - e78e8793: 8176270: Adding ChangeListener to TextField.selectedTextProperty 
causes StringOutOfBoundsException
 - d849c67c: Merge remote-tracking branch 'upstream/master'
 - 846d0483: Merge remote-tracking branch 'upstream/master'
 - 9ceb21bc: Merge remote-tracking branch 'upstream/master'
 - 2109d5a0: Merge remote-tracking branch 'upstream/master'
 - acfa63be: Merge remote-tracking branch 'upstream/master'
 - 7c5cf198: 8232524: Test cleanup: terminate background thread upon failure.
 - 7e80839f: 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - 8ecf3545: JDK-8232524 fixed.

Changes: https://git.openjdk.java.net/jfx/pull/138/files
 Webrev: https://webrevs.openjdk.java.net/jfx/138/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8176270
  Stats: 20 lines in 3 files changed: 19 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/138.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138

PR: https://git.openjdk.java.net/jfx/pull/138

Re: [Rev 03] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

> Test simulates a single mouse-released event.
> Fix simply guards against the null case.

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 39a61821: 8237372: NullPointerException in TabPaneSkin.stopDrag

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/89/files
  - new: https://git.openjdk.java.net/jfx/pull/89/files/a54a5306..39a61821

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/89/webrev.03
 - incr: https://webrevs.openjdk.java.net/jfx/89/webrev.02-03

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

PR: https://git.openjdk.java.net/jfx/pull/89

Re: [Rev 02] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

On Tue, 28 Jan 2020 12:38:32 GMT, Ambarish Rapte  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java
>  line 57:
> 
>> 56: import javafx.scene.input.KeyEvent;
>> 57: import javafx.scene.input.MouseButton;
>> 58: import javafx.scene.input.MouseEvent;
> 
> This import it not used, please remove.

Removed import and extra empty line.

-

PR: https://git.openjdk.java.net/jfx/pull/89

Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

On Fri, 24 Jan 2020 15:41:20 GMT, Kevin Rushforth  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
>  line 1994:
> 
>> 1993: private ListChangeListener childListener = new 
>> ListChangeListener() {
>> 1994: @Override
>> 1995: public void onChanged(Change change) {
> 
> Please revert.

Reverted the Overrides-Annotations inserted by eclipse.

-

PR: https://git.openjdk.java.net/jfx/pull/89

Re: [Rev 02] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

> Test simulates a single mouse-released event.
> Fix simply guards against the null case.

The pull request has been updated with 1 additional commit.

-

Added commits:
 - a54a5306: 8237372: NullPointerException in TabPaneSkin.stopDrag

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/89/files
  - new: https://git.openjdk.java.net/jfx/pull/89/files/30290116..a54a5306

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/89/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/89/webrev.01-02

  Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/89.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89

PR: https://git.openjdk.java.net/jfx/pull/89

Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

> Test simulates a single mouse-released event.
> Fix simply guards against the null case.

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 30290116: 8237372: NullPointerException in TabPaneSkin.stopDrag

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/89/files
  - new: https://git.openjdk.java.net/jfx/pull/89/files/923a63b2..30290116

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/89/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/89/webrev.00-01

  Stats: 7 lines in 1 file changed: 3 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/89.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89

PR: https://git.openjdk.java.net/jfx/pull/89

Re: [Rev 01] RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

On Wed, 22 Jan 2020 08:01:49 GMT, Ambarish Rapte  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java
>  line 2216:
> 
>> 2215: // Animate tab header being dragged to its final position.
>> 2216: if (dragTabHeader != null) {
>> 2217: dragHeaderSourceX = dragTabHeader.getLayoutX();
> 
> This is a situation when reorder was not started.
> So a check as `else if (dragState == DragState.REORDER)` seems more correct 
> instead of `null` check. With this `else if` the `return;` on line 2213 can 
> be removed.

You're right, this makes the method cleaner and easier to understand. I've 
adapted the PR.

-

PR: https://git.openjdk.java.net/jfx/pull/89

RFR: 8237372: NullPointerException in TabPaneSkin.stopDrag

[Robert Lichtenberger]

Test simulates a single mouse-released event.
Fix simply guards against the null case.

-

Commits:
 - 923a63b2: 8237372: NullPointerException in TabPaneSkin.stopDrag

Changes: https://git.openjdk.java.net/jfx/pull/89/files
 Webrev: https://webrevs.openjdk.java.net/jfx/89/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237372
  Stats: 29 lines in 2 files changed: 21 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jfx/pull/89.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/89/head:pull/89

PR: https://git.openjdk.java.net/jfx/pull/89

Correct branch for PR?

[Robert Lichtenberger]

I have a testcase + fix ready for JDK-8237372 (A null pointer exception in
TabPaneSkin if only a mouse release event is sent to the skin).

To avoid the kind of confusion I created with my last pull request ;-)
* For what branch should I create the pull request? (From my point of view
jfx14 is preferable, since this is a real bug affecting our software)
* Will pull requests for jfx14 (like my last one) be automatically pushed
forward to the master branch?

Best regards,
Robert

Re: Running Web-Tests from eclipse

[Robert Lichtenberger]

This looks good to me.

Thanks,
Robert

Am Mi., 15. Jan. 2020 um 07:15 Uhr schrieb Nir Lisker :

> I updated the eclipse instructions. Have a look.
>
> Thanks,
> Nir
>
> On Tue, Jan 14, 2020 at 1:01 PM Robert Lichtenberger <
> r.lichtenber...@gmail.com> wrote:
>
>> As a follow-up to the long thread on using an IDE. I just found out that
>> running test.javafx.scene.web.WebViewTest is not possible with
>> -Djavafx.toolkit=test.com.sun.javafx.pgstub.StubToolkit.
>>
>> The test simply fails to start (waiting forever for
>> com.sun.javafx.application.PlatformImpl.startupLatch).
>>
>> Instead one needs to add the graphics library and the webkit library with
>> e.g.
>>
>> -Djava.library.path=/home/rli/PWEs/jfx/modules/javafx.graphics/build/module-lib:/home/rli/PWEs/jfx/modules/javafx.web/build/module-lib
>>
>> Maybe this could also be added to the wiki page?
>>
>

Re: [Rev 02] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Robert Lichtenberger]

On Tue, 14 Jan 2020 14:43:03 GMT, Kevin Rushforth  wrote:

>> Partially reviewed, Need to re-check after the change of branch.
> 
> Oh, I see the problem. In addition to rebasing you need to exclude any 
> commits that are from the `master` branch and are not your. So what you 
> really need to do is:
> 
> git rebase -i upstream/jfx14
> 
> git push --force origin JDK-8236912
> 
> Sorry for not anticipating this problem.

> Sorry for not anticipating this problem.
I am sorry for not knowing git well enough. Thank you for your patience and 
help ;-). I think the PR now contains what it should ;-).

-

PR: https://git.openjdk.java.net/jfx/pull/85

Re: [Rev 03] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Robert Lichtenberger]

> As documented in JDK-8236912, WebView did not check whether the idMap really 
> contained a mapping for the given button, making it prone to errors, when 
> things are extended (as has happened here).
> 
> The fix consists of two test cases that show the problem in unfixed WebViews 
> and a fix which works analogously to the check whether the given event type 
> is mapped.

Previous commits in this pull request have been removed, probably due to a 
force push. The incremental views will show differences compared to the 
previous content of the PR.

-

Added commits:
 - aa03cf1f: Fix null pointer exception when clicking into WebViews with 
forward/back mouse button.

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/85/files
  - new: https://git.openjdk.java.net/jfx/pull/85/files/8c3f6017..aa03cf1f

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/85/webrev.03
 - incr: https://webrevs.openjdk.java.net/jfx/85/webrev.02-03

  Stats: 187 lines in 4 files changed: 0 ins; 183 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/85.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/85/head:pull/85

PR: https://git.openjdk.java.net/jfx/pull/85

Re: [Rev 02] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Robert Lichtenberger]

On Tue, 14 Jan 2020 14:00:08 GMT, Kevin Rushforth  wrote:

>> I've rebased my branch like this:
>> git fetch upstream
>> git rebase upstream/jfx14
>> git pull
>> git push
>> and changed the base branch of this PR to jfx14. Hope that was the correct 
>> way to do this ;-).
> 
> Your branch still has commits from `master` after jfx14 was branched. This 
> was the problem:
> git pull
> 
> This pulled in additional changes from master that were already in your 
> branch and effectively undid the rebase. You need to redo it, but this time 
> instead of pulling changes after you rebase, you should simply do:
> git push --force origin JDK-8236912

> ```
> git push --force origin JDK-8236912
> ```
I did 
git rebase upstream/jfx14
git push --force origin JDK-8236912
but that has messed up things further, didn't it?

-

PR: https://git.openjdk.java.net/jfx/pull/85

Re: [Rev 02] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Robert Lichtenberger]

> As documented in JDK-8236912, WebView did not check whether the idMap really 
> contained a mapping for the given button, making it prone to errors, when 
> things are extended (as has happened here).
> 
> The fix consists of two test cases that show the problem in unfixed WebViews 
> and a fix which works analogously to the check whether the given event type 
> is mapped.

The pull request has been updated with a new target base due to a merge or a 
rebase.

-

Commits:
 - 8c3f6017: 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - 829920cf: JDK-8232524 fixed.
 - 401b4484: Fix null pointer exception when clicking into WebViews with 
forward/back mouse button.
 - 3bf7e920: 8236733: Change JavaFX release version to 15
 - 0967bbd3: 8232524: Test cleanup: terminate background thread upon failure.
 - e50b0533: 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - 9d1979e7: JDK-8232524 fixed.

Changes: https://git.openjdk.java.net/jfx/pull/85/files
 Webrev: https://webrevs.openjdk.java.net/jfx/85/webrev.02
  Stats: 205 lines in 6 files changed: 198 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jfx/pull/85.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/85/head:pull/85

PR: https://git.openjdk.java.net/jfx/pull/85

Re: [Rev 01] RFR: 8236912: NullPointerException when clicking in WebView with Button 4 or Button 5

[Robert Lichtenberger]

> As documented in JDK-8236912, WebView did not check whether the idMap really 
> contained a mapping for the given button, making it prone to errors, when 
> things are extended (as has happened here).
> 
> The fix consists of two test cases that show the problem in unfixed WebViews 
> and a fix which works analogously to the check whether the given event type 
> is mapped.

The pull request has been updated with 2 additional commits.

-

Added commits:
 - 283c5476: 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - 6aecab95: JDK-8232524 fixed.

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/85/files
  - new: https://git.openjdk.java.net/jfx/pull/85/files/542ff60e..283c5476

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/85/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/85/webrev.00-01

  Stats: 91 lines in 2 files changed: 91 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/85.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/85/head:pull/85

PR: https://git.openjdk.java.net/jfx/pull/85

Re: RFR: 8236912: Preventing NPE when clicking WebView with forward/back mouse buttons

[Robert Lichtenberger]

How strange: Other commits to the master branch of my forked repository
(relating to another issue, already integrated into OpenJFX) are also
showing up here. But the change really only consists of 542ff60e which
contains the fix for JDK-8236912. My older contributions were not checked
into a separate branch (of my fork), maybe that was the problem. If there
is anything else I did wrong, please tell me ;-).

The Changes-Link and the Webrev correctly show that only two files are
changed.

Am Di., 14. Jan. 2020 um 13:02 Uhr schrieb Robert Lichtenberger <
rlich...@openjdk.java.net>:

> As documented in JDK-8236912, WebView did not check whether the idMap
> really contained a mapping for the given button, making it prone to errors,
> when things are extended (as has happened here).
>
> The fix consists of two test cases that show the problem in unfixed
> WebViews and a fix which works analogously to the check whether the given
> event type is mapped.
>
> -
>
> Commits:
>  - 542ff60e: Fix null pointer exception when clicking into WebViews with
> forward/back mouse button.
>  - 2109d5a0: Merge remote-tracking branch 'upstream/master'
>  - acfa63be: Merge remote-tracking branch 'upstream/master'
>  - 7c5cf198: 8232524: Test cleanup: terminate background thread upon
> failure.
>  - 7e80839f: 8232524: SynchronizedObservableMap cannot be be protected for
> copying/iterating
>  - 8ecf3545: JDK-8232524 fixed.
>
> Changes: https://git.openjdk.java.net/jfx/pull/85/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/85/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8236912
>   Stats: 18 lines in 2 files changed: 15 ins; 0 del; 3 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/85.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/85/head:pull/85
>
> PR: https://git.openjdk.java.net/jfx/pull/85
>

RFR: 8236912: Preventing NPE when clicking WebView with forward/back mouse buttons

[Robert Lichtenberger]

As documented in JDK-8236912, WebView did not check whether the idMap really 
contained a mapping for the given button, making it prone to errors, when 
things are extended (as has happened here).

The fix consists of two test cases that show the problem in unfixed WebViews 
and a fix which works analogously to the check whether the given event type is 
mapped.

-

Commits:
 - 542ff60e: Fix null pointer exception when clicking into WebViews with 
forward/back mouse button.
 - 2109d5a0: Merge remote-tracking branch 'upstream/master'
 - acfa63be: Merge remote-tracking branch 'upstream/master'
 - 7c5cf198: 8232524: Test cleanup: terminate background thread upon failure.
 - 7e80839f: 8232524: SynchronizedObservableMap cannot be be protected for 
copying/iterating
 - 8ecf3545: JDK-8232524 fixed.

Changes: https://git.openjdk.java.net/jfx/pull/85/files
 Webrev: https://webrevs.openjdk.java.net/jfx/85/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236912
  Stats: 18 lines in 2 files changed: 15 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/85.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/85/head:pull/85

PR: https://git.openjdk.java.net/jfx/pull/85