RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

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

RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Prasanta Sadhukhan-2
The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
Updated spec to rectify this.

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

Commit messages:
 - 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Alexander Zvegintsev-2
On Tue, 9 Feb 2021 11:57:44 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
> Updated spec to rectify this.

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1206:

> 1204:      * @param s the {@code Shape} to be intersected with the current
> 1205:      *          {@code Clip}.
> 1206:      * @throws NullPointerException if {@code s} is {@code null}

Actually it is not always true, you can check it by commenting `setClip()` call in the test.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Prasanta Sadhukhan-2
On Tue, 9 Feb 2021 12:48:44 GMT, Alexander Zvegintsev <[hidden email]> wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
>> Updated spec to rectify this.
>
> src/java.desktop/share/classes/java/awt/Graphics2D.java line 1206:
>
>> 1204:      * @param s the {@code Shape} to be intersected with the current
>> 1205:      *          {@code Clip}.
>> 1206:      * @throws NullPointerException if {@code s} is {@code null}
>
> Actually it is not always true, you can check it by commenting `setClip()` call in the test.

The spec says "s - the Shape to be intersected with the current Clip" so I assume it means there should be a current clip set, so that is why I have used setClip to "set" a clip. So, setClip() should be there as far I see.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Prasanta Sadhukhan-2
On Tue, 9 Feb 2021 12:52:55 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> src/java.desktop/share/classes/java/awt/Graphics2D.java line 1206:
>>
>>> 1204:      * @param s the {@code Shape} to be intersected with the current
>>> 1205:      *          {@code Clip}.
>>> 1206:      * @throws NullPointerException if {@code s} is {@code null}
>>
>> Actually it is not always true, you can check it by commenting `setClip()` call in the test.
>
> The spec says "s - the Shape to be intersected with the current Clip" so I assume it means there should be a current clip set, so that is why I have used setClip to "set" a clip. So, setClip() should be there as far I see.

Also, if there is no clip set, then the spec statement " If s is null, this method clears the current Clip" does not carry any meaning, so in both regard, setClip() should be there, I presume.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Jayathirth D V-2
On Tue, 9 Feb 2021 12:54:26 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> The spec says "s - the Shape to be intersected with the current Clip" so I assume it means there should be a current clip set, so that is why I have used setClip to "set" a clip. So, setClip() should be there as far I see.
>
> Also, if there is no clip set, then the spec statement " If s is null, this method clears the current Clip" does not carry any meaning, so in both regard, setClip() should be there, I presume.

@prsadhuk At first glance i also thought clip() should be called after calling setClip() but that is not the case.

If we see SunGraphics2D implementation of clip() we dont exit if there is no clip(usrClip object) is set using setClip(). So clip() doesnt depend on whether setClip() is used or not.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Alexander Zvegintsev-2
On Tue, 9 Feb 2021 13:13:51 GMT, Jayathirth D V <[hidden email]> wrote:

>> Also, if there is no clip set, then the spec statement " If s is null, this method clears the current Clip" does not carry any meaning, so in both regard, setClip() should be there, I presume.
>
> @prsadhuk At first glance i also thought clip() should be called after calling setClip() but that is not the case.
>
> If we see SunGraphics2D implementation of clip() we dont exit if there is no clip(usrClip object) is set using setClip(). So clip() doesnt depend on whether setClip() is used or not.

> Also, if there is no clip set, then the spec statement " If s is null, this method clears the current Clip" does not carry any meaning, so in both regard, setClip() should be there, I presume.

The old javadoc is definitely does not conform the current behavior. But as of now it clearly says that it will throw NPE if null argument passed.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Sergey Bylokhov-2
On Tue, 9 Feb 2021 13:59:32 GMT, Alexander Zvegintsev <[hidden email]> wrote:

>> @prsadhuk At first glance i also thought clip() should be called after calling setClip() but that is not the case.
>>
>> If we see SunGraphics2D implementation of clip() we dont exit if there is no clip(usrClip object) is set using setClip(). So clip() doesnt depend on whether setClip() is used or not.
>
>> Also, if there is no clip set, then the spec statement " If s is null, this method clears the current Clip" does not carry any meaning, so in both regard, setClip() should be there, I presume.
>
> The old javadoc is definitely does not conform the current behavior. But as of now it clearly says that it will throw NPE if null argument passed.

Looks like this is just a bug in the implementation, the null should reset the clip.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Prasanta Sadhukhan-2
On Tue, 9 Feb 2021 18:10:44 GMT, Sergey Bylokhov <[hidden email]> wrote:

>>> Also, if there is no clip set, then the spec statement " If s is null, this method clears the current Clip" does not carry any meaning, so in both regard, setClip() should be there, I presume.
>>
>> The old javadoc is definitely does not conform the current behavior. But as of now it clearly says that it will throw NPE if null argument passed.
>
> Looks like this is just a bug in the implementation, the null should reset the clip.

I am not sure if it's implementation bug. As Jim Graham has mentioned in thebug,

> clip(null) is not legal.
>
> It is *setClip*(null) that clears the clip.
I could rephrase the doc to specify NPE willbe thrrown for null Shape if a clip is already set.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Sergey Bylokhov-2
On Wed, 10 Feb 2021 04:26:17 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Looks like this is just a bug in the implementation, the null should reset the clip.
>
> I am not sure if it's implementation bug. As Jim Graham has mentioned in thebug,
>
>> clip(null) is not legal.
>>
>> It is *setClip*(null) that clears the clip.
> I could rephrase the doc to specify NPE willbe thrrown for null Shape if a clip is already set.

If "*setClip*(null)" has to clear the clip then it should be specified, currently, that method said nothing about the null parameter.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

Prasanta Sadhukhan-2
On Wed, 10 Feb 2021 04:47:05 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> I am not sure if it's implementation bug. As Jim Graham has mentioned in thebug,
>>
>>> clip(null) is not legal.
>>>
>>> It is *setClip*(null) that clears the clip.
>> I could rephrase the doc to specify NPE willbe thrrown for null Shape if a clip is already set.
>
> If "*setClip*(null)" has to clear the clip then it should be specified, currently, that method said nothing about the null parameter.

As per code
public void setClip(Shape sh) {
        usrClip = transformShape(sh);

usrClip is set to null if "sh" is null so clip is cleared...I will update the setClip doc too..

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v2]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
> Updated spec to rectify this.

Prasanta Sadhukhan has updated the pull request incrementally with three additional commits since the last revision:

 - Fix
 - Document setClip(null) behaviour too
 - Document setClip(null) behaviour too

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2476/files
  - new: https://git.openjdk.java.net/jdk/pull/2476/files/a7798fb5..94a04228

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

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v3]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
> Updated spec to rectify this.

Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:

  Fix

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2476/files
  - new: https://git.openjdk.java.net/jdk/pull/2476/files/94a04228..0f1ab211

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

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v3]

Phil Race
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Feb 2021 04:51:33 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> If "*setClip*(null)" has to clear the clip then it should be specified, currently, that method said nothing about the null parameter.
>
> As per code
> public void setClip(Shape sh) {
>         usrClip = transformShape(sh);
>
> usrClip is set to null if "sh" is null so clip is cleared...I will update the setClip doc too..

From what I can tell, if clip is null, then calling clip(null) has no effect.
And it won't throw an NPE in that case. So the proposed documentation is less accurate than no documentation at all.
And I would shy away from changing the implementation on this without LOTS of VERY careful testing.  
Oh the javadoc clause you propose is split by many lines of comments. I see you have the rider "and clip is already set via {@code setClip}".
Well I am not sure why it has to be set by setClip for this to be true but just having to say this sounds weird.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v3]

Phil Race
On Wed, 10 Feb 2021 18:30:18 GMT, Phil Race <[hidden email]> wrote:

>> As per code
>> public void setClip(Shape sh) {
>>         usrClip = transformShape(sh);
>>
>> usrClip is set to null if "sh" is null so clip is cleared...I will update the setClip doc too..
>
> From what I can tell, if clip is null, then calling clip(null) has no effect.
> And it won't throw an NPE in that case. So the proposed documentation is less accurate than no documentation at all.
> And I would shy away from changing the implementation on this without LOTS of VERY careful testing.  
> Oh the javadoc clause you propose is split by many lines of comments. I see you have the rider "and clip is already set via {@code setClip}".
> Well I am not sure why it has to be set by setClip for this to be true but just having to say this sounds weird.

> usrClip is set to null if "sh" is null so clip is cleared...I will update the setClip doc too..

Huh ? Not understanding you. If the userClip is non-null then we enter this
        if (usrClip != null) {
            s = intersectShapes(usrClip, s, true, true);
        }
which i where you get the NPE from you are proposing to document.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v3]

