<Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Tejpal Rebari-2
Hi All,
Please review the following fix for jdk17.

Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
installProperty should work as the opaque property is not set by the client.

Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.

Test : Added a  test  to check the same. Also tested internal tests which all are passing.
Link is in JBS.

-------------

Commit messages:
 - intial changes

Changes: https://git.openjdk.java.net/jdk/pull/3167/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253266
  Stats: 83 lines in 3 files changed: 81 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3167.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3167/head:pull/3167

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Prasanta Sadhukhan-2
On Wed, 24 Mar 2021 06:17:04 GMT, Tejpal Rebari <[hidden email]> wrote:

> Hi All,
> Please review the following fix for jdk17.
>
> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
> installProperty should work as the opaque property is not set by the client.
>
> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>
> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
> Link is in JBS.

I am not sure if this is the correct fix, as then we would have to remove setOpaque call for JTree, JToolTip, JRootPane etc where we might face same installProperty issue.
Also, if we have to change this, we then need to change "autoScrolls" property too which is set in this 2 component and setUIProperty() can change those too.
Probably, we need to check if the value to be set is different in setUIProperty and then set it
or
override setUIProperty per component basis (as it is done for JTable) and then not check for OPAQUE_SET flag for opaque property accordingly.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Tejpal Rebari-2
On Wed, 24 Mar 2021 08:27:40 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> I am not sure if this is the correct fix, as then we would have to remove setOpaque call for JTree, JToolTip, JRootPane etc where we might face same installProperty issue.

Yes we face the same issue in JTree, JToolTip, and JViewport ,
but many of the components do not  have this problem like JButton JRootPane , JCheckbox, JInternalFrame, JMenuItem, JCheckBoxMenuItem, JPopupMenu, JMenu ..etc
We can fix the issue with JTree, JTooltip and JViewport same way.

> Also, if we have to change this, we then need to change "autoScrolls" property too which is set in this 2 component and setUIProperty() can change those too.
> Probably, we need to check if the value to be set is different in setUIProperty and then set it
> or
> override setUIProperty per component basis (as it is done for JTable) and then not check for OPAQUE_SET flag for opaque property accordingly.

OPAQUE_SET is private variable and can not be accessed in JTable or JTree.
From the spec of LookAndFeel.installProperty and setUIProperty it
  * Sets the property with the specified name to the specified value if
     * the property has not already been set by the client program.*
 is clear that once the property is set by the client setUIProperty can not change it. And OPAUQE_SET  is  used  for  that
.

Removing the setOpaque() from the constructor of these component will not make any initial difference as it is also set to true in BasicListUI, BasicTableUI, BasicTreeUI, BasicTooltipUI and BasicViewportUI by calling LookAndFeel.installProperty.
The difference which it will make is now by calling LookAndFeel.installProperty(c,opaque,false) will clear the opaque property.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Sergey Bylokhov-2
In reply to this post by Tejpal Rebari-2
On Wed, 24 Mar 2021 06:17:04 GMT, Tejpal Rebari <[hidden email]> wrote:

> Hi All,
> Please review the following fix for jdk17.
>
> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
> installProperty should work as the opaque property is not set by the client.
>
> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>
> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
> Link is in JBS.

test/jdk/javax/swing/JList/TestOpaqueListTable.java line 38:

> 36:
> 37:     public static void main(String[] args) throws Exception {
> 38:         UIManager.LookAndFeelInfo[] installedLookAndFeels;

I did not look close to the fix, but you need to check that the default value of the "opaque" property will not be changed. So all default L&F will set it to true, as before the fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI

Tejpal Rebari-2
On Thu, 25 Mar 2021 06:44:10 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Hi All,
>> Please review the following fix for jdk17.
>>
>> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
>> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
>> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
>> installProperty should work as the opaque property is not set by the client.
>>
>> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>>
>> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
>> Link is in JBS.
>
> test/jdk/javax/swing/JList/TestOpaqueListTable.java line 38:
>
>> 36:
>> 37:     public static void main(String[] args) throws Exception {
>> 38:         UIManager.LookAndFeelInfo[] installedLookAndFeels;
>
> I did not look close to the fix, but you need to check that the default value of the "opaque" property will not be changed. So all default L&F will set it to true, as before the fix.

Yeah, the default value for "opaque" property will not be changed.
It will be true for All L&F after the fix as well , I will add  that check to the test.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v2]

Tejpal Rebari-2
In reply to this post by Tejpal Rebari-2
> Hi All,
> Please review the following fix for jdk17.
>
> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
> installProperty should work as the opaque property is not set by the client.
>
> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>
> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
> Link is in JBS.

Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:

  Added default value check for opaque property

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3167/files
  - new: https://git.openjdk.java.net/jdk/pull/3167/files/aafac8f2..eec8b913

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=00-01

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

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v2]

