Re: [Rev 01] RFR: 8227808: Make GTK3 libraries mandatory for building on Linux

[Ambarish Rapte]

On Thu, 5 Dec 2019 12:16:53 GMT, Johan Vos  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> Works as expected. I recommend removing gtk-specific flags in the launcher.

> Works as expected. I recommend removing gtk-specific flags in the launcher.

Hi Johan, The PR is updated with this change, please take a look.

-

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

RFR: 8223296: NullPointerException in GlassScene.java at line 325

[Ambarish Rapte]

Issue: NPE in GlassScene.frameRendered().

Cause: scenePaintListener is set in setTKScenePaintListener(), used in 
frameRendered() and 
set to null in dispose().
setTKScenePaintListener() and dispose() are called on JavaFX Application Thread 
and 
frameRendered() is called by QuantumRenderer thread.
setTKScenePaintListener() and frameRendered() are synchronized but dispose() is 
not.

Fix:
dispose() should use the synchronized setTKScenePaintListener() to set 
scenePaintListener to null.

Verification:
This is a very rare issue and cannot be reliably reproduced with a test case.

-

Commits:
 - ca95597f: [WIP]8223296: NullPointerException in GlassScene.java at line 325

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

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

Re: [Rev 02] RFR: 8232811: Dialog's preferred size no longer accommodates multi-line strings

[Thiago Milczarek Sayao]

> https://bugs.openjdk.java.net/browse/JDK-8232811
> 
> This one was hard to tackle.
> 
> ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
> test.robot.javafx.scene.dialog.DialogWithOwnerSizingTest

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 7b723c3b: Revert unecessary change

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/63/files
  - new: https://git.openjdk.java.net/jfx/pull/63/files/7a8377e7..7b723c3b

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

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

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

Re: [Rev 01] RFR: 8232811: Dialog's preferred size no longer accommodates multi-line strings

[Thiago Milczarek Sayao]

> https://bugs.openjdk.java.net/browse/JDK-8232811
> 
> This one was hard to tackle.
> 
> ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
> test.robot.javafx.scene.dialog.DialogWithOwnerSizingTest

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 7a8377e7: Asthetics

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/63/files
  - new: https://git.openjdk.java.net/jfx/pull/63/files/338bedbe..7a8377e7

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

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

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

Questions about glass linux implementation

[Thiago Milczarek Sayao]

Well, git blames Antem Ananiev - not sure you're around this list.

Question 1 - Why can't we use gtk_window_set_resizable () ?


Question 2 - Why do we keep separated geometry and not use gtk directly?


Thanks.

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Mon, 9 Dec 2019 21:01:40 GMT, Kevin Rushforth  wrote:

>> The fix looks good to me.
>> 
>> I left a few minor comments, including adding a missing comma in the API 
>> docs, which will also need to be changed in the CSR. Once this is updated 
>> I'll review the CSR and then you can move the CSR to Finalize.
> 
> I would like both @aghaisas and myself to review / approve this. I will 
> sponsor it.
> 
> In addition, the CSR needs to be approved before this can be integrated.

Thanks for the review. I do not have access to a computer right now, I'll
update next week on Monday.

Le lun. 9 déc. 2019 à 17:01, Kevin Rushforth  a
écrit :

> I would like both @aghaisas  and myself to
> review / approve this. I will sponsor it.
>
> In addition, the CSR needs to be approved before this can be integrated.
>
> —
> 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/6

Re: Difference in CSS styling / rendering.

[Kevin Rushforth]

Thanks for the heads-up. Let us know what you find.

-- Kevin


On 12/9/2019 1:13 PM, Dirk Lemmermann wrote:

Hi,

I just noticed that the timeline in FlexGanttFX does not get rendered correctly 
anymore after updating to 14-ea+4. The borders around the date cells 
disappeared.

I assume that this must be somehow related to the CSS fix that made it into 
this version.

This is just a heads-up. If possible I will try to create a standalone example 
for this issue, but considering the complexity of this control it might be 
difficult to do so.

Dirk

Difference in CSS styling / rendering.

[Dirk Lemmermann]

Hi,

I just noticed that the timeline in FlexGanttFX does not get rendered correctly 
anymore after updating to 14-ea+4. The borders around the date cells 
disappeared.

I assume that this must be somehow related to the CSS fix that made it into 
this version. 

This is just a heads-up. If possible I will try to create a standalone example 
for this issue, but considering the complexity of this control it might be 
difficult to do so.

