<Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing

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

<Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing

Prasanta Sadhukhan-2
It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.

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

Commit messages:
 - 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing

Changes: https://git.openjdk.java.net/jdk/pull/2955/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2955&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263472
  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2955.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2955/head:pull/2955

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing

Alexey Ivanov-2
On Fri, 12 Mar 2021 04:33:43 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.

src/java.desktop/share/classes/javax/swing/JComponent.java line 660:

> 658:     /**
> 659:      * Resets the UI property to a value from the current look and feel.
> 660:      * Since default implementation of this method don't do anything,

Suggestion:

     * Since the default implementation of this method doesn't do anything,
Or maybe even:
Suggestion:

     * The default implementation of this method does nothing;
It's a statement. Then it continues with “…subclasses must override this method…”.

src/java.desktop/share/classes/javax/swing/JComponent.java line 659:

> 657:
> 658:     /**
> 659:      * Resets the UI property to a value from the current look and feel.

Shall it rather use plural?
Suggestion:

     * Resets the UI properties to values from the current look and feel.
There are usually several properties that get updated, *all the properties* in fact.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing

Sergey Bylokhov-2
In reply to this post by Prasanta Sadhukhan-2
On Fri, 12 Mar 2021 04:33:43 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.

Do we really want to specify this as a noop method? In this case, we will not be able to change this implementation in the future.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing

Prasanta Sadhukhan-2
On Fri, 12 Mar 2021 22:24:09 GMT, Sergey Bylokhov <[hidden email]> wrote:

>
>
> Do we really want to specify this as a noop method? In this case, we will not be able to change this implementation in the future.

I thought about it earlier but I raised this PR as I don't see this method was updated from inception so I was not sure if it will be in future and also since it already says "JComponent subclasses **must** override this method" so a noop default implementation will not be a problem, I guess.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.

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

  javadoc change

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2955/files
  - new: https://git.openjdk.java.net/jdk/pull/2955/files/9a0b2751..2d5e9112

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

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Prasanta Sadhukhan-2
In reply to this post by Alexey Ivanov-2
On Fri, 12 Mar 2021 21:36:58 GMT, Alexey Ivanov <[hidden email]> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   javadoc change
>
> src/java.desktop/share/classes/javax/swing/JComponent.java line 659:
>
>> 657:
>> 658:     /**
>> 659:      * Resets the UI property to a value from the current look and feel.
>
> Shall it rather use plural?
> Suggestion:
>
>      * Resets the UI properties to values from the current look and feel.
> There are usually several properties that get updated, *all the properties* in fact.

I see that all subclass of JComponent implementing updateUI() cites this sentence so changing only here may not be proper unless we change it everywhere, so I guess let it be as it is.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Mon, 15 Mar 2021 04:23:26 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   javadoc change

Marked as reviewed by aivanov (Reviewer).

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Mon, 15 Mar 2021 04:17:51 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> src/java.desktop/share/classes/javax/swing/JComponent.java line 659:
>>
>>> 657:
>>> 658:     /**
>>> 659:      * Resets the UI property to a value from the current look and feel.
>>
>> Shall it rather use plural?
>> Suggestion:
>>
>>      * Resets the UI properties to values from the current look and feel.
>> There are usually several properties that get updated, *all the properties* in fact.
>
> I see that all subclass of JComponent implementing updateUI() cites this sentence so changing only here may not be proper unless we change it everywhere, so I guess let it be as it is.

Agreed.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Mon, 15 Mar 2021 10:44:51 GMT, Alexey Ivanov <[hidden email]> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   javadoc change
>
> Marked as reviewed by aivanov (Reviewer).

> I thought about it earlier but I raised this PR as I don't see this method was updated from inception so I was not sure if it will be in future and also since it already says "JComponent subclasses **must** override this method" so a noop default implementation will not be a problem, I guess.

Based on the current spec we may add some kind of assertion that this method must not be called.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Sergey Bylokhov-2
In reply to this post by Prasanta Sadhukhan-2
On Mon, 15 Mar 2021 04:23:26 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   javadoc change

src/java.desktop/share/classes/javax/swing/JComponent.java line 660:

> 658:     /**
> 659:      * Resets the UI property to a value from the current look and feel.
> 660:      * Since the default implementation of this method doesn't do anything,

The reason why this method should be overridden is that the subclasses only can implement it properly, and it is not because the current method is empty.

Probably we can move the new text to the @implnote tag?

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Alexey Ivanov-2
On Mon, 15 Mar 2021 17:37:07 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   javadoc change
>
> src/java.desktop/share/classes/javax/swing/JComponent.java line 660:
>
>> 658:     /**
>> 659:      * Resets the UI property to a value from the current look and feel.
>> 660:      * Since the default implementation of this method doesn't do anything,
>
> The reason why this method should be overridden is that the subclasses only can implement it properly, and it is not because the current method is empty.
>
> Probably we can move the new text to the @implnote tag?

Yet specifying that this method is empty implies overridden methods do not need to call `super.updateUI()`.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Alexey Ivanov-2
In reply to this post by Sergey Bylokhov-2
On Mon, 15 Mar 2021 17:34:30 GMT, Sergey Bylokhov <[hidden email]> wrote:

> > I thought about it earlier but I raised this PR as I don't see this method was updated from inception so I was not sure if it will be in future and also since it already says "JComponent subclasses **must** override this method" so a noop default implementation will not be a problem, I guess.
>
> Based on the current spec we may add some kind of assertion that this method must not be called.

I'm afraid this can be interpreted in a wrong way: *never* call `JComponent.updateUI()`, which is not what we want to say, is it?

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Mon, 15 Mar 2021 18:06:57 GMT, Alexey Ivanov <[hidden email]> wrote:

>> src/java.desktop/share/classes/javax/swing/JComponent.java line 660:
>>
>>> 658:     /**
>>> 659:      * Resets the UI property to a value from the current look and feel.
>>> 660:      * Since the default implementation of this method doesn't do anything,
>>
>> The reason why this method should be overridden is that the subclasses only can implement it properly, and it is not because the current method is empty.
>>
>> Probably we can move the new text to the @implnote tag?
>
> Yet specifying that this method is empty implies overridden methods do not need to call `super.updateUI()`.

But probably it might be changed in the future? If for example, we will move some common functionality from the standard subclasses to this method.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Alexey Ivanov-2
On Mon, 15 Mar 2021 21:12:11 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Yet specifying that this method is empty implies overridden methods do not need to call `super.updateUI()`.
>
> But probably it might be changed in the future? If for example, we will move some common functionality from the standard subclasses to this method.

It hasn't changed over all these years… In addition to that, the specification says you *must* override and shows the example without calling `super.updateUI()`.
My take is it's unlikely to change. Yet I understand your concern: Stating explicitly `JComponent.updateUI` is empty, there's no way back. So being cautious wouldn't hurt. From this point of view, `@implnote` still gives some flexibility rather than being cast in stone.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v3]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.

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

  added @implnote

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2955/files
  - new: https://git.openjdk.java.net/jdk/pull/2955/files/2d5e9112..a6484203

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

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Prasanta Sadhukhan-2
In reply to this post by Alexey Ivanov-2
On Mon, 15 Mar 2021 21:35:23 GMT, Alexey Ivanov <[hidden email]> wrote:

>> But probably it might be changed in the future? If for example, we will move some common functionality from the standard subclasses to this method.
>
> It hasn't changed over all these years… In addition to that, the specification says you *must* override and shows the example without calling `super.updateUI()`.
> My take is it's unlikely to change. Yet I understand your concern: Stating explicitly `JComponent.updateUI` is empty means there's no way back. So being cautious wouldn't hurt. From this point of view, `@implnote` still gives some flexibility rather than being cast in stone.

`@implnote` added

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v3]

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Tue, 16 Mar 2021 04:22:27 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   added @implnote

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/JComponent.java line 660:

> 658:     /**
> 659:      * Resets the UI property to a value from the current look and feel.
> 660:      * @implnote Since the default implementation of this method doesn't do anything,

Suggestion:

     * @implNote Since the default implementation of this method doesn't do anything,
The correct tag is `@implNote` with capital N.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v4]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> It's unclear from the spec that the original implementation of the method JComponent.updateUI() does nothing which needs to be explicitly stated in the javadoc.

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

  Use @implNote

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2955/files
  - new: https://git.openjdk.java.net/jdk/pull/2955/files/a6484203..39c4a720

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

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v3]

Prasanta Sadhukhan-2
In reply to this post by Alexey Ivanov-2
On Tue, 16 Mar 2021 12:11:19 GMT, Alexey Ivanov <[hidden email]> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   added @implnote
>
> src/java.desktop/share/classes/javax/swing/JComponent.java line 660:
>
>> 658:     /**
>> 659:      * Resets the UI property to a value from the current look and feel.
>> 660:      * @implnote Since the default implementation of this method doesn't do anything,
>
> Suggestion:
>
>      * @implNote Since the default implementation of this method doesn't do anything,
> The correct tag is `@implNote` with capital N.

It was suggested to use @implnote tag and I saw that couple of places in jdk repo, we used that in Unsafe.java and TrayIcon.java so I used that, although in majority of places, @implNote is used, so I thought maybe both can be used at our discretion.
Anyway, will change to @implNote.

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

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

Re: <Swing Dev> RFR: 8263472: Specification of JComponent::updateUI doesn't mention that the original implementaiton does nothing [v2]

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Tue, 16 Mar 2021 04:24:14 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> It hasn't changed over all these years… In addition to that, the specification says you *must* override and shows the example without calling `super.updateUI()`.
>> My take is it's unlikely to change. Yet I understand your concern: Stating explicitly `JComponent.updateUI` is empty means there's no way back. So being cautious wouldn't hurt. From this point of view, `@implnote` still gives some flexibility rather than being cast in stone.
>
> `@implnote` added

It feels wrong with this `@implNote`.

Subclasses *must still override* the default implementation. Should this statement be included in the regular spec text as it was before? Implementation note could say, “The default implementation doesn't do anything.”

Or alternatively, this change can be rejected at all, leaving the spec as it is now.

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

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