Tejpal Rebari-2
In reply to this post by Prasanta Sadhukhan-2
On Wed, 24 Mar 2021 08:27:40 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Added default value check for opaque property
>
> I am not sure if this is the correct fix, as then we would have to remove setOpaque call for JTree, JToolTip, JRootPane etc where we might face same installProperty issue.
> Also, if we have to change this, we then need to change "autoScrolls" property too which is set in this 2 component and setUIProperty() can change those too.
> Probably, we need to check if the value to be set is different in setUIProperty and then set it
> or
> override setUIProperty per component basis (as it is done for JTable) and then not check for OPAQUE_SET flag for opaque property accordingly.

@prsadhuk  @mrserb is there any more comments on this.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v2]

Sergey Bylokhov-2
In reply to this post by Tejpal Rebari-2
On Thu, 25 Mar 2021 08:03:21 GMT, Tejpal Rebari <[hidden email]> wrote:

>> Hi All,
>> Please review the following fix for jdk17.
>>
>> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
>> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
>> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
>> installProperty should work as the opaque property is not set by the client.
>>
>> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>>
>> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
>> Link is in JBS.
>
> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>
>   Added default value check for opaque property

Please wait for the @prsadhuk approval as well.

-------------

Marked as reviewed by serb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Tejpal Rebari-2
In reply to this post by Tejpal Rebari-2
> Hi All,
> Please review the following fix for jdk17.
>
> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
> installProperty should work as the opaque property is not set by the client.
>
> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>
> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
> Link is in JBS.

Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:

  Fix for JTree JTooltip and JViewport keeping default opaque value same, modified the test

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3167/files
  - new: https://git.openjdk.java.net/jdk/pull/3167/files/eec8b913..67fbef16

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=01-02

  Stats: 88 lines in 5 files changed: 63 ins; 11 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3167.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3167/head:pull/3167

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v2]

Tejpal Rebari-2
In reply to this post by Sergey Bylokhov-2
On Wed, 31 Mar 2021 20:34:51 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Added default value check for opaque property
>
> Please wait for the @prsadhuk approval as well.

I have fixed the issue with JTree, JTooltip and JViewport same way.
While fixing for JTooltip discovered that default value for JToolTip opaque property for Nimbus LookAndFeel is set to false in the skin.laf file. This value was never taken into account because the setOpaque(true) from the JTooltip was overriding this.
As we are now removing it from the constructor of JTooptip, NimbusLAF is setting to the false.
To keep the behaviour same as before i have set it to true for NimbusLAF.

Also modified the test to check for JTooltip , JTree and JViewport.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v2]

Tejpal Rebari-2
On Thu, 1 Apr 2021 14:43:36 GMT, Tejpal Rebari <[hidden email]> wrote:

>> Please wait for the @prsadhuk approval as well.
>
> I have fixed the issue with JTree, JTooltip and JViewport same way.
> While fixing for JTooltip discovered that default value for JToolTip opaque property for Nimbus LookAndFeel is set to false in the skin.laf file. This value was never taken into account because the setOpaque(true) from the JTooltip was overriding this.
> As we are now removing it from the constructor of JTooptip, NimbusLAF is setting to the false.
> To keep the behaviour same as before i have set it to true for NimbusLAF.
>
> Also modified the test to check for JTooltip , JTree and JViewport.

Internal testing looks good, link is in JBS

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Sergey Bylokhov-2
In reply to this post by Tejpal Rebari-2
On Thu, 1 Apr 2021 14:37:48 GMT, Tejpal Rebari <[hidden email]> wrote:

>> Hi All,
>> Please review the following fix for jdk17.
>>
>> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
>> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
>> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
>> installProperty should work as the opaque property is not set by the client.
>>
>> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>>
>> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
>> Link is in JBS.
>
> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix for JTree JTooltip and JViewport keeping default opaque value same, modified the test

Marked as reviewed by serb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Prasanta Sadhukhan-2
In reply to this post by Tejpal Rebari-2
On Thu, 1 Apr 2021 14:37:48 GMT, Tejpal Rebari <[hidden email]> wrote:

>> Hi All,
>> Please review the following fix for jdk17.
>>
>> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
>> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
>> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
>> installProperty should work as the opaque property is not set by the client.
>>
>> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>>
>> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
>> Link is in JBS.
>
> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix for JTree JTooltip and JViewport keeping default opaque value same, modified the test

src/java.desktop/share/classes/javax/swing/plaf/nimbus/skin.laf line 27251:

> (failed to retrieve contents of file, check the PR for context)
In my opinion, instead of changing skin.laf we probably should update SynthToolTipUI.installDefaults() and add
`LookAndFeel.installProperty(c, "opaque", Boolean.TRUE);`

similar to what we do for BasicToolTipUI.installDefaults. Will it not work?

test/jdk/javax/swing/JList/TestOpaqueListTable.java line 24:

> 22:  */
> 23:
> 24: import javax.swing.*;

Can you please update this wildcard imports?

test/jdk/javax/swing/JList/TestOpaqueListTable.java line 31:

> 29:  *  @summary setUIProperty should work when opaque property is not set by
> 30:  *  client
> 31:  *  @key headful

