from:"Phil Race"

Re: [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)

2018-04-11 Thread Phil Race

Use the syntax {@code c} instead of c everywhere
it is applicable.

Use @throws instead of @exception.
It is preferred these days.
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#@exception

doesn't -> does not (we should be formal in the docs).

560  * from the index specified.

should become

"from the specified index."

 * @param index the index from which to start adding elements from

this reads awkardly .. as we have "from" twice. Can we instead say :

|* @param index the position at which to start adding elements Or, maybe 
even better, borrow the wording from java.util.List's docs on the same 
named method since they are very similar : |

|index| - index at which to insert the first element from the specified 
collection

-phil.

On 04/11/2018 04:25 AM, Krishna Addepalli wrote:

Hi All,

Here is the modified webrev: 
http://cr.openjdk.java.net/~kaddepalli/4842658/webrev03/  as per Joe Darcy's 
suggestion to modify the api to accept a collection object that captures 
objects of classes that extend type E, in line with underlying Vector API.

Thanks,
Krishna

-Original Message-
From: Krishna Addepalli
Sent: Tuesday, April 10, 2018 6:14 PM
To: Sergey Bylokhov ; Andrej Golovnin 

Cc: swing-dev@openjdk.java.net
Subject: RE:  [11][JDK-4842658] RFR: DefaultListModel and 
DefaultComboBoxModel should support addAll (Collection c)

Hi Sergey,

Please update the test to catch the difference between v00 and v01.

Done.

Some comments about javadoc:
  - "183  * " is unnecessary
  - Do not use dots at the end of @param/@return tags

Done.

  > - Not sure that the text below is necessary(it is duplicate description of 
@exception tag):
  >  "202  * Throws an IllegalArgumentExceptionif the index was 
invalid."
Removed.

   > - It is unclear what this text means:
   >"207 if c doesn't fall in the range of the list."

   >  - Specify that the new methods will throw NPE on null Done.

   > - Why in one class you use addElements/addElementsAt names and in another 
addAll?
In the DefaultComboBoxModel, the functions to add a single element are named as 
addElement, addElementAt.
To keep with that convention, I named the apis appropriately.

   > - In the exception message you use "list" for "DefaultListModel" and 
"combobox" for DefaultComboBoxModel i
   > guess it should be "xxx model". Or you can simplify it to "Index out of range: 
+ index"
Done.

It is also unclear what exception should be thrown,
IllegalArgumentException is a good candidate but other methods in
these classes like DefaultComboBoxModel.insertElementAt/removeElementAt will throw 
"ArrayIndexOutOfBoundsException", any thoughts/suggestions?

I was also a bit confused about this. Initially I referred to the function 
above (removeRange) and saw that it was throwing IllegalArgumentException. Now, 
I have changed the exception to ArrayIndexOutOfBoundsException as you suggested.

Here is the updated webrev: 
http://cr.openjdk.java.net/~kaddepalli/4842658/webrev02/

Thanks,
Krishna

On 09/04/2018 04:29, Krishna Addepalli wrote:

Hi Andrej,

Appreciate for the  quick review. Here is the updated webrev:
http://cr.openjdk.java.net/~kaddepalli/4842658/webrev01/

Thanks,
Krishna

-Original Message-
From: Andrej Golovnin 
Sent: Monday, April 9, 2018 3:15 PM
To: Krishna Addepalli 
Cc: swing-dev@openjdk.java.net
Subject: Re:  [11][JDK-4842658] RFR: DefaultListModel and
DefaultComboBoxModel should support addAll (Collection c)

Hi Krishna,

Please review a simple fix for enhancement:

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

Webrev: http://cr.openjdk.java.net/~kaddepalli/4842658/webrev00/

CSR: https://bugs.openjdk.java.net/browse/JDK-8201289

src/java.desktop/share/classes/javax/swing/DefaultComboBoxModel.java

195 fireIntervalAdded(this, startIndex, getSize());

It should be:

195 fireIntervalAdded(this, startIndex, getSize() - 1);

221 fireIntervalAdded(this, index, index + c.size());

It should be:

221 fireIntervalAdded(this, index, index + c.size() - 1);

src/java.desktop/share/classes/javax/swing/DefaultListModel.java

574 if(c.isEmpty()) {

There should be a space after 'if'.

Best regards,
Andrej Golovnin

--
Best regards, Sergey.

Re: [11] RFR for JDK-8199627: Use "Per-Monitor V2" High DPI awareness for Windows 10 v1703

2018-04-11 Thread Phil Race

-phil

On 04/11/2018 11:06 AM, Alexey Ivanov wrote:

Thank you, Sergey, for your review.

Any other volunteers?

Thanks,
Alexey

On 05/04/2018 23:56, Sergey Bylokhov wrote:

Looks fine.

On 05/04/2018 15:39, Alexey Ivanov wrote:

Hello,

Could you please review the fix for jdk11?

bug: https://bugs.openjdk.java.net/browse/JDK-8199627
webrev: http://cr.openjdk.java.net/~aivanov/8199627/webrev.0/

Windows 10 v1703 provides improved High DPI mode: Per-Monitor v2. [1]

Java already supports "Per-Monitor v1" mode. This change extends
High DPI support in Java: in addition to element [2], a
new element [3] is added to Java launcher manifest.
The element is recognized by Windows 10 v1607 and
above, PerMonitorV2 value is supported by Windows 10 v1703 and above.

The most notable change for Java applications is that window title
bar is scaled correctly when application window moves from one
monitor to another or the scaling changes.

When building, manifest tool generates warning for unknown
element in manifest. It is because an older Windows
SDK is used to build JDK which does not know about an element that
was added later. The build succeeds despite the warning.

Thank you in advance.

Regards,
Alexey

[1]
https://msdn.microsoft.com/library/windows/desktop/mt843498(v=vs.85).aspx#Per-Monitor_and_Per-Monitor__V2__DPI_Awareness_

[2]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa374191(v=vs.85).aspx#dpiAware

[3]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa374191(v=vs.85).aspx#dpiAwareness

Re: [11][JDK-8153532] RFR: Add @throws NPE javadoc to UIManager.setLookAndFeel(String) method description

2018-04-11 Thread Phil Race


Can we change all occurrences of @exception -> @throws in this method ?
In fact this method appears to be the only one in this class to use 
@exception

so it is doubly anomalous.

-phil.

On 04/10/2018 04:07 AM, Krishna Addepalli wrote:


Hi Pankaj,

The change looks fine to me.

Thanks,

Krishna

*From:* Pankaj Bansal
*Sent:* Tuesday, April 10, 2018 4:27 PM
*To:* swing-dev@openjdk.java.net
*Subject:*  [11][JDK-8153532] RFR: Add @throws NPE javadoc 
to UIManager.setLookAndFeel(String) method description


Hi All,

Please review a very simple fix for documentation change enhancement:

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

Webrev: http://cr.openjdk.java.net/~pbansal/8153532/webrev.00/ 



CSR: https://bugs.openjdk.java.net/browse/JDK-8201363

UIManager.setLookAndFeel (String) throws NPE if the className is null. 
But this is not updated in the API description. Made changes for the same.


Regards,

Pankaj Bansal

Re: [11][JDK-8153532] RFR: Add @throws NPE javadoc to UIManager.setLookAndFeel(String) method description

2018-04-11 Thread Phil Race


+1 (but don't forget to update the CSR and wait for that approval).

-phil


On 4/11/2018 12:06 PM, Pankaj Bansal wrote:


Hi Phil,

Thanks for the review.

< @throws in this method ?
<@exception

<Webrev: http://cr.openjdk.java.net/~pbansal/8153532/webrev.01/ 
<http://cr.openjdk.java.net/%7Epbansal/8153532/webrev.01/>


Regards,

Pankaj Bansal

*From:*Phil Race
*Sent:* Thursday, April 12, 2018 12:08 AM
*To:* Krishna Addepalli; Pankaj Bansal; swing-dev@openjdk.java.net
*Subject:* Re:  [11][JDK-8153532] RFR: Add @throws NPE 
javadoc to UIManager.setLookAndFeel(String) method description


Can we change all occurrences of @exception -> @throws in this method ?
In fact this method appears to be the only one in this class to use 
@exception

so it is doubly anomalous.

-phil.

On 04/10/2018 04:07 AM, Krishna Addepalli wrote:

Hi Pankaj,

The change looks fine to me.

Thanks,

Krishna

*From:* Pankaj Bansal
*Sent:* Tuesday, April 10, 2018 4:27 PM
*To:* swing-dev@openjdk.java.net <mailto:swing-dev@openjdk.java.net>
*Subject:*  [11][JDK-8153532] RFR: Add @throws NPE
javadoc to UIManager.setLookAndFeel(String) method description

Hi All,

Please review a very simple fix for documentation change enhancement:

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

Webrev: http://cr.openjdk.java.net/~pbansal/8153532/webrev.00/
<http://cr.openjdk.java.net/%7Epbansal/8153532/webrev.00/>

CSR: https://bugs.openjdk.java.net/browse/JDK-8201363

UIManager.setLookAndFeel (String) throws NPE if the className is
null. But this is not updated in the API description. Made changes
for the same.

Regards,

Pankaj Bansal

Re: [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)

2018-04-12 Thread Phil Race

I was only expecting you to update your new code to use {@code .. } but
since you've made the changes they are OK to keep.

Just make sure you *exclude* those other changes from the CSR, as
they aren't a spec. change and will clutter the CSR more than they are
cluttering the webrev.

+1

-phil.

On 04/12/2018 03:46 AM, Krishna Addepalli wrote:

Hi Phil,

Thank you for the review.

Here is the updated webrev: 
http://cr.openjdk.java.net/~kaddepalli/4842658/webrev04/ 
<http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev04/>

Thanks,

Krishna

*From:*Phil Race
*Sent:* Wednesday, April 11, 2018 11:33 PM
*To:* Krishna Addepalli ; Sergey 
Bylokhov ; Andrej Golovnin 

*Cc:* swing-dev@openjdk.java.net
*Subject:* Re:  [11][JDK-4842658] RFR: DefaultListModel and 
DefaultComboBoxModel should support addAll (Collection c)

Use the syntax {@code c} instead of c everywhere
it is applicable.

Use @throws instead of @exception.
It is preferred these days.
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#@exception

doesn't -> does not (we should be formal in the docs).

> 560  * from the index specified.

should become

"from the specified index."

 * @param index the index from which to start adding elements from

this reads awkardly .. as we have "from" twice. Can we instead say :

* @param index the position at which to start adding elements
Or, maybe even better, borrow the wording from java.util.List's docs
on the same named method since they are very similar :

|index| - index at which to insert the first element from the 
specified collection

-phil.

On 04/11/2018 04:25 AM, Krishna Addepalli wrote:

Hi All,

Here is the modified 
webrev:http://cr.openjdk.java.net/~kaddepalli/4842658/webrev03/
<http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev03/>   as per Joe 
Darcy's suggestion to modify the api to accept a collection object that captures 
objects of classes that extend type E, in line with underlying Vector API.

Thanks,

Krishna

-Original Message-

From: Krishna Addepalli

Sent: Tuesday, April 10, 2018 6:14 PM

To: Sergey Bylokhov <mailto:sergey.bylok...@oracle.com>; 
Andrej Golovnin <mailto:andrej.golov...@gmail.com>

Cc:swing-dev@openjdk.java.net <mailto:swing-dev@openjdk.java.net>

Subject: RE:  [11][JDK-4842658] RFR: DefaultListModel and 
DefaultComboBoxModel should support addAll (Collection c)

Hi Sergey,

Please update the test to catch the difference between v00 and v01.

Done.

Some comments about javadoc:

  - "183  * " is unnecessary

  - Do not use dots at the end of @param/@return tags

Done.

  > - Not sure that the text below is necessary(it is duplicate description 
of @exception tag):

  >  "202  * Throws an IllegalArgumentExceptionif the index 
was invalid."

Removed.

   > - It is unclear what this text means:

   >"207 if c doesn't fall in the range of the list."

   >  - Specify that the new methods will throw NPE on null Done.

   > - Why in one class you use addElements/addElementsAt names and in 
another addAll?

In the DefaultComboBoxModel, the functions to add a single element are 
named as addElement, addElementAt.

To keep with that convention, I named the apis appropriately.

   > - In the exception message you use "list" for "DefaultListModel" and 
"combobox" for DefaultComboBoxModel i

   > guess it should be "xxx model". Or you can simplify it to "Index out of 
range: + index"

Done.

It is also unclear what exception should be thrown,

IllegalArgumentException is a good candidate but other methods in

these classes like DefaultComboBoxModel.insertElementAt/removeElementAt will 
throw "ArrayIndexOutOfBoundsException", any thoughts/suggestions?

I was also a bit confused about this. Initially I referred to the function 
above (removeRange) and saw that it was throwing IllegalArgumentException. Now, 
I have changed the exception to ArrayIndexOutOfBoundsException as you suggested.

Here is the updated 
webrev:http://cr.openjdk.java.net/~kaddepalli/4842658/webrev02/
<http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev02/>

Thanks,

Krishna

On 09/04/2018 04:29, Krishna Addepalli wrote:

Hi Andrej,

Appreciate for the  quick review. Here is the updated webrev:

http://cr.openjdk.java.net/~kaddepalli/4842658/webrev01/
<http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev01/>

Thanks,

Krishna

-Original Message-

From: Andrej Golovnin 
<mailto:andrej.golov...@gmail.com>

Sent: Monday, April 9, 2018 3:15 PM

Re: [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)

2018-04-13 Thread Phil Race

On 04/12/2018 10:36 PM, Krishna Addepalli wrote:

Hi Phil,

Thank you for the review. Yes, those changes are not updated in the 
CSR. It only contains information about the new APIs being added.

@Phil, Sergey – could you mark the CSR reviewed, so that I can 
finalize it?

We first need to settle whether you will update the method names as
proposed by Sergey.

-phil.

Thanks,

Krishna

*From:*Phil Race
*Sent:* Thursday, April 12, 2018 10:53 PM
*To:* Krishna Addepalli ; Sergey 
Bylokhov ; Andrej Golovnin 

*Cc:* swing-dev@openjdk.java.net
*Subject:* Re:  [11][JDK-4842658] RFR: DefaultListModel and 
DefaultComboBoxModel should support addAll (Collection c)

I was only expecting you to update your new code to use {@code .. } but
since you've made the changes they are OK to keep.

Just make sure you *exclude* those other changes from the CSR, as
they aren't a spec. change and will clutter the CSR more than they are
cluttering the webrev.

+1

-phil.

On 04/12/2018 03:46 AM, Krishna Addepalli wrote:

Hi Phil,

Thank you for the review.

Here is the updated webrev:
http://cr.openjdk.java.net/~kaddepalli/4842658/webrev04/
<http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev04/>

Thanks,

Krishna

*From:*Phil Race
*Sent:* Wednesday, April 11, 2018 11:33 PM
*To:* Krishna Addepalli 
<mailto:krishna.addepa...@oracle.com>; Sergey Bylokhov
 <mailto:sergey.bylok...@oracle.com>;
Andrej Golovnin 
<mailto:andrej.golov...@gmail.com>
*Cc:* swing-dev@openjdk.java.net <mailto:swing-dev@openjdk.java.net>
*Subject:* Re:  [11][JDK-4842658] RFR: DefaultListModel
and DefaultComboBoxModel should support addAll (Collection c)

Use the syntax {@code c} instead of c everywhere
it is applicable.

Use @throws instead of @exception.
It is preferred these days.

http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#@exception

doesn't -> does not (we should be formal in the docs).

> 560  * from the index specified.

should become

"from the specified index."

 * @param index the index from which to start adding elements from

this reads awkardly .. as we have "from" twice. Can we instead say :

* @param index the position at which to start adding elements

Or, maybe even better, borrow the wording from java.util.List's docs

on the same named method since they are very similar :

|index| - index at which to insert the first element from the
specified collection

-phil.

On 04/11/2018 04:25 AM, Krishna Addepalli wrote:

Hi All,

Here is the modified 
webrev:http://cr.openjdk.java.net/~kaddepalli/4842658/webrev03/
<http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev03/>   as per 
Joe Darcy's suggestion to modify the api to accept a collection object that captures 
objects of classes that extend type E, in line with underlying Vector API.

Thanks,

Krishna

-Original Message-

From: Krishna Addepalli

Sent: Tuesday, April 10, 2018 6:14 PM

To: Sergey Bylokhov 
<mailto:sergey.bylok...@oracle.com>; Andrej Golovnin 
<mailto:andrej.golov...@gmail.com>

Cc:swing-dev@openjdk.java.net <mailto:swing-dev@openjdk.java.net>

Subject: RE:  [11][JDK-4842658] RFR: DefaultListModel and 
DefaultComboBoxModel should support addAll (Collection c)

Hi Sergey,

Please update the test to catch the difference between v00 and v01.

Done.

Some comments about javadoc:

  - "183  * " is unnecessary

  - Do not use dots at the end of @param/@return tags

Done.

  > - Not sure that the text below is necessary(it is duplicate 
description of @exception tag):

  >  "202  * Throws an IllegalArgumentExceptionif the 
index was invalid."

Removed.

   > - It is unclear what this text means:

   >"207 if c doesn't fall in the range of the list."

   >  - Specify that the new methods will throw NPE on null Done.

   > - Why in one class you use addElements/addElementsAt names and in 
another addAll?

In the DefaultComboBoxModel, the functions to add a single element are 
named as addElement, addElementAt.

To keep with that convention, I named the apis appropriately.

   > - In the exception message you use "list" for "DefaultListModel" and 
"combobox" for DefaultComboBoxModel i

   > guess it should be "xxx model". Or you can simplify it to "Index out of 
range: + i

Re: [11]JDK-8200605: Create test for GridBagLayoutDemo

2018-05-02 Thread Phil Race


  90 checkInteractionOnDispaly();
 239 private void checkInteractionOnDispaly() {

typo in the name here.

-phil.

On 05/01/2018 10:33 PM, Vikrant Agarwal wrote:

Hi Phil,

Please review this new Swingset client sanity test.

Best Regards,
Vikrant

-Original Message-
From: Vikrant Agarwal
Sent: Tuesday, April 10, 2018 6:54 PM
To: Sergey Bylokhov ; swing-dev@openjdk.java.net
Cc: Aleksandre Iline 
Subject: Re:  [11]JDK-8200605: Create test for GridBagLayoutDemo

Hi Sergey,

Thanks for the feedback, this failure was due to a bug in Mac, 
Component.getLocation() was giving an incorrect/inconsistent initial value when 
the component is located at (0,0). I have filed a bug for this issue: 
https://bugs.openjdk.java.net/browse/JDK-8201364 .

I have also updated the test to prevent JDK-8201364 from affecting the test.
Updated Webrev: http://cr.openjdk.java.net/~vagarwal/8200605/webrev.1/

Why we did not see this in our SBR?
I had very recently added this check and this was not sync'd with SBR code.

Best Regards,
Vikrant

-Original Message-
From: Sergey Bylokhov
Sent: Saturday, April 07, 2018 4:38 AM
To: Vikrant Agarwal ; swing-dev@openjdk.java.net
Cc: Aleksandre Iline 
Subject: Re:  [11]JDK-8200605: Create test for GridBagLayoutDemo

Hi, Vikrant.
I tried to run the new test but it fails on my local system(macos):

===
sanity/client/SwingSet/src/GridBagLayoutDemoTest.java
Total tests run: 4, Failures: 1, Skips: 0 
===

--System.err:(16/1069)--
Error:
"Wait "Component reaches location between :java.awt.Point[x=0,y=0]and 
java.awt.Point[x=0,y=0]" state to be reached (ComponentOperator.WaitStateTimeout)" action 
has not been produced in
60001 milliseconds
java.lang.Exception: failures: 1
at 
com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:96)
at 
com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:115)
at java.base/java.lang.Thread.run(Thread.java:831)

JavaTest Message: Test threw exception: java.lang.Exception: failures: 1 
JavaTest Message: shutting down test



On 03/04/2018 03:10, Vikrant Agarwal wrote:

Hi All,

Please review this new test for SwingSet GridBagLayoutDemo:

Webrev: http://cr.openjdk.java.net/~vagarwal/8200605/webrev.0/

Bug: https://bugs.openjdk.java.net/browse/JDK-8200605

Summary: This adds an automated test for SwingSet GridBagLayoutDemo
for all the available look and feels.

We have set up Same Binaries Run to check for stability and this test
is consistently passing for Linux, Mac and Windows for all the look and feels.

Best Regards,

Vikrant



--
Best regards, Sergey.

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

2018-05-04 Thread Phil Race


Two quick comments.

1) I'd like "Peer" removed from all the exported API.
2) It is probably stable enough now that you can start to add some javadoc.

-phil.

On 05/04/2018 05:00 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review an enhancement to remove the tight coupling of JDK 
internal class from FX so that
when javafx.* modules are removed from Openjdk build in jdk11, FX in 
general, and fx swing interop, in particular, can still build and 
function.


Right now, FX uses 6 jdk internal packages
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does so 
with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it need 
not be in the default module graph.


The module-info.java will look like this:

|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||


Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

||

Re: [11] RFR [TEST][JDK-8197948] Create test for SwingSet2 main window

2018-05-08 Thread Phil Race


+1

-phil.

On 05/02/2018 07:04 PM, Muneer Kolarkunnu wrote:


Hi All,

Please review the following, a new client sanity test case:

Task: https://bugs.openjdk.java.net/browse/JDK-8197948

Webrev Link:http://cr.openjdk.java.net/~akolarkunnu/8197948/webrev.01/ 



Summary: This is a new test for automated testing of SwingSet2 main 
window using Jemmy.


Aim of this is to test all swing components which are new in Swingset2 
main window and are not part of SwingSet3 demos.


Components to include in this test are JCheckBoxMenuItem , 
JRadioButtonMenuItem and different themes for Metal look and feel.


Only the file 
test/jdk/sanity/client/SwingSet/src/SwingSet2DemoTest.java is newly 
created. All others are taken from SwingSet2 demo with necessary 
changes for testing (mainly removed unwanted codes).


Regards,

Muneer

Re: [11] RFR: JDK-8199441: Wrong caret position in multiline text components on Windows with a screen resolution higher than 100%

2018-05-08 Thread Phil Race

I am not sure this is the right fix.
If the intention had been to change all calls to getTabbedTextOffset()
to use the FP
API it would have just been changed .. rather than adding a new API that
provides

the option to specify whether to use FP.

So perhaps the problem is that internal code needs to be updated to call
the FP

version directly ..
I'd like to read https://bugs.openjdk.java.net/browse/JDK-8168992 to see
what was said there but JBS just went down for 5 hours maintenance ..

-phil.

On 05/07/2018 11:54 PM, Prasanta Sadhukhan wrote:

Hi Sergey,

I have run javax/swing/text jtreg and jck tests and did not observe
any regressions.

Regards
Prasanta
On 4/28/2018 5:38 AM, Sergey Bylokhov wrote:

Hi, Prasanta.
Please confirm that you run related jck/reg tests.

On 26/04/2018 23:52, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix for an issue where it is seen that,
with a screen resolution higher than 100% and then clicking in a
JTextArea having setLineWrap(true) set, the caret (insertion point)
is not aligned with the cursor.

The issue seems to stem from the fact that caret position
calculation in DefaultCaret class utilises API that uses integer
calculation than floating point calculations.

The code flow as seen is
DefaultCaret#positionCaret=>BasicTextUI#viewToModel=>BoxView.viewToModel=>CompositeView#viewToModel=>WrappedPlainView#viewToModel=>Utilities.getTabbedOffset

Now, getTabbedOffset utilises FontMetrics.charsWidth() which uses
integer arithmetic to get the caret position.
The same getTabbedOffset uses Font.getStringBounds() which uses
floating point arithmetic via Rectangle2D.Float.

Proposed fix is to make sure getTabbedOffset uses floating point
calculations by using getStringBounds() instead of charsWidth() so
that it calculates
the character width(s) of text present in JTextArea in floating
point to align the caret.

Bug: https://bugs.openjdk.java.net/browse/JDK-8199441
webrev: http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.00/

Regards
Prasanta

RFR: 8202811: Problem List some tests that leave windows open on the desktop

2018-05-11 Thread Phil Race


bug: https://bugs.openjdk.java.net/browse/JDK-8202811
webrev: http://cr.openjdk.java.net/~prr/8202811/

This started out as wanting to problem list a few tests that left 
windows open
or otherwise did not terminate. And these are in there, but I also tried 
to clean up

most of the reproducible Linux failures I see on Ubuntu 16.04.
All the problem listed tests fail when being run one per jtreg 
invocation and

were repeatable.

-phil.

Re: RFR: 8202811: Problem List some tests that leave windows open on the desktop

2018-05-11 Thread Phil Race


PS .. I forgot I wanted to piggy back an update to TEST.ROOT to
add some remaining directories for jtreg to use othervm mode :

http://cr.openjdk.java.net/~prr/8202811.1/

-phil.


On 05/11/2018 10:46 AM, Phil Race wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8202811
webrev: http://cr.openjdk.java.net/~prr/8202811/

This started out as wanting to problem list a few tests that left 
windows open
or otherwise did not terminate. And these are in there, but I also 
tried to clean up

most of the reproducible Linux failures I see on Ubuntu 16.04.
All the problem listed tests fail when being run one per jtreg 
invocation and

were repeatable.

-phil.

Re: RFR: 8202811: Problem List some tests that leave windows open on the desktop

2018-05-11 Thread Phil Race


I expect that some tests that fail one one system will pass on another
and identifying the set up issues or environmental issues may be tricky.

-phil.


On 05/11/2018 01:57 PM, Sergey Bylokhov wrote:

Looks fine as a first step in our cleanup of the tests on lin/mac.

Note: I think that some of the tests fail because of some unrelated 
issues(other tests or some setup issues), at least some of them works 
fine on my mac, for example:
java/awt/Choice/ChoicePopupLocation/ChoicePopupLocation.java 8202931 
macosx-all


On 11/05/2018 12:08, Phil Race wrote:

PS .. I forgot I wanted to piggy back an update to TEST.ROOT to
add some remaining directories for jtreg to use othervm mode :

http://cr.openjdk.java.net/~prr/8202811.1/

-phil.


On 05/11/2018 10:46 AM, Phil Race wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8202811
webrev: http://cr.openjdk.java.net/~prr/8202811/

This started out as wanting to problem list a few tests that left 
windows open
or otherwise did not terminate. And these are in there, but I also 
tried to clean up

most of the reproducible Linux failures I see on Ubuntu 16.04.
All the problem listed tests fail when being run one per jtreg 
invocation and

were repeatable.

-phil.

Re: [11] Review Request: 8202878 com/apple/laf/ScreenMenu/ScreenMenuMemoryLeakTest.java fails

2018-05-11 Thread Phil Race


sorry about that.. +1 again.

-phil.

On 05/11/2018 02:48 PM, Sergey Bylokhov wrote:

The fix problemList is updated after JDK-8202811[1] was pushed:
http://cr.openjdk.java.net/~serb/8202878/webrev.01

[1] http://cr.openjdk.java.net/~prr/8202811.1/

On 10/05/2018 18:45, Philip Race wrote:

+1

-phil.

On 5/10/18, 6:42 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202878
Fix: http://cr.openjdk.java.net/~serb/8202878/webrev.00

The test tries to check that the JMenuItem will be collected by GC 
if it was removed from the menu. The problem is that the code below 
does not guarantied that gc will happen:


68 System.gc();
69 System.runFinalization();
70 Thread.sleep(1000);

In the fix it is replaced by the code which will generate OOM, to be 
sure that gc will happen.

Re: [OpenJDK 2D-Dev] [11] Review Request: 8199150 and 8150156

2018-05-30 Thread Phil Race


+1

-phil.

On 05/16/2018 08:16 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11, which fixed some accessibility issues 
in the specification.


Bug: https://bugs.openjdk.java.net/browse/JDK-8199150
 https://bugs.openjdk.java.net/browse/JDK-8150156
Fix: http://cr.openjdk.java.net/~serb/8199150/webrev.01

Description:
 - The links to "bugs.sun.com" were replaced by "bugs.java.com"
 - To most of our doc-files the "" tag is added.
 - In some files the numbers in "H" tag are renumbered to h1/h2/h3
 - In JTextComponent the tag was removed, this line was missed in 
JDK-8184219
 - The accessibility checkers report that the red color on the 
white/gray background has "Insufficient contrast", so I replaced red 
to blue color

Re: [11] RFR: JDK-8199441: Wrong caret position in multiline text components on Windows with a screen resolution higher than 100%

2018-06-05 Thread Phil Race

My explanation for my thinking is already written in the email you are
replying to ..

-phil.

On 05/31/2018 11:07 PM, Prasanta Sadhukhan wrote:

Hi Phil,

Any feedback as to why you find this is not the right fix? I could not
find anything in JDK-8168992 to guess it is not.

As told, there was no regression with jck and jtreg tests with the fix.

Regards
Prasanta
On 5/9/2018 12:41 AM, Phil Race wrote:

I am not sure this is the right fix.
If the intention had been to change all calls to
getTabbedTextOffset() to use the FP
API it would have just been changed .. rather than adding a new API
that provides

the option to specify whether to use FP.

So perhaps the problem is that internal code needs to be updated to
call the FP

version directly ..
I'd like to read https://bugs.openjdk.java.net/browse/JDK-8168992 to see
what was said there but JBS just went down for 5 hours maintenance ..

-phil.

On 05/07/2018 11:54 PM, Prasanta Sadhukhan wrote:

Hi Sergey,

I have run javax/swing/text jtreg and jck tests and did not observe
any regressions.

Regards
Prasanta
On 4/28/2018 5:38 AM, Sergey Bylokhov wrote:

Hi, Prasanta.
Please confirm that you run related jck/reg tests.

On 26/04/2018 23:52, Prasanta Sadhukhan wrote:

Hi All,

The issue seems to stem from the fact that caret position
calculation in DefaultCaret class utilises API that uses integer
calculation than floating point calculations.

The code flow as seen is
DefaultCaret#positionCaret=>BasicTextUI#viewToModel=>BoxView.viewToModel=>CompositeView#viewToModel=>WrappedPlainView#viewToModel=>Utilities.getTabbedOffset

Bug: https://bugs.openjdk.java.net/browse/JDK-8199441
webrev: http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.00/

Regards
Prasanta

RFR: 8203499 Uninitialised memory in WinAccessBridge.cpp

2018-06-05 Thread Phil Race


Bug: Uninitialised memory in WinAccessBridge.cpp:485
webrev: http://cr.openjdk.java.net/~prr/8203499/

element is not initialised on declaration.

It should be initialised when its address is passed here
messageQueue->remove(&element);

but it is too hard to prove, so just initialise it to NULL .. and remove 
DEBUG code which will SEGV if it is NULL.


-phil.

Re: [11] RFR: JDK-8199441: Wrong caret position in multiline text components on Windows with a screen resolution higher than 100%

2018-06-06 Thread Phil Race

This is changing the internals of getTabbedTextOffset and using FP 
regardless

of whether useFPAPI is true. This is not what I said.

I wrote that you should call the FP version directly.

Look at this
---
 * @deprecated replaced by
 * {@link #getTabbedTextOffset(Segment, FontMetrics, float, float,
 * TabExpander, int, boolean)}
 */
@Deprecated(since = "9")
public static final int getTabbedTextOffset(Segment s, FontMetrics 
metrics,

 int x0, int x, TabExpander e,
 int startOffset) {
return getTabbedTextOffset(s, metrics, x0, x, e, startOffset, 
true);


If we had wanted what you proposed in the first webrev and I think
this one is effectively same, we'd have altered the behaviour to and not
deprecated and added new API

So whatever code in our L&F is currently calling

getTabbedTextOffset(Segment s,
FontMetrics metrics,
int x0, int x, TabExpander e,
int startOffset)

should instead call

getTabbedTextOffset(Segment s,
FontMetrics metrics,
float x0, float x, TabExpander e,
int startOffset,
boolean round)

I am not sure if round should be true or false but calling the float version is 
the key.

This (and no other change needed) does the trick for me :

--- a/src/java.desktop/share/classes/javax/swing/text/WrappedPlainView.java
+++ b/src/java.desktop/share/classes/javax/swing/text/WrappedPlainView.java
@@ -360,11 +360,11 @@
 int currentWidth = getWidth();
 if (wordWrap) {
 p = p0 + Utilities.getBreakLocation(segment, metrics,
-tabBase, tabBase + 
currentWidth,
+(float)tabBase, 
(float)(tabBase + currentWidth),
 this, p0);
 } else {
 p = p0 + Utilities.getTabbedTextOffset(segment, metrics,
-   tabBase, tabBase + 
currentWidth,
+   (float)tabBase, 
(float)(tabBase + currentWidth),
this, p0, false);
 }
 SegmentCache.releaseSharedSegment(segment);
@@ -847,8 +847,8 @@
 Segment segment = SegmentCache.getSharedSegment();
 loadText(segment, p0, p1);
 int n = Utilities.getTabbedTextOffset(segment, metrics,
-   alloc.x, x,
-   WrappedPlainView.this, p0);
+   (float)(alloc.x), (float)x,
+   WrappedPlainView.this, p0, 
false);
 SegmentCache.releaseSharedSegment(segment);
 return Math.min(p0 + n, p1 - 1);
 }

I may have missed something else but to verify I downloaded and used your test, 
which BTW has DOS line-endings !

-phil.

On 06/06/2018 12:04 AM, Prasanta Sadhukhan wrote:



On 5/9/2018 12:41 AM, Phil Race wrote:

I am not sure this is the right fix.
If the intention had been to change all calls to 
getTabbedTextOffset() to use the FP
API it would have just been changed .. rather than adding a new API 
that provides

the option to specify whether to use FP.

So perhaps the problem is that internal code needs to be updated to 
call the FP

version directly ..

I have updated the internal code to use the FP version directly
http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.01/
There are no new regressions in jtreg and jck tests.

Regards
Prasanta

I'd like to read https://bugs.openjdk.java.net/browse/JDK-8168992 to see
what was said there but JBS just went down for 5 hours maintenance ..

-phil.

On 05/07/2018 11:54 PM, Prasanta Sadhukhan wrote:

Hi Sergey,

I have run javax/swing/text jtreg and jck tests and did not observe 
any regressions.


Regards
Prasanta
On 4/28/2018 5:38 AM, Sergey Bylokhov wrote:

Hi, Prasanta.
Please confirm that you run related jck/reg tests.

On 26/04/2018 23:52, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix for an issue where it is seen that,
  with a screen resolution higher than 100% and then clicking in a 
JTextArea having setLineWrap(true) set, the caret (insertion 
point) is not aligned with the cursor.


The issue seems to stem from the fact that caret position 
calculation in DefaultCaret class utilises API that uses integer 
calculation than floating point calculations.

The code flow as seen is
DefaultCaret#positionCaret=>BasicTextUI#viewToModel=>BoxView.viewToModel=>CompositeView#viewToModel=>WrappedPlainView#viewToModel=>Utilities.getTabbe

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

2018-06-14 Thread Phil Race


+1 from me for the JDK changes.

-phil.

On 06/13/2018 06:04 PM, Kevin Rushforth wrote:
The JDK changes look good to me, and as far as I have tested them, it 
works for me. I haven't tried Drag and Drop yet with the latest 
interface changes.


To repeat something I've mentioned before, just so there is no 
confusion, the FX side of the changes are intended to be a proof of 
concept to test the JDK side at this point. They will need refactoring 
before they can go in, so that FX will still be buildable and runnable 
with JDK 10.


-- Kevin


On 6/13/2018 12:48 AM, Prasanta Sadhukhan wrote:


Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
On 5/30/2018 4:13 AM, Philip Race wrote:

Some follow up comments.

On 5/10/18, 5:49 PM, Philip Race wrote:
I've looked over the Swing code. No time to actually try it out so 
I can only comment on what I see.


I am not sure what I think about class docs like "This class wraps 
 .."
I know no javadoc is generated but this module is exported and 
probably should say something

less specific.

In general I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.


Specific things we should look at in this file
- getDefaultScaleX/Y .. JDK 9 will set a transform on the graphics that
is passed to JComponent.paintComponent() .. so I wonder if we really
need this internal API. I doubt any other code is updating the transform
in a way that would make it a problem so the information you get 
should be valid.


The code to get the data array for the raster and associated information
can be extracted via standard API like this :

DataBuffer  db = BufferedImage.getRaster().getDataBuffer();
if (db instanceof DataBufferInt) {
 data = (DataBufferInt)db.getData();
}

So all of those can be fixed.

The code that accepts a native window handle maybe should require it 
is accessed via JNI ...
It could still be unsupported, but it would be package private or 
similar.

If you have a native ID, then you are already using native code.
This sidesteps any questions about the stability of such an API which
accepts a native resource.

The code that finds a component by location is probably harmless ...

I'm less sure about the event posting and focus grabbing support.
I'd like to have Sergey comment on those.

the DnD code is too much for me to examine in detail.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by 
deprecated-for-removal being included

today so we can get rid of it the next release if we want to.


I've looked at jdk.unsupported and they don't seem to have 
deprecation tags so

maybe that isn't an issue.

-phil


No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
I confirm that this fixes the issue. The interface change to make 
the two static methods e instance methods is a good change in any 
case.


I am not a Swing "R"eviewer, but the .13 webrev gets a +1 from me.

-- Kevin


On 5/10/2018 8:20 AM, Prasanta Sadhukhan wrote:


Hi Kevin,All,

Please find the modified webrev fixing this #1 issue
http://cr.openjdk.java.net/~psadhukhan/fxswing.13/
via change in 
jdk/swing/interop/DropTargetContextWrapper.java#setDropTargetContext 
and FXDnD.java#postDropTargetEvent


For me, #2 works, #3 doesn't work even now due to JDK-8141391 
 and #4 works 
for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

Hi Prasanta,

The API looks good now.

All of our automated tests work (except for the ones with a 
security manager due to JDK-8202451 
).


The only functional problem that I see is that Drag and Drop 
onto a SwingNode doesn't work. We need to make sure that we test 
the following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

Modified webrev to cater to this

http://cr.openjdk.java.net/~psadhukhan/fxswing.12/

Regards
Prasanta
On 5/9/2018 5:58 PM, Kevin Rushforth wrote:

The following can also be abstract:

LightweightContentWrapper:
  getComponent, createDragGestureRecognizer, 
createDragSourceContextPeer


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLo

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

2018-06-14 Thread Phil Race


you surely mean
@since 11

I believe it has been verified that it is excluded from the docs build 
.. right Prasanta ?


-phil

On 06/14/2018 01:26 PM, mandy chung wrote:
I reviewed the module-info.java change.  @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

2018-06-15 Thread Phil Race


+1

-phil.

On 06/15/2018 12:31 AM, Prasanta Sadhukhan wrote:
Webrev to add @since 11 to jdk.swing.interop classes (only difference 
from .14)


http://cr.openjdk.java.net/~psadhukhan/fxswing.15/

I also tried submitting mach5 job from linux but it is failing to 
download jib (although I candownload from browser and ping 
java.se.oracle.com)
~/Downloads/mach5-1.0.1865-distribution/bin/mach5 --remote-build 
--email prasanta.sadhuk...@oracle.com

Mach 5 Health State: green

Creating job description... done
Creating build ID... 2018-06-15-0626444.prasanta.sadhukhan.open
Publishing source using JIB... 
[2018-06-15T06:26:42,870Z][INFO][main][c.o.j.s.c.SparkyClient] 
Downloading JIB bootstrap script
Failed to download 
https://java.se.oracle.com/artifactory/jdk-virtual/com/oracle/java/jib/jib/3.0-SNAPSHOT/jib-3.0-SNAPSHOT.jib.sh.gz


I am trying from windows but Phil told windows build does not probably 
built docs.

Are there any easier alternative to verify the doc build?

Regards
Prasanta
On 6/15/2018 1:56 AM, mandy chung wrote:
I reviewed the module-info.java change. @since 12 is missing in 
jdk.unsupported.desktop module-info.java


Otherwise it's fine.

Does the docs build exclude jdk.unsupported.desktop?  You might have 
confirmed that that I missed.


Mandy

On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote:

Hi Phil, All

Please find modified webrev taking care of streamlining 
SwingInteropUtils class and handling of native window handle in 
LightweightFrameWrapper class by using JNI call in FX

http://cr.openjdk.java.net/~psadhukhan/fxswing.14/

Corresponding CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

RFR: 8205119: SwingApplet demo should be removed

2018-06-21 Thread Phil Race


Bug: https://bugs.openjdk.java.net/browse/JDK-8205119
Webrev : http://cr.openjdk.java.net/~prr/8205119/

This demo only demonstrates how to create a Swing Applet, nothing more, but
Applet is now deprecated, and the demo can't be run without 
appletviewer, so remove it.


-phil.

Re: [11] Review Request: 8205454 & is displayed in some Swing docs

2018-06-22 Thread Phil Race


+1

-phil.

On 06/22/2018 12:50 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8205454
Webrev: http://cr.openjdk.java.net/~serb/8205454/webrev.00

A few typos were fixed.
 - In some cases it was caused by the JDK-8025230(L&F in the 
javadoc in JPanel.java).
 - The bug in javabeans descriptions was caused by the converting 
these lines from the javadoc to the text in annotation JDK-4763438.

Re: [11] Review Request: 8201552 Ellipsis in "Classical" label in SwingSet2 demo with Windows L&F at Hidpi

2018-06-22 Thread Phil Race

The bug was seen when switching L&F .. how is that related to 
GraphicsConfiguration ?



-phil.

On 06/22/2018 01:48 PM, Sergey Bylokhov wrote:

Any volunteers for review?
=)

On 17/06/2018 15:37, Sergey Bylokhov wrote:
Unfortunately after additional testing I found a bug in our text 
related components. In the JTextPane the text looks broken if we 
request some change in the component after it is became visible.


For example if we change the font then the text will be overlapping. 
So if I will be applied this fix, which will force text component to 
relayout(because of the change in graphics config), then the text 
will be broken from the beginning.


But before the fix it will be broken only if the application will 
change the pane after it became visible(BTW text rendering during 
editing is broken in both cases).


So I temporary reverted the changes in the text related components:
http://cr.openjdk.java.net/~serb/8201552/webrev.02

Two follow bugs were created:
  - Text components: https://bugs.openjdk.java.net/browse/JDK-8205143
  - JSpinner: https://bugs.openjdk.java.net/browse/JDK-8205144

On 15/06/2018 23:31, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8201552
Webrev: http://cr.openjdk.java.net/~serb/8201552/webrev.01/

Short description:
This fix enhance implementation of JDK-8178025[1] for most of our 
Swing components. The main rule which I tried to follow:
"If layout of the component depends from the font then it should 
depend on the current graphics configuration as well, because 
FontMetrics depends on graphics configuration".


Long description:
The fix for JDK-8178025 added a special property 
"graphicsConfiguration" which: fired when the graphicsConfiguration 
is changed from one non-null value to another non-null value.
Those fix also updated some of the components(to refresh/re-validate 
its states when the "graphicsConfiguration" or "ancestor" were 
changed).


The usage of "ancestor" was not obvious, so I modify the code to 
fire "graphicsConfiguration" every time, this cover a situation when 
the "GC=null" is changed to "GC=non-null"(previously it was covered 
by "ancestor" property). So after this fix our components will 
listen only "font" and "graphicsConfiguration".


In implementation of JDK-8178025 the "graphicsConfiguration" is 
fired immediately after GC is changed, it caused the issues in some 
containers like JTree. When the container get such notification it 
usually tries to get some information from children, but in this 
moment children had previous graphic config, so the result 
calculated(and usually cached) in the container was wrong. In this 
fix I changed implementation of this property. Now it will fired 
only when the container and all its children are updated.


===
Note that the new test StalePreferredSize.java has a TODO block. 
Because JSpinner does not work properly even after the current fix. 
The reason is that during validation it is unexpectedly change the 
font from normal to bold(I'll fix this in a separate bug)



[1] https://bugs.openjdk.java.net/browse/JDK-8178025

Re: [11] Review Request: 8201552 Ellipsis in "Classical" label in SwingSet2 demo with Windows L&F at Hidpi

2018-06-28 Thread Phil Race

2336 var newGC = (GraphicsConfiguration) oldValue; 2337 var oldGC = 
(GraphicsConfiguration) newValue; 2338 var newTx = newGC != null ? 
newGC.getDefaultTransform() : null;

2339 var oldTx = oldGC != null ? oldGC.getDefaultTransform() : null;
2340 return !Objects.equals(newTx, oldTx); var ?? I suppose it I can 
infer what the type of newTx and oldTx is because I know the API, but 
for someone else this makes it less readable. I suppose your comments 
below are discussing the effect of line 2340 We generally consider a 
null TX to mean identity and so consider them equal but this would do 
the opposite .. and you say this is what Swing wants ? -phil.




On 6/28/2018 1:54 PM, Sergey Bylokhov wrote:

Hi, Phil.
The new webrev:
http://cr.openjdk.java.net/~serb/8201552/webrev.03/

I have checked why validation of the component is necessary, when the 
"ancestor" is changed (I removed it, but the new validation on GC 
change null->non-null is equivalent).


Such validation is used as a last chance to update the state of the 
component before show it to the user. In theory it is not necessary 
but some components skips validation on font change(like jtree) and 
maybe other properties. I have files a separate bug for that: 
https://bugs.openjdk.java.net/browse/JDK-8206024


 - In the fix I have moved these checks to SwingU2.
 - TODO block for JSpinner: is removed because this bug was fixed
https://bugs.openjdk.java.net/browse/JDK-8205144

On 22/06/2018 16:54, Philip Race wrote:

It is not really the GC .. there is no GC .. but because of that the
code uses a default FontRenderContext.

I think Swing is conflating GC + FRC but given that, then the logic
to recalculate based on graphicsconfiguration should be fine, although
maybe you can check if we really need to trigger it ..
if the screen we are shown on has no scale (ie uiScale == 1) then
we may be triggering a pointless invalidation with consequent overhead.
Can  you check if we can skip it in some cases ?

-phil.

On 6/22/18, 4:16 PM, Sergey Bylokhov wrote:
The place where the current graphicsConfiguration of the component 
is matter:

 - JComponent.getFontMetrics(Font)
 -- SwingUtilities2.getFontMetrics(this, font)
 --- SwingUtilities2.getFRCProperty(JComponent c)

    if (c != null) {
    GraphicsConfiguration gc = c.getGraphicsConfiguration();
    AffineTransform tx = (gc == null) ? null : 
gc.getDefaultTransform();

    Object aaHint = c.getClientProperty(KEY_TEXT_ANTIALIASING);
    return getFRCFromCache(tx, aaHint);
    }

When the bug is reproduced we calculated the size of the tree based 
on one GC and draw it using another GC. The first GC which is used 
for size calculation may be null or it maybe a non-null value 
pointed to another screen, for example if the jtree was shown on one 
screen and then dragged to another.


On 22/06/2018 14:40, Sergey Bylokhov wrote:
In the SwingSet2 the bug is visible only if the user switches the 
L&F immediately before switching to the tree demo. And is not 
reproduced if the user switches to the tree demo and then change 
the L&F.


  - If the tree is not visible then it will calculate the size of 
the text based on some default GC, and will not update this size 
when it will be added to the frame(which has another actual GC). So 
the tree will use the size calculated using one GC, and will draw 
the text using another GC. If the user will switch L&F the size 
will be recalculated using correct GraphicsConfiguration.






-phil.

On 06/22/2018 01:48 PM, Sergey Bylokhov wrote:

Any volunteers for review?
=)

On 17/06/2018 15:37, Sergey Bylokhov wrote:
Unfortunately after additional testing I found a bug in our text 
related components. In the JTextPane the text looks broken if we 
request some change in the component after it is became visible.


For example if we change the font then the text will be 
overlapping. So if I will be applied this fix, which will force 
text component to relayout(because of the change in graphics 
config), then the text will be broken from the beginning.


But before the fix it will be broken only if the application 
will change the pane after it became visible(BTW text rendering 
during editing is broken in both cases).


So I temporary reverted the changes in the text related components:
http://cr.openjdk.java.net/~serb/8201552/webrev.02

Two follow bugs were created:
  - Text components: 
https://bugs.openjdk.java.net/browse/JDK-8205143

  - JSpinner: https://bugs.openjdk.java.net/browse/JDK-8205144

On 15/06/2018 23:31, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk11.

Bug: https://bugs.openjdk.java.net/browse/JDK-8201552
Webrev: http://cr.openjdk.java.net/~serb/8201552/webrev.01/

Short description:
This fix enhance implementation of JDK-8178025[1] for most of 
our Swing components. The main rule which I tried to follow:
"If layout of the component depends from the font then it 
should depend on the current graphics configuration as well, 
because Font

Re: [11] Review Request: 8201552 Ellipsis in "Classical" label in SwingSet2 demo with Windows L&F at Hidpi

2018-06-28 Thread Phil Race


On 6/28/2018 3:14 PM, Sergey Bylokhov wrote:

On 28/06/2018 14:20, Phil Race wrote:
2336 var newGC = (GraphicsConfiguration) oldValue; 2337 var oldGC = 
(GraphicsConfiguration) newValue; 2338 var newTx = newGC != null ? 
newGC.getDefaultTransform() : null;

2339 var oldTx = oldGC != null ? oldGC.getDefaultTransform() : null;
2340 return !Objects.equals(newTx, oldTx); var ?? I suppose it I can 
infer what the type of newTx and oldTx is because I know the API, but 
for someone else this makes it less readable.


The type of "newGC/oldGC" is on the right where we apply the cast:
var newGC = (GraphicsConfiguration) oldValue;


and I wasn't questioning that one for that reason ..


The type of newTx/oldTx is not so important since we only check they 
are equivalent оr not. It will works even if the type will be Object 
instead of var.


That's the one I am questioning. I can't tell by reading the code what 
type is, so

I think this is a borderline use of var.

I suppose your comments below are discussing the effect of line 2340 
We generally consider a null TX to mean identity and so consider them 
equal but this would do the opposite .. and you say this is what 
Swing wants ? -phil.


In theory Swing wants to consider null as a identity TX, but in 
practice if we skip such validation we will expose other bugs like 
JDK-8206024. The change of "GC=null" to "GC=non-null" is occurred when 
the component id added to the parent and is used as a last chance to 
update the state of the component before show it to the user.


For example:
1 Jtree tree;
2 Dimension before = component.getPreferredSize();
3 tree.setFont(new Font());
 some other initialization
4 Dimension after = component.getPreferredSize();
5 frame.add(tree);

In line 3 the components should be revalidated because the font which 
affects the size was changed. But this validation can be skipped 
because of the some bugs. And it is not visible to the user just 
because we do validation at step 5(when the GC=null will be replaced 
by the GC=non-null). Before my fix it it was done using "ancestor" 
property(in some cases)


You are confirming what I thought you were saying.
Perhaps all such bugs could be fixed, and then we could fix the logic 
but that is

something that is beyond the scope of this fix.

Approved.

-phil.







On 6/28/2018 1:54 PM, Sergey Bylokhov wrote:

Hi, Phil.
The new webrev:
http://cr.openjdk.java.net/~serb/8201552/webrev.03/

I have checked why validation of the component is necessary, when 
the "ancestor" is changed (I removed it, but the new validation on 
GC change null->non-null is equivalent).


Such validation is used as a last chance to update the state of the 
component before show it to the user. In theory it is not necessary 
but some components skips validation on font change(like jtree) and 
maybe other properties. I have files a separate bug for that: 
https://bugs.openjdk.java.net/browse/JDK-8206024


 - In the fix I have moved these checks to SwingU2.
 - TODO block for JSpinner: is removed because this bug was fixed
https://bugs.openjdk.java.net/browse/JDK-8205144

On 22/06/2018 16:54, Philip Race wrote:

It is not really the GC .. there is no GC .. but because of that the
code uses a default FontRenderContext.

I think Swing is conflating GC + FRC but given that, then the logic
to recalculate based on graphicsconfiguration should be fine, although
maybe you can check if we really need to trigger it ..
if the screen we are shown on has no scale (ie uiScale == 1) then
we may be triggering a pointless invalidation with consequent 
overhead.

Can  you check if we can skip it in some cases ?

-phil.

On 6/22/18, 4:16 PM, Sergey Bylokhov wrote:
The place where the current graphicsConfiguration of the component 
is matter:

 - JComponent.getFontMetrics(Font)
 -- SwingUtilities2.getFontMetrics(this, font)
 --- SwingUtilities2.getFRCProperty(JComponent c)

    if (c != null) {
    GraphicsConfiguration gc = c.getGraphicsConfiguration();
    AffineTransform tx = (gc == null) ? null : 
gc.getDefaultTransform();

    Object aaHint = c.getClientProperty(KEY_TEXT_ANTIALIASING);
    return getFRCFromCache(tx, aaHint);
    }

When the bug is reproduced we calculated the size of the tree 
based on one GC and draw it using another GC. The first GC which 
is used for size calculation may be null or it maybe a non-null 
value pointed to another screen, for example if the jtree was 
shown on one screen and then dragged to another.


On 22/06/2018 14:40, Sergey Bylokhov wrote:
In the SwingSet2 the bug is visible only if the user switches the 
L&F immediately before switching to the tree demo. And is not 
reproduced if the user switches to the tree demo and then change 
the L&F.


  - If the tree is not visible then it will calculate the size of 
the text based on some default GC, and will not update this size 
when it will be add

RFR: 8206375 : ProblemList update of bug ID for SwingFontMetricsTest

2018-07-05 Thread Phil Race


Bug : https://bugs.openjdk.java.net/browse/JDK-8206375

One bug was closed as a dup of another, so the Problem List needs to be 
updated

to refer to the one that is still open:

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -798,7 +798,7 @@
 javax/swing/PopupFactory/8048506/bug8048506.java 8202660 windows-all
 javax/swing/JTextArea/TextViewOOM/TextViewOOM.java 8167355 generic-all
 javax/swing/JEditorPane/8195095/ImageViewTest.java 8202656 windows-all
-javax/swing/text/Utilities/8142966/SwingFontMetricsTest.java 8202663 
windows-all
+javax/swing/text/Utilities/8142966/SwingFontMetricsTest.java 8199529 
windows-all



-phil.

Re: [12] RFR: JDK-8206343: There is a typo in the java documentation of javax.swing.JScrollBar.

2018-07-16 Thread Phil Race

Since you are updating these lines, maybe you could fix the grammar as 
well as the spelling ? - * the most look and feels will not provide the 
scrolling to the right/down.
+ * then most look and feels will not provide the scrolling to the 
right/down.



- * up or down by large amount. Subclasses my override this
+ * up or down by large amount. Subclasses may override this - * the 
most look and feels will not provide the scrolling to the right/down.
+ * then most look and feels will not provide the scrolling to the 
right/down. -phil.



On 07/12/2018 04:46 AM, Krishna Addepalli wrote:


Hi All,

Please review a trivial typo corrections in JScrollbar. Apart from 
those specified in the bug, I have also corrected few others that I 
could find.


Bug: https://bugs.openjdk.java.net/browse/JDK-8206343

Webrev: http://cr.openjdk.java.net/~kaddepalli/8206343/webrev00/ 



Thanks,

Krishna

Re: [11]RFR:JDK-8208640: [a11y][macosx] Unable to navigate between Radiobuttons in Radio group using keyboard

2018-08-13 Thread Phil Race





On 08/13/2018 12:08 AM, Krishna Addepalli wrote:


Hi Phil,

I have run the JCK tests for JRadioButton and also focus traversal, 
and found no failures.


Here is the new webrev with additional formatting issues fixed: 
http://cr.openjdk.java.net/~kaddepalli/8208640/webrev02/ 





You are referring to this comment of mine ?

> You may have fixed the "){" pattern but I still see "if(" in v.01.

But v02 still has these

