The fix looks good to me.
Thanks, Alexandr. On 11/19/2015 11:43 AM, Ambarish Rapte wrote:
Dear Alex, Please take a look at updated webrev, http://cr.openjdk.java.net/~rchamyal/ambarish/8055197/webrev.03/ Updated the test code for testing de-serialization. Many Thanks Ambarish -----Original Message----- From: Alexander Scherbatiy Sent: Friday, November 06, 2015 5:36 PM To: Ambarish Rapte Cc: awt-dev@openjdk.java.net Subject: Re: <AWT Dev> Review Request for 8055197: TextField deletes multiline strings On 11/5/2015 2:34 PM, Alexander Scherbatiy wrote:On 11/5/2015 1:33 PM, Ambarish Rapte wrote:Hi Alex, I have updated the webrev with changes for deserialization, http://cr.openjdk.java.net/~rchamyal/ambarish/8055197/webrev.02/ Please review. Verified that serialization & de-serialization of text field object works fine. Also updated the test in this webrev to test de-serialization.There is one thing to note that readObject() in the fix first use s.defaultReadObject() that can read a value with new lines into text field so there would be a small time when the TextField has an inconsistence state. It is recommended to read all fields by fields.get(fieldName) to avoid this: ObjectInputStream.GetField fields = in.readFields(); String name = (String) fields.get("name", DEFAULT); Thanks, Alexandr.It is true if a serialized file was created by TextField serialization. The text field does not contain new lines in this case because they were removed by constructor or set method. The question was about serialized files which were generated manually or changed after a serialization. In this case the text field can be rewritten with a value which contains new lines. Thanks, Alexandr.Many Thanks, Ambarish -----Original Message----- From: Alexander Scherbatiy Sent: Thursday, October 29, 2015 9:59 PM To: Ambarish Rapte Cc: Prasanta Sadhukhan; Semyon Sadetsky; awt-dev@openjdk.java.net; Sergey Bylokhov Subject: Re: Review Request for 8055197: TextField deletes multiline strings On 10/29/2015 5:31 PM, Ambarish Rapte wrote:Hi Alexandr, Please review the updated webrev, with changes only in TextField.java. Webrev: http://cr.openjdk.java.net/~rchamyal/ambarish/8055197/webrev.01/ replaceEOL() is declared static, as a class member method cannot be called before call to super from Constructor. Also made changes, as per Sergey's comment for, System.lineSeparator()Thank you. Could you also check the TextField deserialization? It always should be kept in mind that fields in a deserialized object need to passe the same checks which are applied in constructors and set methods. Thanks, Alexandr.Many Thanks, Ambarish -----Original Message----- From: Alexander Scherbatiy Sent: Wednesday, October 28, 2015 8:48 PM To: Ambarish Rapte Cc: awt-dev@openjdk.java.net; Sergey Bylokhov Subject: Re: Review Request for 8055197: TextField deletes multiline strings On 10/26/2015 11:21 PM, Ambarish Rapte wrote:Hi Alexandr, Yes, This is windows specific issue. Initially there was a native side fix, Please refer below webrev for the earlier changes.http://cr.openjdk.java.net/~psadhukhan/ambarish/8055197/webrev.00/But there was an issue pointed for this fix, and after discussion with Phil, We arrived at this new solution, of replacing on java side.http://cr.openjdk.java.net/~rchamyal/ambarish/8055197/webrev.00/A major review comment for this previous fix was, Sergey: Note that the fix replace the eol char on the peer level. So there will be a difference in behavior of setText/GetText when the peer exists or notI see the point now. Is it possible to add the 'String replaceEOL(String text)' method to the TextField? In this case TextField constructors can remove EOL from the given text and pass it to the super class. It could avoid the TextComponent class changing. Thanks, Alexandr.However both the fix have no side effect, Have executed jtreg on almost all tests which use or test TextField. There is confusion for most correct way to patch this. Please guide Thanks Ambarish -----Original Message----- From: Alexander Scherbatiy Sent: Wednesday, October 21, 2015 7:29 PM To: Ambarish Rapte Cc: awt-dev@openjdk.java.net; Sergey Bylokhov Subject: Re: Review Request for 8055197: TextField deletes multiline strings Is this is Windows specific issue only? If so, it is better to replace EOL on window text peer side or may be even better to do it on the native side before setting a text to the single-line RichEdit. May be there are methods that can remove EOL from a string exactly in the same way as it is done by Edit control. Thanks, Alexandr. On 10/19/2015 9:31 PM, Ambarish Rapte wrote:Hi Alexander, In single line mode , there is no rich edit control style to handle EOL. Referenced documentation: https://msdn.microsoft.com/en-us/library/windows/desktop/bb787605( v= v s .85).aspx Many Thanks Ambarish -----Original Message----- From: Alexander Scherbatiy Sent: Friday, October 16, 2015 4:45 PM To: Ambarish Rapte Cc: awt-dev@openjdk.java.net; Sergey Bylokhov Subject: Re: Review Request for 8055197: TextField deletes multiline strings The TextField is based on Rich Edit control on Windows. Does the Rich Edit control have properties to properly handle multiline strings in single-line mode? Thanks, Alexandr. On 10/14/2015 7:44 PM, Ambarish Rapte wrote:Dear All, Please review the patch for jdk9. Bug: https://bugs.openjdk.java.net/browse/JDK-8055197 Webrev: http://cr.openjdk.java.net/~rchamyal/ambarish/8055197/webrev.00/ Issue: - If text containing new line character is set to TextField, Text after new line character was terminated. - Issue occurs only on windows. Cause: - For windows new line character is ‘\r\n’. - For windows code this new line character was not replaced by space character. - Other platforms replace the EOL character by space character. Fix: - Added code to TextComponent.java to remove EOL on java side before passing to peer. => Added a private method replaceEOL() , which replaces EOL by space. => removeEOL will be called by newly added TextComponent construcotr and setText() - The text variable on in TextComponent.java & on string displayed on native side will be same. Many Thanks, Ambarish
