[Seaside] Encoding problem

[Philippe Marschall]

On Tue, May 20, 2014 at 11:45 PM, Sven Van Caekenberghe <sven at stfx.eu> wrote:
>
> On 20 May 2014, at 23:15, Sven Van Caekenberghe <sven at stfx.eu> wrote:
>
>> The error is in
>>
>> GRPharoUtf8CodecStream>>#greaseNext:putAllFast:startingAt:
>>
>> the first escape clause in the whileFalse computes a wrong first argument to #greaseNext:putAllFast:startingAt:
>>
>> however, it is a bit hard to say what the correct one should be, I will try with
>>
>> startIndex + anInteger - lastIndex
>>
>> I hope there are enough tests.
>
> I added an extra assert
>
>         self assert: 'ab', umlaut next: 1 startingAt: 1 gives:  'a'.
>
> to GRPharoCodecTest>>#testGreaseNextPutAllStartingAt so that it now reads:
>
>         | umlaut encodedUmlaut |
>         umlaut := String with: (Character value: 228).
>         encodedUmlaut := String with: (Character value: 195) with: (Character value: 164).
>         self assert: 'ab' next: 1 startingAt: 1 gives:  'a'.
>         self assert: 'a', umlaut, 'b' next: 1 startingAt: 1 gives:  'a'.
>         self assert: 'ab', umlaut next: 1 startingAt: 1 gives:  'a'.
>         self assert: 'a', umlaut, 'b' next: 2 startingAt: 1 gives:  'a', encodedUmlaut.
>         self assert: 'a', umlaut, 'b' next: 1 startingAt: 2 gives:  encodedUmlaut.
>         self assert: 'a', umlaut, 'b' next: 2 startingAt: 2 gives:  encodedUmlaut, 'b'.
>         self assert: 'a', umlaut, umlaut next: 2 startingAt: 2 gives:  encodedUmlaut, encodedUmlaut.
>         self assert: 'ab', umlaut, 'b', umlaut next: 3 startingAt: 2 gives:  'b', encodedUmlaut, 'b'
>
> it fails before the proposed change, it succeeds after it - and the functional test now passes.

The test worked for me in Pharo 1.3 but fails on Pharo 3.0. When I
compared GRPharoUtf8CodecStream >> #greaseNext:putAll:startingAt: in
Pharo 1.3 and Pharo 3.0 they are different.

This is how it looks in Pharo 1.3

greaseNext: anInteger putAll: aCollection startingAt: startIndex
    aCollection isByteString
        ifTrue: [
            | index |
            index := ByteString findFirstInString: aCollection inSet:
Latin1ToUtf8Map startingAt: startIndex.
            (index = 0 or: [ index > (startIndex + anInteger - 1) ])
                ifTrue: [ stream greaseNext: anInteger putAll:
aCollection startingAt: startIndex ]
                ifFalse: [ super greaseNext: anInteger putAll:
aCollection startingAt: startIndex ] ]
        ifFalse: [ super greaseNext: anInteger putAll: aCollection
startingAt: startIndex ]

And this is how it looks in Pharo 3.0

greaseNext: anInteger putAll: aCollection startingAt: startIndex
    aCollection isByteString
        ifTrue: [ self greaseNext: anInteger putAllFast: aCollection
startingAt: startIndex ]
        ifFalse: [ super greaseNext: anInteger putAll: aCollection
startingAt: startIndex ]

I think the version in Pharo 3.0 is better because it can end up
sending #findFirstInString:inSet:startingAt: and #isByteString one
time less.

If I change the implementation in Pharo 3.0 the test succeeds. But
this is only because we don't end up sending
#greaseNext:putAllFast:startingAt: in this case and directly send
#greaseNext:putAllFast:startingAt:. The issue in
#findFirstInString:inSet:startingAt: remains. Your fix looks right (I
first determined what you be correct and then compared it with your
solution and came up with he same thing).

Thanks for all the work.

Sorry Johan I kinda took the issue from you, that was a bit rude. I
only saw it was assigned to you after I committed.

Philippe