Does it need to be headful?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Tejpal Rebari-2
On Mon, 5 Apr 2021 05:20:26 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix for JTree JTooltip and JViewport keeping default opaque value same, modified the test
>
> test/jdk/javax/swing/JList/TestOpaqueListTable.java line 31:
>
>> 29:  *  @summary setUIProperty should work when opaque property is not set by
>> 30:  *  client
>> 31:  *  @key headful
>
> Does it need to be headful?

It is better to run this test on headful systems, which has the required components installed.
This test checks for all look and feel. I remember making one of the non ui test as not headful and it thrown LookAndFeelNot installed exception.
So one of the suggestion was to make it headful, so doing the same here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v4]

Tejpal Rebari-2
In reply to this post by Tejpal Rebari-2
> Hi All,
> Please review the following fix for jdk17.
>
> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
> installProperty should work as the opaque property is not set by the client.
>
> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>
> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
> Link is in JBS.

Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:

  removed wildcard imports

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3167/files
  - new: https://git.openjdk.java.net/jdk/pull/3167/files/67fbef16..953bc3aa

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=02-03

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

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Tejpal Rebari-2
In reply to this post by Prasanta Sadhukhan-2
On Mon, 5 Apr 2021 05:19:49 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix for JTree JTooltip and JViewport keeping default opaque value same, modified the test
>
> test/jdk/javax/swing/JList/TestOpaqueListTable.java line 24:
>
>> 22:  */
>> 23:
>> 24: import javax.swing.*;
>
> Can you please update this wildcard imports?

Done

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Tejpal Rebari-2
In reply to this post by Prasanta Sadhukhan-2
On Mon, 5 Apr 2021 05:19:06 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix for JTree JTooltip and JViewport keeping default opaque value same, modified the test
>
> src/java.desktop/share/classes/javax/swing/plaf/nimbus/skin.laf line 27251:
>
>> (failed to retrieve contents of file, check the PR for context)
> In my opinion, instead of changing skin.laf we probably should update SynthToolTipUI.installDefaults() and add
> `LookAndFeel.installProperty(c, "opaque", Boolean.TRUE);`
>
> similar to what we do for BasicToolTipUI.installDefaults. Will it not work?

No, it doesn't work, i have checked.
The change needs to be skin.laf file only.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Prasanta Sadhukhan-2
On Mon, 5 Apr 2021 06:34:17 GMT, Tejpal Rebari <[hidden email]> wrote:

>> src/java.desktop/share/classes/javax/swing/plaf/nimbus/skin.laf line 27251:
>>
>>> (failed to retrieve contents of file, check the PR for context)
>> In my opinion, instead of changing skin.laf we probably should update SynthToolTipUI.installDefaults() and add
>> `LookAndFeel.installProperty(c, "opaque", Boolean.TRUE);`
>>
>> similar to what we do for BasicToolTipUI.installDefaults. Will it not work?
>
> No, it doesn't work, i have checked.
> The change needs to be skin.laf file only.

I am not sure where you placed the code. I think it's need to be added after updateStyle() as updateStyle() uninstall the default and install for new style.
protected void installDefaults(JComponent c) {
        updateStyle(c);
    }

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v3]

Tejpal Rebari-2
On Mon, 5 Apr 2021 07:08:32 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> No, it doesn't work, i have checked.
>> The change needs to be skin.laf file only.
>
> I am not sure where you placed the code. I think it's need to be added after updateStyle() as updateStyle() uninstall the default and install for new style.
> protected void installDefaults(JComponent c) {
>         updateStyle(c);
>     }

yeah setting the property after updateStyle(c) works fine.
updating the change in the SynthToolTipUI.installDefaults()

-------------

PR: https://git.openjdk.java.net/jdk/pull/3167
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> RFR: 8253266 : JList and JTable constructors should clear OPAQUE_SET before calling updateUI [v5]

Tejpal Rebari-2
In reply to this post by Tejpal Rebari-2
> Hi All,
> Please review the following fix for jdk17.
>
> Issue : LookAndFeel.installProperty(list, "opaque", false) is not able to set the opaque property for JList and JTable.
> LookAndFeel.installProperty calls the setUIProperty, and setUIProperty checks for OPAQUE_SET to change the opaque property as requested by the client.
> OPAQUE_SET is always set to true when there is call to setOpaque(boolean).So when the constructor calls setOpaque(true) OPAQUE_SET is set to true and wont allow the setUIProperty to change the opaque property.
> installProperty should work as the opaque property is not set by the client.
>
> Fix. Fix is to remove the call to the setOpaque() from the constructor of JList and JTable. This will allow the client to change the opaque property calling LookAndFeel.installProperty() when the property is already not set.
>
> Test : Added a  test  to check the same. Also tested internal tests which all are passing.
> Link is in JBS.

Tejpal Rebari has updated the pull request incrementally with one additional commit since the last revision:

  set default opaque property for nimbus JToolTip in SynthToolTipUI instead of skin.laf

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3167/files
  - new: https://git.openjdk.java.net/jdk/pull/3167/files/953bc3aa..e3b3e8c6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3167&range=03-04

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

PR: https://git.openjdk.java.net/jdk/pull/3167
12