Re: [9] Review Request: 8180889 Cleanup of javadoc in java.datatransfer module
Looks good to me. Thanks, Alexander. On 25/05/2017 06:54, Sergey Bylokhov wrote: Hi, Phil. Thank you for a review, the fix is updated: Webrev can be found at: http://cr.openjdk.java.net/~serb/8180889/webrev.01 Specdiff: http://cr.openjdk.java.net/~serb/8180889/specdiff.01/java/awt/datatransfer/package-summary.html Note that I have updated the text about Oracle's implementation and moved it to @implNote tag: http://cr.openjdk.java.net/~serb/8180889/specdiff.01/java/awt/datatransfer/DataFlavor-report.html#method:getTextPlainUnicodeFlavor() So I'll file a ccc after the technical review. - philip.r...@oracle.com wrote: > I don't know why it was necessary to update the non-API classes here, and I also see some lines reformatted without any actual changes, even white space fixes but apart from adding to noise in the change they are OK. also this is not strictly doc since you cleaned up (not just reordered) some import statements so I added noreg-cleanup to the bug labels. A few nits : DataFlavor.java 104 * to Use Drag and Drop and Data Transfer, section in 105 * Java Tutorial. in Java Tutorial -> in the Java Tutorial 483 * successfully loaded, then an {@code ClassNotFoundException} is an -> a 594 * Sun's implementation for Microsoft Windows uses the encoding 595 * {@code utf-16le}. 596 * Sun's implementation for Solaris and Linux uses the encoding Sun -> Oracle ?? And what about MacOS ? 1249 * @return {@code true} if the {@code DataFlavor} specified represents a 1250 * List of File objects 1250 should perhaps be : {@code java.util.List} of {@code java.io.File} objects -phil. > On 05/23/2017 07:19 PM, Sergey Bylokhov wrote: > Hello, Please review the fix for jdk9. Bug:https://bugs.openjdk.java.net/browse/JDK-8180889 Webrev can be found at:http://cr.openjdk.java.net/~serb/8180889/webrev.00 Specdiff:http://cr.openjdk.java.net/~serb/8180889/specdiff.00/java/awt/datatransfer/package-summary.html In jdk9 a lots of specifications and javadocs were updated, but it seems that java.datatransfer module was missed, because the client team cleanup only the java.desktop module. In this fix the only javadoc is updated and the next rules were applied: - should be replaced by {@tag } - @deprecated tag should have some text - 80 column limit - description of the class/method/field should be followed by dot - @param, @return should not end with a dot, except a case when more than one sentences are used - empty line after description/before the first tag was added - unnecessary empty lines were removed - sets of spaces in the middle of text were deleted - @param, @throws, @return should be aligned, to be more readable - unnecessary imports should be removed - the "null"/"true"/"false"/"this" should be wrapped in {@code } when necessary - the order of different tags were unified across the package The specdiff is provided and the most visible changes are: http://cr.openjdk.java.net/~serb/8180889/specdiff.00/java/awt/datatransfer/DataFlavor-report.html#method:normalizeMimeTypeParameter(java.lang.String,%20java.lang.String) http://cr.openjdk.java.net/~serb/8180889/specdiff.00/java/awt/datatransfer/DataFlavor-report.html#method:normalizeMimeType(java.lang.String) where the text about deprecation was moved to the @deprecated tag. >
Re: [10] Review request for JDK-8180370: Characters are skipped on input of Korean text on OS X
Perhaps the test can be simplified further(some flags can be removed as well as the number of try/catch may be minimized), but this version loos fine. Thanks. - sreeprakas...@oracle.com wrote: > Thanks for the inputs Sergey. > > I have changed the test case to use CountDownLatch and Swing > components. I have also moved the test location from awt/TextField to > swing/JTextField > > Updated Webrev : > http://cr.openjdk.java.net/~rpatil/8180370/webrev.03/ > > I have also tested out the cases that you had mentioned (JDK-8148555, > JDK-8132503,JDK-8073008 & JDK-8068283) and they all worked fine. > > Regards, > Sreeprakash > > -Original Message- > From: Sergey Bylokhov > Sent: Tuesday, June 6, 2017 9:57 AM > To: Sreeprakash Sreedharan> Cc: Philip Race ; awt-dev@openjdk.java.net > Subject: Re: [10] Review request for JDK-8180370: Characters > are skipped on input of Korean text on OS X > > Hi, > I suggest to check the Swing component first because they have more > priority, so the swing test will be useful. > A few notes about the fix: > - Can you please double check that the cases described in > JDK-8148555, JDK-8132503,JDK-8073008,JDK-8068283 still works as > expected. We had some regressions here so it will be useful to double > check. > - Its possible to tweak the test a little bit, and use CountDownLatch > instead of Thread.sleep + interrupt()+TimerTask +Timer. Just await() > the main method and countDown() then the user press the button. In > this case you can move all logic from time to the main method. I guess > this will simplify the test. > - Note that in case of swing you will need to call validateInput() on > EDT. > > > > > As the term "Special character" was ambiguous, I have renamed the > test case and the appropriate comments and also removed some swing > dependencies. > > Updated Webrev : > http://cr.openjdk.java.net/~rpatil/8180370/webrev.02/ > > > > Also, I have written the test case for the AWT component (TextField) > only. > > Is it required to write a duplicate test case for the equivalent > swing component (JTextField) ? > > > > Regards, > > Sreeprakash > > > > -Original Message- > > From: Phil Race > > Sent: Saturday, June 3, 2017 2:29 AM > > To: Sreeprakash Sreedharan ; > > awt-dev@openjdk.java.net > > Subject: Re: [10] Review request for JDK-8180370: > Characters > > are skipped on input of Korean text on OS X > > > > OK .. although I think this may need bake time before being > backported. > > but since I suppose hardly anyone is using or testing 10 at the > moment that is going to be tricky. > > > > -phil. > > > > On 06/02/2017 01:47 PM, Sreeprakash Sreedharan wrote: > >> Thanks for the inputs Phil. > >> > >> I have updated the code and the comments. > >> > >> Updated Webrev : > >> http://cr.openjdk.java.net/~rpatil/8180370/webrev.01/ > >> > >> The issue was that any non-alpha character(numbers and symbols) > typed immediately after a Korean character was getting skipped. > >> The non-alpha character will come if you type again. > >> For example, an exclamation mark (!) entered after the Korean > character for q will not show up. > >> However, if you type it again it will appear. But instead of having > 2 exclamation marks we have just one. > >> > >> I had tested it out with 2-Set Korean, Wubi Xing (Chinese) and ABC > - Extended keyboard layouts on mac with different input methods . > >> > >> I had run all the manual and automatic regression tests for all > text based controls in awt (like TextField, TextArea etc) and swing > (like JTextField, JTextArea, JTextPane, JEditorPane etc) and did not > find any issues. > >> > >> Thanks, > >> Sreeprakash > >> > >> > >> From: Phil Race > >> Sent: Friday, June 2, 2017 9:44 PM > >> To: Sreeprakash Sreedharan ; > >> awt-dev@openjdk.java.net > >> Subject: Re: [10] Review request for JDK-8180370: > >> Characters are skipped on input of Korean text on OS X > >> > >> I am not familiar with this code but I have a few comments anyway > >> > >> 1. I dislike cluttering the source with bug ids. If we did that for > every fix > >> quite soon the source code would be a mess of semi-random > numbers. > >> Anyone who really wants to know when this change was made has > the > >> history > >> > >> 2. if is not a function. So "if(" -> "if (" > >> > >> 3. When a marked text -> When marked text > >> > >> 4. "!" is not a "special character" .. its quite ordinary .. so > what do you mean ? > >> > >> 5. What testing have you done to make sure no other cases are > broken by this change ? > >> The new test is manual and I'd bet that most tests that might > cover this are manual > >> I'd expect to hear that you have tested different input > scenarios such as a couple > >> of input methods/locales, and AWT and Swing input with a > representative set of > >> input as well as running
Re: [10] Review request for JDK-8180370: Characters are skipped on input of Korean text on OS X
Thanks for the inputs Sergey. I have changed the test case to use CountDownLatch and Swing components. I have also moved the test location from awt/TextField to swing/JTextField Updated Webrev : http://cr.openjdk.java.net/~rpatil/8180370/webrev.03/ I have also tested out the cases that you had mentioned (JDK-8148555, JDK-8132503,JDK-8073008 & JDK-8068283) and they all worked fine. Regards, Sreeprakash -Original Message- From: Sergey Bylokhov Sent: Tuesday, June 6, 2017 9:57 AM To: Sreeprakash SreedharanCc: Philip Race ; awt-dev@openjdk.java.net Subject: Re: [10] Review request for JDK-8180370: Characters are skipped on input of Korean text on OS X Hi, I suggest to check the Swing component first because they have more priority, so the swing test will be useful. A few notes about the fix: - Can you please double check that the cases described in JDK-8148555, JDK-8132503,JDK-8073008,JDK-8068283 still works as expected. We had some regressions here so it will be useful to double check. - Its possible to tweak the test a little bit, and use CountDownLatch instead of Thread.sleep + interrupt()+TimerTask +Timer. Just await() the main method and countDown() then the user press the button. In this case you can move all logic from time to the main method. I guess this will simplify the test. - Note that in case of swing you will need to call validateInput() on EDT. > > As the term "Special character" was ambiguous, I have renamed the test case > and the appropriate comments and also removed some swing dependencies. > Updated Webrev : http://cr.openjdk.java.net/~rpatil/8180370/webrev.02/ > > Also, I have written the test case for the AWT component (TextField) only. > Is it required to write a duplicate test case for the equivalent swing > component (JTextField) ? > > Regards, > Sreeprakash > > -Original Message- > From: Phil Race > Sent: Saturday, June 3, 2017 2:29 AM > To: Sreeprakash Sreedharan ; > awt-dev@openjdk.java.net > Subject: Re: [10] Review request for JDK-8180370: Characters > are skipped on input of Korean text on OS X > > OK .. although I think this may need bake time before being backported. > but since I suppose hardly anyone is using or testing 10 at the moment that > is going to be tricky. > > -phil. > > On 06/02/2017 01:47 PM, Sreeprakash Sreedharan wrote: >> Thanks for the inputs Phil. >> >> I have updated the code and the comments. >> >> Updated Webrev : >> http://cr.openjdk.java.net/~rpatil/8180370/webrev.01/ >> >> The issue was that any non-alpha character(numbers and symbols) typed >> immediately after a Korean character was getting skipped. >> The non-alpha character will come if you type again. >> For example, an exclamation mark (!) entered after the Korean character for >> q will not show up. >> However, if you type it again it will appear. But instead of having 2 >> exclamation marks we have just one. >> >> I had tested it out with 2-Set Korean, Wubi Xing (Chinese) and ABC - >> Extended keyboard layouts on mac with different input methods . >> >> I had run all the manual and automatic regression tests for all text based >> controls in awt (like TextField, TextArea etc) and swing (like JTextField, >> JTextArea, JTextPane, JEditorPane etc) and did not find any issues. >> >> Thanks, >> Sreeprakash >> >> >> From: Phil Race >> Sent: Friday, June 2, 2017 9:44 PM >> To: Sreeprakash Sreedharan ; >> awt-dev@openjdk.java.net >> Subject: Re: [10] Review request for JDK-8180370: >> Characters are skipped on input of Korean text on OS X >> >> I am not familiar with this code but I have a few comments anyway >> >> 1. I dislike cluttering the source with bug ids. If we did that for every fix >> quite soon the source code would be a mess of semi-random numbers. >> Anyone who really wants to know when this change was made has the >> history >> >> 2. if is not a function. So "if(" -> "if (" >> >> 3. When a marked text -> When marked text >> >> 4. "!" is not a "special character" .. its quite ordinary .. so what do you >> mean ? >> >> 5. What testing have you done to make sure no other cases are broken by this >> change ? >> The new test is manual and I'd bet that most tests that might cover this >> are manual >> I'd expect to hear that you have tested different input scenarios such >> as a couple >> of input methods/locales, and AWT and Swing input with a representative >> set of >> input as well as running all the relevant regression tests. >> >> -phil. >> On 06/02/2017 07:48 AM, Sreeprakash Sreedharan wrote: >> Hi All, >> >> Kindly review the fix for JDK10. >> Bug: https://bugs.openjdk.java.net/browse/JDK-8180370 >> Webrev: http://cr.openjdk.java.net/~rpatil/8180370/webrev.00/ >> Issue: Special characters (like !,/\<> ) were getting skipped when >> immediately entered after a marked text on
Re: [9] Review Request: 8180326 Update the tables in java.desktop to be HTML-5 friendly
Hi Sergey, Why do we omitting closing th tag? e.g. + * Metal's system color mapping + * + * + * Key + * Value + * I know that HTML parsers are usually forgiving such things. But sometimes it may make thing worse: https://stackoverflow.com/questions/7125354/what-are-the-actual-problems-of-not-closing-tags-and-attributes-in-html/7135378#7135378 Thanks, Alexander. On 05/06/2017 06:23, Sergey Bylokhov wrote: If there are no objections I'll change the target ws from dev to client, to minimize the merges between some other javadoc fixes. - sergey.bylok...@oracle.com wrote: Hello. Here is an updated version where most of the caption are visible. Bug: https://bugs.openjdk.java.net/browse/JDK-8180326 Webrev can be found at: http://cr.openjdk.java.net/~serb/8180326/webrev.02/ Specdiff: http://cr.openjdk.java.net/~serb/8180326/specdiff.02/overview-summary.html You can use search to check the changes in some specific class: Old docs: http://cr.openjdk.java.net/~serb/8180326/api_old.02/overview-summary.html New docs: http://cr.openjdk.java.net/~serb/8180326/api.02/overview-summary.html - jonathan.gibb...@oracle.com wrote: Phil, I have no evidence one way or the other whether screen readers pay attention to undisplayed or invisible captions. It seemed safest to assume that they would read a visible caption, and that we should head in that general direction. -- Jon On 05/17/2017 11:58 AM, Phil Race wrote: And PS I was not saying anything to contradict tables should not have a summary attribute and should have a caption. However that the docs I read on the web did seem to imply that summary was very much intended for ATs but it was not at all clear this is the point of caption. I'm sure they can read it, but I don't get how making it visible matters to them so how it making it visible relates to accessibility requirements is not an obvious connection to me. So why do we have to make it visible for ATs ? -phil. On 05/17/2017 11:54 AM, Phil Race wrote: I will leave the decision on whether to do that now up to Sergey although it seems all he has to do here is remove "invisible". Many of the "summary" ones had wrong or misleading text but they seem to have been all fixed. I'd want to see what the new HTML looks like with a visible title of course .. -phil. On 05/17/2017 11:52 AM, Jonathan Gibbons wrote: Phil, The bottom line is that in the JDK docs, tables should not have a summary attribute and should have a caption. This comes down to accessibility requirements, where we are slowly raising the bar on our docs, to be in accordance with Oracle's guidelines. Hiding the caption (style="display:none") is an interim measure we have been using during the HTML 5 updates, especially in cases where the person doing the markup changes did not know enough to create an appropriate caption that should be displayed. In time, we should locate and update all table captions (in our standard docs bundle) that are not being displayed such that the text is both appropriate and visible. If you guys want to do that as part of this update, go ahead. FWIW, that is what we did for the java.xml module in the jaxp repo ... pretty much all tables there now have a reasonable, visible caption. -- Jon On 05/17/2017 11:19 AM, Phil Race wrote: I am not sure we are using the summary in a way that makes it worthwhile. As you noted in the other mail "The summary attribute was used to give a more descriptive value of the contents of the table. A caption is more like a title" The values I see are more like a title and as you say that is not the idea. See the example here https://www.w3.org/TR/WCAG20-TECHS/H73.html Caption sounds like a title so it might actually be more appropriate than summary for the text we have except that its not clear why we'd want it to be visible when we were fine without. But being there and invisible may be pointless unless screen readers look for it even if invisible. But if its not doing any harm I guess we can leave it as proposed I still need to look at the rest of the changes. -phil. On 05/12/2017 05:11 PM, Jonathan Gibbons wrote: Sergey, FWIW, the invisible caption should be regarded as a temporary solution, until content authors can review/update the text of the caption and make it visible. The general guideline in this conversion work has been to avoid changing the visible text of the specification, and captions fall into a grey area of whether the text is significant/normative or not. Hence the temporary step to make them not displayed for now. -- Jon On 05/12/2017 05:00 PM, Sergey Bylokhov wrote: The "summary" is unsupported by the HTML5 and we replace it by invisible caption. These new styles are located in the stylesheet.css in the root of the JavaDoc api folder, so I assume these styles should be used by others as well. They were added by this fix: