<Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

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

<Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

Prasanta Sadhukhan-2
BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
and other properties are set if the current value is null
but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
Fixed the javadoc to specify the same.

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

Commit messages:
 - 6251901: BasicTextUI: installDefaults method are contrary to the documentation

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

Alexander Zvegintsev-2
On Tue, 9 Mar 2021 08:09:04 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
> and other properties are set if the current value is null
> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
> Fixed the javadoc to specify the same.

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 306:

> 304:      * Initializes component properties, such as font, foreground,
> 305:      * background, caret color, selection color, selected text color,
> 306:      * disabled text color, and border color.  All properties are set

This method is also affects a margin, but it is does not mentioned in the javadoc.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

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

> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
> and other properties are set if the current value is null
> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
> Fixed the javadoc to specify the same.

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 307:

> 305:      * background, caret color, selection color, selected text color,
> 306:      * disabled text color, and border color.  All properties are set
> 307:      * if their current value is either null or a UIResource.

Suggestion:

     * disabled text color, and border color.  Each property is set,
     * if its current value is either null or a UIResource.

I think this conveys the behavior better. Each property is checked separately, independently from other properties.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

Alexey Ivanov-2
In reply to this post by Alexander Zvegintsev-2
On Tue, 9 Mar 2021 13:34:20 GMT, Alexander Zvegintsev <[hidden email]> wrote:

>> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
>> and other properties are set if the current value is null
>> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
>> Fixed the javadoc to specify the same.
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 306:
>
>> 304:      * Initializes component properties, such as font, foreground,
>> 305:      * background, caret color, selection color, selected text color,
>> 306:      * disabled text color, and border color.  All properties are set
>
> This method is also affects a margin, but it is does not mentioned in the javadoc.

Does the list of properties listed in the javadoc have to be comprehensive? However, `margin` seems to be the only property omitted from the list.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

Alexander Zuev-3
In reply to this post by Alexey Ivanov-2
On Tue, 9 Mar 2021 21:12:08 GMT, Alexey Ivanov <[hidden email]> wrote:

>> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
>> and other properties are set if the current value is null
>> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
>> Fixed the javadoc to specify the same.
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 307:
>
>> 305:      * background, caret color, selection color, selected text color,
>> 306:      * disabled text color, and border color.  All properties are set
>> 307:      * if their current value is either null or a UIResource.
>
> Suggestion:
>
>      * disabled text color, and border color.  Each property is set,
>      * if its current value is either null or a UIResource.
>
> I think this conveys the behavior better. Each property is checked separately, independently from other properties.

I'm not an expert in English but i think comma after set is not needed.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

Alexey Ivanov-2
On Tue, 9 Mar 2021 21:31:03 GMT, Alexander Zuev <[hidden email]> wrote:

> I'm not an expert in English but i think comma after set is not needed.

You're right, comma is not needed in this case. I updated my comment and removed the comma.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v2]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
> and other properties are set if the current value is null
> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
> Fixed the javadoc to specify the same.

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/2888/files
  - new: https://git.openjdk.java.net/jdk/pull/2888/files/96ae8509..b27104e5

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

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
> and other properties are set if the current value is null
> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
> Fixed the javadoc to specify the same.

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

  copyright change

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2888/files
  - new: https://git.openjdk.java.net/jdk/pull/2888/files/b27104e5..fad21b0d

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

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Prasanta Sadhukhan-2
In reply to this post by Alexey Ivanov-2
On Tue, 9 Mar 2021 21:17:13 GMT, Alexey Ivanov <[hidden email]> wrote:

>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 306:
>>
>>> 304:      * Initializes component properties, such as font, foreground,
>>> 305:      * background, caret color, selection color, selected text color,
>>> 306:      * disabled text color, and border color.  All properties are set
>>
>> This method is also affects a margin, but it is does not mentioned in the javadoc.
>
> Does the list of properties listed in the javadoc have to be comprehensive? However, `margin` seems to be the only property omitted from the list.

I have added margin too,

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Alexander Zvegintsev-2
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Mar 2021 04:38:19 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
>> and other properties are set if the current value is null
>> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
>> Fixed the javadoc to specify the same.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   copyright change

Marked as reviewed by azvegint (Reviewer).

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Alexander Zuev-3
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Mar 2021 04:38:19 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
>> and other properties are set if the current value is null
>> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
>> Fixed the javadoc to specify the same.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   copyright change

Marked as reviewed by kizune (Reviewer).

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Mar 2021 04:38:19 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
>> and other properties are set if the current value is null
>> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
>> Fixed the javadoc to specify the same.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   copyright change

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 306:

> 304:      * Initializes component properties, such as font, foreground,
> 305:      * background, caret color, selection color, selected text color,
> 306:      * disabled text color, border and margin. Each property is set

Suggestion:

     * disabled text color, border, and margin. Each property is set
I suggest keeping the command before “and” as it was before.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Prasanta Sadhukhan-2
On Wed, 10 Mar 2021 11:49:40 GMT, Alexey Ivanov <[hidden email]> wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   copyright change
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java line 306:
>
>> 304:      * Initializes component properties, such as font, foreground,
>> 305:      * background, caret color, selection color, selected text color,
>> 306:      * disabled text color, border and margin. Each property is set
>
> Suggestion:
>
>      * disabled text color, border, and margin. Each property is set
> I suggest keeping the command before “and” as it was before.

I think these are nouns and as per I know, we shouldn't place comma before "and" when we are simply listing the items.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

Alexey Ivanov-2
On Wed, 10 Mar 2021 12:13:43 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> I think these are nouns and as per I know, we shouldn't place comma before "and" when we are simply listing the items.

This is called [Oxford Comma](https://www.grammarly.com/blog/what-is-the-oxford-comma-and-why-do-people-care-so-much-about-it/). It is quite standard in American English and javadoc uses American English. The comma was present before therefore I suggest following the style and add the now missing comma.

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v4]

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
> and other properties are set if the current value is null
> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
> Fixed the javadoc to specify the same.

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

  Add Oxford Comma

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2888/files
  - new: https://git.openjdk.java.net/jdk/pull/2888/files/fad21b0d..fb7aa827

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

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v3]

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

>> I think these are nouns and as per I know, we shouldn't place comma before "and" when we are simply listing the items.
>
>> I think these are nouns and as per I know, we shouldn't place comma before "and" when we are simply listing the items.
>
> This is called [Oxford Comma](https://www.grammarly.com/blog/what-is-the-oxford-comma-and-why-do-people-care-so-much-about-it/). It is quite standard in American English and javadoc uses American English. The comma was present before therefore I suggest following the style and add the now missing comma.

ok

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

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

Re: <Swing Dev> RFR: 6251901: BasicTextUI: installDefaults method are contrary to the documentation [v4]

Alexey Ivanov-2
In reply to this post by Prasanta Sadhukhan-2
On Wed, 10 Mar 2021 12:28:18 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
>> and other properties are set if the current value is null
>> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
>> Fixed the javadoc to specify the same.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add Oxford Comma

Marked as reviewed by aivanov (Reviewer).

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

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

<Swing Dev> Integrated: 6251901: BasicTextUI: installDefaults method are contrary to the documentation

Prasanta Sadhukhan-2
In reply to this post by Prasanta Sadhukhan-2
On Tue, 9 Mar 2021 08:09:04 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

> BasicTextUI: installDefaults javadoc specifies only font, foreground and background properties are set  if their current value is either null or a UIResource
> and other properties are set if the current value is null
> but in reality all properties such as font, foreground, background, caret color, selection color, selected text color, disabled text color, and border color are set if their current value is either null or a UIResource.
> Fixed the javadoc to specify the same.

This pull request has now been integrated.

Changeset: c0542ed8
Author:    Prasanta Sadhukhan <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/c0542ed8
Stats:     4 lines in 1 file changed: 0 ins; 2 del; 2 mod

6251901: BasicTextUI: installDefaults method are contrary to the documentation

Reviewed-by: azvegint, kizune, aivanov

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

PR: https://git.openjdk.java.net/jdk/pull/2888