Phil Race
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Feb 2021 06:01:59 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix

test/jdk/java/awt/Graphics2D/TestNullClip.java line 52:

> 50:             throw new RuntimeException("NPE is expected");
> 51:         } catch (NullPointerException e) {
> 52:             //expected

Why is this wrapping everything ? There's only one line here where you expect an NPE and so you should only catch an exception from that one line.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v3]

Phil Race
In reply to this post by Phil Race
On Wed, 10 Feb 2021 18:46:33 GMT, Phil Race <[hidden email]> wrote:

>> From what I can tell, if clip is null, then calling clip(null) has no effect.
>> And it won't throw an NPE in that case. So the proposed documentation is less accurate than no documentation at all.
>> And I would shy away from changing the implementation on this without LOTS of VERY careful testing.  
>> Oh the javadoc clause you propose is split by many lines of comments. I see you have the rider "and clip is already set via {@code setClip}".
>> Well I am not sure why it has to be set by setClip for this to be true but just having to say this sounds weird.
>
>> usrClip is set to null if "sh" is null so clip is cleared...I will update the setClip doc too..
>
> Huh ? Not understanding you. If the userClip is non-null then we enter this
>         if (usrClip != null) {
>             s = intersectShapes(usrClip, s, true, true);
>         }
> which i where you get the NPE from you are proposing to document.

It seems to me to be a little accidental from the implementation that clip(null) will just silently return
if the user clip is currently null.
But I think it unduly risky to "clean that up" after so many years.
So I think we are stuck with some variant on the weird wording.
The bit about using setClip is bogus. I suggest adding the clause worded like this
@throws NullPointerException if {@code s} is {@ code null) and a user clip is currently set.

and explanatory text like this
Since this method intersects the specified shape with the current clip, it will throw NPE for a null shape
unless the user clip is also null. So calling this method with a null argument is not recommended.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v4]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
> Updated spec to rectify this.

Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:

  Fix as per Phil's comment

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2476/files
  - new: https://git.openjdk.java.net/jdk/pull/2476/files/0f1ab211..e4053de8

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

  Stats: 21 lines in 2 files changed: 11 ins; 8 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2476.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2476/head:pull/2476

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v4]

Alexander Zvegintsev-2
On Thu, 11 Feb 2021 04:14:54 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix as per Phil's comment

test/jdk/java/awt/Graphics2D/TestNullClip.java line 41:

> 39:
> 40:         g2d.setClip(0, 0, 100, 100);
> 41:         Shape clip = g2d.getClip();

Looks like the `clip` variable is not used.
Also I've just noticed that the years are not updated in copyright headers of Graphics and Graphics2d.

test/jdk/java/awt/Graphics2D/TestNullClip.java line 46:

> 44:         if (clip1 != null) {
> 45:             throw new RuntimeException("Clip is not cleared");
> 46:         }

There is no check that `g2d.clip(null);` call does not throw NPE when no user clip is set.

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v5]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
> Updated spec to rectify this.

Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:

  Review comment fix. Test updated

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2476/files
  - new: https://git.openjdk.java.net/jdk/pull/2476/files/e4053de8..51fbe247

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

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

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

Re: RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v5]

Alexey Ivanov-2
On Thu, 11 Feb 2021 15:44:57 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() method. G2D.clip() would throw a NullPointerException when it encounters a null argument.
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   Review comment fix. Test updated

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1204:

> 1202:      * argument, the specified {@code Shape} becomes the new
> 1203:      * user clip. Since this method intersects the specified shape
> 1204:      * with the current clip, it will throw NPE for a null shape

Shall NPE be spelled out as `{@code NullPointerException}`?

Also should it rather be, “…it <del>will</del> throws NPE…”?

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

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