Re: [9] Review Request for 8081722: Provide public API for file hierarchy provided by sun.awt.shell.ShellFolder
Please look at the updated webrev:
http://cr.openjdk.java.net/~ssadetsky/8081722/webrev.05/
Changes was made According to off-line discussion wit Sergey:
- IAE -> NPE for file == null
- FNFE is corrected for getLinkLocation()
--Semyon
On 1/22/2016 9:34 AM, Semyon Sadetsky wrote:
On 1/21/2016 6:25 PM, Sergey Bylokhov wrote:
On 21/01/16 09:16, Semyon Sadetsky wrote:
There are still inconsistency. The isFileSystem(), isParent(),
getSystemIcon, getSystemDisplayName() etc do not throw an exception in
case of null, but return some default value(null,true,false). Same for
FileNotFoundException() most of the methods just catch this exception
and return the default value.
The methods you've mentioned do not throw NPE but other methods in the
FSV class do throw NPE if called with null argument.
they raise NPE not an IAE. ok I am fine to use NPE.
Some other issue is inconsistency between isLink() vs getLinkLocation()
getLinkLocation() should "Returns {@code null} if the specified file
is not a shell interpreted link", but isLink() return false if
FileNotFoundException which means that getLinkLocation() should
return null;
the next code can work incorrectly:
FileSystemView fsv = FileSystemView.getFileSystemView();
File file = "some unexisted file";
if (fsv.isLink(file)) {
if (fsv.getLinkLocation(file) == null) {
throw new RuntimeException();
}
} else {
if (fsv.getLinkLocation(file) != null) {
throw new RuntimeException();
}
}
The code will not compile because FileNotFoundException is not caught.
The logic of getLinkLocation() is pretty simple:
- If the file is not a link, it refers to nothing and the method
returns nothing (null).
- If the file is a link, it refers to an another file but in case the
another file cannot be found in the FS the FileNotFoundException is
thrown.
There are some checks in the code related to "C:\pagefile.sys" and
"C:\Winnt\Profiles\joe\history\History.IE5" can you double check how
the new methods will work with these files.
This situations are only possible for the listFiles() return. If it
happens for a File object then it is a real error which should be
reported.
Your point that there is inconsistency sounds a bit odd. If this
reasoning we can not add a new method which throws an exception to the
class not having methods throwing this exception. How methods of the
FSV
class related to different aspects of the file system can be
inconsistent? They are simply not related to each other.
Re: [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@openjdk.java.net Subject: [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/SynthTableHeaderUI.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
[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/SynthTableHeaderUI.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
Re: Review Request for 7126823 : JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Looks good to me.
Regards,
Rajeev Chamyal
From: Prem Balakrishnan
Sent: 15 February 2016 14:22
To: Rajeev Chamyal; Alexander Scherbatiy
Cc: Sergey Bylokhov; Semyon Sadetsky; Ambarish Rapte; swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hi Rajeev,
Test updated as per your comments.
Webrev: http://cr.openjdk.java.net/~arapte/prem/7126823/webrev.02/
Regards,
Prem
From: Rajeev Chamyal
Sent: Monday, February 15, 2016 12:00 PM
To: Prem Balakrishnan
Cc: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hello Prem,
1) UI should be created in a swing thread so please update the createUI
method to use a swing thread.
2) Also please use SwingUtilities.invokeAndWait instead of
SwingUtilities.invokeLater.
Regards,
Rajeev Chamyal
From: Prem Balakrishnan
Sent: 11 February 2016 12:49
To: Rajeev Chamyal
Cc: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hi Rajeev,
I have tested for all supported LAF's across all platforms(Windows, Linux and
Mac)
Test updated for the same.
Webrev: http://cr.openjdk.java.net/~arapte/prem/7126823/webrev.01/
Regards,
Prem
From: Rajeev Chamyal
Sent: Wednesday, February 10, 2016 2:00 PM
To: Prem Balakrishnan
Cc: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hello Prem,
Did you test this fix for other LAF's as well.
Regards,
Rajeev Chamyal
From: Prem Balakrishnan
Sent: 09 February 2016 14:40
To: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net;
HYPERLINK "mailto:awt-...@openjdk.java.net"awt-...@openjdk.java.net
Subject: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hi,
Please review fix for JDK9,
Bug: https://bugs.openjdk.java.net/browse/JDK-7126823
Webrev: http://cr.openjdk.java.net/~arapte/prem/7126823/webrev.00/
Issue:
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Cause:
Regression: due to 4424247: DefaultDesktopManager does not handle InternalFrame
state changes as expected.
https://bugs.openjdk.java.net/browse/JDK-4424247 related to
https://bugs.openjdk.java.net/browse/JDK-4900778
Fix:
DefaultDesktopManager.java
setNormalBounds(getBounds()); call removed from iconifyFrame(JInternalFrame f)
method.
Justification:
This fix is not breaking any of the previous fix i.e., related to 4424247 and
4900778.
(all previous scenarios are tested and validated with the current fix)
In JInternalFrame.java
getNormalBounds() returns getBounds() when normalBounds is NULL,
Hence no need to setNormaBounds externally.
public Rectangle getNormalBounds() {
/* we used to test (!isMaximum) here, but since this
method is used by the property listener for the
IS_MAXIMUM_PROPERTY, it ended up getting the wrong
answer... Since normalBounds get set to null when the
frame is restored, this should work better */
if (normalBounds != null) {
return normalBounds;
} else {
return getBounds();
}
}
As per Javadoc:
public Rectangle getNormalBounds()
If the JInternalFrame is not in maximized state, returns getBounds();
otherwise, returns the bounds that the JInternalFrame would be restored to.
In maximizeFrame() , normalBounds is set to a value which is not NULL, i.e.,
setNormalBounds(getBounds());
In minimizeFrame(), normalBounds is set to NULL, i.e., setNormalBounds(null);
NormalBounds should NOT be set elsewhere.
Test:
Integrated test for current bug and regression.
Regards,
Prem
Re: Review Request for 7126823 : JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hi Rajeev,
Test updated as per your comments.
Webrev: http://cr.openjdk.java.net/~arapte/prem/7126823/webrev.02/
Regards,
Prem
From: Rajeev Chamyal
Sent: Monday, February 15, 2016 12:00 PM
To: Prem Balakrishnan
Cc: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hello Prem,
1) UI should be created in a swing thread so please update the createUI
method to use a swing thread.
2) Also please use SwingUtilities.invokeAndWait instead of
SwingUtilities.invokeLater.
Regards,
Rajeev Chamyal
From: Prem Balakrishnan
Sent: 11 February 2016 12:49
To: Rajeev Chamyal
Cc: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hi Rajeev,
I have tested for all supported LAF's across all platforms(Windows, Linux and
Mac)
Test updated for the same.
Webrev: http://cr.openjdk.java.net/~arapte/prem/7126823/webrev.01/
Regards,
Prem
From: Rajeev Chamyal
Sent: Wednesday, February 10, 2016 2:00 PM
To: Prem Balakrishnan
Cc: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net
Subject: RE: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hello Prem,
Did you test this fix for other LAF's as well.
Regards,
Rajeev Chamyal
From: Prem Balakrishnan
Sent: 09 February 2016 14:40
To: Sergey Bylokhov; Semyon Sadetsky; Alexander Scherbatiy; Ambarish Rapte;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net;
HYPERLINK "mailto:awt-...@openjdk.java.net"awt-...@openjdk.java.net
Subject: Review Request for 7126823 :
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Hi,
Please review fix for JDK9,
Bug: https://bugs.openjdk.java.net/browse/JDK-7126823
Webrev: http://cr.openjdk.java.net/~arapte/prem/7126823/webrev.00/
Issue:
JInternalFrame.getNormalBounds() returns bad value after iconify/deiconify
Cause:
Regression: due to 4424247: DefaultDesktopManager does not handle InternalFrame
state changes as expected.
https://bugs.openjdk.java.net/browse/JDK-4424247 related to
https://bugs.openjdk.java.net/browse/JDK-4900778
Fix:
DefaultDesktopManager.java
setNormalBounds(getBounds()); call removed from iconifyFrame(JInternalFrame f)
method.
Justification:
This fix is not breaking any of the previous fix i.e., related to 4424247 and
4900778.
(all previous scenarios are tested and validated with the current fix)
In JInternalFrame.java
getNormalBounds() returns getBounds() when normalBounds is NULL,
Hence no need to setNormaBounds externally.
public Rectangle getNormalBounds() {
/* we used to test (!isMaximum) here, but since this
method is used by the property listener for the
IS_MAXIMUM_PROPERTY, it ended up getting the wrong
answer... Since normalBounds get set to null when the
frame is restored, this should work better */
if (normalBounds != null) {
return normalBounds;
} else {
return getBounds();
}
}
As per Javadoc:
public Rectangle getNormalBounds()
If the JInternalFrame is not in maximized state, returns getBounds();
otherwise, returns the bounds that the JInternalFrame would be restored to.
In maximizeFrame() , normalBounds is set to a value which is not NULL, i.e.,
setNormalBounds(getBounds());
In minimizeFrame(), normalBounds is set to NULL, i.e., setNormalBounds(null);
NormalBounds should NOT be set elsewhere.
Test:
Integrated test for current bug and regression.
Regards,
Prem