Dirk

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Mon, 9 Dec 2019 20:58:28 GMT, Kevin Rushforth  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> The fix looks good to me.
> 
> I left a few minor comments, including adding a missing comma in the API 
> docs, which will also need to be changed in the CSR. Once this is updated 
> I'll review the CSR and then you can move the CSR to Finalize.

I would like both @aghaisas and myself to review / approve this. I will sponsor 
it.

In addition, the CSR needs to be approved before this can be integrated.

-

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

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Mon, 9 Dec 2019 20:58:46 GMT, Hadzic Samir  wrote:

>> Allright, this is a fix for JDK-8207957
> 
> The pull request has been updated with 1 additional commit.

The fix looks good to me.

I left a few minor comments, including adding a missing comma in the API docs, 
which will also need to be changed in the CSR. Once this is updated I'll review 
the CSR and then you can move the CSR to Finalize.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
 line 40:

> 39: import javafx.collections.WeakListChangeListener;
> 40: import javafx.css.*;
> 41: import javafx.css.converter.SizeConverter;

Please revert this change. In general, we do not use wildcard imports.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
 line 605:

> 604:  * custom implementation (such as ones that exclude the header, 
> exclude {@code null} content, compute the minimum
> 605:  * width etc.).
> 606:  *

There should be a `,` before `etc.`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
 line 2:

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

add 2019, so:

  * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
 line 56:

> 55: @Before
> 56: public void before(){
> 57: ObservableList model = FXCollections.observableArrayList(

Minor: add a space before the `{`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
 line 80:

> 79: @After
> 80: public void after(){
> 81: sl.dispose();

Minor: add a space before the `{`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
 line 152:

> 151: tableView.getItems().get(2).setFirstName("Orrin Davies");
> 152: tableView.getItems().get(3).setFirstName("Emma Wilson");
> 153: 

It would be nice to not have to repeat the set of strings used. I recommend 
putting them in a static string array (they appears either 2 or 3 times in the 
test file). Maybe create 4 static strings for the 4 names?

-



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

Re: Minor code changes that aren't necessarily bugs

[Kevin Rushforth]

It depends. Some cleanup like this can be good if it reduces overall 
maintenance going forward. In many cases, though, it would take more 
effort to get it reviewed and prove that there aren't any regressions 
than it is worth. In particular, I don't think there is high value in 
the sort of automatic refactoring that IDEs like to suggest.


In the specific case of removing a possibly-redundant call to setFocus, 
I'm not sure. One thing to consider is the case where the scene on the 
stage goes from null to non-null. You would need to make sure that the 
behavior doesn't change. Also, there is the case of multiple stages to 
consider.


-- Kevin


On 12/9/2019 12:07 PM, Thiago Milczarek Sayao wrote:

While going across the source code I have noticed some code that might be 
incorrect , such as:

- WindowStage.setScene() has a call to requestFocus()

I might be missing something, but I think it does not belong there because the 
scene is usually set before show(), and while it may be set after the show and 
even multiple times, why should it requestFocus() ?

Removing it may also fix https://bugs.openjdk.java.net/browse/JDK-8212060. It 
is already fixed on glass side, but still...

- WindowStage.hadleFocusDisabled() calls requestToFront() and requestFocus() 
while requestToFront() already calls requestFocus()

- Intellij points me some cleanups such as null checks before instanceof, ifs 
that can be merged, etc.


Would those kind of PR be welcome, accepted (if correct) ?

Cheers.

Minor code changes that aren't necessarily bugs

[Thiago Milczarek Sayao]

While going across the source code I have noticed some code that might be 
incorrect , such as:

- WindowStage.setScene() has a call to requestFocus()

I might be missing something, but I think it does not belong there because the 
scene is usually set before show(), and while it may be set after the show and 
even multiple times, why should it requestFocus() ?

Removing it may also fix https://bugs.openjdk.java.net/browse/JDK-8212060. It 
is already fixed on glass side, but still...

- WindowStage.hadleFocusDisabled() calls requestToFront() and requestFocus() 
while requestToFront() already calls requestFocus()

- Intellij points me some cleanups such as null checks before instanceof, ifs 
that can be merged, etc.


Would those kind of PR be welcome, accepted (if correct) ?

Cheers.

RFR: 8232811: Dialog's preferred size no longer accommodates multi-line strings

[Thiago Milczarek Sayao]

https://bugs.openjdk.java.net/browse/JDK-8232811

This one was hard to tackle.

./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
test.robot.javafx.scene.dialog.DialogWithOwnerSizingTest

-

Commits:
 - 338bedbe: Fix mistake
 - a02894d6: Fix JDK-8232811
 - 7a23cdbd: Merge pull request #2 from openjdk/master
 - fc36a292: Merge pull request #1 from openjdk/master

Changes: https://git.openjdk.java.net/jfx/pull/63/files
 Webrev: https://webrevs.openjdk.java.net/jfx/63/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8232811
  Stats: 188 lines in 2 files changed: 176 ins; 11 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/63.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/63/head:pull/63

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

Re: Test drive of Skara feature to automatically resolve JBS issues

[Jeanette Winzenburg]

Thanks for the cleanup :)

Zitat von Kevin Rushforth :

I was planning to do that this morning (I was going to do it Friday  
afternoon, but ran into the JBS maintenance window, and then ran out  
of time). They actually had turned it on before the below bug was  
integrated, but there was a configuration error in the Skara bot  
that prevented it from working, and they haven't implemented the  
retry logic yet.


Thanks.

-- Kevin


On 12/9/2019 2:56 AM, Jeanette Winzenburg wrote:


Zitat von Kevin Rushforth :

The plan is to put this into place tomorrow (Friday, Dec 6) and  
test drive it for a week or two so that the Skara team can get  
feedback on how it is working for our project (they already use it  
for the Skara project).




hmm .. looks like https://github.com/openjdk/jfx/pull/56 was  
integrated too early for the magic to shine? (With a fix integrated  
today everything went fine :)


Should I update the corresponding issue manually?

-- CU, Jeanette

Re: Test drive of Skara feature to automatically resolve JBS issues

[Kevin Rushforth]

I was planning to do that this morning (I was going to do it Friday 
afternoon, but ran into the JBS maintenance window, and then ran out of 
time). They actually had turned it on before the below bug was 
integrated, but there was a configuration error in the Skara bot that 
prevented it from working, and they haven't implemented the retry logic yet.


Thanks.

-- Kevin


On 12/9/2019 2:56 AM, Jeanette Winzenburg wrote:


Zitat von Kevin Rushforth :

The plan is to put this into place tomorrow (Friday, Dec 6) and test 
drive it for a week or two so that the Skara team can get feedback on 
how it is working for our project (they already use it for the Skara 
project).




hmm .. looks like https://github.com/openjdk/jfx/pull/56 was 
integrated too early for the magic to shine? (With a fix integrated 
today everything went fine :)


Should I update the corresponding issue manually?

-- CU, Jeanette

RFR: 8234471: Canvas in webview displayed with wrong scale on Windows

[Arun Joseph]

This bug can be reproduced when the screen resolution is at 125%, 150% and 175% 
for Windows, which correpsonds to `pixelScale` values of 1.25, 1.5 and 1.75, 
respectively.

Issue: The rectangle inside canvas is rendered on `pixelScale` while the 
borders are rendered on `Math.ceil(pixelScale)`

Fix: Use `Math.ceil(pixelScale)` for calculating `pixelScaleTransform`

-

Commits:
 - fde17b8c: 8234471: Canvas in webview displayed with wrong scale on Windows

Changes: https://git.openjdk.java.net/jfx/pull/62/files
 Webrev: https://webrevs.openjdk.java.net/jfx/62/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8234471
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/62.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/62/head:pull/62

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

Re: Test drive of Skara feature to automatically resolve JBS issues

[Jeanette Winzenburg]

Zitat von Kevin Rushforth :

The plan is to put this into place tomorrow (Friday, Dec 6) and test  
drive it for a week or two so that the Skara team can get feedback  
on how it is working for our project (they already use it for the  
Skara project).




hmm .. looks like https://github.com/openjdk/jfx/pull/56 was  
integrated too early for the magic to shine? (With a fix integrated  
today everything went fine :)


Should I update the corresponding issue manually?

-- CU, Jeanette

Re: [Integrated] RFR: 8220722: ProgressBarSkin: adds strong listener to control's width property

[Ajit Ghaisas]

Changeset: 71ca899f
Author:Jeanette Winzenburg 
Committer: Ajit Ghaisas 
Date:  2019-12-09 08:08:34 +
URL:   https://git.openjdk.java.net/jfx/commit/71ca899f

8220722: ProgressBarSkin: adds strong listener to control's width property

Reviewed-by: aghaisas, arapte

! 
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressBarSkin.java
! 
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ProgressBarSkinTest.java