+ if(!isValidRadioButtonObj(eventSrc))
+ if(!isValidRadioButtonObj(curElement)) + if(activeBtn != null) { 
Comparing time stamps it looks like you just reposted v01 as v02 
http://cr.openjdk.java.net/~kaddepalli/8208640/webrev01/ 
http://cr.openjdk.java.net/~kaddepalli/8208640/webrev02/ I am APPROVING 
the fix, and assuming that this will be taken care of in the push. Go 
ahead and file the jdk11-fix-request paperwork ASAP. -phil.



Thanks,

Krishna

*From:*Prasanta Sadhukhan
*Sent:* Monday, August 13, 2018 9:00 AM
*To:* Philip Race 
*Cc:* Krishna Addepalli ; 
swing-dev@openjdk.java.net
*Subject:* Re:  [11]RFR:JDK-8208640: [a11y][macosx] Unable 
to navigate between Radiobuttons in Radio group using keyboard


On 8/12/2018 11:02 PM, Philip Race wrote:

I did notice the inner classes are not static, but this is the same as
the class from which they were copied and it seems like they need
access to the enclosing instance.

Can you expand on the path to the problem you see ?

I don't think you will have a memory leak due to multiple calls to
installListeners()
which is how I read your statement. That would suggest you are
talking about
the SelectNextBtn instance leaking ?

First, the reference is one-way. Once the map entry is
over-written, the previous
SelectNextBtn() instance will become eligible to be gc'd since
there will be nothing
referencing it. Marginally inefficient, but that is all I see.

But what about just the single call to installListeners() ? And if
you mean the RadioButtonUI will
leak ? Multiple calls to installListeners() won't affect that so
far as I can see.

I was referring to AquaRadioButtonUI leak. I guess you are right, if 
uninstallListeners is called, then it can be gced.


Regards
Prasanta

So long as uninstallListeners() is called then the map entry will
be cleared,
making the values referenced eligible for GC, in turn making the
RadioButtonUI
eligible for GC - which probably will typically happen only if the
app switches L&F so that is rare.

So I think there is only a bug if uninstallListeners() is not
called, and I presume
there has to be a pattern to make sure this happens across Swing ...

-phil.

On 8/12/18, 9:13 AM, Prasanta Sadhukhan wrote:

One thing from 1st look...

Every time installListeners is called, it instantiates
SelectPreviousBtn and SelectNextBtn which is a inner class
(and not a static class) and it has a strong reference to
AquaButtonRadioUI so it may not be garbage collected and can
cause a leak.

Regards
Prasanta

On 8/11/2018 8:51 PM, Krishna Addepalli wrote:

Hi All,

Please review a fix for JDK-8208640:
https://bugs.openjdk.java.net/browse/JDK-8208640

Webrev:
http://cr.openjdk.java.net/~kaddepalli/8208640/webrev00/


The problem is that the arrow key navigation for Aqua L&F
was not implemented, which is why neither the tab keys nor
the arrow keys were allowing to navigate through the radio
buttons through a button group.

Proposed fix is to copy the implementation from Basic L&F
into Aqua L&F. Also fixed the test that was written to
include the Aqua L&F to be tested, and it passes on Mac
successfully.

Thanks,

Krishna

Re: [12] JDK-8202013: JEditorPane shows large HTML unordered list bullets

2018-09-04 Thread Phil Race


+1

-phil.

On 09/04/2018 12:52 AM, Prasanta Sadhukhan wrote:
Modified webrev caching the original antialias state and restoring it 
at the end


http://cr.openjdk.java.net/~psadhukhan/8202013/webrev.1/

Regards
Prasanta
On 9/4/2018 4:58 AM, Philip Race wrote:



On 9/3/18, 12:24 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix for an issue where it is seen that
when displaying an HTML unordered list in a JEditorPane, the bullets 
generated by  looks larger than needed relative to text font 
size as seen in

https://www.dropbox.com/s/onv6v5xzutnvuyz/large-text-bullets.png?dl=0

This is because the StyleSheet#drawShape() routine hardcodes the 
bullet size to 8 which might look larger relative to some font size 
and smaller compared to larger font size.

Ideally, the bullet size should be relative to text font size.
Proposed fix is to make sure the bullet size is relative to text 
font size.

I have used bullet size to be 1/3rd of text font size as
https://www.w3schools.com/html/html_lists.asp "An Unordered List" 
bullets size comparison to text looks similar
and I could not find any docs stating what should be the ideal 
bullet size relative to font.


I expect it is allowed to display a bullet using a font.
The exact size would be out of control of the app.


Also, the fix takes into account the bullet is shown in middle to 
text lists, whereas previously, it is at the bottom (although we 
could not figure it out as the bullet size was large)


Bug: https://bugs.openjdk.java.net/browse/JDK-8202013
webrev: http://cr.openjdk.java.net/~psadhukhan/8202013/webrev.0/

This also fixes JDK-8201925: JEditorPane unordered list bullets look 
pixelated by using antialiasing to make the bullet shape smoother.


Same comment as the other fix .. you should leave the graphics state 
as you found it.


-phil.


Regards
Prasanta

Re: RFR; 8211031: Remove un-needed qualified export to java.desktop from java.base on macos

2018-09-26 Thread Phil Race


Not 100% sure what happened there. I've re-uploaded it.
Shift-reload to avoid browser caching and look again.

-phil.

On 09/26/2018 01:51 PM, Sergey Bylokhov wrote:

Hi, Phil.
On 21/09/2018 22:24, Philip Race wrote:
[*] at this exact moment, cr.openjdk.java.net isn't working but 
hopefully

that will be fixed by the time anyone reads this :-)


The classExists() method still there in version .1

Re: [12] RFR JDK-6828982: UIDefaults.getUI swallows original exception

2018-10-08 Thread Phil Race

That fixes one call site of getUIError, but like the submitter at least 
implied,

the problem is arguably down to the design of getUIError.
It will always print a stack trace that starts in getUIError itself.

///
   /**
 * If getUI() fails for any reason,
 * it calls this method before returning null.
 * Subclasses may choose to do more or less here.
 *
 * @param msg message string to print
 * @see #getUI
 */

    protected void getUIError(String msg) {
    try {
    throw new Error(msg);
    }
    catch (Throwable e) {
    e.printStackTrace();
    }
    }
///

So I agree it would have been better if it could have taken a possibly null
second exception parameter.
But I am not sure what we can do about it since as you can read,
the spec. actually says this exact method will be called by getUI()
Obviously the designers wanted to ensure a developer would see
the message but the exception would be handled, so as not to
possibly hang the UI.  I don't think this is worth the trouble to
add new API + respecify so somewhat reluctantly I will give it a +1,
although I'd be open to ideas.

-phil.

On 10/08/2018 12:40 AM, Prasanta Sadhukhan wrote:

Can I get a 2nd (R)eviewer for this?

Regards
Prasanta
On 29-Sep-18 3:55 PM, Krishna Addepalli wrote:

Looks fine.

Krishna

On 29-Sep-2018, at 10:28 AM, Prasanta Sadhukhan 
 wrote:


Hi All,

Please review a fix for an issue where it is seen that 
UIDefaults.getUI() swallows the original exception and only emits 
InvocationTargetException without the original stacktrace/message.
Proposed fix is to make sure the original exception/stackTrace is 
embedded in getUIError() message.


Bug: https://bugs.openjdk.java.net/browse/JDK-6828982
webrev: http://cr.openjdk.java.net/~psadhukhan/6828982/webrev.0/

Regards
Prasanta

Re: [8u-backport] JDK 8145547: [AWT/Swing] Conditional support for GTK 3 on Linux

2018-10-08 Thread Phil Race


Hi,

I've looked over the code and tested on Ubuntu 18.04 and can confirm it 
works

as well as the JDK 11 code does.

I reproduced the problems Semyon noted that are present on JDK 11 as well.

The UI hang when switching themes is not specific to using GTK3 - it 
occurs with
GTK2 as well, and not specific to Ubuntu 18.04, it occurs on Ubuntu 
16.04 too.



"AWT-EventQueue-0" #14 prio=6 os_prio=0 cpu=1537.25ms elapsed=16.86s 
tid=0x7f7d2c6dc000 nid=0x4a32 runnable  [0x7f7cca817000]

   java.lang.Thread.State: RUNNABLE
    at 
com.sun.java.swing.plaf.gtk.GTKEngine.native_switch_theme(java.desktop/Native 
Method)
    at 
com.sun.java.swing.plaf.gtk.GTKEngine.themeChanged(java.desktop/GTKEngine.java:625)

    - locked <0x000706613b68> (a java.lang.Object)
    at 
com.sun.java.swing.plaf.gtk.GTKLookAndFeel$WeakPCL$1.run(java.desktop/GTKLookAndFeel.java:1496)




It does NOT occur with current JDK 8u, so there is something in the GTK 
3 work
causing it. This is not a stopper for the backport since switching 
themes is unlikely / rare.
But we should file bugs against that and the rendering issues and fix in 
JDK 12 and backport
those too - as a follow on as at least some of these could be blockers 
for GTK3 on by default in 8u.


"+1" to this main backport.

-phil.

On 10/08/2018 01:23 AM, Pankaj Bansal wrote:

Hi,

The patch in 02 version no longer applies cleanly as check-ins have been done 
since it was created. I have updated the webrev. Also fixed the whitespace in 
gtk2_interface.c

Webrev:
http://cr.openjdk.java.net/~pbansal/gtk3_backport/webrev.03/

Regards,
Pankaj

-Original Message-
From: Kevin Rushforth
Sent: Wednesday, September 12, 2018 8:19 PM
To: Pankaj Bansal; awt-...@openjdk.java.net; swing-dev@openjdk.java.net
Subject: Re:   [8u-backport] JDK 8145547: [AWT/Swing] 
Conditional support for GTK 3 on Linux

Looks fine. Btw, you have a trailing whitespace in gtk2_interface.c that you 
need to fix before you push (jcheck will tell you about that).

-- Kevin


On 9/12/2018 5:29 AM, Pankaj Bansal wrote:

Hi,

I have found a small mistake in gtk_interface.c. I had used wrong name of gtk3 
lib at line 56. I have correct the same in webrev.02.
It is just a typo and should not have any effect on functionality. All should 
work the same as 01 version.

Webrev:
http://cr.openjdk.java.net/~pbansal/gtk3_backport/webrev.02/

Regards,
Pankaj


-Original Message-
From: Kevin Rushforth
Sent: Saturday, September 8, 2018 1:53 AM
To: Pankaj Bansal; Sergey Bylokhov; awt-...@openjdk.java.net;
swing-dev@openjdk.java.net
Subject: Re:   [8u-backport] JDK 8145547:
[AWT/Swing] Conditional support for GTK 3 on Linux

I tested various combinations of Swing / FX interop and it all looks fine to 
me. I only glanced through the AWT code changes, though, but I didn't spot 
anything amiss.

-- Kevin


On 9/6/2018 4:35 AM, Pankaj Bansal wrote:

Hello Sergey/Kevin,

I have removed the backport for 
https://bugs.openjdk.java.net/browse/JDK-8154546 : Retire 
sun.misc.GThreadHelper. I did a clean build and tried few tests for Swing-FX 
interop. They all run fine. I have attached the link to tests if you would like 
to have a look. I did run the awt and swing jtreg tests also.

Webrev: http://cr.openjdk.java.net/~pbansal/gtk3_backport/webrev.01/
SwingFXInteropTests:
http://cr.openjdk.java.net/~pbansal/gtk3_backport/SwingFXInteropTests
/

Please let me know if you would like me to do some further testing for Swing-FX 
interop.

Regards,
Pankaj Bansal

-Original Message-
From: Kevin Rushforth
Sent: Thursday, September 6, 2018 3:29 AM
To: Sergey Bylokhov; Pankaj Bansal; awt-...@openjdk.java.net;
swing-dev@openjdk.java.net
Subject: Re:   [8u-backport] JDK 8145547:
[AWT/Swing] Conditional support for GTK 3 on Linux

The simple testing that I did -- one each of a Swing app + JFXPanel
and a JavaFX app + SwingNode -- worked for me on my local build after
restoring that file. Some additional testing (necessarily limited to
GTK
2 until the FX backport for GTK 3 is done) might be needed.

-- Kevin


On 9/5/2018 2:35 PM, Sergey Bylokhov wrote:

HI, Pankaj.
Can you please recheck that FX<-->Swing interop still works?
Probably there are some other than GThreadHelper issues

On 05/09/2018 11:44, Pankaj Bansal wrote:

Hello Kevin,

Thanks for pointing it out. I will remove this fix for now. I hope
its ok, if I create new webrev when I get some more comments here.

Regards,

Pankaj Bansal

*From:*Kevin Rushforth
*Sent:* Wednesday, September 5, 2018 10:29 PM
*To:* Pankaj Bansal; awt-...@openjdk.java.net;
swing-dev@openjdk.java.net
*Subject:* Re:  [8u-backport] JDK 8145547: [AWT/Swing]
Conditional support for GTK 3 on Linux

The backport of the following fix, which removes
sun.misc.GThreadHelper, will break all FX interop applications,
even if GTK2 is used:

https://bugs.openjdk.java.net/browse/JDK-8154546 : Retire
sun.misc.GThreadHelper

The FX GTK port still uses that class, so the class needs to

Re: [12] Review Request: 8133713 [macosx] Accessible JTables always reported as empty

2018-10-11 Thread Phil Race

On 10/11/2018 3:32 PM, Sergey Bylokhov wrote:

On 10/10/2018 21:43, Philip Race wrote:
This then seems like only a partial solution and not exactly a step
to the complete solution.
Maybe we can push this fix but acknowledge it is just a bandage until
the right solution.
So is there a plan to fix this so we can announce a table as a table
- and announce

row & column names - rather than treat it as a grid ?

Yes, as described in the starting email it is not complete. We will
need to update this, so the jtable without the column/row headers will
be appeared as a grid(as in current fix), and the jtable with headers
will be appeared as a table. So the current code will not be dropped.

Ok. Let's push what we have.

-phil.

What does it take to fix it properly ?

I'll start to working on it after complete some other related things,
like jtree which is also complete not implemented.

-phil.

On 10/3/18, 5:56 PM, Sergey Bylokhov wrote:

On 02/10/2018 17:50, Philip Race wrote:

1) Why was "Table" wrong ? And why is "Grid" right ?

It is not wrong, but it requires a little bit different api to be
used. For example the table have an options to read the rows/columns
headers.
But the grid may work based on our current implementation(which were
added for lists). This is a current limitation.

2) What was the purpose of that anyway without the code to back it
up like :

+ } else if ([javaRole isEqualToString:@"table"]) {
+ newChild = [TableAccessibility alloc]; 3)

The code above creates an object of TableAccessibility which is our
class. This class provide support for the count of columns/rows in
the grid.

+ AccessibleTable table =

ac.getAccessibleTable(); Could table ever be null here ?

It should not be null for our tables, but it could be null for some
custom components:

http://cr.openjdk.java.net/~serb/8133713/webrev.01

-phil.

On 8/29/18, 3:03 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8133713
Webrev: http://cr.openjdk.java.net/~serb/8133713/webrev.00

This fix provides the minimal support of accessibility(VoiceOver)
for JTable.

The table will be represented as a grid. The user will get audio
information about:

- number of columns
- number of rows
- the selected cells
- the description of the whole table
- the text in the current cell

The user will be able to interact with the table(select/deselect
cells.)

Example and fix limitations are provided in the bug:
https://bugs.openjdk.java.net/browse/JDK-8133713?focusedCommentId=14207364&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14207364

Re: [12] Review Request: JDK-8212903 : [TestBug] Tests test/jdk/javax/swing/LookAndFeel/8145547/DemandGTK2.sh and DemandGTK3.sh fail on Ubuntu 18.04 LTS

2018-10-30 Thread Phil Race


+1

-phil.

On 10/30/18 4:00 AM, Pankaj Bansal wrote:


Hi All, Please review test fix for the below bug:

Bug: https://bugs.openjdk.java.net/browse/JDK-8212903

webrev: http://cr.openjdk.java.net/~pbansal/8212903/webrev.00/

Issue:

The tests DemandGTK2.sh and DemandGTK3.sh verify whether a particular 
version of gtk library is being used or not while using the 
GTKLookAndFeel. They use “strace” to find which gtk library has been 
opened by tracing the system command used to load the libraries. 
Currently, these tests are tracing the “open” which is used to open 
the library on Ubuntu 16.04 LTS.


These tests are failing on Ubuntu 18.04 LTS as the “openat”  command 
is used to open the libs instead of “open”. So the “openat” should be 
traced on 18.04 LTS.


Fix:

Made changes to the tests to trace both “open” and “openat”. The tests 
now pass on both Ubuntu 16.04 LTS and 18.04 LTS.


Regards,
Pankaj Bansal

Re: [12] RFR JDK-8213116:javax/swing/JComboBox/WindowsComboBoxSize/WindowsComboBoxSizeTest.java fails in Windows

2018-11-05 Thread Phil Race

On 11/4/18 8:54 PM, Prasanta Sadhukhan wrote:

On 04-Nov-18 12:02 AM, Philip Race wrote:

What a sorry list of bugs.
Didn't quite understand this statement... are you saying the original
combobox fix has spawned bad list of bugs?

Yes. I was just commenting on the list of issues you cite.

Can you
1) Add this evaluation to the bug report ?
2) Make sure - if you didn't already - that all related tests pass ?
It probably isn't too many ..

FYI...I have run all JCombobox swing tests which has passed.

Ok. +1, but update the bug report with the evaluation.

-phil.

Regards
Prasanta

-phil.

On 11/1/18, 11:02 PM, Prasanta Sadhukhan wrote:

Hi Sergey,

On 01-Nov-18 12:25 AM, Sergey Bylokhov wrote:

Hi, Prasanta.
On 31/10/2018 11:40, Prasanta Sadhukhan wrote:
This is a regression ofJDK-8203281:
[Windows]
JComboBox change in ui when editor.setBorder() is called where
combobox arrow button height is reduced by 2

It is reduced if the EmptyBorder is in use. Should the current fix
use the similar check?

We can use the similar check in the fix. Updated webrev
http://cr.openjdk.java.net/~psadhukhan/8213116/webrev.1/

Regards
Prasanta
[http://hg.openjdk.java.net/jdk/client/file/20e47d686221/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsComboBoxUI.java#l381]

but the minimum size of combobox is set to old values which was
added during
JDK-8179027:
JComboBox too
small under Windows LAF

which was done to address regression caused by
JDK-6490753:
Vista:JComboBox doesn't looks as native combobox in different
states of component.

Now, JDK-8203281
was also a
regression caused by JDK-6490753
so the combobox
size needs to be altered in the fix done for JDK-8179027
to make sure
the values are compatible.

Bug: https://bugs.openjdk.java.net/browse/JDK-8213116
webrev: http://cr.openjdk.java.net/~psadhukhan/8213116/webrev.0/

Regards
Prasanta

Re: [12] Review Request: 8213817 @return has already been specified (4 occurrences, in AWT and Swing)

2018-11-15 Thread Phil Race

Since you saw these with a docs build I assume you also verified with a 
docs build.


+1 on that assumption ..

-phil.

On 11/15/2018 10:42 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8213817
Webrev: http://cr.openjdk.java.net/~serb/8213817/webrev.00

In the fix a few duplicate @return tags were removed.

Note that in a few places like:
http://cr.openjdk.java.net/~serb/8213817/webrev.00/src/java.desktop/share/classes/javax/swing/table/JTableHeader.java.sdiff.html 


The duplicate tag is a result of the cleanup where we tried to change:
  @return "XXXUI"
to
  @return the string "XXXUI"

But it seems it was not applied to all classes, and
in a few cases like above the old tag was not removed.
This change fixes that as well.

Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

2018-12-11 Thread Phil Race

1) Thanks for uploading the webrev. much better for one person to do 
this than make
everyone who wants to look at it go through a tedious and off-putting 
set of steps.


2) I've added some client lists since you are touching UI client files, 
not just core-libs.
   To me the client ones look OK, one looks more like it was a typo 
than anything intentional,

   and the other was pretty mild.

3) Regarding the comment in the bug report about hb-private.hh and the 
use of

/* CRAP pool: Common Region for Access Protection. */
since it not only is in an upstream library, but also used 14 times in 
variable names,
then I can't possibly agree with your comment that an argument for 
leaving them
would be "shaky". Take this up with the upstream library ... I have no 
interest in

renaming these every time we upgrade this library.

-phil.

On 12/11/18 8:45 AM, Adam Farley8 wrote:

Sure thing:

http://cr.openjdk.java.net/~afarley/8215217/webrev/

Best Regards

Adam Farley
IBM Runtimes


Volker Simonis  wrote on 11/12/2018 15:46:44:


From: Volker Simonis 
To: adam.far...@uk.ibm.com
Cc: Java Core Libs 
Date: 11/12/2018 15:47
Subject: Re: RFR: JDK-8215217: OpenJDK Source Has Too Many Swear Words

Hi Adam,

in order to prevent me from using swear words, could you please upload
your webrev to cr.openjdk.java.net :)

As you may have realized webrevs are a collection of HTML files and it
makes no big sense to provide them as a zip file.

Thank you and best regards,
Volker
On Tue, Dec 11, 2018 at 4:04 PM Adam Farley8 

wrote:

Hey All,

I've spotted 12 instances of swear words in OpenJDK source comments,

and

it seems appropriate to remove them.

Bug: INVALID URI REMOVED

u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8215217&d=DwIBaQ&c=jf_iaSHvJObTbx-

siA1ZOg&r=P5m8KWUXJf-


CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=GfAb5QlDParO6DVrhdvPZTSafShnFACNF3JgqF-

_RkM&s=Qscaf2tTpPcZKpIelJ6SrP0uRYSFoKaCNATns0FX7_Y&e=

I've created a webrev and attached to the bug.

Also, I've mentioned in the bug that there are additional swears in

more

excusable locations. It would be good to get the community's take on
those.

Reviews and opinions welcome. :)

Best Regards

Adam Farley
IBM Runtimes

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with

number

741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6

3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: [12] JDK-8213516: jck test api/javax_accessibility/AccessibleState/fields.html fails intermittent

2018-12-20 Thread Phil Race


My understanding is that there is a JCK test that explicitly calls
AccessibleResourceBundle.getContents().

And yet the class docs for AccessibleResourceBundle state

 * This is meant only for internal use by Java Accessibility and is not
 * meant to be used by assistive technologies or applications

So arguably JCK should not have this test, but let's no go there yet.

The interesting thing is that nothing in the JDK uses this class anymore,
so JCK is the only thing that loads it.

So can we follow the clue, to see if that explicit loading
is somehow pre-empting the resources that should be used ?

-phil.


On 12/20/18 8:22 AM, Sergey Bylokhov wrote:

Hi, Shashi.

Can you please provide more details, why this deprecated resource 
bundle is loaded by the test?

It is strange because it should not be used, and was replaced by
"com.sun.accessibility.internal.resources.accessibility".
The AccessibleBundle explicitly load it via ResourceBundle.getBundle();

On 20/12/2018 00:40, Shashidhara Veerabhadraiah wrote:

Hi All,  Please find the updated Webrev for this problem:
http://cr.openjdk.java.net/~sveerabhadra/8213516/webrev.01/

Per the comments @ 
http://hg.openjdk.java.net/jdk/client/file/b5c564a1367c/src/java.desktop/share/classes/javax/accessibility/AccessibleState.java#l49, 
we need to update the missing entry for key 'managesDescendants' in 
AccessibleResourceBundle class. Any AccessibleState changes needs to 
be added an entry in AccessibleResourceBundle.  The same has been 
added under this fix. After this fix, I am unable to reproduce it on 
my local machine.


Thanks and regards,
Shashi

-Original Message-
From: Shashidhara Veerabhadraiah
Sent: Thursday, December 20, 2018 9:49 AM
To: Sergey Bylokhov ; 
swing-dev@openjdk.java.net; Philip Race ; 
Dmitry Bessonov 
Subject: Re:  [12] JDK-8213516: jck test 
api/javax_accessibility/AccessibleState/fields.html fails intermittent


Hi Sergey and Krishna, I was able to reproduce the problem on my 
machine yesterday evening. I will get back with another Webrev for 
this problem. So please wait for a day on this.


Thanks and regards,
Shashi

-Original Message-
From: Sergey Bylokhov
Sent: Thursday, December 20, 2018 2:58 AM
To: Shashidhara Veerabhadraiah 
; swing-dev@openjdk.java.net; 
Philip Race ; Dmitry Bessonov 

Subject: Re: [12] JDK-8213516: jck test 
api/javax_accessibility/AccessibleState/fields.html fails intermittent


Hi, Shashi.

Can you please describe the sequence of calls which might break the 
the static table?


BTW did you tried the system where Dmitry was able to reproduce the bug?

On 18/12/2018 22:24, Shashidhara Veerabhadraiah wrote:

Hi All, Please review a fix for the JDK 12 for the below bug:

Bug: https://bugs.openjdk.java.net/browse/JDK-8213516

Webrev: http://cr.openjdk.java.net/~sveerabhadra/8213516/webrev.00/

This issue is happening intermittently and am not able to reproduce it
on my local machine(corner case). As the bug reports issue is 
happening with jtreg running multiple tests on same VM mode. Upon 
analysis of the "AccessibleState" class and it's parent 
"AccessibleBundle" I think there may be a chance that class 
variable(static variable: table) present in AccessibleBundle can get 
into a corrupted state when multiple AccessibleState class variables 
are calling the method of parent class (loadResourceBundle() thro' 
toDisplayString() public functions) as it tries to load the same 
resource file into the same table(static variable) possibly from 
different threads(because of jtreg execution of multiple tests of 
the same bundle). Hence added a synchronized keyword to the function 
so that execution goes sequentially while loading the table(static 
hashtable). Since the problem is intermittent and not reproducible 
on local machine, test file is not required I think. Please suggest 
if you can find a different solution for this problem.



--
Best regards, Sergey.

Re: [12] JDK-8213516: jck test api/javax_accessibility/AccessibleState/fields.html fails intermittent

2018-12-20 Thread Phil Race





On 12/20/18 2:51 PM, Sergey Bylokhov wrote:
I have checked the test which uses AccessibleResourceBundle and I have 
two comments:
 - This test should not be a part of jck since it is not a part of 
public specification. 


What isn't ? Do you mean the class ?

If you mean the comment that it is not supposed to be called by external 
applications,

then yes, as I already pointed out,  but the class does appear in the spec.

https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/javax/accessibility/AccessibleResourceBundle.html

-phil.

Re: [12] JDK-8213516: jck test api/javax_accessibility/AccessibleState/fields.html fails intermittent

2018-12-20 Thread Phil Race


The peers were not part of the SE specification.
This class is, it just became obsolete so has been deprecated which
on its own has no spec impact. So I would not call it a similar situation.

As I pointed out in what might have been an off-list comment, we can 
consider
the deprecation for removal route, but that wouldn't solve the problem 
today.


But excluding the test is a possible option for 12, so we could defer fixing
the underlying regression until 13.

-phil.


On 12/20/18 3:33 PM, Sergey Bylokhov wrote:

On 20/12/2018 15:25, Phil Race wrote:

On 12/20/18 2:51 PM, Sergey Bylokhov wrote:
I have checked the test which uses AccessibleResourceBundle and I 
have two comments:
 - This test should not be a part of jck since it is not a part of 
public specification. 


What isn't ? Do you mean the class ?


I meant the class and the test which use it. BTW this class is a good 
candidate for "removal=true"


If you mean the comment that it is not supposed to be called by 
external applications,
then yes, as I already pointed out,  but the class does appear in the 
spec.


It is there because we generate the javadoc for all public classes, 
but the text for
this class clearly state that it should not be used. This situation is 
similar

to the API which uses peers.

Re: [12] JDK-8213516: jck test api/javax_accessibility/AccessibleState/fields.html fails intermittent

2019-01-15 Thread Phil Race


We definitely don't want a new public class here !
The fix seems very heavyweight for the problem, and I'm sure there must 
be simple

ways of creating a compound key like

resourceBundleName + locale.toString() ... or similar.

-phil.

On 1/14/2019 3:21 PM, Sergey Bylokhov wrote:

Hi, Shashi.

I am not sure I am understand the purpose of AccessibleBundleKey in 
your fix,

you created a class which can be used as a key, but eventually you
do not use it, except the calling of hashCode(). Note that the 
hashCode which you

use as a key for a table may be the same for different objects.

It will be useful to have a test case for the fix, please.


On 14/01/2019 02:49, Shashidhara Veerabhadraiah wrote:
Hi All, Please find the new Webrev implementing a different key for 
the accessibility resource tables.


http://cr.openjdk.java.net/~sveerabhadra/8213516/webrev.02/

Now with this implementation all the tests passes consistently.

Thanks and regards,
Shashi

-Original Message-
From: Sergey Bylokhov
Sent: Friday, December 21, 2018 12:42 PM
To: Shashidhara Veerabhadraiah 
; Philip Race 

Cc: swing-dev@openjdk.java.net; Dmitry Bessonov 
; Krishna Addepalli 

Subject: Re:  [12] JDK-8213516: jck test 
api/javax_accessibility/AccessibleState/fields.html fails intermittent


Hi, Shashi.
Yes we need a fix for this, see my comments here:
http://mail.openjdk.java.net/pipermail/swing-dev/2018-December/009248.html 



On 20/12/2018 21:57, Shashidhara Veerabhadraiah wrote:
Hi Phil, In that case I have put up the fix which fixes the problem 
as I was unable to reproduce this problem after that fix. So it does 
work for this situation. Can you please review it?


It adds the same missing element to the list which is being loaded 
to the cache and the same is used. Another solution is to force the 
resource bundle loading but that may be a bigger change and is too 
late to do that. The fix that I proposed is small and should not 
cause any issues at this late moment.


Thanks and regards,
Shashi

-Original Message-
From: Philip Race
Sent: Friday, December 21, 2018 10:48 AM
To: Shashidhara Veerabhadraiah 
Cc: Sergey Bylokhov ;
swing-dev@openjdk.java.net; Dmitry Bessonov
; Krishna Addepalli

Subject: Re:  [12] JDK-8213516: jck test
api/javax_accessibility/AccessibleState/fields.html fails intermittent

No, we can't remove it in 13 unless we deprecate for removal in 12 
and we are too late for that and in any case 6 months is very short.

This is not important enough to rush such a thing.
Removal before the next LTS should suffiice.

We can ask the JCK team as discussed but that does not mean we then 
don't need to fix the problem that caused this, even if we can defer 
it to 13.
Also "asking" does not guarantee approval, so a fix still needs to 
be in the works.


-phil.

On 12/20/18, 8:49 PM, Shashidhara Veerabhadraiah wrote:
Hi Phil\Sergey, As I understand from this, we should remove the 
AccessibleResourceBundle from JDK in 13(By using 
@Deprecated(forRemoval = true)) and meanwhile ask the JCK team to 
remove this test from the test suite?


Should the forremoval needs to be done now or later?

Thanks and regards,
Shashi

-Original Message-
From: Philip Race
Sent: Friday, December 21, 2018 6:54 AM
To: Sergey Bylokhov
Cc: Shashidhara
Veerabhadraiah;
swing-dev@openjdk.java.net; Dmitry
Bessonov; Krishna
Addepalli
Subject: Re:  [12] JDK-8213516: jck test
api/javax_accessibility/AccessibleState/fields.html fails
intermittent



On 12/20/18, 4:10 PM, Sergey Bylokhov wrote:

On 20/12/2018 15:44, Phil Race wrote:

The peers were not part of the SE specification.
This class is, it just became obsolete so has been deprecated which
on its own has no spec impact. So I would not call it a similar
situation.
No it was not part of the spec(and the deprecation notion is 
unrelated).

The notion that it should not be used and internal use only, is
there from the moment the class was moved to the 
"javax.accessibility"

package in 1998.

It is a public class in a public package and so forth.
But you can argue it out with JCK, as it is a waste of time to 
discuss it further here.


-phil.

As I pointed out in what might have been an off-list comment, we
can consider the deprecation for removal route, but that wouldn't
solve the problem today.

But excluding the test is a possible option for 12, so we could
defer fixing the underlying regression until 13.

-phil.


On 12/20/18 3:33 PM, Sergey Bylokhov wrote:

On 20/12/2018 15:25, Phil Race wrote:

On 12/20/18 2:51 PM, Sergey Bylokhov wrote:

I have checked the test which uses AccessibleResourceBundle and
I have two comments:
    - This test should not be a part of jck since it is not a
part of public specification.

What isn't ? Do you mean the class ?

I meant the class and the test which use it. BTW this class is a
good candidate for "removal=true"


If you mean the comment that it is not supposed to be cal

