<Swing Dev> [9] Review fix for JDK-8020039 : SynthTableHeaderUI refers to possibly null parameter in cell renderer

Fri Feb 19 21:30:20 UTC 2016

According to the discussion you linked to, "Swing is trying to paint the header you've already removed from the table..." which is why the table is null. This is a good example of my original point. Why is Swing trying to paint a header that has been removed? 
In any case, whenever it's not obvious why a seemingly vital parameter could be null, it would be useful if there were some comments in the code to explain this. I have long felt that the many java APIs could be improved by specifying which parameters are allowed to be null.
-- Miguel 

    On Friday, February 19, 2016 1:04 AM, Ajit Ghaisas <ajit.ghaisas at oracle.com> wrote:

 #yiv6463762851 #yiv6463762851 -- _filtered #yiv6463762851 {font-family:Helvetica;panose-1:2 11 6 4 2 2 2 2 2 4;} _filtered #yiv6463762851 {panose-1:2 4 5 3 5 4 6 3 2 4;} _filtered #yiv6463762851 {font-family:Calibri;panose-1:2 15 5 2 2 2 4 3 2 4;}#yiv6463762851 #yiv6463762851 p.yiv6463762851MsoNormal, #yiv6463762851 li.yiv6463762851MsoNormal, #yiv6463762851 div.yiv6463762851MsoNormal {margin:0in;margin-bottom:.0001pt;font-size:12.0pt;}#yiv6463762851 a:link, #yiv6463762851 span.yiv6463762851MsoHyperlink {color:blue;text-decoration:underline;}#yiv6463762851 a:visited, #yiv6463762851 span.yiv6463762851MsoHyperlinkFollowed {color:purple;text-decoration:underline;}#yiv6463762851 span.yiv6463762851EmailStyle17 {color:#1F497D;}#yiv6463762851 .yiv6463762851MsoChpDefault {font-size:10.0pt;} _filtered #yiv6463762851 {margin:1.0in 1.0in 1.0in 1.0in;}#yiv6463762851 div.yiv6463762851WordSection1 {}#yiv6463762851 Hi,     There are the some good points made by Miguel regarding null checks – but, in this case the table being null is a valid case.    While rendering the TableHeader, it is possible to be in a situation where ‘table’ parameter can be null.     Please refer to a similar discussion : https://community.oracle.com/thread/2279601?start=0&tstart=0          I have done corrections to address Sergey’s second question and simplified the test code.    Please review updated webrev :  http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.04/   Regards,Ajit  From: Miguel Munoz [mailto:swingguy1024 at yahoo.com] 
Sent: Thursday, February 18, 2016 4:29 PM
To: Ajit Ghaisas; Sergey Bylokhov; Rajeev Chamyal; Alexander Scherbatiy; swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> [9] Review fix for JDK-8020039 : SynthTableHeaderUI refers to possibly null parameter in cell renderer  It would seem to me that if the table is null, you have a bug in another class. Is there a valid reason for the table to be null? I would guess not.

In my experience, many null checks are either unnecessary or only serve to hide bugs. Steve Maguire writes about this in his book Writing Solid Code.  -- Miguel Muñoz  On Wednesday, February 17, 2016 10:52 PM, Ajit Ghaisas <ajit.ghaisas at oracle.com> wrote:  Hi,

    Goods finding Sergey.

    1. In file, src/java.desktop/macosx/classes/com/apple/laf/AquaTableHeaderUI.java, call to setText() and setBorder() should be made irrespective of whether table in null or not. I have made this change.
    2. The changes to SynthTableHeaderUI.java are correct. We cannot assume a table property (enabled) when table is null. 
        A call to super. getTableCellRendererComponent() is already made from this method which in turn calls setText() and setBorder().

    Please review updated webrev : http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.03/

Regards,
Ajit

-----Original Message-----
From: Sergey Bylokhov 
Sent: Wednesday, February 17, 2016 8:41 PM
To: Rajeev Chamyal; Ajit Ghaisas; Alexander Scherbatiy; swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> [9] Review fix for JDK-8020039 : SynthTableHeaderUI refers to possibly null parameter in cell renderer

Hi, Hello Ajit.
I am not sure that exclusion of the code is a correct fix here. For example can you clarify should we call setText(or other setXXX) when the table is null or not? Another question is: should we skip the code in SynthTableHeaderUI.java or we can assume table==null as enabled table?

On 17.02.16 14:29, Rajeev Chamyal wrote:
> Looks good to me.
>
> Regards,
>
> Rajeev Chamyal
>
> *From:*Ajit Ghaisas
> *Sent:* 17 February 2016 16:51
> *To:* Rajeev Chamyal; Sergey Bylokhov; Alexander Scherbatiy; 
> swing-dev at openjdk.java.net
> *Subject:* RE: <Swing Dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hi,
>
>      I have corrected formatting of test code and removed the 
> additional System.out.printlns.
>
>      Please review :
> http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.02/
>
> Regards,
>
> Ajit
>
> *From:*Ajit Ghaisas
> *Sent:* Tuesday, February 16, 2016 3:12 PM
> *To:* Rajeev Chamyal; Sergey Bylokhov; Alexander Scherbatiy; 
> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
> *Subject:* Re: <Swing Dev> <Swing-dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hi,
>
>      Thanks Rajeev for review comments.
>
>      I have checked - windows LAF WindowsTableHeaderUI handles it 
> correctly.
>
>      I have added null check for MacOs Aqua.
>
>      Also, I have added a test checking for this null pointer access.
>
>      Please review --
> http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.01/
>
> Regards,
>
> Ajit
>
> *From:*Rajeev Chamyal
> *Sent:* Monday, February 15, 2016 5:39 PM
> *To:* Ajit Ghaisas; Sergey Bylokhov; Alexander Scherbatiy; 
> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
> *Subject:* RE: <Swing-dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hello Ajit,
>
> Can you please if similar fix is required for other LAF windows ,Aqua etc.
>
> Please add a regression test case also.
>
> Regards,
>
> Rajeev Chamyal
>
> *From:*Ajit Ghaisas
> *Sent:* 15 February 2016 17:30
> *To:* Rajeev Chamyal; Sergey Bylokhov; Alexander Scherbatiy; 
> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
> *Subject:* <Swing-dev> [9] Review fix for JDK-8020039 :
> SynthTableHeaderUI refers to possibly null parameter in cell renderer
>
> Hi,
>
> Please review the fix for jdk9,
>
>                  Bug: https://bugs.openjdk.java.net/browse/JDK-8020039
>
> Webrev: http://cr.openjdk.java.net/~arapte/ajit/8020039/webrev.00/
>
> Issue :
>
>      If null is passed as 'table' parameter to
> SynthTableHeaderUI::getTableCellRendererComponent() method in
>  
> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthTableHeader
> UI.java,
>
>      there is Null Pointer Exception.
>
> Analysis :
>
>    This method already has a null check for 'table' parameter for 
> second access of this parameter in method.
>
>    Whereas the first access of the 'table' parameter lacks this check.
>
> Fix :
>
>    Added null check for the first access of 'table' parameter in 
> SynthTableHeaderUI::getTableCellRendererComponent().
>
>    There is no else block added as the flow continues and passes 
> table to base class method using super. getTableCellRendererComponent().
>
>    The passed parameter is already checked in base class method 
> correctly. Hence, no change is needed in base class.
>
> Test :
>
>    The fix is pretty straight forward.
>
>    Executed the code snippet given in the bug description. There is 
> no NPE after the fix.
>
> Regards,
>
> Ajit
>

--
Best regards, Sergey.  

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160219/b2698488/attachment.html>