[rfc][icedtea-web] Console Output Encoding Fix

[Jiri Vanek]

On 07/15/2014 04:45 PM, Jiri Vanek wrote:
> On 07/15/2014 04:22 PM, Jiri Vanek wrote:
>> On 07/15/2014 04:10 PM, Jie Kang wrote:
>>> Hello,
>>>
>>> This patch resolves the bug here http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1858
>>>
>>> Characters such as 'ó' were not appearing in the Java Console due to the implementation of
>>> TeeOutputStream appending bytes to a StringBuffer in a byte-by-byte fashion ignoring the fact that
>>> the encodings involve multi-byte characters.
>>>
>>> Also, as far as I can tell the StringBuffer is not used by multiple threads and has been replaced
>>> by StringBuilder (see http://docs.oracle.com/javase/7/docs/api/java/lang/StringBuilder.html)
>>>
>>>
>>> Regards,
>>>
>>
>>
>> Hi!
>>
>> I like this patch!
>>
>> Few nits:
>>
>>
>> This willneed backport to 1.5. Although I strongly agree with StringBuffer->StringBuilder  change, I
>> would be more happy if 1.5 donot get this change, but stays with Buffer (???).
>>
>>
>> +    private void appendString(String s) {
>> +        string.append(s);
>> +        flushLog();
>> +    }
>>
>> Why are you flushing there? I thing the flushing is done right. Have you encountered some  case when
>> some logs were missing?
>>
>> @@ -90,9 +90,11 @@
>>           } else if (len == 0) {
>>               return;
>>           }
>> -        for (int i = 0; i < len; i++) {
>> -            appendChar(b[off + i]);
>> +        byte[] offsetString = new byte[len];
>> +        for (int i=0; i<len; i++) {
>> +            offsetString[i] = b[off+i];
>>           }
>> +        appendString(new String(offsetString));
>>           super.write(b, off, len);
>>       }
>>
>>
>> Looking onto this, I'mnot sure how correct it is. I'm a bit afraid it may not handle various
>> encoding correctly (/me must try and check) ).  Also I need to check  the performancy here.
>>
>>
>> And most uncomfortable nit last:
>> You can see that teeOutputStream is lacking unittests. This is great opportunity to add some. And
>> also cover the 1858.
>>
>>
>> ty!
>>
>>   J.
>>
>
> Ok. I looked a bit more inside, and I might be wrong, but this do not seam to me complete fix.
>
> First issue - you fixed only
>
> write(byte[] b, int off, int len) {
>
> but the
>
> void write(int b) {
>
> maybe affected in same way.
>
> Also the write(byte[] b, int off, int len)  bay end in middle of character.
>
> I think the only correct solution here is to have bytebuffer, to append individidual bytes, and once
> the last byte is \n (yes it have its just number) then pretend it is line-end, and convert whole
> bytebuffer to String. (and yes, probably stay with platform default encoding)
>
> I think it needs physical redesign of this class.
>
>
> I'm not sure if my suggestion is overkilling, or if I'm not judging your approach wrongly. I would
> be happy for more voices and sorry if I do.
>
> J.
>
> ps: this doubles the need of unittests.


Fwd - the lists are broken somehow:(