Re: [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text

2019-01-24 Thread Phil Race


I can't work out what you are trying to say.
The changes are only in the printing code path.

However what it looks like to me is that Prasanta is now using 
TextLayout for printing only under
the same conditions as we use it on screen -- which is when 
"international" text is being rendered.
This will reintroduce clipped text bugs which the existing code was 
meant to fix so I don't think it

can be right.

It may not be pretty or a complete solution but I think we need to look 
at breaking the text into
the leading spaces + the rest of the trimmed string and using the screen 
advance of the

leading spaces to position the rest of the justified text.

-phil.

On 1/24/19 9:33 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

On 24/01/2019 02:18, Prasanta Sadhukhan wrote:
Proposed fix is to check if we need text layouting for printing too 
as it is done for swing text drawing on console. If we need text 
layouting, then only use textlayout justification.


For swing components we skip textlayout by default because of 
performance reason(when we know that it should be safe to do), are you 
sure that it is safe in your case? Will the text scales in the same 
way as the components on which it drawn?


BTW the new codepath missed the call:
g2d.setColor(((PrintColorUIResource)col).getPrintColor());

Re: [13] RFR JDK-8217464: Remove resolved client bugs from the ProblemList.txt

2019-01-30 Thread Phil Race


+1

-phil

On 1/30/19 2:26 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review an updation of ProblemList to remove/update resolved 
client bugs from ProblemList


Bug: https://bugs.openjdk.java.net/browse/JDK-8217464
webrev: http://cr.openjdk.java.net/~psadhukhan/8217464/webrev.0/

Few test are closed as duplicate of other bug so the ProblemList is 
updated to use the open bugid.
JDK-8192929 is fixed in jdk12 forked repo with test removed from 
ProblemList so the updated ProblemList will be merged back to jdk/client

during merge process, so it is not updated now to avoid conflict.

Regards
Prasanta

Re: [OpenJDK 2D-Dev] [12] Review Request: 8213110 Remove the use of applets in automatic tests

2019-01-30 Thread Phil Race



You discuss the life cycle of an applet in jtreg
---
  - Call init() and start() methods of the Applet
  - Waits for 2 seconds
  - Call stop() and destroy() methods of the Applet
  - Dispose the frame
--

I see init() and start() being used but I don't see the rest replaced. 
Why not ?
Do you really mean jtreg() kills the applet just two seconds after 
start() returns ?



For the tests where you do something like
 main(...) {

obj.init();
obj.start();

eg here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html
would it make sense to invoke that code on the EDT instead of the main 
thread ?


EventQueue.invokeLater(new Runnable() {

Like already happens here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java.sdiff.html
We have lots of AWT tests that use the main thread so not a blocker but 
just a question and
likely could mean further updates to some tests are needed if an 
exception no longer propagates

on an expected thread.


-phil.

On 11/26/18 6:29 PM, Sergey Bylokhov wrote:

The "ProblemList.txt" is updated in the new version:
http://cr.openjdk.java.net/~serb/8213110/webrev.04

On 08/11/2018 10:57, Sergey Bylokhov wrote:
Note that for some of the tests I'll need to update the problem list 
and replace the .html by the .java.
But since we actively updates the problem lists right now I postponed 
this task for a weekend.


On 07/11/2018 15:47, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8213110
Webrev: http://cr.openjdk.java.net/~serb/8213110/webrev.03

Description of the bug:
   A number of our tests in jdk still launch by applets via "@run 
applet".

Usually we drop this usage when we update the test for some reason, and
in this fix we will drop them completely for automated tests from 
the open repo.
I did not update the tests which are specific for Applet API, manual 
tests, or tests

which are currently under review for some other bugs.

Note: the "@run applet" means, that the jtreg will do these steps:
  - Creates a frame as a holder for the Applet
  - Creates an applet(which is undecorated panel) and adds it to the 
frame

  - Sets the size of the frame
  - Place the frame to the center of the screen
  - Make the frame visible
  - Call init() and start() methods of the Applet
  - Waits for 2 seconds
  - Call stop() and destroy() methods of the Applet
  - Dispose the frame


Description of the fix:

  - In all cases the usage of the Applet API was dropped
  - In the common case when the applet was used as launcher, this 
code now used instead:

    public static void main(final String[] args) {
   TestName app = new TestName();
   app.init();
   app.start();
    }
    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Choice/PopdownGeneratesMouseEvents/PopdownGeneratesMouseEvents.java.sdiff.html

  - In some cases it was possible to replace init()/start() by the 
simple main() method.

    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Choice/PopupPosTest/PopupPosTest.java.sdiff.html

  - Some of the tests were used the "extend Applet" w/o a reasons:
    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowBlockingTest.java.sdiff.html

  - Some of the tests shows the applet window(which was unrelated to 
the test itself) in the center of the screen.

    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowRetaining.java.sdiff.html

  - Some of the tests uses the applet as a place holder for the test 
components.
    In this case it was necessary to change the Applet API to the 
Frame API, and complete

    the steps which were done by the jtreg:
    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html


Notes:
  - To simplify the review I tried to not change the logic of the 
tests,
    so if the test fail before the fix, then it will fail after the 
fix.
    I would like to create the separate CRs for all additional(if 
any) cleanup of these tests.

  - Just a few exception from the above is additional calls to:
    "setLocationRelativeTo" to place the window to the center of the 
screen
    "robot.waitForIdle()"/"setUndecorated()" to make the tests more 
stable(when I compared the tests before/after the fix)

Re: [13] Review Request: 8061381 [macosx] Accelerators does not spelled for JMenuItems by Voice Over

2019-01-30 Thread Phil Race


+1

-phil.

On 1/24/19 9:16 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 13.

Bug: https://bugs.openjdk.java.net/browse/JDK-8061381
Fix: http://cr.openjdk.java.net/~serb/8061381/webrev.00

This fix adds the support of swing menu accelerators for Voice Over on 
macOS
(note that swing menu is used when the "apple.laf.useScreenMenuBar" is 
false).
If the property "apple.laf.useScreenMenuBar" is true we use the native 
menu which

is already supported by the VoiceOver.

The fix will convert the accelerator to the string which is passed to 
the VO, the logic
which is used to access the KeyStroke and convert it to the string is 
similar to what

we use on windows in the accessbridge.

Re: [12] RFR JDK-8210807:Printing a JTable with a JScrollPane prints table without rows populated

2019-01-30 Thread Phil Race


755 if (g instanceof Graphics2D &&
756 !((g instanceof sun.print.PeekGraphics) ||
757 (g instanceof sun.print.PathGraphics))) {


You should test for the public interfaces .. I am not sure what you are 
doing here
is going to do when we end up in banded printing which uses neither of 
these


sun/swing/SwingUtilities2.java has this :
   /*
 * returns true if the Graphics is print Graphics
 * false otherwise
 */
    static boolean isPrinting(Graphics g) {
    return (g instanceof PrinterGraphics || g instanceof 
PrintGraphics);

    }

you could perhaps make it public and call that.

-phil

On 11/12/18 3:36 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix for an issue where it is seen that a simple JTable 
doesn't print correctly if the table is contained in a JScrollPane.
This is a regression of the fixJDK-8164032: 
JViewport backing 
store image is not scaled on HiDPI display

where JViewport's backing store image is scaled in the fix.
It seems this scaling of backing store image is not needed for 
printing as the original backing store image or base image was printed 
correctly before this fix.
Proposed fix is to check if the graphics object is been used for 
printing ie PeekGraphics/pathGraphics then skip this backing store 
image scaling.


Bug: https://bugs.openjdk.java.net/browse/JDK-8210807
webrev: http://cr.openjdk.java.net/~psadhukhan/8210807/webrev.0/

8164032's manual regression testcase behaves in the same way before 
and after this fix.


Regards
Prasanta

Re: [OpenJDK 2D-Dev] [12] Review Request: 8213110 Remove the use of applets in automatic tests

2019-01-30 Thread Phil Race

Oh .. and I meant to ask what probably is an obvious question but needs 
to be asked anyway.
You say you did not try to fix tests that already fail - this is just a 
conversion - but did you

confirm there are no NEW failures on any platform ?

-phil.

On 1/30/19 2:29 PM, Phil Race wrote:


You discuss the life cycle of an applet in jtreg
---
  - Call init() and start() methods of the Applet
  - Waits for 2 seconds
  - Call stop() and destroy() methods of the Applet
  - Dispose the frame
--

I see init() and start() being used but I don't see the rest replaced. 
Why not ?
Do you really mean jtreg() kills the applet just two seconds after 
start() returns ?



For the tests where you do something like
 main(...) {

obj.init();
obj.start();

eg here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html
would it make sense to invoke that code on the EDT instead of the main 
thread ?


EventQueue.invokeLater(new Runnable() {
Like already happens here :
http://cr.openjdk.java.net/~serb/8213110/webrev.04/test/jdk/java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java.sdiff.html
We have lots of AWT tests that use the main thread so not a blocker 
but just a question and
likely could mean further updates to some tests are needed if an 
exception no longer propagates

on an expected thread.


-phil.

On 11/26/18 6:29 PM, Sergey Bylokhov wrote:

The "ProblemList.txt" is updated in the new version:
http://cr.openjdk.java.net/~serb/8213110/webrev.04

On 08/11/2018 10:57, Sergey Bylokhov wrote:
Note that for some of the tests I'll need to update the problem list 
and replace the .html by the .java.
But since we actively updates the problem lists right now I 
postponed this task for a weekend.


On 07/11/2018 15:47, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk 12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8213110
Webrev: http://cr.openjdk.java.net/~serb/8213110/webrev.03

Description of the bug:
   A number of our tests in jdk still launch by applets via "@run 
applet".
Usually we drop this usage when we update the test for some reason, 
and
in this fix we will drop them completely for automated tests from 
the open repo.
I did not update the tests which are specific for Applet API, 
manual tests, or tests

which are currently under review for some other bugs.

Note: the "@run applet" means, that the jtreg will do these steps:
  - Creates a frame as a holder for the Applet
  - Creates an applet(which is undecorated panel) and adds it to 
the frame

  - Sets the size of the frame
  - Place the frame to the center of the screen
  - Make the frame visible
  - Call init() and start() methods of the Applet
  - Waits for 2 seconds
  - Call stop() and destroy() methods of the Applet
  - Dispose the frame


Description of the fix:

  - In all cases the usage of the Applet API was dropped
  - In the common case when the applet was used as launcher, this 
code now used instead:

    public static void main(final String[] args) {
   TestName app = new TestName();
   app.init();
   app.start();
    }
    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Choice/PopdownGeneratesMouseEvents/PopdownGeneratesMouseEvents.java.sdiff.html

  - In some cases it was possible to replace init()/start() by the 
simple main() method.

    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Choice/PopupPosTest/PopupPosTest.java.sdiff.html

  - Some of the tests were used the "extend Applet" w/o a reasons:
    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowBlockingTest.java.sdiff.html

  - Some of the tests shows the applet window(which was unrelated 
to the test itself) in the center of the screen.

    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowRetaining.java.sdiff.html

  - Some of the tests uses the applet as a place holder for the 
test components.
    In this case it was necessary to change the Applet API to the 
Frame API, and complete

    the steps which were done by the jtreg:
    Example:
http://cr.openjdk.java.net/~serb/8213110/webrev.03/test/jdk/java/awt/Focus/AppletInitialFocusTest/AppletInitialFocusTest.java.sdiff.html


Notes:
  - To simplify the review I tried to not change the logic of the 
tests,
    so if the test fail before the fix, then it will fail after the 
fix.
    I would like to create the separate CRs for all additional(if 
any) cleanup of these tests.

  - Just a few exception from the above is additional calls to:
    "setLocationRelativeTo" to place the window to the center of 
the screen
    "robot.waitForIdle()"/"setUndecorated()" to make the tests more 
stable(when I compared the tests before/after the fix)

Re: [12] JDK-8182043: Access to Windows Large Icons

2019-01-30 Thread Phil Race


I am not sure how an application is supposed to know what size to use,
which seems a bit of a problem with this API.

And does this work equally well on all platforms and resolutions ?
The extreme weirdness that you give it the name of a system object the 
application

has to find and extract icons from is very odd.

I don't like this API at all. I'd rather do nothing.

And the supported size range is odd, and the potential for null returns 
isn't specified.



-phil.

On 1/22/19 10:03 AM, Shashidhara Veerabhadraiah wrote:


Hi All, Please review the below new fix compared to earlier webrevs. 
Now the new api to get the icons of different sizes is being made part 
of a new class SystemIcon. Please consider this as a different 
solution than the one that was under review for a long time till now.


http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.05/

Part of the fix is being borrowed from Semyon’s fix and would like to 
thank Semyon for that.


Thanks and regards,

Shashi

*From:*Alexey Ivanov
*Sent:* Thursday, October 11, 2018 4:32 AM
*To:* Shashidhara Veerabhadraiah 
; Prasanta Sadhukhan 
; swing-dev 
; awt-dev 
*Subject:* Re:   [12] JDK-8182043: Access to 
Windows Large Icons


Hi Shashi,

Thank you for updating the review.

With updated copyright years, the patch does not apply cleanly because 
some files already have 2018. It's a minor nuisance which could be 
easily resolved.


Other comments inline:

On 28/09/2018 09:58, Shashidhara Veerabhadraiah wrote:

Hi Alexey, Thank you for your thorough review. I have updated the
copyrights as well and please see below for my comments:

Here is the new Webrev:
http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.04/


Thanks and regards,

Shashi

*From:*Alexey Ivanov
*Sent:* Tuesday, September 25, 2018 3:00 AM
*To:* Shashidhara Veerabhadraiah

; Prasanta Sadhukhan

; swing-dev
 ;
awt-dev  
*Subject:* Re:   [12] JDK-8182043: Access to
Windows Large Icons

Hi Shashi,

Please see my comments inline:

On 21/09/2018 23:22, Shashidhara Veerabhadraiah wrote:

Hi Alexey, Thanks for your review and below is the new Webrev.

http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.03/


Please see below for inline comments.

Thanks and regards,
Shashi

*From:*Alexey Ivanov
*Sent:* Friday, September 21, 2018 2:09 PM
*To:* Shashidhara Veerabhadraiah

; Prasanta
Sadhukhan 
; swing-dev

; awt-dev
 
*Subject:* Re:   [12] JDK-8182043: Access
to Windows Large Icons

Hi Shashi,

SystemIcon.java
What is the purpose of new SystemIcon class?
It's not used anywhere but the provided test. Is this class
really needed then?
Is it supposed to become the public API for accessing system
icons?
Why can't FileSystemView be used for that purpose as it was
proposed in Semyon's review?
*/[Shashi] /*SystemIcon is going to be the front face to
access the icons and that is the purpose of this class. The
reason for choosing this is that FileSystemView class can be
used internally and did not wanted to expose it externally
too. Externally exposing may cause certain restriction in
maintaining the classes hence the indirection.


Still, I cannot understand the rationale for a new class the only
purpose of which is to provide public access to
getSystemIcon(File, int, int).
FileSystemView is already a public class, and it's used
internally. (I guess it would not have existed, if it hadn't.) It
has a public method getSystemIcon(File). As such, extending its
functionality to get access to larger icons seems logical. This is
what the new protected getSystemIcon(File f, int size) does.

It can be made public to facilitate access to file icons.
After all, protected method is also a contract, it cannot be
changed without affecting backward compatibility.

It is this new protected method that performs the task of getting
the icon from the system.

Do we really need other methods?

*/[Shashi] I think that system icons functions as part of
filesystemview class is also a kind of corrupted creation of the
filesystemview class. Icons forms a different functionality
compared to file system and should have been kept as a separate
class in the first place./*


I agree

Re: [13] JDK-8190361: Incorrect version info in jaccessinspector.exe and jaccesswalker.exe

2019-02-08 Thread Phil Race

So yes .. it changes that .. but could you explain why the new string is 
correct ?
I don't see an evaluation here, or in the bug report - where there 
should be one!

Also the indentation seems wrong  - even when I look at the "raw" file.

-phil.

On 2/8/19 1:07 AM, Shashidhara Veerabhadraiah wrote:


Hi All, Please review a fix for the below customer bug.

Bug: https://bugs.openjdk.java.net/browse/JDK-8190361

Webrev: http://cr.openjdk.java.net/~sveerabhadra/8190361/webrev.01/

The version string for the jaccessinspector and jaccesswalker modules 
had the string “JDK_BUILD_ID” and this change fixes that. The only 
doubt I had is that, hopefully these resource files are not generated 
from a different file and is editable by manual way.


Thanks and regards,

Shashi

Re: [12] RFR JDK-8218479: JTextPane display issue with GTKLookAndFeel

2019-02-13 Thread Phil Race


+1

-phil.

On 2/12/19 2:46 PM, Semyon Sadetsky wrote:

Hello,

Please review a fix for JDK 12.

JBS: https://bugs.openjdk.java.net/browse/JDK-8218479

webrev: http://cr.openjdk.java.net/~ssadetsky/8218479/webrev.00/

This is a JCK test failure fix. The test fails under the 
GTKLookAndFeel on Linux platforms with gtk+-3.20+ library installed.


Starting gtklib- 3.20 the  widget drawing styles were totally reworked 
so the background layer of the TEXT_PANE widget should be painted 
explicitly. To fix the issue the corresponding area type is made 
opaque. The fix was tested on all major supported Linux platforms.


--Semyon

Re: [12] RFR JDK-8218473: JOptionPane display issue with GTKLookAndFeel

2019-02-13 Thread Phil Race


+1 - fixed it for me.

-phil.

On 2/12/19 6:55 PM, Sergey Bylokhov wrote:

Looks fine.

On 12/02/2019 14:40, Semyon Sadetsky wrote:

Hello,

Please review a fix for JDK 12.

JBS: https://bugs.openjdk.java.net/browse/JDK-8218473

webrev: http://cr.openjdk.java.net/~ssadetsky/8218473/webrev.00/

This is a JCK test failure fix. The test fails under the 
GTKLookAndFeel on Linux platforms with gtk+-3.20+ library installed.


Starting gtklib- 3.20 the  widget drawing styles were totally 
reworked so the text selection background cannot be obtained using 
TEXT_FIELD widget anymore. To fix that the corresponding request is 
redirected to the TEXT_AREA widget. The fix was tested on all major 
supported Linux platforms.


--Semyon

Re: [12] RFR JDK-8218469, JDK-8218470, JDK-8218472, JDK-8203627 : GTK3 rendering fixes for tck-red bugs

2019-02-13 Thread Phil Race


I've tested this fix on
- OL 7.3 (aka RH 7.3)
- OL 7.6 (aka RH 7.6)
- Ubuntu 16.04
- Ubuntu 18.04
- Ubuntu 18.10

All looks good to me.

-phil.

On 2/12/19 1:42 PM, Pankaj Bansal wrote:


Hi All,

Some more fixes are included in the patch to fix some rendering issues 
with RadioButton or CheckBox in MenuItems, RadioButton and Checkbox 
state flags.


webrev: http://cr.openjdk.java.net/~pbansal/8218469/webrev01/

-Pankaj

*From:*Pankaj Bansal
*Sent:* Tuesday, February 12, 2019 11:38 PM
*To:* swing-dev@openjdk.java.net
*Subject:*  [12] RFR JDK-8218469, JDK-8218470, JDK-8218472, 
JDK-8203627 : GTK3 rendering fixes for tck-red bugs


Hi All,

Please review the following fix.


Bug:

https://bugs.openjdk.java.net/browse/JDK-8218469 -  JSlider display 
issue with slider for GTKLookAndFeel


https://bugs.openjdk.java.net/browse/JDK-8218470-  JScrollBar display 
issue with GTKLookAndFeel


https://bugs.openjdk.java.net/browse/JDK-8218472- JProgressBar display 
issue with GTKLookAndFeel


https://bugs.openjdk.java.net/browse/JDK-8203627 - Swing applications 
with JRadioButton and JCheckbox fail to render correctly when using 
GTK3 and the GTK L&F


webrev

http://cr.openjdk.java.net/~pbansal/8218469/webrev00/

Issue:

From gtk 3.20, gtk has changed the way themes and styles work for many 
widgets. Due to which, jdk is not able to render some widgets properly 
including JSlider, JProgressBar, JScrollBar, JRadioButton, JCheckbox.


gtk 3.20 release notes: 
https://developer.gnome.org/gtk3/stable/ch32s10.html


Fix:

This patch fixes rendering issues in all these widgets by using 
correct way to define styles.



Regards,
Pankaj Bansal

Re: [13] JDK-8216008: -Djavax.accessibility.assistive_technologies empty list leads to crash

2019-02-14 Thread Phil Race

+ * activate method. If the list of assistive technology providers is 
empty string, is "the" empty string. The CSR needs to be updated to 
include this spec. What is there now in the specification section needs 
to be fixed It should not point to the webrev or review and right now 
doesn't contain the change to the javadoc in the body of the CSR I think 
I even have to question most of the rest of it. Any one reading it would 
think that we always used to throw an exception in such case and now 
want to stop doing so. Isn't the bug that we did NOT throw an exception 
and now we do ? You only hint at that when you say at the end of 
"Solution", "as was the case earlier" If I am right the only thing you 
need in the CSR is to say that you are reverting the implementation to 
previous behaviour and updating the specification to make it clear that 
this behaviour is allowed by the spec since this case was unclear. -phil.




On 2/12/19 1:08 AM, Shashidhara Veerabhadraiah wrote:


Hi Phil, Here is the new Webrev and CSR for the same:

http://cr.openjdk.java.net/~sveerabhadra/8216008/webrev.04/

CSR: https://bugs.openjdk.java.net/browse/JDK-8218737

Thanks and regards,

Shashi

*From:*Philip Race
*Sent:* Saturday, February 9, 2019 2:18 AM
*To:* Shashidhara Veerabhadraiah 
*Cc:* Sergey Bylokhov ; 
awt-...@openjdk.java.net; swing-dev@openjdk.java.net
*Subject:* Re:  [13] JDK-8216008: 
-Djavax.accessibility.assistive_technologies empty list leads to crash


FWIW I think this could be closed out as "not a bug".
An empty value string is an error and the spec. says AWTError is
how errors are reported. In fact I'll argue that 8 was wrong not
to have thrown an exception in such a case.
It was an accident of the implementation that it did not.
But if we want to be behaviourally compatible then ...
Note that since you now document this you need an approved CSR BEFORE 
pushing it.


And if you go the CSR route you may need to be more precise about what
you mean by "empty".

I think it needs to say

"If the list of assistive technology providers is null, or contains 
only white

space characters then the method returns immediately.
All other errors are handled by throwing {@code AWTError}"

@throws AWTError if there is any error in parsing or loading the ATs.

-phil.

On 1/21/19, 9:12 AM, Shashidhara Veerabhadraiah wrote:

Hi Sergey, Here is the new Webrev for your comments:

http://cr.openjdk.java.net/~sveerabhadra/8216008/webrev.02/

Thanks and regards,

Shashi

-Original Message-

From: Sergey Bylokhov

Sent: Saturday, January 19, 2019 3:47 AM

To: Shashidhara Veerabhadraiah  
;awt-...@openjdk.java.net  
;swing-dev@openjdk.java.net  


Subject: Re:  [13] JDK-8216008: 
-Djavax.accessibility.assistive_technologies empty list leads to crash

On 16/01/2019 23:03,shashidhara.veerabhadra...@oracle.com  
  wrote:

Another point is that the whitespace trimming should not trigger additional input 
processing  for the custom class that will be used for assistive technology. For eg., if custom 
class "FooProvider" is implemented and if we pass "  FooProvider  " by mistake, 
a bug may be created to trim the whitespace in this case as well!!

Yes, my previous email suggested to always trim the content of the "input", it will cover all 
cases, the "atNames" is empty, the "atNames" contains only the whitespace, or the name of 
the class has some spaces at the start/end.

Thanks and regards,

Shashi

On 17/01/19 10:28 AM, Shashidhara Veerabhadraiah wrote:

We need one way to tell the system not to load any assistive 
technologies and which is being provided.

If we add trailing white spaces removal then we may need to add 
another functionality to see if that class exists or not and based on that not 
to load anything(without throwing the error because we are parsing the input 
content). I think that would take a different direction. I feel it is fair 
enough to provide one way to tell not to load any assistive technologies and 
additional parsing would only lead to other ways/expectations.

Thanks and regards,

Shashi

-Original Message-

From: Sergey Bylokhov

Sent: Thursday, January 17, 2019 1:33 AM

To: Shashidhara Veerabhadraiah  
;awt-...@openjdk.java.net  
;swing-dev@openjdk.java.net  


Subject: Re:  [13] JDK-8216008: 
-Djavax.accessibility.assistive_technologies empty list leads to crash

I guess you meant whitespaces, then it is unclear why we should not consider 
"empty" parameter as a "mistake"?

On 15/01/2019 20:37, Shashidhara Veerab

Re: [12] RFR JDK-8218469, JDK-8218470, JDK-8218472, JDK-8203627 : GTK3 rendering fixes for tck-red bugs

2019-02-14 Thread Phil Race

One of the bugs covered by this fix 
https://bugs.openjdk.java.net/browse/JDK-8218469

has had a release note (RN) attached declaring that JDK 12 won't consider
the GTKLookAndFeel running on top of GTK+3.20 and later supported.
This RN also covers the related bugs which are also caused by GTK+3.20 :

https://bugs.openjdk.java.net/browse/JDK-8218470
https://bugs.openjdk.java.net/browse/JDK-8218472
https://bugs.openjdk.java.net/browse/JDK-8218473
https://bugs.openjdk.java.net/browse/JDK-8218479

So we have downgraded all of these to P2, and therefore they are no longer
candidates to fix in JDK 12 in the current RC phase per 
https://openjdk.java.net/jeps/3


Please push the fixes to jdk/client (ie JDK 13) instead.
The existing reviews should be OK so long as the patches still apply (eg
offset changes are OK, but no rejections).

-phil.


On 2/13/19 10:59 AM, Phil Race wrote:

I've tested this fix on
- OL 7.3 (aka RH 7.3)
- OL 7.6 (aka RH 7.6)
- Ubuntu 16.04
- Ubuntu 18.04
- Ubuntu 18.10

All looks good to me.

-phil.

On 2/12/19 1:42 PM, Pankaj Bansal wrote:


Hi All,

Some more fixes are included in the patch to fix some rendering 
issues with RadioButton or CheckBox in MenuItems, RadioButton and 
Checkbox state flags.


webrev: http://cr.openjdk.java.net/~pbansal/8218469/webrev01/

-Pankaj

*From:*Pankaj Bansal
*Sent:* Tuesday, February 12, 2019 11:38 PM
*To:* swing-dev@openjdk.java.net
*Subject:*  [12] RFR JDK-8218469, JDK-8218470, 
JDK-8218472, JDK-8203627 : GTK3 rendering fixes for tck-red bugs


Hi All,

Please review the following fix.


Bug:

https://bugs.openjdk.java.net/browse/JDK-8218469 -  JSlider display 
issue with slider for GTKLookAndFeel


https://bugs.openjdk.java.net/browse/JDK-8218470-  JScrollBar display 
issue with GTKLookAndFeel


https://bugs.openjdk.java.net/browse/JDK-8218472- JProgressBar 
display issue with GTKLookAndFeel


https://bugs.openjdk.java.net/browse/JDK-8203627 - Swing applications 
with JRadioButton and JCheckbox fail to render correctly when using 
GTK3 and the GTK L&F


webrev

http://cr.openjdk.java.net/~pbansal/8218469/webrev00/

Issue:

From gtk 3.20, gtk has changed the way themes and styles work for 
many widgets. Due to which, jdk is not able to render some widgets 
properly including JSlider, JProgressBar, JScrollBar, JRadioButton, 
JCheckbox.


gtk 3.20 release notes: 
https://developer.gnome.org/gtk3/stable/ch32s10.html


Fix:

This patch fixes rendering issues in all these widgets by using 
correct way to define styles.



Regards,
Pankaj Bansal

Re: [13] JDK-8216008: -Djavax.accessibility.assistive_technologies empty list leads to crash

2019-02-15 Thread Phil Race


That text may actually be worse.
Although I maybe didn't get your distinction of null vs the empty string 
here ?


Oh, wait. Why is this doc clarification on the PRIVATE implementation 
method ?
It needs to go on the public getDefaultToolkit() method to have any 
point whatsoever

especially from a CSR perspective.
Try this :
- * toolkit is created. All errors are handled via an AWTError 
exception.

+ * toolkit is created.
+ * If the list of assistive technology providers is the empty 
string, or
+ * contains only white space characters then the method returns 
immediately.

+ * All other errors are handled via an AWTError exception.


I think the CSR needs some work too. I've made some updates there.

-phil.

On 2/15/19 2:10 AM, Shashidhara Veerabhadraiah wrote:


Hi Phil, I have updated the CSR and updated the information for the 
function and here is the new Webrev:


http://cr.openjdk.java.net/~sveerabhadra/8216008/webrev.05/

Thanks and regards,

Shashi

*From:*Phil Race
*Sent:* Friday, February 15, 2019 2:58 AM
*To:* Shashidhara Veerabhadraiah 
*Cc:* Sergey Bylokhov ; 
awt-...@openjdk.java.net; swing-dev@openjdk.java.net
*Subject:* Re:  [13] JDK-8216008: 
-Djavax.accessibility.assistive_technologies empty list leads to crash


+ * activate method.  If the list of assistive technology 
providers is empty string,

is "the" empty string.
The CSR needs to be updated to include this spec.
What is there now in the specification section needs to be fixed
It should not point to the webrev or review and right now
doesn't contain the change to the javadoc in the body of the CSR
I think I even have to question most of the rest of it.
Any one reading it would think that we always used to throw an 
exception in such
case and now want to stop doing so. Isn't the bug that we did NOT 
throw an exception
and now we do ? You only hint at that when you say at the end of 
"Solution", "as was the case earlier"

If I am right the only thing you need in the CSR is to say that you
are reverting the implementation to previous behaviour and updating the
specification to make it clear that this behaviour is allowed by the spec
since this case was unclear.
-phil.

On 2/12/19 1:08 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Here is the new Webrev and CSR for the same:

http://cr.openjdk.java.net/~sveerabhadra/8216008/webrev.04/

CSR: https://bugs.openjdk.java.net/browse/JDK-8218737

Thanks and regards,

Shashi

*From:*Philip Race
*Sent:* Saturday, February 9, 2019 2:18 AM
*To:* Shashidhara Veerabhadraiah

<mailto:shashidhara.veerabhadra...@oracle.com>
*Cc:* Sergey Bylokhov 
<mailto:sergey.bylok...@oracle.com>; awt-...@openjdk.java.net
<mailto:awt-...@openjdk.java.net>; swing-dev@openjdk.java.net
<mailto:swing-dev@openjdk.java.net>
*Subject:* Re:  [13] JDK-8216008:
-Djavax.accessibility.assistive_technologies empty list leads to crash

FWIW I think this could be closed out as "not a bug".
An empty value string is an error and the spec. says AWTError is
how errors are reported. In fact I'll argue that 8 was wrong not
to have thrown an exception in such a case.
It was an accident of the implementation that it did not.
But if we want to be behaviourally compatible then ...
Note that since you now document this you need an approved CSR
BEFORE pushing it.

And if you go the CSR route you may need to be more precise about what
you mean by "empty".

I think it needs to say

"If the list of assistive technology providers is null, or
contains only white
space characters then the method returns immediately.
All other errors are handled by throwing {@code AWTError}"

@throws AWTError if there is any error in parsing or loading the ATs.

-phil.

On 1/21/19, 9:12 AM, Shashidhara Veerabhadraiah wrote:

Hi Sergey, Here is the new Webrev for your comments:

  


http://cr.openjdk.java.net/~sveerabhadra/8216008/webrev.02/

  


Thanks and regards,

Shashi

  


-Original Message-

From: Sergey Bylokhov

Sent: Saturday, January 19, 2019 3:47 AM

To: Shashidhara Veerabhadraiah  
<mailto:shashidhara.veerabhadra...@oracle.com>;awt-...@openjdk.java.net  
<mailto:awt-...@openjdk.java.net>;swing-dev@openjdk.java.net  
<mailto:swing-dev@openjdk.java.net>

Subject: Re:  [13] JDK-8216008: 
-Djavax.accessibility.assistive_technologies empty list leads to crash

  


On 16/01/2019 23:03,shashidhara.veerabhadra...@oracle.com  
<mailto:shashidhara.veerabhadra...@oracle.com>  wrote:

Another point is that the whitespace trimming should not trigger additional input 
processing  for the custom class that will be used for assistive tec

Re: [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text

2019-02-25 Thread Phil Race


The current fix confused me for a while as I could not see how it was
at all different than the existing code, since I can't imagine when we'd
ever take the "else" branch here :

533 TextLayout layout;
534 if (!isFontRenderContextPrintCompatible(frc, deviceFRC)) {
535 layout = createTextLayout(c, text, g2d.getFont(), deviceFRC);
536 } else {
537 layout = createTextLayout(c, text, g2d.getFont(), frc);
538 } Eventually when walking through it I noticed this 531 FontMetrics 
fm = g2d.getFontMetrics();

532 float screenWidth = SwingUtilities2.stringWidth(c, fm ,trimmedText);


"fm" from line 532 is getting a FontMetrics from the PRINTER - ie the 
scaled FontRenderContext.
It then uses this to calculate the advance width for such a case - ie 
the printer

but then *assigns it to a variable called screenWidth*.

Previously we were using DEFAULT_FRC to make it a screen width which except
for maybe needing to be updated for hi-dpi screens is what we want.

So in the updated proposed fix the wrong width is passed to  
getJustifiedLayout().


This may not matter here because there is plenty of space, but in other 
cases
Swing printing will be clipped as a result. And there were many, many, 
bug reports about
that. Which is why the code is laying out to the screenwidth because 
that is where the
UI component size available came from. Buttons & Label text are the 
typical cases where

this showed up.

There maybe other things to change, as well but the incorrect 
screenWidth is the

main problem I see here.

-phil.

On 2/25/19 12:05 AM, Prasanta Sadhukhan wrote:



On 21-Feb-19 4:50 AM, Sergey Bylokhov wrote:

On 13/02/2019 22:53, Prasanta Sadhukhan wrote:

Hi Sergey,

I believe drawChars() also has same printing issue [and should be 
changed like modified drawString()] but I am not able to test it as 
reproducer testcase uses JLabel whose constructor can only accept 
"String" and not char[] so I can only test drawString(). Using 
drawChars() implementation in drawString() still reproduces the issue.


Is it possible temporary replace the call to drawString() by the 
drawChars(), to check how drawChars() will work?
As I told, it behaves similarly to unmodified drawString and the issue 
can still be seen. I think we should commit this drawString() change 
in this fix and I can open another bug to investigate drawChars() impl 
and reproducer. Will that be fine?


Regards
Prasanta

or probably we can implement drawChars() on top of drawString()?



Regards
Prasanta
On 14-Feb-19 4:12 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

I modified the fix to use deviceFRC if not compatible and in sync 
with the comment which says "obtain a TextLayout with advances for 
the printer graphics FRC"
I used SwingUtilies2.getStringWidth() which calculates the 
advances of the string if text layouting is used.


http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.2/


Can you please take a look to the existed drawChars() method, which 
is implemented in the similar way as drawStringImpl() and new 
version of drawString(), but they have some small difference. Why 
we cannot use the same logic?


For example in the drawChars:
=
    FontRenderContext deviceFontRenderContext = g2d.
    getFontRenderContext();
    FontRenderContext frc = getFontRenderContext(c);
    if (frc != null &&
    !isFontRenderContextPrintCompatible
    (deviceFontRenderContext, frc)) {
 String text = new String(data, offset, length);
 TextLayout layout = new TextLayout(text, 
g2d.getFont(),

   deviceFontRenderContext);
 String trimmedText = trimTrailingSpaces(text);
 if (!trimmedText.isEmpty()) {
 float screenWidth = (float)g2d.getFont().
 getStringBounds(trimmedText, frc).getWidth();
 layout = layout.getJustifiedLayout(screenWidth);

==
Similar but not the same logic in the fix:

 524 FontRenderContext frc = getFontRenderContext(c);
 525 if (frc.isAntiAliased() || 
frc.usesFractionalMetrics()) {
 526 frc = new 
FontRenderContext(frc.getTransform(), false, false);

 527 }
 528 FontRenderContext deviceFRC = 
g2d.getFontRenderContext();

 529 String trimmedText = trimTrailingSpaces(text);
 530 if (!trimmedText.isEmpty()) {
 531 FontMetrics fm = g2d.getFontMetrics();
 532 float screenWidth = 
SwingUtilities2.stringWidth(c, fm ,trimmedText);

 533 TextLayout layout;
 534 if 
(!isFontRenderContextPrintCompatible(frc, deviceFRC)) {
 535 layout = createTextLayout(c, text, 
g2d.getFont(), deviceFRC);

 536 } else {
 537 layout = createTextLayout(c, text, 
g2d.getFont(), frc);

 538

Re: [12] JDK-8182043: Access to Windows Large Icons

2019-03-01 Thread Phil Race


Alexey,

We already nixed this approach
See https://mail.openjdk.java.net/pipermail/awt-dev/2019-January/014972.html

Shashi is re-thinking it to use MultiResolutionImage.

-phil.

On 3/1/19 12:25 PM, Alexey Ivanov wrote:

Hi Shashi,

Sorry for my belated review.

On 22/01/2019 18:03, Shashidhara Veerabhadraiah wrote:


Hi All, Please review the below new fix compared to earlier webrevs. 
Now the new api to get the icons of different sizes is being made 
part of a new class SystemIcon. Please consider this as a different 
solution than the one that was under review for a long time till now.


http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.05/



This version looks cleaner.

The new SystemIcon class contains only one method, getSystemIcon. Does 
it justify creating a new class for this? Can't we add this extended 
version into FileSystemView class?


FileSystemView has method:
public Icon getSystemIcon(File f)

We're extending it with
public Icon getSystemIcon(File f, int size)

Are there any objections to this approach?


The class name, if we're going this route, could be more specific: 
FileSystemIcon seems a better alternative.



Javadoc for SystemIcon says, “SystemIcon is JFileChooser's gateway to 
the file system icons.” Yet JFileChooser uses 
FileSystemView.getSystemIcon(File). So Javadoc is not accurate at the 
very least.


There should also be @since tag for the class.

As this is a new API, I think it makes sense to throw 
IllegalArgumentException for invalid parameters: file == null or size 
is out of supported range.


You don't usually want to print exception stack trace or messages from 
a public API:

74 System.err.println("FileSystemView.getShellFolder: f="+f);
75 e.printStackTrace();

As Phil mentioned, you should clearly state when a null value can be 
returned.



static public Icon getSystemIcon(File f, int size)
should be
public static Icon getSystemIcon(File f, int size)

At this time, SystemIcon is an utility class which provides only 
static methods. Yet it can be instantiated which does not look good.

The code indents by three spaces instead of four.

Win32ShellFolder2.java
 public Image getIcon(int size) {
The method should have @Override annotation.

It can also be added to
1040 public Image getIcon(final boolean getLargeIcon) {


Win32ShellFolderManager2.java
383  key.startsWith("shell32LargeIcon 
")?LARGE_ICON_SIZE : SMALL_ICON_SIZE);


There should be spaces around ‘?’.


Part of the fix is being borrowed from Semyon’s fix and would like to 
thank Semyon for that.




You can give Semyon attribution by including "Contributed-by" tag in 
your commit message and referencing Semyon and yourself there.



Regards,
Alexey


Thanks and regards,

Shashi

Re: [13] RFR: JDK-8219914: Change the environment variable for Java Access Bridge logging to have a directory.

2019-03-26 Thread Phil Race

Can we just call it JAVA_ACCESSBRIDGE_LOGDIR ? 
 filePath[envFilePathLength] = '/'; Is this right ? Does fopen on 
Windows expect this unix style separator ?

53 memcpy(filePath, envfilePath, envFilePathLength*sizeof(char));
56 memcpy(filePath + envFilePathLength + 1 + fileNameLength, ".log", 
4*sizeof(char)); Interesting that you feel it necessary to use 
sizeof(char) when clearly the whole logic, eg see :
50 auto filePathSize = envFilePathLength + 1 + fileNameLength + 5; //1 
for "/", 5 for ".log" and 0; assumes it is 1 ...   PrintDebugString("couldnot open file %s", filePath);


couldnot -> could not

-phil.

On 3/26/19 2:45 AM, Krishna Addepalli wrote:


Hi Phil,

Per our discussion, I have changed the JAVA_ACCESSBRIDGE_LOGFILE to 
JAVA_ACCESSBRIDGE_LOGDIRECTORY to reflect that it accepts only a 
directory value in the variable.


I have also changed the code in AccessBridgeDebug.cpp appropriately.

So, currently, the code will look for the environment variable, which 
should contain path to the directory, and two log files namely 
“java_access_bridge.log” and “windows_access_bridge.log” will be created.


Link to the JDK Issue: https://bugs.openjdk.java.net/browse/JDK-8219914

Here is the webrev: 
http://cr.openjdk.java.net/~kaddepalli/8219914/webrev00/


Thanks,

Krishna

Re: OpenJDK 11 and Swing

2019-03-27 Thread Phil Race

Swing is there in OpenJDK 11 which is an LTS and JDK 11 will be 
supported for 8 years by
Oracle : 
https://www.oracle.com/technetwork/java/java-se-support-roadmap.html
and perhaps also by other vendors. It is still being maintained and 
updated in current

JDK development releases (JDK 13 at the moment).
If you want something more than that you'll need to go ask on some kind 
of customer

support list for one of the OpenJDK vendors, not here.
And I'd say the same about any OpenJDK feature, not just Swing.

-phil.


On 3/27/19 7:18 AM, Keith Pullen wrote:


Hi,

I hope this makes some sense. The company that I work for has 
developed an application with a large, monolithic, GUI, build using 
swing components. We currently run java 8. Or assumption is that we 
will need to move to use OpenJDK 11 in the future, and I am wondering 
what support for Swing will be available moving on into the future,


Keith

*Keith Pullen*

Senior Software Engineer

Ultra Electronics

COMMAND & SONAR SYSTEMS

Waverley House, Hampshire Rd, Weymouth

Dorset, DT4 9XD, U.K.

keith.pul...@ultra-css.com 

Tel: +44 (0)1305 762172

Re: [13] RFR JDK-8212904:JTextArea line wrapping incorrect when using UI scale

2019-03-27 Thread Phil Race

The product code change looks fine. I didn't examine the test 
super-closely but

I am curious why you used System.setProperty("sun.java2d.uiScale", "1.25" )
instead of setting it on the command line ?

-phil.

On 3/26/19 10:25 PM, Prasanta Sadhukhan wrote:

OK. Modified webrev

http://cr.openjdk.java.net/~psadhukhan/8212904/webrev.2/

Regards
Prasanta
On 27-Mar-19 2:25 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

The "textArea.getHeight()" should be called on EDT, since this is a 
swing component.


On 25/03/2019 22:01, Prasanta Sadhukhan wrote:
Thanks Sergey for the pointer. Modified test to be automated in the 
webrev


http://cr.openjdk.java.net/~psadhukhan/8212904/webrev.1/

Regards
Prasanta
On 26-Mar-19 12:02 AM, Sergey Bylokhov wrote:

On 25/03/2019 02:05, Prasanta Sadhukhan wrote:
Thanks Sergey for the pointer. But horizontal scrollbar is not 
visible for both correct and broken wrap if scrollbar policy is 
set to HORIZONTAL_SCROLLBAR_AS_NEEDED.
Also, the textArea.getHeight() is same for both 
textArea.setWrapStyleWord( true ) and not. Attached is the 
automated testcase I tried.


In the test above the case for doWrapOffTest() still uses 
setLineWrap(true) which disable scroll bar, if you drop it the 
scrollbar will be visible.


As of the height, you need to read the value a little bit later, 
not immediately after "frame.setVisible", for example if you read 
these values at lines 75,76 you will get something like that:

textArea.getHeight() = 3216
textArea1.getHeight() = 1616

Re: [13] RFR: JDK-8219914: Change the environment variable for Java Access Bridge logging to have a directory.

2019-03-27 Thread Phil Race


Hi,

It is still inconsistent. the "+1" and "+5" logic relies on the answer 
to sizeof(char) being 1.
So either get rid of all those calls or use sizeof('/') and 
sizeof(".logX"); instead of 1 and 5.


I'd get rid of them 

-phil

On 3/27/19 1:41 AM, Krishna Addepalli wrote:


Hi Phil,

Thanks for the review.

I have changed the variable to “JAVA_ACCESSBRIDGE_LOGDIR”.

Yes, ‘/’ file separator character works on windows. I have used it in 
the past and have also currently tested it on my machine and it works.


I have added the multiplier “sizeof(char)” for all memcpy and memset 
lines in the code, to keep it consistent. Thanks for pointing that out.


Here is the link to the webrev: 
http://cr.openjdk.java.net/~kaddepalli/8219914/webrev01/


Thanks,

Krishna

*From:*Phil Race
*Sent:* Tuesday, March 26, 2019 10:42 PM
*To:* Krishna Addepalli ; 
swing-dev@openjdk.java.net
*Subject:* Re:  [13] RFR: JDK-8219914: Change the 
environment variable for Java Access Bridge logging to have a directory.


Can we just call it  JAVA_ACCESSBRIDGE_LOGDIR ?
 filePath[envFilePathLength] = '/';
Is this right ? Does fopen on Windows expect this unix style separator ?
53 memcpy(filePath, envfilePath, envFilePathLength*sizeof(char));
56 memcpy(filePath + envFilePathLength + 1 + fileNameLength, 
".log", 4*sizeof(char));
Interesting that you feel it necessary to use sizeof(char) when 
clearly the whole logic, eg see :
50 auto filePathSize = envFilePathLength + 1 + fileNameLength 
+ 5; //1 for "/", 5 for ".log" and 0;

assumes it is 1 ...
  PrintDebugString("couldnot open file %s", filePath);

couldnot -> could not

-phil.


On 3/26/19 2:45 AM, Krishna Addepalli wrote:

Hi Phil,

Per our discussion, I have changed the JAVA_ACCESSBRIDGE_LOGFILE
to JAVA_ACCESSBRIDGE_LOGDIRECTORY to reflect that it accepts only
a directory value in the variable.

I have also changed the code in AccessBridgeDebug.cpp appropriately.

So, currently, the code will look for the environment variable,
which should contain path to the directory, and two log files
namely “java_access_bridge.log” and “windows_access_bridge.log”
will be created.

Link to the JDK Issue:
https://bugs.openjdk.java.net/browse/JDK-8219914

Here is the webrev:
http://cr.openjdk.java.net/~kaddepalli/8219914/webrev00/

Thanks,

Krishna

Re: [13] RFR JDK-8222097: ProblemList tests that are failing recurringly and intermittently in PIT

2019-04-09 Thread Phil Race

+1

-phil.

On 4/8/19 11:32 PM, Krishna Addepalli wrote:

Hmm!
looks ok

-Krishna

On 09-Apr-2019, at 10:11 AM, Prasanta Sadhukhan 
> wrote:

I dont think we can AFAIK.

Regards
Prasanta
On 08-Apr-19 4:01 PM, Krishna Addepalli wrote:
I understand that, but my question is if we can replace all those 
lines in the problem list with just one line - like specifying 
TEST_ROOT in jtreg or something.

On 08-Apr-2019, at 12:12 PM, Prasanta Sadhukhan 
> wrote:

Only 1 bug is created..8158801so whoever is fixing that should test 
all these tests pass.

I need all these in ProblemList as in PIT all those tests fail on 
windows recurringly,

Regards
Prasanta
On 08-Apr-19 12:08 PM, Krishna Addepalli wrote:

Hi Prasanta,

As per my experience, the Mixing related tests are failing because of the first 
test which passes and probably doesn’t clean up some state which makes others 
fail.
Is it possible to have one bug for the head, and remove the rest of them from 
the problem list, just to keep it manageable?

Thanks,
Krishna

On 08-Apr-2019, at 11:54 AM, Prasanta Sadhukhan  
wrote:

Hi All,

Please review an addition of some tests in ProblemList that fails recurringly 
and some intermittently in PIT.

Bug:https://bugs.openjdk.java.net/browse/JDK-8222097
webrev:http://cr.openjdk.java.net/~psadhukhan/8222097/webrev.0/

Regards
Prasanta

Re: [13] Review Request: 8216008 -Djavax.accessibility.assistive_technologies empty list leads to exception

2019-04-15 Thread Phil Race


+1

-phil

On 4/14/19 8:34 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 13.

Bug: https://bugs.openjdk.java.net/browse/JDK-8216008
Fix: http://cr.openjdk.java.net/~serb/8216008/webrev.00

The previous discussion of this fix can be found here[1].

Changes in this version:
 - The check "atNames.strip().isEmpty()" was replaced by the 
"atNames.isBlank()".
 - As suggested in the CSR the link to the 
"Character#isWhitespace(int)" was added to the text about white spaces.


[1] 
http://mail.openjdk.java.net/pipermail/swing-dev/2019-January/009260.html

Re: [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text

2019-04-17 Thread Phil Race




On 4/16/19 3:38 AM, Prasanta Sadhukhan wrote:


Hi Phil,

It seems screenWidth or "advance of the string at screen-resolution" 
is 533 whereas string advance calculated using printer fontmetrics is 503


Now, TextLine#getJustifiedLine() [called from 
TextLayout.getJustifiedLayout] again calculates "justifyAdvance" for 
TextLineComponent which comes out to be 503,then it calculates
the actual justification "delta" by subtracting justifyAdvance from 
screenWidth which is 533-503=30 and it then does 
TextJustifier.justify(delta)
which calculates the amount by which each side of each glyph should 
grow or shrink.


Then TextLine.getJustifiedLine() applies this value by calling 
TextLineComponent.applyJustificationDeltas() where it
 handle whitespace by modifying advance but  handle everything else by 
modifying position before and after,
 so "spaces" seem to grow in size resulting in shifting of text with 
whitespaces.


The spaces being used to add needed or absorb surplus space is what I 
understood the current code to be doing,


However I am not sure what you mean by this :-
"but  handle everything else by modifying position before and after,"

Code run through text layout will always have glyph positions assigned, 
so I would suppose modifying the position
is how the spaces were handled too .. and of course this means running 
through all preceding glyphs to make
it consistent .. and I thought it was only adjusting the spaces, so what 
is "everything else"


 Since it was mentioned in TextLayout.getJustifiedLayout() that " For 
best results, justificationWidth should not be too different from the 
current advance of the line"


So for "best" results don't try to adjust a string that's 3 words and 80 
pixels to a 500 pixel width.


I am proposing a fix to conditionally call 
TextLayout.getJustifiedLayout() only if the difference between 
justificationWidth and advance with for printer is more than 10.




I had proposed investigating what happens if you simply don't use 
justification when the text will fit.

Maybe you are refining that but I am not sure about the details.
10 is one arbitrary number and is not proportional to the length - which 
is what I would be

thinking about first in succh an approach
And I am not even sure you mean what you say.
You say "more" than 10, yet your code implements "less" than 10.
And moreover its an absolute value you are using, which re-introduces 
the clipping problem, doesn't it ?
ie you are purely looking at the difference and it isn't what I has 
proposed and I thought you were trying.


-phil


webrev: http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.3/

Regards
Prasanta
On 26-Feb-19 12:30 PM, Philip Race wrote:



On 2/25/19, 10:21 PM, Prasanta Sadhukhan wrote:


Thanks Phil for review. So, you are doubting it will regress swing 
printing tests. As you told earlier, I have ran the following 
regression test with this fix




It may not regress a test if the test is not being tested under the
same conditions for which it is created but I am telling you for a
fact that the fix is wrong. screenWidth should be "width on the screen"


(https://bugs.openjdk.java.net/browse/JDK-6488219 The bug above is 
covered by java/awt/print/PrinterJob/SwingUIText.java)
and I did not notice any regression. Any other test we have for 
swing printing that we can run?




No idea which tests will show this today but I know it is an issue.
No way we can push this and then just wait for the complaints.

>>Previously we were using DEFAULT_FRC to make it a screen width 
which except for maybe needing to be updated for hi-dpi screens is 
what we want.
This issue was there from jdk1.6. If it is for hidpi screen, we 
would have seen it from jdk9 onwards where we supported hidpi, no?


What I am saying here is that DEFAULT_FRC means "screen frc" and
I think that should have been updated in 1.9 but was missed because
it (hidpi for windows) was not a small or contained task.
This is an ancilliary observation of something that should be looked
at entirely independent of this bug.

-phil.



Regards
Prasanta
On 26-Feb-19 3:25 AM, Phil Race wrote:

The current fix confused me for a while as I could not see how it was
at all different than the existing code, since I can't imagine when 
we'd

ever take the "else" branch here :
533 TextLayout layout;
534 if (!isFontRenderContextPrintCompatible(frc, deviceFRC)) {
535 layout = createTextLayout(c, text, g2d.getFont(), deviceFRC);
536 } else {
537 layout = createTextLayout(c, text, g2d.getFont(), frc);
538 } Eventually when walking through it I noticed this 531 
FontMetrics fm = g2d.getFontMetrics();
532 float screenWidth = SwingUtilities2.stringWidth(c, fm 
,trimmedText);


"fm" from line 532 is getting a FontMetrics from the PRINTER - ie 
the scaled FontRenderContext.

RFR: 8212701: remove sun.desktop property from launcher code

2019-04-22 Thread Phil Race


Bug: https://bugs.openjdk.java.net/browse/JDK-8212701
CSR: https://bugs.openjdk.java.net/browse/JDK-8222814
Webrev: http://cr.openjdk.java.net/~prr/8212701/index.html

Please review the above CSR and webrev which eliminates the sun.desktop 
system property

which was set in launcher code to the value
1) "windows" on Windows for no apparent reason
2) "gnome" if running in a Gnome desktop session
3) left unset in all other cases/platforms/desktops, so it being null 
seems unlikely to be a problem.


The Swing code in the desktop module that consumed the value
has been updated to get it in a different way internal to the desktop 
module.


-phil.

Re: [13] RFR JDK-8214702:Wrong text position for whitespaced string in printing Swing text

2019-05-15 Thread Phil Race

OK. Let's try that. I am sure it is still going to be non-ideal in a 
couple of ways

1) where printed is shorter than screenwidth and we don't adjust, there may
be space after the text and before the "edge" of the component, but at least
there's no clipping and the text should look natural initself

2) Where printed is longer than screenwidth and we do adjust we may still
run into similar cases as this ..

-phil.

On 4/19/19 12:56 AM, Prasanta Sadhukhan wrote:




On 18-Apr-19 12:31 AM, Phil Race wrote:



On 4/16/19 3:38 AM, Prasanta Sadhukhan wrote:


Hi Phil,

It seems screenWidth or "advance of the string at screen-resolution" 
is 533 whereas string advance calculated using printer fontmetrics 
is 503


Now, TextLine#getJustifiedLine() [called from 
TextLayout.getJustifiedLayout] again calculates "justifyAdvance" for 
TextLineComponent which comes out to be 503,then it calculates
the actual justification "delta" by subtracting justifyAdvance from 
screenWidth which is 533-503=30 and it then does 
TextJustifier.justify(delta)
which calculates the amount by which each side of each glyph should 
grow or shrink.


Then TextLine.getJustifiedLine() applies this value by calling 
TextLineComponent.applyJustificationDeltas() where it
 handle whitespace by modifying advance but  handle everything else 
by modifying position before and after,
 so "spaces" seem to grow in size resulting in shifting of text with 
whitespaces.


The spaces being used to add needed or absorb surplus space is what I 
understood the current code to be doing,


However I am not sure what you mean by this :-
"but  handle everything else by modifying position before and after,"

Code run through text layout will always have glyph positions 
assigned, so I would suppose modifying the position
is how the spaces were handled too .. and of course this means 
running through all preceding glyphs to make
it consistent .. and I thought it was only adjusting the spaces, so 
what is "everything else"
It seems when TextLineComponent.applyJustificationDeltas() is called, 
ExtendedTextSourceLabel.applyJustificationDeltas() handles that

http://hg.openjdk.java.net/jdk/client/file/dc6c5c53669b/src/java.desktop/share/classes/sun/font/ExtendedTextSourceLabel.java#l1015
where it handles "whitespace" a bit different (if block) from other 
glyph (else block) where advance is is not considered, which is also 
conveyed  in the comment @1011


 Since it was mentioned in TextLayout.getJustifiedLayout() that "  
For best results, justificationWidth should not be too different 
from the current advance of the line"


So for "best" results don't try to adjust a string that's 3 words and 
80 pixels to a 500 pixel width.


I am proposing a fix to conditionally call 
TextLayout.getJustifiedLayout() only if the difference between 
justificationWidth and advance with for printer is more than 10.




I had proposed investigating what happens if you simply don't use 
justification when the text will fit.

Maybe you are refining that but I am not sure about the details.
10 is one arbitrary number and is not proportional to the length - 
which is what I would be

thinking about first in succh an approach
And I am not even sure you mean what you say.
You say "more" than 10, yet your code implements "less" than 10.

It was a typo in mail.
Modified  webrev to not use justification if string width of text is 
within screenWidth

http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.4/

Regards
Prasanta
And moreover its an absolute value you are using, which re-introduces 
the clipping problem, doesn't it ?
ie you are purely looking at the difference and it isn't what I has 
proposed and I thought you were trying.


-phil


webrev: http://cr.openjdk.java.net/~psadhukhan/8214702/webrev.3/

Regards
Prasanta
On 26-Feb-19 12:30 PM, Philip Race wrote:



On 2/25/19, 10:21 PM, Prasanta Sadhukhan wrote:


Thanks Phil for review. So, you are doubting it will regress swing 
printing tests. As you told earlier, I have ran the following 
regression test with this fix




It may not regress a test if the test is not being tested under the
same conditions for which it is created but I am telling you for a
fact that the fix is wrong. screenWidth should be "width on the screen"


(https://bugs.openjdk.java.net/browse/JDK-6488219 The bug above is 
covered by java/awt/print/PrinterJob/SwingUIText.java)
and I did not notice any regression. Any other test we have for 
swing printing that we can run?




No idea which tests will show this today but I know it is an issue.
No way we can push this and then just wait for the complaints.

>>Previously we were using DEFAULT_FRC to make it a screen width 
which except for maybe needing to be updated for hi-dpi screens is 
what we want.
This issue was there from jdk1.6. If it is

Re: [13] Review Request: 8213516 jck test api/javax_accessibility/AccessibleState/fields.html fails intermittent

2019-05-20 Thread Phil Race


Looks fine.

-phil.

On 4/29/19 1:06 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 13.

Bug: https://bugs.openjdk.java.net/browse/JDK-8213516
Fix: http://cr.openjdk.java.net/~serb/8213516/webrev.00

We have a bug in our implementation of the cache in the 
AccessibleBundle class. The current cache should cache the "value" per 
compound key "locale+name_bundle", but the only locale is used. The 
problem is reproduced when we try to use different resource bundles 
for one locale.


One of the previous version of the fix was sent here:
https://mail.openjdk.java.net/pipermail/swing-dev/2019-March/009509.html
http://cr.openjdk.java.net/~sveerabhadra/8213516/webrev.06

Since this is a cache I have an assumption that the performance of 
this code matters. So I decided to measure the difference before/after 
the fix, and w/o cache of 
AccessibleRole.LABEL.toDisplayString(Locale.ENGLISH);


BEFORE: 25.826 ops/MICROSECONDS
AFTER: 5.459 ops/MICROSECONDS
WITHOUT CACHE 11.015 ops/MICROSECONDS

===
This cache was added in jdk 1.3 in 1999 using this description: 
"Obtaining resource bundles can be expensive,...A time performance 
improvement can be made if we cache the resource bundles by locale. We 
probably should also see if ResourceBundle itself caches these for us"

===

I have checked that the current implementation of ResourceBundle 
caches its state and our implementation does not provide any real 
benefit.

Re: [12] RFR JDK-8211703: JInternalFrame : java.lang.AssertionError: cannot find the internal frame

2019-05-22 Thread Phil Race


This would be fine with me.

-phil.

On 5/20/19 2:49 AM, Prasanta Sadhukhan wrote:

Hi Sergey,

I have unified approach to get InternalFrame from button. Since 
updateFrameGeometry() was called with 
"javax.swing.plaf.basic.BasicInternalFrameTitlePane$NoFocusButton" 
component whose parent does not have internal frame,it was asserting
so as done in paintButtonBackground(), updateFrameGeometry() is 
modified to check if the comp is JButton ,then extract the titlePane 
and use titlePane's parent to find internalFrame.

http://cr.openjdk.java.net/~psadhukhan/8211703/webrev.2/

Regards
Prasanta
On 14-Nov-18 2:01 AM, Sergey Bylokhov wrote:

On 13/11/2018 01:25, Prasanta Sadhukhan wrote:
findInternalFrame() is called from paintButtonBackground(), 
paintFrameBorder() and updateFrameGeometry()

but in paintButtonBackground() and paintFrameBorder()
updateFrameGeometry() is called before findInternalFrame() so 
basically it is updateFrameGeometry()#findInternalFrame that is get 
called during assertion.


But in these cases it is used in a little bit different ways, take a 
look to the paintButtonBackground().


 - In first line we will call updateFrameGeometry(), and inside:
   1. We will call "comp = context.getComponent()"
   2. Then will try to find titlePane using findChild() -  I guess it 
will always fail for button.

   3. Then we pass the comp to the findInternalFrame
 - After that we return to the paintButtonBackground and:
   1. We will call "button = context.getComponent()"
   2. We will take a "titlePane = (JComponent)button.getParent()"; 
NOTE: It is different that in "updateFrameGeometry"

   3. Then we will take titlePaneParent from the titlepane
   4. Then we pass the titlePaneParent to the findInternalFrame

Note that context in both cases is the same, but we pass different 
components to findInternalFrame.
I tried to move the call of findInternalFrame in the 
paintButtonBackground before
updateFrameGeometry(to compare the second and first approaches) and 
it is completes successfully. So it
seems we need to unify approach on how we get a InternalFrame from 
the button.


Can you please check this.




During assertion,
paintButtonBackground() is called which calls updateFrameGeometry() 
with
comp=javax.swing.plaf.basic.BasicInternalFrameTitlePane$NoFocusButton[InternalFrameTitlePane.iconifyButton,240,3,18x18,alignmentX=0.0,alignmentY=0.5,border=,flags=290,maximumSize=,minimumSize=, 

preferredSize=,defaultIcon=,disabledIcon=,disabledSelectedIcon=,margin=java.awt.Insets[top=0,left=0,bottom=0,right=0],paintBorder=true,paintFocus=false,pressedIcon=, 

rolloverEnabled=true,rolloverIcon=,rolloverSelectedIcon=,selectedIcon=,text=,defaultCapable=true] 



and comp's parent is
javax.swing.plaf.synth.SynthInternalFrameTitlePane[InternalFrame.northPane,0,0,300x24,layout=com.sun.java.swing.plaf.gtk.Metacity$TitlePaneLayout, 

alignmentX=0.0,alignmentY=0.0,border=javax.swing.plaf.synth.SynthBorder@26e3b5d8,flags=8388610,maximumSize=,minimumSize=,preferredSize=] 



so "comp.getParent() instanceof BasicInternalFrameTitlePane" check 
is satisfied so "comp" becomes SynthInternalFrameTitlePane.


Since SynthInternalFrameTitlePane is not an instance of 
JInternalFrame nor JInternalFrame.JDesktopIcon, it results in 
assertion, so as a fix I extracted the internalframe from 
BasicInternalFrameTitlePane.


Regards
Prasanta
On 13-Nov-18 7:56 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

Can you please provide more details on the hierarchy of components 
which trigger this assertion.


This method is used in 3 places, in two places we try to find the 
child "InternalFrame.northPane"
and pass it to the findInternalFrame(). But in one place we take a 
"button.getParent()", then pass

it to the findInternalFrame(), and take the parent one more time:
"comp.getParent() instanceof BasicInternalFrameTitlePane"

Perhaps one call to "getParent()" is superfluous? What is the 
hierarchy of panes/buttons/internal frames?



On 12/11/2018 03:21, Prasanta Sadhukhan wrote:
Thanks Muneer for the confirmation. Gentle reminder to review 
which is pending for over a month now.


http://cr.openjdk.java.net/~psadhukhan/8211703/webrev.1/

Regards
Prasanta
On 24-Oct-18 3:13 PM, Muneer Kolarkunnu wrote:

Hi Prasanta,

I tested with provided binary and issue is resolved with this 
fix. Thanks for fixing this issue.


Regards,
Muneer

-Original Message-
From: Muneer Kolarkunnu
Sent: Wednesday, October 24, 2018 12:00 PM
To: Prasanta Sadhukhan ; Sergey 
Bylokhov ; swing-dev@openjdk.java.net
Subject: Re:  [12] RFR JDK-8211703: JInternalFrame : 
java.lang.AssertionError: cannot find the internal frame


Hi Prasanta,

I can verify it if you can share a binary with fix.

Regards,
Muneer

-Original Message-
From: Prasanta Sadhukhan
Sent: Wednesday, October 24, 2018 11:41 AM
To: Sergey Bylokhov ; 
swing-dev@openjdk.java.net; ABDUL.KOLARKUNNU 

Subject: Re:  [12] RFR JDK-8211703: JInternalFrame : 
java.lang.AssertionError: cannot find the

RFR: 8225020: Problem list some sanity test failures

2019-05-29 Thread Phil Race


bug : https://bugs.openjdk.java.net/browse/JDK-8225020
Problem listing some sanity tests which consistently fail
Bugs are open on each of these.

diff below


-phil.

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -795,6 +795,10 @@
 javax/swing/RepaintManager/IconifyTest/IconifyTest.java 8221903 linux-all
 javax/swing/JRadioButton/FocusTraversal/FocusTraversal.java 8221902 
linux-all


+sanity/client/SwingSet/src/ColorChooserDemoTest.java 8221312 generic-all
+sanity/client/SwingSet/src/ToolTipDemoTest.java 8225012 windows-all
+sanity/client/SwingSet/src/ScrollPaneDemoTest.java 8225013 linux-all
+

Re: RFR: 8225020: Problem list some sanity test failures

2019-05-30 Thread Phil Race





On 5/30/19 12:33 AM, Jie Fu wrote:

Hi all,

+1

Also when running jdk-tier3 on Ubuntu18.04, the following tests may fail:
---
sanity/client/SwingSet/src/ButtonDemoScreenshotTest.java
sanity/client/SwingSet/src/ColorChooserDemoTest.java


That one is one I problem list below

sanity/client/SwingSet/src/EditorPaneDemoTest.java
sanity/client/SwingSet/src/InternalFrameDemoTest.java


This one is very recently fixed


sanity/client/SwingSet/src/SwingSet2DemoTest.java
sanity/client/SwingSet/src/TextFieldDemoTest.java
sanity/client/SwingSet/src/ToolTipDemoTest.java


I've only seen that one fail on windows and none of the others fail on 
18.04 in our runs.


-phil.


---

Thanks a lot.
Best regards,
Jie

On 2019/5/30 下午3:16, Prasanta Sadhukhan wrote:

+1

Regards
Prasanta
On 30-May-19 2:48 AM, Phil Race wrote:

bug : https://bugs.openjdk.java.net/browse/JDK-8225020
Problem listing some sanity tests which consistently fail
Bugs are open on each of these.

diff below


-phil.

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -795,6 +795,10 @@
 javax/swing/RepaintManager/IconifyTest/IconifyTest.java 8221903 
linux-all
 javax/swing/JRadioButton/FocusTraversal/FocusTraversal.java 8221902 
linux-all


+sanity/client/SwingSet/src/ColorChooserDemoTest.java 8221312 
generic-all

+sanity/client/SwingSet/src/ToolTipDemoTest.java 8225012 windows-all
+sanity/client/SwingSet/src/ScrollPaneDemoTest.java 8225013 linux-all
+

Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly

2019-06-03 Thread Phil Race

The test update tests the OS but if I understand correctly, it is the 
Aqua L&F that does not support

lightweight popups. Can we check if it is Aqua rather than by OS ?
If you don't want to hard code Aqua then check if it is macos and system 
L&F.
Ideally there'd be some way to query whether a L&F supports this case 
but there isn't,

so I'd like to see  the test include a comment that says
// No API exists to check this, but Aqua currently does not support 
lightweight popups.


Lightweight popups seem to me to be principally a performance optimisation.
The thing you have to support is heavyweights and lightweights are 
optional, and
since I don't think we are likely to investigate whether this can be 
done for Aqua anytime

soon then the test update seems like an OK step.

-phil

On 5/24/19 1:26 PM, semyon.sadet...@oracle.com wrote:

webrev: http://cr.openjdk.java.net/~ssadetsky/7184956/webrev.00/

bug: https://bugs.openjdk.java.net/browse/JDK-7184956

Fix for a test: Mac light weigh popup testing excluded.

--Semyon

Re: [13] RFR JDK-8225511:javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedTranslucentPerPixelTranslucentGradient.java fails in linux-x64

2019-06-10 Thread Phil Race


+1

-phil.

On 6/10/19 11:52 AM, Sergey Bylokhov wrote:

Looks fine.

On 10/06/2019 03:13, Prasanta Sadhukhan wrote:

Hi All,

Similar to JDK-8224876 (as below), 
ShapedTranslucentPerPixelTranslucentGradient.java fails time to time 
on mach5 linux headful system, Proposed fix is similar to other one 
which is to add some more delay for translucent case too.


Bug: https://bugs.openjdk.java.net/browse/JDK-8225511

diff -r de1d2a535c08 
test/jdk/javax/swing/JWindow/ShapedAndTranslucentWindows/Common.java
--- 
a/test/jdk/javax/swing/JWindow/ShapedAndTranslucentWindows/Common.java 
Mon Jun 10 10:52:11 2019 +0530
+++ 
b/test/jdk/javax/swing/JWindow/ShapedAndTranslucentWindows/Common.java 
Mon Jun 10 15:37:58 2019 +0530

@@ -367,13 +367,13 @@
  // Drag
  Point location = window.getLocationOnScreen();
  robot.dragAndDrop(location.x + 30, location.y + 5, 
location.x + dl + random.nextInt(dl), location.y + random.nextInt(dl));

-    robot.waitForIdle(delay);
+    robot.waitForIdle(2*delay);
  checkTranslucentShape();

  // Resize
  location = window.getLocationOnScreen();
  robot.dragAndDrop(location.x + 4, location.y + 4, 
location.x + random.nextInt(2*dl)-dl, location.y + 
random.nextInt(2*dl)-dl);

-    robot.waitForIdle(delay);
+    robot.waitForIdle(2*delay);
  checkTranslucentShape();

Regards
Prasanta
On 04-Jun-19 1:30 AM, Sergey Bylokhov wrote:
Looks fine, I have checked it and looks like it can really  lag time 
to time.


On 03/06/2019 04:00, Jayathirth D V wrote:
Changes are fine but we should wait on Sergey's manual check of 
test in the system.


Thanks,
Jay

-Original Message-
From: Prasanta Sadhukhan
Sent: Monday, June 03, 2019 3:54 PM
To: Sergey Bylokhov; swing-dev@openjdk.java.net
Subject: Re:  [13] RFR 
8224876:javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedPerPixelTranslucentGradient.java 
fails on linux-x64


Gentle reminder...


On 30-May-19 12:32 PM, Prasanta Sadhukhan wrote:

I could not find anything amiss when I capture the screen output at
the time of fail, so I decided to go to timeout increase route. I
guess it is needed to check the test manually in that mach5 specific
linux system to see what is happening.

Regards
Prasanta
On 29-May-19 2:29 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

Isn't strange that 1 second is not enough to wait for redraw?
I'll reply soon after rechecking the test manually on this system.

On 28/05/2019 04:01, Prasanta Sadhukhan wrote:

Hi All,

Please review a test-fix for an issue where the test check fails
after the content of window is dragged & resized.
Test is passing in local ubuntu18.04 system but is failing on mach5
linux headful system as test check for pixels happens too quickly
after window drag/resize.
Increased delay between test drag&drop and check to make sure check
happens after reasonable amount of time. Test now pass in all mach5
systems.

Bug: https://bugs.openjdk.java.net/browse/JDK-8224876
webrev: http://cr.openjdk.java.net/~psadhukhan/8224876/webrev.0/

Regards
Prasanta

Re: [13] RFR 8224876:javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedPerPixelTranslucentGradient.java fails on linux-x64

2019-06-11 Thread Phil Race


Prasanta,

This test still fails every day - and I do 
meanShapedPerPixelTranslucentGradient.java,

not the similarly named ShapedTranslucentPerPixelTranslucentGradient.java
which you just fixed yesterday.

-phil.

On 6/3/19 1:00 PM, Sergey Bylokhov wrote:
Looks fine, I have checked it and looks like it can really  lag time 
to time.


On 03/06/2019 04:00, Jayathirth D V wrote:
Changes are fine but we should wait on Sergey's manual check of test 
in the system.


Thanks,
Jay

-Original Message-
From: Prasanta Sadhukhan
Sent: Monday, June 03, 2019 3:54 PM
To: Sergey Bylokhov; swing-dev@openjdk.java.net
Subject: Re:  [13] RFR 
8224876:javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedPerPixelTranslucentGradient.java 
fails on linux-x64


Gentle reminder...


On 30-May-19 12:32 PM, Prasanta Sadhukhan wrote:

I could not find anything amiss when I capture the screen output at
the time of fail, so I decided to go to timeout increase route. I
guess it is needed to check the test manually in that mach5 specific
linux system to see what is happening.

Regards
Prasanta
On 29-May-19 2:29 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

Isn't strange that 1 second is not enough to wait for redraw?
I'll reply soon after rechecking the test manually on this system.

On 28/05/2019 04:01, Prasanta Sadhukhan wrote:

Hi All,

Please review a test-fix for an issue where the test check fails
after the content of window is dragged & resized.
Test is passing in local ubuntu18.04 system but is failing on mach5
linux headful system as test check for pixels happens too quickly
after window drag/resize.
Increased delay between test drag&drop and check to make sure check
happens after reasonable amount of time. Test now pass in all mach5
systems.

Bug: https://bugs.openjdk.java.net/browse/JDK-8224876
webrev: http://cr.openjdk.java.net/~psadhukhan/8224876/webrev.0/

Regards
Prasanta

RFR: 8226697: Several tests which need the @key headful keyword are missing it.

2019-06-24 Thread Phil Race


bug : https://bugs.openjdk.java.net/browse/JDK-8226697
webrev : http://cr.openjdk.java.net/~prr/8226697/

These tests need to be run in headful mode, so for our test systems
to handle them properly they need to be tagged as such.
See the bug report for more details.

-phil.

RFR [13] 8226783: GTK is not being returned as the System L&F on Gnome.

2019-06-26 Thread Phil Race


Bug: https://bugs.openjdk.java.net/browse/JDK-8226783
Webrev: http://cr.openjdk.java.net/~prr/8226783/

This fix is for JDK 13 to fix a regression in a recent build of JDK 13.

We use the presence of an environment variable

GNOME_DESKTOP_SESSION_ID

to determine if GTK should be the System L&F on Linux & Solaris

Due to a typo when relocating this from native code to Java code
JDK 13 fails to find it and defaults to Metal as the system L&F - this
is what we do on a KDE desktop for example.

I've added a test that would have helped spot this outage earlier
and should help in a similar future situation.

-phil.

Re: [14] JDK-8225423: GTK L&F: JSplitPane: There is no divider shown

2019-07-08 Thread Phil Race


Hi,


I am a bit confused by some things in the bug report where first it
was said it is also not visible in a native app, but now it seems it
should be visible ? Why the discrepancy ?

Have you verified this fix on the OS versions you list below ? I think 
you should also verify it on OL 8 Beta. fp_gtk_hpaned_new and 
fp_gtk_vpaned_new look to be unused after this change, so should be 
deleted. -phil.




On 7/8/19 2:47 AM, Prasanta Sadhukhan wrote:


Hi All,

Please review a fix for an issue where it is seen that the splitpane 
divider is not seen for GTK3 in ubuntu18.04, 19.04, oel7.5.


Due to change in style handling for many widgets from gtk3.20, many 
widgets was not rendered correctly of which we had solved many in 
http://hg.openjdk.java.net/jdk/jdk/rev/76668d618a99

but JSplitPane was not rectified.

It was seen that from

https://developer.gnome.org/gtk3/stable/GtkVPaned.html#gtk-vpaned-new 
and https://developer.gnome.org/gtk3/stable/GtkHPaned.html#gtk-hpaned-new


that GtkHPaned and GtkVPaned has been deprecated and we should use 
GtkPaned instead.


Proposed fix makes sure that we use GtkPaned for JSplitpane and also 
adjust the position of the handle/splitpane divider in render_handle() 
method to make sure it is visible.


With this change, we can now see a thin line between the pane as is 
observed via native gtk program.


Bug:https://bugs.openjdk.java.net/browse/JDK-8225423

webrev: http://cr.openjdk.java.net/~psadhukhan/8225423/webrev.0/

Snapshot Before fix and After fix is shown Before Fix 
and 
After Fix 


Regards
Prasanta

Re: [14] JDK-8225423: GTK L&F: JSplitPane: There is no divider shown

2019-07-09 Thread Phil Race


I had thought about this and checked yesterday and the API spec says the
new method is there since 3.0 so I think it is OK to use it on all GTK 3 
versions.


https://developer.gnome.org/gtk3/stable/GtkPaned.html#gtk-paned-new

-phil.

On 7/9/19 9:59 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

On 09/07/2019 06:10, Prasanta Sadhukhan wrote:
fp_gtk_hpaned_new and fp_gtk_vpaned_new look to be unused after this 
change, so should be deleted.


Done...Modified webrev 
http://cr.openjdk.java.net/~psadhukhan/8225423/webrev.1/


Probably we should use these methods on gtk prior 3.20?

Re: [13] Review Request: 8226653 [accessibility] Can edit text cell correctly, but Accessibility Tool reads nothing about editor

2019-07-11 Thread Phil Race

It seems we don't have a method like boolean 
Toolkit.isModifierKeyCode(int keyCode) ? I mean if we did, then it could 
internally know what are the modifier keys for the current platform and 
you wouldn't have that set of ORs. Why was the return value being 
ignored here ? I presume this method must have a side-effect that is 
part of what you are describing ? 7113 Component component = 
renderer.getTableCellRendererComponent(

7114 JTable.this, null, false, false,
7115 row, column); -phil



On 7/10/19 10:16 PM, Sergey Bylokhov wrote:

Hello.
Please review the fix for JDK 13.

Bug: https://bugs.openjdk.java.net/browse/JDK-8226653
Fix: http://cr.openjdk.java.net/~serb/8226653/webrev.01

Short description:

  If the user starts to edit the cell in the "default" JTable then we
show the text editor in the cell but did not transfer the focus to it.
This feature breaks the accessibility tools, which cannot report
information about the current editor.
I suggest to try it in the SwingSet2 to see how this feature actually
works. The idea of the current fix is to disable this feature + some
other related small fixes.


Long description(from small changes to big):

 - CAccessible.java:
    When the user completes the editing of the cell we post
    ACCESSIBLE_TABLE_MODEL_CHANGED event, and we need to notify the
    native system that the data inside the table was changed.

 - JTable.processKeyBinding():
    1) We do not want to start editing of the cell if the only modifier
   key is pressed, but some of the modifier keys were missed:
   VK_ALT_GRAPH and KeyEvent.VK_META(which is CMD key on the macOS).
    2) To enable the "surrendersFocusOnKeystroke" property, I have to add
   one more flag, because I cannot do this via the public API:
https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/javax/swing/JTable.html#setSurrendersFocusOnKeystroke(boolean)
   Note that it is specified as "false" by default, and setting it
   to "true" even under the Accessibility tool may break the spec.
   Probably we need to clarify this behavior in JDK 14.

 - JTable.java.getAccessibleAt()/getAccessibleChild():
    The accessibility API represent the table as a grid of cells, and 
ignores

    the real child of the table. Each cell is represented as a
    AccessibleJTableCell, this AccessibleJTableCell is used as a proxy 
object
    and transfer all requests to the real component which is 
responsible to
    draw the cell. Note that it is specified that the render is 
returned by

    some of its methods:
https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/javax/swing/JTable.AccessibleJTable.AccessibleJTableCell.html#getCurrentAccessibleContext()
    So we cannot modify these methods to return context of the editor, 
instead

    I added the switch before we return "AccessibleJTableCell".
    In JDK 14 it can be moved to the "right" place.


Addition thoughts about "FocusOnKeystroke" feature:
 - We can leave this feature as-is and report the current editor to
   the accessibility tool even if the editor is not focused, but this may
   confuse the user because it will not be possible to move the cursor or
   select the text from the keyboard and this is not expected behavior 
of the

   text editor.
 - We also can disable the editing of the cell on the fly, and start 
it only

   if the user emulates the mouse click inside the table.
 - Or we can leave everything as-is and move the responsibility for this
   feature to the application, which will enable or disable 
"FocusOnKeystroke" feature.

Re: [13] RFR JDK-8226964 [Yaru] - GTK L&F: There is no difference between menu selected and de-selected

2019-08-02 Thread Phil Race

I just finished testing it and it is +1 and I've added the jdk13-fix-yes 
label.


-phil.

On 8/2/19 11:47 AM, Philip Race wrote:

I have looked at it and I think it OK now.
But I have not been able to test it as I am not near any Linux systems 
right now.


Please follow the jdk13 fix request process I sent you (off-line) two 
days ago.


-phil.

On 8/2/19, 11:31 AM, Pankaj Bansal wrote:


Hi Kevin,

I have tested this with various color combinations and results were fine.

Regards,

Pankaj

*From:*Kevin Rushforth
*Sent:* Friday, August 2, 2019 9:32 PM
*To:* Pankaj Bansal; Philip Race; swing-dev@openjdk.java.net
*Subject:* Re:  [13] RFR JDK-8226964 [Yaru] - GTK L&F: 
There is no difference between menu selected and de-selected


This version looks good to me. I presume you've tested various 
combinations of colors for highlight and background colors?


-- Kevin

On 8/2/2019 8:34 AM, Pankaj Bansal wrote:

Hi Phil/Kevin,

I have updated the code for the suggestions given offline. Now,
we are not using YCrCb color model and just using the color
difference in individual components of background and highlight
color to change the highlight color.

webrev: http://cr.openjdk.java.net/~pbansal/8226964/webrev03/


Regards,

Pankaj

*From:*Kevin Rushforth
*Sent:* Friday, August 2, 2019 3:47 AM
*To:* Philip Race; Pankaj Bansal
*Cc:* swing-dev@openjdk.java.net 
*Subject:* Re:  [13] RFR JDK-8226964 [Yaru] - GTK L&F:
There is no difference between menu selected and de-selected

The conversion to / from RGB and YCbCr look OK, but what you do
to derive a new color is not. It will work for gray-scale colors,
but if you deviate from gray it will drastically change the
color, even generating out of range values if one component is
small and another is large.

Here are some examples in YCbCr space :

orig color: 100, 50, 0
derived color: 112, 194, 99   (too green)

orig color: 55, 0, 225
derived color: 286, 32, 324   (out of range)

I think it will be better to convert to something like HSV (aka
HSB) and do the lightening or darkening in that space. I tried a
quick test program using JavaFX, which has built-in RGB <--> HSB
conversion and that works as I would expect.

Here are the same examples in HSB space:

orig color: 100, 50, 0
derived color: 200, 100, 0

orig color: 55, 0, 225
derived color: 31, 0, 125

-- Kevin


On 8/1/2019 2:50 PM, Philip Race wrote:

The if/else looks OK now.

But can you point to the exact the source you used for the
formulas for RGB->YCbCr
conversion and the conversion back from YCbCr to RGB ?

Kevin & I were trying to figure it out and then Kevin wrote
some test
programs to see how round tripping works and found some
significant problems.

I'll let Kevin reply with his findings.

-phil.


On 8/1/19, 11:58 AM, Pankaj Bansal wrote:

Hello Phil,

<< So why can't we simplify it to this ?

I was trying something similar, but your method looks
much simpler to understand. I have used it.

I think in line2,  instead of

if (highlightLuminance + minLuminanceDifference <=
maxLuminance) {

you meant this. Rest I have used as such.

if (backgroundLuminance + minLuminanceDifference <=
maxLuminance) {

<<1)  can we avoid "if(" and include a space as in "if (" >
<<2) There are some lines much > 80 chars.

Fixed

webrev:
http://cr.openjdk.java.net/~pbansal/8226964/webrev02/


Regards,

Pankaj

*From:*Philip Race
*Sent:* Thursday, August 1, 2019 10:24 PM
*To:* Pankaj Bansal
*Cc:* swing-dev@openjdk.java.net

*Subject:* Re:  [13] RFR JDK-8226964 [Yaru] -
GTK L&F: There is no difference between menu selected and
de-selected

I am not sure about the logic in the if/else blocks.
The way it is structured and the way the conditions are
expressed
aren't making it easy for me to follow, or perhaps I am
just failing
to understand some subtle requirements.

And why do you do this ?

+    highlightLuminance +=
minLuminanceDifference;


Suppose that the highLight luminance difference was
already "99",
why do you need to add another "100" to it ?

It seems to me that once we are in the block where we've
decided that
t

Re: [14] RFR 8226513:, JEditorPane is shown with incorrect size

2019-08-07 Thread Phil Race

I have a minor quibble that "rootViewInitialized" is no longer a very 
appropriate variable name,

since it is not just about initialization.
Can we rename it to "rootViewNeedsLayout" ?


Also because the windows 10 issue has a different cause, potentially 
this test can be adjusted

to allow some tolerance to tell the difference between
"not re-layed out at all", and "I'm a few pixels off in my expectations".
ie the test should be about "did I relayout?", not "is my preferred size 
exactly the actual size?".


Could this be a hi-dpi issue ? Are you running at 96 dpi or something 
else when this fails ?



-phil.


On 8/7/19 2:04 AM, Prasanta Sadhukhan wrote:


I confirm that the test pass on linux and mac with the fix and fail 
without the fix. Only on windows, it fails.


But I also see that the failure is not because of the fix. The 
"problem" was there even before the fix, for example with jdk13 (fails 
with jdk11, jdk12 too )


jdk13-b24/bin/java JEditorPaneLayoutTest
Exception in thread "main" java.lang.RuntimeException: Wrong size 
java.awt.Dimension[width=183,height=10] expected 
java.awt.Dimension[width=177,height=44]

    at JEditorPaneLayoutTest.main(JEditorPaneLayoutTest.java:84)

which got somewhat improved by JDK-8217731 



jdk13-b25/bin/java JEditorPaneLayoutTest
Exception in thread "main" java.lang.RuntimeException: Wrong size 
java.awt.Dimension[width=181,height=10] expected 
java.awt.Dimension[width=180,height=44]

    at JEditorPaneLayoutTest.main(JEditorPaneLayoutTest.java:84)

So, the 1 pixel difference in width was there even before this fix and 
also this fix fixes the height of the basic text component and the 
unit tests of JBS works ok, so to me this fix looks ok to me.


+1 for the fix from me and

I have added the test to problem list only for windows to figure out 
the 1 pixel difference in width which seems to be because of layouting 
in windows.


http://cr.openjdk.java.net/~psadhukhan/8226513/webrev.01/

Regards
Prasanta
On 22-Jul-19 4:01 PM, Prasanta Sadhukhan wrote:

Hi Semyon,

Although the JBS testcase is passing with your change, your testcase 
is failing even with the fix for me in windows10


java.lang.RuntimeException: Wrong size 
java.awt.Dimension[width=181,height=44] expected 
java.awt.Dimension[width=180,height=44]

    at JEditorPaneLayoutTest.main(JEditorPaneLayoutTest.java:84)

Regards

Prasanta

On 17-Jul-19 1:33 AM, semyon.sadet...@oracle.com wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8226513

webrev: http://cr.openjdk.java.net/~ssadetsky/8226513/webrev.00/

The fix adds resetting the root view initialization flag when the 
text component underling document is changed and also removes the 
check for the zero component size for the root view initialization 
to correct the resulting preferred component size in some scenarios 
when the root view need to be initially layouted.


--Semyon

Re: [14] RFR 8226513:, JEditorPane is shown with incorrect size

2019-08-08 Thread Phil Race


OK +1 from me !

-phil.

On 8/8/19 2:23 AM, Prasanta Sadhukhan wrote:



On 07-Aug-19 9:42 PM, Phil Race wrote:
I have a minor quibble that "rootViewInitialized" is no longer a very 
appropriate variable name,

since it is not just about initialization.
Can we rename it to "rootViewNeedsLayout" ?


Renamed.


Also because the windows 10 issue has a different cause, potentially 
this test can be adjusted

to allow some tolerance to tell the difference between
"not re-layed out at all", and "I'm a few pixels off in my expectations".
ie the test should be about "did I relayout?", not "is my preferred 
size exactly the actual size?".


Could this be a hi-dpi issue ? Are you running at 96 dpi or something 
else when this fails ?


Yes, it is. I have modified the jtreg command line to test in uiScale 
1. It now pass in windows and others and fails without the fix.


http://cr.openjdk.java.net/~psadhukhan/8226513/webrev.02/

Regards
Prasanta


-phil.


On 8/7/19 2:04 AM, Prasanta Sadhukhan wrote:


I confirm that the test pass on linux and mac with the fix and fail 
without the fix. Only on windows, it fails.


But I also see that the failure is not because of the fix. The 
"problem" was there even before the fix, for example with jdk13 
(fails with jdk11, jdk12 too )


jdk13-b24/bin/java JEditorPaneLayoutTest
Exception in thread "main" java.lang.RuntimeException: Wrong size 
java.awt.Dimension[width=183,height=10] expected 
java.awt.Dimension[width=177,height=44]

    at JEditorPaneLayoutTest.main(JEditorPaneLayoutTest.java:84)

which got somewhat improved by JDK-8217731 
<https://bugs.openjdk.java.net/browse/JDK-8217731>


jdk13-b25/bin/java JEditorPaneLayoutTest
Exception in thread "main" java.lang.RuntimeException: Wrong size 
java.awt.Dimension[width=181,height=10] expected 
java.awt.Dimension[width=180,height=44]

    at JEditorPaneLayoutTest.main(JEditorPaneLayoutTest.java:84)

So, the 1 pixel difference in width was there even before this fix 
and also this fix fixes the height of the basic text component and 
the unit tests of JBS works ok, so to me this fix looks ok to me.


+1 for the fix from me and

I have added the test to problem list only for windows to figure out 
the 1 pixel difference in width which seems to be because of 
layouting in windows.


http://cr.openjdk.java.net/~psadhukhan/8226513/webrev.01/

Regards
Prasanta
On 22-Jul-19 4:01 PM, Prasanta Sadhukhan wrote:

Hi Semyon,

Although the JBS testcase is passing with your change, your 
testcase is failing even with the fix for me in windows10


java.lang.RuntimeException: Wrong size 
java.awt.Dimension[width=181,height=44] expected 
java.awt.Dimension[width=180,height=44]

    at JEditorPaneLayoutTest.main(JEditorPaneLayoutTest.java:84)

Regards

Prasanta

On 17-Jul-19 1:33 AM, semyon.sadet...@oracle.com wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8226513

webrev: http://cr.openjdk.java.net/~ssadetsky/8226513/webrev.00/

The fix adds resetting the root view initialization flag when the 
text component underling document is changed and also removes the 
check for the zero component size for the root view initialization 
to correct the resulting preferred component size in some 
scenarios when the root view need to be initially layouted.


--Semyon

Re: [14] RFR JDK-8231084:Large performance regression in SwingMark TextArea in 14-b13

2019-10-28 Thread Phil Race


Can we just problem list the tests rather than deleting them ?
Won't they be uiseful for the new version of the fix ?
Perhaps the JEditorLayoutPaneTest.java won't be needed if
it was only to confirm the first regression was resolved, but I am not sure.

-phil.


On 10/23/19 4:07 PM, Prasanta Sadhukhan wrote:

Hi All,

Please review a rollback of the bug fix which caused large performance 
regression for TextArea.


JDK-8226513 removes the zero component size check to layout the root 
view when text component is updated by resetting the 
rootViewNeedsLayout flag, but it caused textarea performance 
regression as rootview initialization is triggered multiple times.


JDK-8226513 was a regression of JDK-8160246, which was again 
regression of JDK-8130892, which in turn was a regression of JDK-8001470
where view's preferred size is set only when text component's height 
or width is 0. This check is modified in subsequent fixes which caused 
more regressions.


Proposed fix for performance regression is to roll back to original 
issue and figure out why negative insets are obtained for 
JEditorPane.getPreferredSize()


Bug: https://bugs.openjdk.java.net/browse/JDK-8231084

webrev: http://cr.openjdk.java.net/~psadhukhan/8231084/webrev.0/

Changeset of 8001470 : 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/ff8622450b29


A new bug JDK-8232917 has been created to investigate a fix for the 
original issue JDK-8001470.


Regards
Prasanta

Re: [14] RFR JDK-8231084:Large performance regression in SwingMark TextArea in 14-b13

2019-10-29 Thread Phil Race


Ok then, keep just the one on the problem list.

-phil.

On 10/28/19 10:22 PM, Prasanta Sadhukhan wrote:
We probably could keep bug8001470.java testcase but would like to take 
a relook at the test during fix of the original issue. The original 
issue was JDK-8001470was identified as generic but the testcase fails 
only in linux even after the removal of fix. Other testcases was for 
the regressions and since we are rolling those back, my take is not to 
keep them.


Regards
Prasanta
On 28-Oct-19 11:19 PM, Phil Race wrote:

Can we just problem list the tests rather than deleting them ?
Won't they be uiseful for the new version of the fix ?
Perhaps the JEditorLayoutPaneTest.java won't be needed if
it was only to confirm the first regression was resolved, but I am 
not sure.


-phil.


On 10/23/19 4:07 PM, Prasanta Sadhukhan wrote:

Hi All,

Please review a rollback of the bug fix which caused large 
performance regression for TextArea.


JDK-8226513 removes the zero component size check to layout the root 
view when text component is updated by resetting the 
rootViewNeedsLayout flag, but it caused textarea performance 
regression as rootview initialization is triggered multiple times.


JDK-8226513 was a regression of JDK-8160246, which was again 
regression of JDK-8130892, which in turn was a regression of 
JDK-8001470
where view's preferred size is set only when text component's height 
or width is 0. This check is modified in subsequent fixes which 
caused more regressions.


Proposed fix for performance regression is to roll back to original 
issue and figure out why negative insets are obtained for 
JEditorPane.getPreferredSize()


Bug: https://bugs.openjdk.java.net/browse/JDK-8231084

webrev: http://cr.openjdk.java.net/~psadhukhan/8231084/webrev.0/

Changeset of 8001470 : 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/ff8622450b29


A new bug JDK-8232917 has been created to investigate a fix for the 
original issue JDK-8001470.


Regards
Prasanta

Re: [14] RFR JDK-8231084:Large performance regression in SwingMark TextArea in 14-b13

2019-11-05 Thread Phil Race


+1

-phil

On 11/1/19 2:03 PM, Sergey Bylokhov wrote:

Looks fine.

On 10/29/19 11:49 pm, Prasanta Sadhukhan wrote:
http://cr.openjdk.java.net/~psadhukhan/8231084/webrev.1/ 
problemlisting the original testcase to be fixed in new CR 
JDK-8233177 <https://bugs.openjdk.java.net/browse/JDK-8233177> (clone 
of 8001470).


Regards
Prasanta
On 30-Oct-19 6:03 AM, Sergey Bylokhov wrote:

I suggest to attach these tests to the new CR just as
an additional verification step for the future fix.

On 10/29/19 11:03 am, Phil Race wrote:

Ok then, keep just the one on the problem list.

-phil.

On 10/28/19 10:22 PM, Prasanta Sadhukhan wrote:
We probably could keep bug8001470.java testcase but would like to 
take a relook at the test during fix of the original issue. The 
original issue was JDK-8001470was identified as generic but the 
testcase fails only in linux even after the removal of fix. Other 
testcases was for the regressions and since we are rolling those 
back, my take is not to keep them.


Regards
Prasanta
On 28-Oct-19 11:19 PM, Phil Race wrote:

Can we just problem list the tests rather than deleting them ?
Won't they be uiseful for the new version of the fix ?
Perhaps the JEditorLayoutPaneTest.java won't be needed if
it was only to confirm the first regression was resolved, but I 
am not sure.


-phil.


On 10/23/19 4:07 PM, Prasanta Sadhukhan wrote:

Hi All,

Please review a rollback of the bug fix which caused large 
performance regression for TextArea.


JDK-8226513 removes the zero component size check to layout the 
root view when text component is updated by resetting the 
rootViewNeedsLayout flag, but it caused textarea performance 
regression as rootview initialization is triggered multiple times.


JDK-8226513 was a regression of JDK-8160246, which was again 
regression of JDK-8130892, which in turn was a regression of 
JDK-8001470
where view's preferred size is set only when text component's 
height or width is 0. This check is modified in subsequent fixes 
which caused more regressions.


Proposed fix for performance regression is to roll back to 
original issue and figure out why negative insets are obtained 
for JEditorPane.getPreferredSize()


Bug: https://bugs.openjdk.java.net/browse/JDK-8231084

webrev: http://cr.openjdk.java.net/~psadhukhan/8231084/webrev.0/

Changeset of 8001470 : 
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/ff8622450b29


A new bug JDK-8232917 has been created to investigate a fix for 
the original issue JDK-8001470.


Regards
Prasanta

RFR: 8233649: Update ProblemList.txt to exclude failing headful tests on macos

2019-11-05 Thread Phil Race


bug : https://bugs.openjdk.java.net/browse/JDK-8233649
webrev : http://cr.openjdk.java.net/~prr/8233649/

These swing and awt tests need to be diagnosed / stabilised
since they fail repeatedly in full tests runs.

Excluding them for now. Many have new bugs filed against them.

-phil

Re: [14] RFR : JDK-4949105 : Access Bridge lacks html tags parsing

2019-11-25 Thread Phil Race


Seems OK to me, but I expect Sergey should review this to see if
it addresses his concern.s

-phil.

On 10/31/19 4:57 AM, Ambarish Rapte wrote:

Hi Sergey,
Thanks for the review,
Here is the updated webrev: 
http://cr.openjdk.java.net/~arapte/a11y/4949105/webrev.01/

Fixed both the review comments:
1.  AccessibleContext.ACCESSIBLE_NAME_PROPERTY shall be set only if the text is 
html string, otherwise it is set to null which is existing behavior.
2.  Removed printStackTrace() call.  In case of exception  
AccessibleContext.ACCESSIBLE_NAME_PROPERTY  shall be set to null which is 
existing behavior.
3.  Added a test for setText() scenario.


Regards,
Ambarish

-Original Message-
From: Sergey Bylokhov
Sent: Thursday, October 31, 2019 3:25 AM
To: Ambarish Rapte ; swing-dev@openjdk.java.net
Subject: Re:  [14] RFR : JDK-4949105 : Access Bridge lacks html tags 
parsing

Hi, Ambarish.

I have checked behavior change, and it looks like it has some unexpected issues.

If the text of some component like JLabel sets the text to HTML and then to the 
simple text, then HTML based a11y name will be used instead of simple text.

Note that you should not use "printStackTrace" I guess the old behavior should 
be used if some errors occur.

On 9/24/19 9:45 am, Ambarish Rapte wrote:

Hi All,

Please review this accessibility fix,

JBS: https://bugs.openjdk.java.net/browse/JDK-4949105

Webrev: http://cr.openjdk.java.net/~arapte/a11y/4949105/webrev.00/

Issue:

When a Swing component is created with html text. The same html text is shared 
as AccessibleName with screen reader application.

If screen reader fails to parse the html string correctly, then it may read the 
text different from the text displayed for the component.

Fix:

The same parsed html string for the component should be shared to screen reader 
as AccessibleName.

No existing Swing component tests fail due to this change.

Regards,

Ambarish



--
Best regards, Sergey.

Re: [14] RFR JDK-8227607: Broken external links in java.desktop

2019-12-11 Thread Phil Race

+1

-phil.

On 12/10/19 2:12 AM, Prasanta Sadhukhan wrote:

Modified webrev: http://cr.openjdk.java.net/~psadhukhan/8227607/webrev.3/

with updated spec 
http://cr.openjdk.java.net/~psadhukhan/8227607/docs1/api/java.desktop/javax/swing/text/Document.html

Regards

Prasanta

On 09-Dec-19 11:00 PM, Philip Race wrote:

>

  * ||

For more information on the |Document| class, see The Swing 
Connection 

This link above  seems to be obsolete as well. I think it should just 
be removed

> If an element is a a leaf,isLeaf() returns true.

add a space after the comma.

Also can we wrap all  - or at least the ones that make sense - API 
references in {@code ... }  ?

Method calls for sure.

interrface -> interface.

-phil.

On 12/9/19, 3:16 AM, Prasanta Sadhukhan wrote:

Modified version with new subtitle ""Overview and Programming Tips" 
and graphical_schema

http://cr.openjdk.java.net/~psadhukhan/8227607/webrev.2/

spec change will look like 
http://cr.openjdk.java.net/~psadhukhan/8227607/docs/api/java.desktop/javax/swing/text/Document.html

Regards
Prasanta
On 08-Dec-19 1:47 AM, Philip Race wrote:
I got very confused as to whether there was even an updated version 
here.

This conversation became too complex for the inline diff.
I think that it would be best to first generate a webrev put it on cr.

I do think we need to have a subtitle that prefaces this new 
content rather

than just appending it to "Properties" ..

I suggest "Overview and Programming Tips" which makes it clearer it 
is not (so much) specification as developer guide.

> Swing has a interesting interface, called the _|Element| 
_,

I do think this needs rephrasing.

"Element is an important interface used in constructing a Document.
It has the power "

And each time it says "In Swing" I think, why does it bother saying 
that ...

confusion with an SGML Element ?
Maybe it would read better written as "In the Swing text API's 
document model"

-phil.

On 12/6/19, 7:22 PM, Prasanta Sadhukhan wrote:

Anything more needs to be added?!!! Can it be committed?

On 06-Dec-19 9:49 AM, Prasanta Sadhukhan wrote:
I guess I already shared... 
http://cr.openjdk.java.net/~psadhukhan/docs/api/java.desktop/javax/swing/text/Document.html

Regards

Prasanta

On 06-Dec-19 4:44 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

Can you please share the final generated JavaDoc, it is hard to 
read the diff below.

On 12/4/19 10:09 am, Prasanta Sadhukhan wrote:
Inlined information from element_interface page into Document 
class http://cr.openjdk.java.net/~psadhukhan/docs/api/java.desktop/javax/swing/text/Document.html]

/diff -r 53eff6c5da11 
src/java.desktop/share/classes/javax/swing/Timer.java/

--- a/src/java.desktop/share/classes/javax/swing/Timer.java Sun 
Oct 06 21:42:50 2019 -0700
+++ b/src/java.desktop/share/classes/javax/swing/Timer.java Wed 
Dec 04 22:07:48 2019 +0530

@@ -124,13 +124,6 @@
   * href="https://docs.oracle.com/javase/tutorial/uiswing/misc/timer.html"; 

   * target = "_top">How to Use Timers,
   * a section in The Java Tutorial.
*- * For more examples and help in choosing between**
**- * this Timer class and**
**- * java.util.Timer,**
**- * see**
**- * href="http://java.sun.com/products/jfc/tsc/articles/timer/"**

**- * target="_top">Using Timers in Swing Applications,**
**- * an article in The Swing Connection.**
*  * 
   * Warning:
   * Serialized objects of this class will not be compatible with

/diff -r 53eff6c5da11 
src/java.desktop/share/classes/javax/swing/text/Document.java/

--- 
a/src/java.desktop/share/classes/javax/swing/text/Document.java 
Sun Oct 06 21:42:50 2019 -0700
+++ 
b/src/java.desktop/share/classes/javax/swing/text/Document.java 
Wed Dec 04 22:07:48 2019 +0530

@@ -167,9 +167,38 @@
   *
   * For more information on the Document 
class, see
   * href="http://www.oracle.com/technetwork/java/javase/tech/articles-jsp-139072.html";>The 
Swing Connection

*- * and most particularly the article,**
**- * href="http://java.sun.com/products/jfc/tsc/articles/text/element_interface";>**

**- * The Element Interface.**
**+ * Swing has a interesting interface, called the 
{@link javax.swing.text.Element}, **
**+ * which has the power to describe various structural parts 
of a document, **
**+ * such as paragraphs, lines of text, or even (in HTML 
documents) items in lists. **
**+ * Conceptually, the Element interface captures some of the 
spirit of an SGML document. **
**+ * So if you know SGML, you may already have some 
understanding of Swing's Element interface.**
**+ * In Swing, the interface Element defines a structural 
piece of a Document, **
**+ * like a paragraph, a line of text, o

Re: [15] RFR JDK-8235744: PIT: test/jdk/javax/swing/text/html/TestJLabelWithHTMLText.java times out in linux-x64

2019-12-11 Thread Phil Race

The bug is still in "new" state and has no evaluation. Please add what 
you added here.
Also I think it best to push this test fix to 14, not 15, once approved, 
so it does not interfere with JDK 14 final testing.

Can't this be tested with a file: URL ? What are other similar tests using ?


-phil.

On 12/11/19 2:45 AM, Pankaj Bansal wrote:


Hi All,

Please review the following test only fix for jdk15.


Bug:

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

webrev:

http://cr.openjdk.java.net/~pbansal/8235744/webrev00/

Issue:

The test test/jdk/javax/swing/text/html/TestJLabelWithHTMLText.java on 
linux-64.


The test was added under 
https://bugs.openjdk.java.net/browse/JDK-8230235. The test has a 
JLabel and a html text with empty image tag. The client property 
“DocumentBaseKey” is set on the JLabel with value 
“https://www.google.com/”. If the test is run on a linux machine on a 
network which requires proxy settings to access internet, the test is 
timed out if no proxy is set for java. The test just keeps trying to 
access the url set in “DocumentBaseKey” and as it is set to 
“https://www.google.com/”, it requires a proxy. I think the test 
passes on mach5 system as no system proxy is required there to access 
internet there.


Fix:

The fix is to change the “DocumentBaseKey” value to a URL where proxy 
is not required even if the network needs a proxy to access internet. 
This can be localhost, some local directory url etc. In the fix, 
localhost is used. I have verified that after making this change, the 
test passes with the fix done in 
https://bugs.openjdk.java.net/browse/JDK-8230235 and fails without the 
fix.


Testing:

I have run this on local machines on network which required proxy to 
access internet. I have also ran a mach5 job and it is working fine.



Regards,
Pankaj Bansal

Re: [14] RFR JDK-7020860 - BasicTreeUI contains getters/setters with unclear spec

2019-12-11 Thread Phil Race


OK. +1. although Sergey should also approve this

-phil

On 12/4/19 1:32 AM, Tejpal Rebari wrote:


Hi Phil,

On 27-Nov-2019, at 9:59 PM, Philip Race > wrote:


Yes it needs a CSR.

Will raise a CSR once the review is done.
The updated wording explains when the "set*" methods are used/called 
but does not do the same for the get* methods. Why not for both ?
The setters will be called when the property (like rowHeight, 
cellRenderer etc.) changes in the drawn tree component.
But the getters won’t be called when the property changes, so they 
need not to be updated.


Regards
Tejpal


-phil.

On 11/24/19, 11:31 PM, Jayathirth Rao wrote:

Its looks okay.
Lets wait for Sergey’s inputs on the same.

And I think it needs CSR also.

Thanks,
Jay

On 22-Nov-2019, at 3:04 PM, Tejpal Rebari > wrote:


Hi Jay,
I have appended the text instead of replacing it,
and also updated the SelectionModel methods.

Please find the updated webrev  :
http://cr.openjdk.java.net/~trebari/swing/7020860/webrev1/ 



Thanks
Tejpal

On 18-Nov-2019, at 1:00 PM, Jayathirth D V 
mailto:jayathirth@oracle.com>> wrote:


Hi Tejpal,
I think Sergey has given more inputs on the present change that 
you have at :

http://mail.openjdk.java.net/pipermail/swing-dev/2016-October/006815.html
andhttp://mail.openjdk.java.net/pipermail/swing-dev/2017-June/007495.html
Apart from this changes are fine.
Thanks,
Jay
*From:*Tejpal Rebari
*Sent:*Thursday, October 31, 2019 1:55 PM
*To:*swing-dev@openjdk.java.net 
*Subject:* [14] RFR JDK-7020860 - BasicTreeUI contains 
getters/setters with unclear spec

Hi All,
Please review the following fix for JDK 14.
Bug : https://bugs.openjdk.java.net/browse/JDK-7020860
Webrev : 
http://cr.openjdk.java.net/~trebari/swing/7020860/webrev0/ 

Issue : Class javax.swing.plaf.basic.BasicTreeUI contains some 
getters/setters that are not consistent and
and have specification that says nothing about how they should be 
used and how do they behave in BasicTreeUI.
Fix : This bug was already sent for review by semyon 
http://cr.openjdk.java.net/~ssadetsky/7020860/webrev.00/ 
.

I have made the following changes to the above webrev :
* @param newValue the new value of the {@code editable}> property
changed to —>>
* @param newValue the new value of the {@code editable} property
And
* Returns whether the root node of the drawn tree component is 
displayable.

changed to —>>
* Returns whether the root node of the drawn tree component should 
be displayed.

Regards
Tejpal

Re: [15] RFR JDK-8235744: PIT: test/jdk/javax/swing/text/html/TestJLabelWithHTMLText.java times out in linux-x64

2019-12-12 Thread Phil Race


OK, if we are using localhost already this should be fine.

+1 to push to the JDK 14 stabilisation forest: 
http://hg.openjdk.java.net/jdk/jdk14


-phil

On 12/11/19 10:25 PM, Pankaj Bansal wrote:


Hello Phil,

<what you added here.


Done
<approved, so it does not interfere with JDK 14 final testing.


Ok, I will push this to 14.
<using ?


This can be tested with any url like localhost, file: url. We only 
need a url which can be accessed without proxy. There are many tests 
which are using one of these two. That is why I just used localhost. I 
could use file: url too, but I think that won’t make much difference here.


Regards,

Pankaj

*From:*Phil Race
*Sent:* Thursday, December 12, 2019 12:13 AM
*To:* Pankaj Bansal; swing-dev@openjdk.java.net
*Subject:* Re:  [15] RFR JDK-8235744: PIT: 
test/jdk/javax/swing/text/html/TestJLabelWithHTMLText.java times out 
in linux-x64


The bug is still in "new" state and has no evaluation. Please add what 
you added here.
Also I think it best to push this test fix to 14, not 15, once 
approved, so it does not interfere with JDK 14 final testing.
Can't this be tested with a file: URL ? What are other similar tests 
using ?



-phil.

On 12/11/19 2:45 AM, Pankaj Bansal wrote:

Hi All,

Please review the following test only fix for jdk15.


Bug:

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

webrev:

http://cr.openjdk.java.net/~pbansal/8235744/webrev00/

Issue:

The test
test/jdk/javax/swing/text/html/TestJLabelWithHTMLText.java on
linux-64.

The test was added under
https://bugs.openjdk.java.net/browse/JDK-8230235. The test has a
JLabel and a html text with empty image tag. The client property
“DocumentBaseKey” is set on the JLabel with value
“https://www.google.com/”. If the test is run on a linux machine
on a network which requires proxy settings to access internet, the
test is timed out if no proxy is set for java. The test just keeps
trying to access the url set in “DocumentBaseKey” and as it is set
to “https://www.google.com/”, it requires a proxy. I think the
test passes on mach5 system as no system proxy is required there
to access internet there.

Fix:

The fix is to change the “DocumentBaseKey” value to a URL where
proxy is not required even if the network needs a proxy to access
internet. This can be localhost, some local directory url etc. In
the fix, localhost is used. I have verified that after making this
change, the test passes with the fix done in
https://bugs.openjdk.java.net/browse/JDK-8230235 and fails without
the fix.

Testing:

I have run this on local machines on network which required proxy
to access internet. I have also ran a mach5 job and it is working
fine.


Regards,
Pankaj Bansal

Re: RFR: JDK-8235818: Inline information from broken external links in java.desktop

2019-12-13 Thread Phil Race

On 12/12/19 2:40 AM, Prasanta Sadhukhan wrote:

It seems jdk/jdk has introduced stricter check for img tag to have 
"alt" subtag.

I don't think a new check has been introduced.
The old patch fails for me on a client repo that is a whole 7 days out 
of date.

doclint was enabled in the build in January 2013 and we have a bug fix 
in swing on the alt image tag from the same period :

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

Modified webrev to add alt subtag to img tag like this

201  * 

No visible change to the spec for this modification. mach5 is green

Bug: https://bugs.openjdk.java.net/browse/JDK-8235818

webrev: http://cr.openjdk.java.net/~psadhukhan/8235818/webrev.0/

Ok. Approved for push to jdk/client (not jdk/jdk)

-phil.

Regards

Prasanta

On 12-Dec-19 12:06 AM, Phil Race wrote:

+1

-phil.

On 12/10/19 2:12 AM, Prasanta Sadhukhan wrote:

Modified webrev: 
http://cr.openjdk.java.net/~psadhukhan/8227607/webrev.3/

with updated spec 
http://cr.openjdk.java.net/~psadhukhan/8227607/docs1/api/java.desktop/javax/swing/text/Document.html

Regards

Prasanta

On 09-Dec-19 11:00 PM, Philip Race wrote:

>

  * ||

<http://cr.openjdk.java.net/%7Epsadhukhan/8227607/docs/api/java.desktop/javax/swing/text/Document.html#putProperty%28java.lang.Object,java.lang.Object%29>

For more information on the |Document| class, see The Swing 
Connection 
<http://www.oracle.com/technetwork/java/javase/tech/articles-jsp-139072.html> 

This link above  seems to be obsolete as well. I think it should 
just be removed

> If an element is a a leaf,isLeaf() returns true.

add a space after the comma.

Also can we wrap all  - or at least the ones that make sense -  API 
references in {@code ... }  ?

Method calls for sure.

interrface -> interface.

-phil.

On 12/9/19, 3:16 AM, Prasanta Sadhukhan wrote:

Modified version with new subtitle ""Overview and Programming 
Tips" and graphical_schema

http://cr.openjdk.java.net/~psadhukhan/8227607/webrev.2/

spec change will look like 
http://cr.openjdk.java.net/~psadhukhan/8227607/docs/api/java.desktop/javax/swing/text/Document.html

Regards
Prasanta
On 08-Dec-19 1:47 AM, Philip Race wrote:
I got very confused as to whether there was even an updated 
version here.

This conversation became too complex for the inline diff.
I think that it would be best to first generate a webrev put it 
on cr.

I do think we need to have a subtitle that prefaces this new 
content rather

than just appending it to "Properties" ..

I suggest "Overview and Programming Tips" which makes it clearer 
it is not (so much) specification as developer guide.

> Swing has a interesting interface, called the _|Element| 
<http://cr.openjdk.java.net/%7Epsadhukhan/docs/api/java.desktop/javax/swing/text/Element.html>_,

I do think this needs rephrasing.

"Element is an important interface used in constructing a Document.
It has the power "

And each time it says "In Swing" I think, why does it bother 
saying that ...

confusion with an SGML Element ?
Maybe it would read better written as "In the Swing text API's 
document model"

-phil.

On 12/6/19, 7:22 PM, Prasanta Sadhukhan wrote:

Anything more needs to be added?!!! Can it be committed?

On 06-Dec-19 9:49 AM, Prasanta Sadhukhan wrote:
I guess I already shared... 
http://cr.openjdk.java.net/~psadhukhan/docs/api/java.desktop/javax/swing/text/Document.html

Regards

Prasanta

On 06-Dec-19 4:44 AM, Sergey Bylokhov wrote:

Hi, Prasanta.

Can you please share the final generated JavaDoc, it is hard 
to read the diff below.

On 12/4/19 10:09 am, Prasanta Sadhukhan wrote:
Inlined information from element_interface page into Document 
class http://cr.openjdk.java.net/~psadhukhan/docs/api/java.desktop/javax/swing/text/Document.html]

/diff -r 53eff6c5da11 
src/java.desktop/share/classes/javax/swing/Timer.java/

--- a/src/java.desktop/share/classes/javax/swing/Timer.java 
Sun Oct 06 21:42:50 2019 -0700
+++ b/src/java.desktop/share/classes/javax/swing/Timer.java 
Wed Dec 04 22:07:48 2019 +0530

@@ -124,13 +124,6 @@
   * href="https://docs.oracle.com/javase/tutorial/uiswing/misc/timer.html"; 

   * target = "_top">How to Use Timers,
   * a section in The Java Tutorial.
*- * For more examples and help in choosing between**
**- * this Timer class and**
**- * java.util.Timer,**
**- * see**
**- * href="http://java.sun.com/products/jfc/tsc/articles/timer/"**

**- * target="_top">Using Timers in Swing Applications,**
**- * an article in The Swing Connection.**
*  * 
   * Warning:
   * Serialized objects of this class will not be compatible 
with

/diff -r 53eff6c5da11 
src/java.desktop/share/classes/javax/swing/text/Document.java/

--- 
a/src/java.desktop/share/classes/javax/swing/text/Document.java 
Sun Oct 06 21:42:50 2019 -0700
+++ 
b/src/java.desktop/share/classe

Re: [13] RFR JDK-8234733: We can't distinguish if the spinner button is pressed or unpressed

2019-12-17 Thread Phil Race


  66 
UIManager.setLookAndFeel("com.sun.java.swing.plaf.gtk.GTKLookAndFeel");

I think it would be better here to do something like what we have
in javax/swing/JComboBox/7082443/bug7082443.java
Eg :

public static final String GTK_LAF_CLASS = "GTKLookAndFeel";
for (UIManager.LookAndFeelInfo lookAndFeelInfo : 
UIManager.getInstalledLookAndFeels()) {
if (lookAndFeelInfo.getClassName().contains(GTK_LAF_CLASS)) {
try {
UIManager.setLookAndFeel(lookAndFeelInfo.getClassName());
} catch (final UnsupportedLookAndFeelException ignored) {
return; // can't instantiate, so not a scenario to test ?
}
-phil.



On 12/5/19 1:53 AM, Pankaj Bansal wrote:


Hello Prasanta,

I have made the changes suggested by you.

webrev: http://cr.openjdk.java.net/~pbansal/8234733/webrev02/

Regards,

Pankaj Bansal

*From:*Prasanta Sadhukhan
*Sent:* Wednesday, December 4, 2019 12:58 PM
*To:* Pankaj Bansal; Philip Race
*Cc:* swing-dev@openjdk.java.net
*Subject:* Re:  [13] RFR JDK-8234733: We can't distinguish 
if the spinner button is pressed or unpressed


Hi Pankaj,

Some comments about the test:

I guess since we are testing for Platform.isLinux() in the test, 
there's no need of "requires" tag. But, I think it's better to use


|System|.getProperty("os.name") so that this test can be run in standalone 
mode also(without using Platform class), otherwise it needs to be run 
in conjunction with jtreg(in which case @requires tag is better)


Also, I guess it will be good if you use 
UIManager.setLookAndFeel(...GtkLookAndFeel..) instead of passing 
through command line as user who is running standalone may forget to 
test in GTKL&F


Also, you are not disposing the frame if it times out so the 
instruction frame will linger even when the test fails.


Regards

Prasanta

On 02-Dec-19 4:45 PM, Pankaj Bansal wrote:

Hello Prasanta/Phil/Sergey,

<<1) I suppose this fix is for 14, not 13.

Yes, that was by mistake. I am not changes the subject line, as
this would create separate mail thread. This will be pushed to 14
only.
2) It is "OL", not "OEL"

Ok.
3) Can we create a reliable automated test for this ? If not we
should at least create a manual one.

Done. I have added a manual test case.
4) A separate issue but we should consider adding a JSpinner demo
to SwingSet2

Yes, I was also thinking that we should do it. Do we need separate
approvals for changing the demos. I mean approval from some place
other than swing-dev like demo team etc.

<< Does it mean that this is the behavior of the gtk3 native
spinner component?

Yes, this behavior happens in native spinner. This change makes
our Spinner much more similar to native gtk3 spinner

webrev: http://cr.openjdk.java.net/~pbansal/8234733/webrev01/

Regards,

Pankaj

*From:*Philip Race
*Sent:* Wednesday, November 27, 2019 9:49 PM
*To:* Prasanta Sadhukhan
*Cc:* swing-dev@openjdk.java.net 
*Subject:* Re:  [13] RFR JDK-8234733: We can't
distinguish if the spinner button is pressed or unpressed

1) I suppose this fix is for 14, not 13.
2) It is "OL", not "OEL"
3) Can we create a reliable automated test for this ?
If not we should at least create a manual one.

4) A separate issue but we should consider adding a JSpinner demo
to SwingSet2

-phil.


On 11/27/19, 2:31 AM, Prasanta Sadhukhan wrote:

It works for oel8 too. So the fix looks fine to me.

Regards

Prasanta

On 27-Nov-19 3:54 PM, Prasanta Sadhukhan wrote:

Hi Pankaj,

I can see the issue resolved in ubuntu18.04 but not in
OEL8. Also, SwingSet2 does not have JSpinner demo so we
cannot put noreg-demo in JBS, so we probably need a
regression test, even manual, for this.

Regards

Prasanta

On 25-Nov-19 4:13 PM, Pankaj Bansal wrote:

Hi All,

Please review the following fix for jdk14.


Bug:

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

webrev:

http://cr.openjdk.java.net/~pbansal/8234733/webrev00/


Issue:

In GTKL&F, the spinner’s up/down buttons are drawn in
same way whether the button is pressed or not. We
can't distinguish visually whether a button is pressed
or not. The buttons should be highlighted when they
are in pressed state. The issue is due to style and
theme changes done in gtk3 in version gtk3.20.

Fix:

The fix sets the style properly when drawing the
spinner buttons with gtk3.20 or

Re: [13] RFR JDK-8234733: We can't distinguish if the spinner button is pressed or unpressed

2019-12-18 Thread Phil Race


Ok, +1

-phil

On 12/18/19 1:04 AM, Pankaj Bansal wrote:


Hi Phil,

I have made the change you suggested. Please have a look.

webrev: http://cr.openjdk.java.net/~pbansal/8234733/webrev03/

Regards,

Pankaj

*From:*Phil Race
*Sent:* Wednesday, December 18, 2019 12:20 AM
*To:* Pankaj Bansal; Prasanta Sadhukhan
*Cc:* swing-dev@openjdk.java.net
*Subject:* Re:  [13] RFR JDK-8234733: We can't distinguish 
if the spinner button is pressed or unpressed


   66 
UIManager.setLookAndFeel("com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
I think it would be better here to do something like what we have
in javax/swing/JComboBox/7082443/bug7082443.java
Eg :
     public static final String GTK_LAF_CLASS = "GTKLookAndFeel";
     for (UIManager.LookAndFeelInfo lookAndFeelInfo : 
UIManager.getInstalledLookAndFeels()) {
     if (lookAndFeelInfo.getClassName().contains(GTK_LAF_CLASS)) {
     try {
     UIManager.setLookAndFeel(lookAndFeelInfo.getClassName());
     } catch (final UnsupportedLookAndFeelException ignored) {
     return; // can't instantiate, so not a scenario to test ?
     }
-phil.

On 12/5/19 1:53 AM, Pankaj Bansal wrote:

Hello Prasanta,

I have made the changes suggested by you.

webrev: http://cr.openjdk.java.net/~pbansal/8234733/webrev02/

Regards,

Pankaj Bansal

*From:*Prasanta Sadhukhan
*Sent:* Wednesday, December 4, 2019 12:58 PM
*To:* Pankaj Bansal; Philip Race
*Cc:* swing-dev@openjdk.java.net <mailto:swing-dev@openjdk.java.net>
*Subject:* Re:  [13] RFR JDK-8234733: We can't
distinguish if the spinner button is pressed or unpressed

Hi Pankaj,

Some comments about the test:

I guess since we are testing for Platform.isLinux() in the test,
there's no need of "requires" tag. But, I think it's better to use

|System|.getProperty("os.name") so that this test can be run in standalone
mode also(without using Platform class), otherwise it needs to be
run in conjunction with jtreg(in which case @requires tag is better)

Also, I guess it will be good if you use
UIManager.setLookAndFeel(...GtkLookAndFeel..) instead of passing
through command line as user who is running standalone may forget
to test in GTKL&F

Also, you are not disposing the frame if it times out so the
instruction frame will linger even when the test fails.

Regards

Prasanta

On 02-Dec-19 4:45 PM, Pankaj Bansal wrote:

Hello Prasanta/Phil/Sergey,

<<1) I suppose this fix is for 14, not 13.

Yes, that was by mistake. I am not changes the subject line,
as this would create separate mail thread. This will be pushed
to 14 only.
2) It is "OL", not "OEL"

Ok.
3) Can we create a reliable automated test for this ? If not
we should at least create a manual one.

Done. I have added a manual test case.
4) A separate issue but we should consider adding a JSpinner
demo to SwingSet2

Yes, I was also thinking that we should do it. Do we need
separate approvals for changing the demos. I mean approval
from some place other than swing-dev like demo team etc.

<< Does it mean that this is the behavior of the gtk3 native
spinner component?

Yes, this behavior happens in native spinner. This change
makes our Spinner much more similar to native gtk3 spinner

webrev: http://cr.openjdk.java.net/~pbansal/8234733/webrev01/

Regards,

Pankaj

*From:*Philip Race
*Sent:* Wednesday, November 27, 2019 9:49 PM
*To:* Prasanta Sadhukhan
*Cc:* swing-dev@openjdk.java.net
<mailto:swing-dev@openjdk.java.net>
*Subject:* Re:  [13] RFR JDK-8234733: We can't
distinguish if the spinner button is pressed or unpressed

1) I suppose this fix is for 14, not 13.
2) It is "OL", not "OEL"
3) Can we create a reliable automated test for this ?
If not we should at least create a manual one.

4) A separate issue but we should consider adding a JSpinner
demo to SwingSet2

-phil.


On 11/27/19, 2:31 AM, Prasanta Sadhukhan wrote:

It works for oel8 too. So the fix looks fine to me.

Regards

Prasanta

On 27-Nov-19 3:54 PM, Prasanta Sadhukhan wrote:

Hi Pankaj,

I can see the issue resolved in ubuntu18.04 but not in
OEL8. Also, SwingSet2 does not have JSpinner demo so
we cannot put noreg-demo in JBS, so we probably need a
regression test, even manual, for this.

Regards

Prasanta

On 25-Nov-19 4:13 PM, P

Re: [15] RFR JDK-8222759: com.sun.java.swing.plaf.gtk.GTKLookAndFeel Unneccessary casts to GTKStyleFactory

2019-12-20 Thread Phil Race

Looks fine. Please use the amended and corrected  bug synopsis in the 
commit message


-phil.

On 12/19/19 11:36 PM, Pankaj Bansal wrote:


Hi All,

Please review the following fix for jdk15.


Bug:

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

webrev:

http://cr.openjdk.java.net/~pbansal/8222759/webrev00/

Issue:

There are some unnecessary casts of 
javax.swing.plaf.synth.SynthStyleFactory to 
com.sun.java.swing.plaf.gtk.GTKStyleFactory in GTKLookAndFeel class. 
There is only one function “getStyle” being called after this cast, 
but that is declared in SynthStyleFactory as abstract and defined in 
GTKStyleFactory. So this function can be called without this cast too.


Because of this cast, if someone is trying to use a custom 
SynthStyleFactory with GTKLookAndFeel, there is a ClassCastException.


Fix:

Removed the cast as it is not required here.

Testing:

I have run SwingSet2 with GTKLookAndFeel after this change. All works 
fine. Added testcase fails without fix and passes after the fix.



Regards,
Pankaj Bansal

Re: RFR: 8240202 A few client tests leave mouse buttons pressed

2020-02-28 Thread Phil Race


+1

-phil

On 2/28/20 10:41 AM, Sergey Bylokhov wrote:

Hello.
Please review the fix for jdk/client.

Bug: https://bugs.openjdk.java.net/browse/JDK-8240202
Fix: http://cr.openjdk.java.net/~serb/8240202/webrev.00

Two tests leave the mouse button pressed:
  test/jdk/javax/swing/JButton/PressedButtonRightClickTest.java : 
affects our CI
  test/jdk/java/awt/Mixing/AWT_Mixing/JSplitPaneOverlapping.java : not 
a big issue, the test is in the problem list already

Re: Remove System.out.println from ImageIcon.loadImage

2020-03-09 Thread Phil Race


I am not arguing the general case. I am addressing this case where we
have long standing behaviour.

And I can't find anywhere JOptionPane or similar is calling interrupt().

-phil

On 3/9/20 11:33 AM, Jason Mehrens wrote:

Phil,

Point 6, "The worst thing you can do with InterruptedException is swallow it -- 
catch it and neither rethrow it nor reassert the thread's interrupted status." 
-Brian Goetz
https://www.ibm.com/developerworks/library/j-jtp05236/index.html

The swallowing of interrupts interferes with implementing cooperative 
cancelable tasks.   For modal dialogs like the JOptionPane, interrupting the 
EDT from the application thread can be used to cancel modal blocking.  The 
JOptionPane for INFO, ERROR, WARNING icons shown are provided by the ImageIcon 
class which means the ImageIcon can if the timing is just right swallow the 
interrupt that is important for the JOptionPane to see as a signal not wait.

Jason



From: Philip Race 
Sent: Sunday, March 8, 2020 5:56 PM
To: Volodin, Vladislav
Cc: Jason Mehrens; swing-dev@openjdk.java.net
Subject: Re:  Remove System.out.println from ImageIcon.loadImage

1) That bug is still closed. Did you want it re-opened ? I've just done so.
BTW I don't know why, but it was closed less than 3 months ago
by an engineer from the hotspot team (!?) I have NO idea why
since there is no comment.

2) If you are starting a review thread for a bug, the email subject line
should have the bug id and its synopsis thus :
RFR: 6421373: ImageIcon does not reassert interrupt status.
Although depending on what we end up doing the synopsis may
need to change (see 6 + 7 below).

3) The test has very long lines. Please curtail them to 80 chars.

4) What Christoph said.

5) I know where it was coming from, but I wish the docs said
"Returns when the image is loaded, or loading has failed.
Call getImageLoadStatus()" to check this."
Also a whole bunch of other questions arise like what
should happen if you call setImage() twice for different images.
If the first image succeeded, but the second failed, then what ?

6) I am not completely sold on reasserting the interrupted status
since it is a change of behaviour and you will need an approved CSR
to push this. Not sure if you need to update the javadoc as well ...

7) Now .. if all you really wanted was to get rid of the print statement
this is a lot of effort with some compatibility impact.

-phil.



On 2/22/20, 2:38 AM, Volodin, Vladislav wrote:
Hello Jason and everyone else,

here is my webrev that my colleague published 
http://cr.openjdk.java.net/~clanger/webrevs/6421373.0/
Can you please review it and let me know if I should change anything? Currently 
I am OOO, but I will do the changes in March.

Kind regards,
Vlad

Get Outlook for iOS

From: Jason Mehrens 

Sent: Tuesday, February 11, 2020 9:13:41 PM
To: Volodin, Vladislav 

Cc: swing-dev@openjdk.java.net 

Subject: Re:  Remove System.out.println from ImageIcon.loadImage

Vlad,

Yea that looks good.

Jason


From: Volodin, Vladislav 

Sent: Wednesday, February 5, 2020 9:59 AM
To: Jason Mehrens
Cc: swing-dev@openjdk.java.net
Subject: RE:  Remove System.out.println from ImageIcon.loadImage

Hi Jason,

thank you for your advice. I have changed my code, now it simulates the 
behavior of the interrupted thread. Can you please check my patch?
I don't have the "bug" ticket, so in my test case "@bug JDK-123456" should be 
adjusted.

I will appreciate if you and somebody else can review my patch and submit it to 
JDK.

Thanks,
Vlad

-Original Message-
From: Jason Mehrens 

Sent: Mittwoch, 29. Januar 2020 17:55
To: Volodin, Vladislav 

Cc: swing-dev@openjdk.java.net
Subject: Re:  Remove System.out.println from ImageIcon.loadImage

1. Agreed.
2. I was just pulling from the jdk8 source (because I'm lazy) to express the 
idea.  Feel free to adjust.
3. Reasserting in the finally ensure we are not forcefully setting the 
interrupted status on the current thread and calling 'statusID' and 
'removeImage'.   It also ensures that the interrupt is set even if an 
unexpected exception is thrown.  Reasserting at the end of 'e1' and never in 
'e2' should work too.

The main issue that I'm trying to convey to you is that your test is incomplete 
in that it does check that the interrupt was swallowed.  Swallowing interrupts 
is bad practice.  Reasserting in e2 only means that we swallow interrupts from 
e1.

Change your test to this and retest:
===
public static void main(String[] args) throws Exce

Re: RFR: 8252825: Add automated test for fix done in JDK-8218479

2020-09-06 Thread Phil Race

On Sun, 6 Sep 2020 15:21:45 GMT, Pankaj Bansal  wrote:

> Under JDK-8218479, fix was made to correct the rendering of JTextPane as the 
> background color of JTextPane was similar
> to Panel color, so it looked like the JTextPane has transparent background. 
> No automated test was written to verify the
> results. Current bug is to write the automated tests for the same.  The test 
> fails without the fix done in JDK-8218479
> and pass with the fix. I have run the tests multiple times in mach5. Links in 
> JBS.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/30

Re: RFR: 8252721: Nested classes in Swing APIs rely on default constructors [v2]

2020-09-09 Thread Phil Race

On Wed, 9 Sep 2020 05:40:30 GMT, Prasanta Sadhukhan  
wrote:

>> Please review a fix for issue where it was seen that several
>> nested classes rely on default constructors as part of their public API.
>> 
>> It's to be noted that "A no-arg public constructor is generated by the 
>> compiler for a class if it does not declare an
>> explicit constructor. While convenient, this is inappropriate for many kinds 
>> of formal classes, both because the
>> constructor will have no javadoc and because the constructor may be 
>> unintended."  For the JDK, classes intended to be
>> used outside of the JDK, public classes in exported packages, should not 
>> rely on default constructors.
>> Proposed fix is to add explicit public no-arg constructors for public 
>> classes and protected no-arg constructor for
>> public abstract classes for javax.swing module (as one part of overalll 
>> java.desktop change)
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8252908
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Modify access modifier to protected for protected classes

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/72

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

2020-09-10 Thread Phil Race

On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy 
 wrote:

> **isEmpty** is faster + has less byte code + easier to read. Benchmarks could 
> be found
>   
> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416).

1) This is un-necessary churn.
2) I can't even be sure I am finding the ones in my area because there's so 
much here
3) The ones I can find have no need of whatever performance improvement this 
might bring.
I think this whole PR should be withdrawn, and there may an attempt at 
measuring the benefits of this for the various
cases before submitting in appropriate smaller chunks. But I'll tell you now I 
don't see a need for the test updates
you are making.

-

Changes requested by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/29

1 2 3 4 5 >

1 - 100 of 496 matches

Mail list logo