Issue 6565 in pharo: Fix #next:putAll:startingAt: for MultiByteFileStream

Status: Accepted
Owner: marianopeck
CC: [hidden email]
Labels: Type-Bug

New issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

Hi guys. Together with the help of Sven, he found there is a problem with  
MultiByteFileStream since it does NOT override #next:putAll:startingAt: to  
add support for the converters, and therefore uses the one of  
StandardFileStream. This breaks with using text-based streams with non-utf  
characters.

I found that this is fixed in Squeak and I took the changes from there. All  
I can say about it is that tests are ok, and my particular case that was  
broken, is now working correctly.

Here I attach the .cs. I would really need someone to take a look.

Thanks

Attachments:
        FixMultiByteFileStream.cs  2.5 KB


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixReviewNeeded

Comment #1 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Cc: [hidden email]

Comment #2 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: WorkNeeded

Comment #3 on issue 6565 by [hidden email]: Fix #next:putAll:startingAt:  
for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

The categories look wrong.
In addition, there is a bug when putting non-ascii characters from a string  
startingAt: > 1, since the nextIndex > anInteger check is incorrect.

conv := ISO885915TextConverter new.
str := (String new: 10) writeStream.
source := 'abcdefåghijklmnopqrst'. "Notice å at 6)
conv next: 5 putAll: source startingAt: 5 toStream: str
self assert: str contents = 'efågh' "Though 8859-15 is a different  
character set, å has the same codePoint as in 8859-1, so comparing like  
this is ok"

I'll provide fixed slices shortly

_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #4 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

THANKS HENRY!!! :)  btw, will you attend ESUG?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #5 on issue 6565 by [hidden email]: Fix  
#next:putAll:startingAt: for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

We should have a test as well, something like

| file |
file := '/Users/sven/Desktop/foo.txt' asFileReference.
file writeStream nextPutAll: 'élève en Français'; close.
file readStream contents.

| file string |
file := '/Users/sven/Desktop/foo.txt' asFileReference.
string := 'élève en Français'.
file writeStream next: string size putAll: string startingAt: 1.
file readStream contents.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #6 on issue 6565 by [hidden email]: Fix #next:putAll:startingAt:  
for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

Well, bad example, since the latin1map doesn't contain entries for  
equivalent characters...
The bug is still valid though, I'll provide a working test with the final  
slice set.

Slices MUST be loaded in order in separate updates, or you might end up  
removing crucial streaming methods before they are no longer used

Also, I've followed the preexisting pattern of implementing basicNext: ...  
on Stream, so it can be used with regular WriteStreams as well.
For non-writable streams it should ofcourse still fail with  
ShouldNotBeImplemented due to implementation eventually using a nextPut:


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixReviewNeeded

Comment #7 on issue 6565 by [hidden email]: Fix #next:putAll:startingAt:  
for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

See SLICE-Issue-6565-nextputAllstartingAt-using-converters , version 1 and  
2.
Like said, they need to be loaded in order to avoid possibly borking your  
system.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #8 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

Hi Henry. I couldn't load the first slice. It throws a Zinc exception. I  
attach here the Pharodebug.log

Attachments:
        PharoDebug.log  22.2 KB


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #9 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

it seems nobody implements #unescapePercents


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #10 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

mmmm #unescapePercents does exists in the image, in String under the  
category "*multilingual-textConversion"  so maybe they are removed/losts  
while loading the slice?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #11 on issue 6565 by [hidden email]: Fix  
#next:putAll:startingAt: for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

I can load them just fine, 2.0 ##20201


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #12 on issue 6565 by [hidden email]: Fix #next:putAll:startingAt:  
for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

Not sure why it'sbeen removed, but it used to be in String, category  
*network-uri, and look like this:
unescapePercents
        "decode %xx form.  This is the opposite of #encodeForHTTP"
        ^ self unescapePercentsWithTextEncoding: 'utf-8'.

and the accompanying:

unescapePercentsWithTextEncoding: encodingName
        "decode string including %XX form"
        | unescaped char asciiVal specialChars oldPos pos converter |
        unescaped := ReadWriteStream on: String new.
        specialChars := '+%' asCharacterSet.
        oldPos := 1.
        [pos := self indexOfAnyOf: specialChars startingAt: oldPos.
        pos > 0]
                whileTrue: [unescaped
                                nextPutAll: (self copyFrom: oldPos to: pos - 1).
                        char := self at: pos.
                        (char = $%
                                        and: [pos + 2 <= self size])
                                ifTrue: [asciiVal := (self at: pos + 1) asUppercase digitValue * 16 +  
(self at: pos + 2) asUppercase digitValue.
                                        asciiVal > 255
                                                ifTrue: [^ self].
                                        unescaped
                                                nextPut: (Character value: asciiVal).
                                        pos := pos + 3.
                                        pos <= self size
                                                ifFalse: [char := nil].
                                        oldPos := pos]
                                ifFalse: [char = $+
                                                ifTrue: [unescaped nextPut: Character space]
                                                ifFalse: [unescaped nextPut: char].
                                        oldPos := pos + 1]].
        oldPos <= self size
                ifTrue: [unescaped
                                nextPutAll: (self copyFrom: oldPos to: self size)].
        converter := (TextConverter newForEncoding: encodingName)
                                ifNil: [TextConverter newForEncoding: nil].
        ^ [unescaped contents convertFromWithConverter: converter]
                on: Error
                do: ["the contents may be squeak-encoded"
                        unescaped contents]


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #13 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

Sven: I am in 20248. So maybe for some reason it doesn't work in new  
Pharo? :(
Henry: in which image are you testing?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #14 on issue 6565 by [hidden email]: Fix #next:putAll:startingAt:  
for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

well yes, they will have been removed when loading the slice, since it's  
based on the version which was in my one-click. I'll try to merge in and  
rebase on latest version.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #15 on issue 6565 by [hidden email]: Fix #next:putAll:startingAt:  
for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

OK, slices marked with UPDATE are applicable to latest 2.0.

The old ones may be suitable for a patch to 1.4 ;)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #16 on issue 6565 by marianopeck: Fix #next:putAll:startingAt: for  
MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

Hi Henry. Thanks! I have just tried the slices and they now do work and  
solve my original problem. I checked the slices and from my newbie point of  
view, they are correct. So...FixToInclude or Sven wants to review them?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #17 on issue 6565 by [hidden email]: Fix  
#next:putAll:startingAt: for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

thanks a lot of all the positive energy :)



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: HumanReviewNeeded
        Labels: Milestone-2.0

Comment #18 on issue 6565 by [hidden email]: Fix  
#next:putAll:startingAt: for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixToInclude

Comment #19 on issue 6565 by [hidden email]: Fix  
#next:putAll:startingAt: for MultiByteFileStream
http://code.google.com/p/pharo/issues/detail?id=6565

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker