Quantcast

<AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

<AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Sergey Bylokhov
Hello,
Please review the fix for jdk9.


 - Two typos were fixed in AbstractMultiResolutionImage.java(incorrect tag) and java/awt/package-info.java("con tainer"instead of "container")
 - The code formatting was fixed in "module-info.java", it seems that different iterations of modules refresh patches used a different line wrapping and code shifting(4 or 5 spaces)
 - In java sound there are a few places where we replace tabs to spaces and this caused strange alignment of constants


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Semyon Sadetsky

On 04/19/2017 10:29 AM, Sergey Bylokhov wrote:
Hello,
Please review the fix for jdk9.


 - Two typos were fixed in AbstractMultiResolutionImage.java(incorrect tag) and java/awt/package-info.java("con tainer"instead of "container")
 - The code formatting was fixed in "module-info.java", it seems that different iterations of modules refresh patches used a different line wrapping and code shifting(4 or 5 spaces)
 - In java sound there are a few places where we replace tabs to spaces and this caused strange alignment of constants
Same issues can be found in so many classes of java.desktop

grep -rE "\\S+\\s{2,}=|\\S+=\\s{2,}\S+|^\\s{5,5}(public|private|static|final|void)" --include \*.java jdk/src/java.desktop | wc -l

2685 !!!

Why you decided to fix only those several?

-Semyon
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Sergey Bylokhov
>>
> Same issues can be found in so many classes of java.desktop
>
> grep -rE "\\S+\\s{2,}=|\\S+=\\s{2,}\S+|^\\s{5,5}(public|private|static|final|void)" --include \*.java jdk/src/java.desktop | wc -l
>
> 2685 !!!
>
> Why you decided to fix only those several?

This change fix the problem in the public api of java sound which were produced by replacing of tabs to spaces. If you take a look to the output of your grep you will see that it find something different.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> <Sound Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Philip Race
In reply to this post by Sergey Bylokhov
+1

-phil.

On 04/19/2017 10:29 AM, Sergey Bylokhov wrote:

> Hello,
> Please review the fix for jdk9.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8178971
> Webrev can be found at:
> http://cr.openjdk.java.net/~serb/8178971/webrev.00 
> <http://cr.openjdk.java.net/%7Eserb/8178971/webrev.00>
> Webrev which ignores spaces:
> http://cr.openjdk.java.net/~serb/8178971/webrev_no_space.00 
> <http://cr.openjdk.java.net/%7Eserb/8178971/webrev_no_space.00>
>
>  - Two typos were fixed in AbstractMultiResolutionImage.java(incorrect
> tag) and java/awt/package-info.java("con tainer"instead of "container")
>  - The code formatting was fixed in "module-info.java", it seems that
> different iterations of modules refresh patches used a different line
> wrapping and code shifting(4 or 5 spaces)
>  - In java sound there are a few places where we replace tabs to
> spaces and this caused strange alignment of constants
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Semyon Sadetsky
In reply to this post by Sergey Bylokhov
On 04/19/2017 12:43 PM, Sergey Bylokhov wrote:

>> Same issues can be found in so many classes of java.desktop
>>
>> grep -rE "\\S+\\s{2,}=|\\S+=\\s{2,}\S+|^\\s{5,5}(public|private|static|final|void)" --include \*.java jdk/src/java.desktop | wc -l
>>
>> 2685 !!!
>>
>> Why you decided to fix only those several?
> This change fix the problem in the public api of java sound which were produced by replacing of tabs to spaces. If you take a look to the output of your grep you will see that it find something different.
>
In this grep output I see absolutely the same 5 spaces indentation
instead of 4 spaces, multiple spaces  around "=".
The source of those formatting issues maybe the same (replacing tabs by
spaces). And I did not get why the source is important?

 From the descripton of the bug you pretend:

"Uncommon formatting and typos in java.desktop module"

Then why only javax.sound is considered? A bug tittle issue?

But this contradicts to the webrev you've sent because it contains

src/java.desktop/share/classes/java/awt/image/AbstractMultiResolutionImage.java
src/java.desktop/share/classes/java/awt/package-info.java
src/java.desktop/share/classes/module-info.java

files which are outside javax.sound AFAIK.

Also,

src/java.desktop/share/classes/javax/swing/JComponent.java

3738         * the same functionality and it is handled in {@Component}.

should this mistake be corrected? JComponent is also a public API.

--Semyon






Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Sergey Bylokhov
>>
> In this grep output I see absolutely the same 5 spaces indentation instead of 4 spaces, multiple spaces  around "=".
> The source of those formatting issues maybe the same (replacing tabs by spaces). And I did not get why the source is important?

Its is unimportant because most of them are not in the public API, some of them are aligned intentionally, but if you will find something useful feel free to file it file it and fix.

>
> From the descripton of the bug you pretend:
>
> "Uncommon formatting and typos in java.desktop module"
>
> Then why only javax.sound is considered? A bug tittle issue?

Did you read the text which were in the first email, which describe what things were fixed?  

>
> But this contradicts to the webrev you've sent because it contains
>
> src/java.desktop/share/classes/java/awt/image/AbstractMultiResolutionImage.java
> src/java.desktop/share/classes/java/awt/package-info.java
> src/java.desktop/share/classes/module-info.java
>
> files which are outside javax.sound AFAIK.

Just reread an initial message which has 3 points.

>
> Also,
>
> src/java.desktop/share/classes/javax/swing/JComponent.java
>
> 3738         * the same functionality and it is handled in {@Component}.
>
> should this mistake be corrected? JComponent is also a public API.

Yes, it should be fixed. This line was added after I created the patch. The new version is here:
http://cr.openjdk.java.net/~serb/8178971/webrev.01
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Semyon Sadetsky
On 04/19/2017 02:23 PM, Sergey Bylokhov wrote:
>> In this grep output I see absolutely the same 5 spaces indentation instead of 4 spaces, multiple spaces  around "=".
>> The source of those formatting issues maybe the same (replacing tabs by spaces). And I did not get why the source is important?
> Its is unimportant because most of them are not in the public API, some of them are aligned intentionally, but if you will find something useful feel free to file it file it and fix.
I see a lot of such formatting issues (hundreds) in public APIs. Are you
proposing to create bugs per each of them?
Why not fix them in this bug? This is very easy and harmless.
>
>>  From the descripton of the bug you pretend:
>>
>> "Uncommon formatting and typos in java.desktop module"
>>
>> Then why only javax.sound is considered? A bug tittle issue?
> Did you read the text which were in the first email, which describe what things were fixed?
Yes, I did. But it doesn't correspond to JIRA's title and description.

>
>> But this contradicts to the webrev you've sent because it contains
>>
>> src/java.desktop/share/classes/java/awt/image/AbstractMultiResolutionImage.java
>> src/java.desktop/share/classes/java/awt/package-info.java
>> src/java.desktop/share/classes/module-info.java
>>
>> files which are outside javax.sound AFAIK.
> Just reread an initial message which has 3 points.
>
>> Also,
>>
>> src/java.desktop/share/classes/javax/swing/JComponent.java
>>
>> 3738         * the same functionality and it is handled in {@Component}.
>>
>> should this mistake be corrected? JComponent is also a public API.
> Yes, it should be fixed. This line was added after I created the patch. The new version is here:
> http://cr.openjdk.java.net/~serb/8178971/webrev.01
Thanks. Yet another one:

src/java.desktop/share/classes/javax/swing/text/html/AccessibleHTML.java

1655             * @gsee #getAccessibleChild

I suggest to grep code for javadoc keywords (@...) typos.

--Semyon




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Sergey Bylokhov
Its is unimportant because most of them are not in the public API, some of them are aligned intentionally, but if you will find something useful feel free to file it file it and fix.
I see a lot of such formatting issues (hundreds) in public APIs. Are you proposing to create bugs per each of them?
Why not fix them in this bug? This is very easy and harmless.

Why hundreds? In the previous email you mention 2500 issues, so feel free to file them and fix.
I fixed an issues which I found when I reread the specification of the methods and tried to find a typos in it.


From the descripton of the bug you pretend:

"Uncommon formatting and typos in java.desktop module"

Then why only javax.sound is considered? A bug tittle issue?
Did you read the text which were in the first email, which describe what things were fixed?
Yes, I did. But it doesn't correspond to JIRA's title and description.

I updated description of the bug.


But this contradicts to the webrev you've sent because it contains

src/java.desktop/share/classes/javax/swing/text/html/AccessibleHTML.java

1655             * @gsee #getAccessibleChild

I suggest to grep code for javadoc keywords (@...) typos.

This is not a public API.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: <AWT Dev> [9] Review Request: 8178971 Uncommon formatting and typos in java.desktop module

Semyon Sadetsky

On 04/19/2017 03:32 PM, Sergey Bylokhov wrote:

Its is unimportant because most of them are not in the public API, some of them are aligned intentionally, but if you will find something useful feel free to file it file it and fix.
I see a lot of such formatting issues (hundreds) in public APIs. Are you proposing to create bugs per each of them?
Why not fix them in this bug? This is very easy and harmless.

Why hundreds? In the previous email you mention 2500 issues, so feel free to file them and fix.
I fixed an issues which I found when I reread the specification of the methods and tried to find a typos in it.
In this situation I'd suggest to grep for similar typos in the rest code. It is not time consumptive.


From the descripton of the bug you pretend:

"Uncommon formatting and typos in java.desktop module"

Then why only javax.sound is considered? A bug tittle issue?
Did you read the text which were in the first email, which describe what things were fixed?
Yes, I did. But it doesn't correspond to JIRA's title and description.

I updated description of the bug.\
When I was reading the JIRA I've decided that the purpose of the bug is to search and cleanup all javadoc and formatting issues in java.desktop module. Next time please use something like "Several typos and formatting issues."...


But this contradicts to the webrev you've sent because it contains

src/java.desktop/share/classes/javax/swing/text/html/AccessibleHTML.java

1655             * @gsee #getAccessibleChild

I suggest to grep code for javadoc keywords (@...) typos.

This is not a public API.

Okay. Looks good.

--Semyon

Loading...