svenvc / ston

STON - Smalltalk Object Notation - A lightweight text-based, human-readable data interchange format for class-based object-oriented languages like Smalltalk.
MIT License
135 stars 32 forks source link

Bug reading instances of Text #9

Closed tinchodias closed 10 years ago

tinchodias commented 10 years ago

Hi,

This happens only when there is a repeated character. Reproduce with:

1)

| aString |

aString := STON toString: 'hh' asText. 
" ---> 'Text[Character[''h''],@2]' "

STON fromString: aString.

2) Get ByteString(Object)>>error:

I'm on Pharo 30780 and STON-Core-SvenVanCaekenberghe.47

svenvc commented 10 years ago

I am not sure it is a bug, rather an issue of two behaviours coming together in an unintended way. The real problem is that there is no and maybe - for you - there should be a correct way to serialise Text instances.

Right now, Text is treated as a generic SequenceableCollection subclass (which is wrong, it contains string characters as well as runs), much like an OrderedCollection. This also leads to the individual elements being treated as independent Character objects (that can be shared). When parsing and reconstructing, #streamContents: is used but that does not accept all objects (in particular it does not accept the not yet resolved STONReference).

IMHO, the solution is to add proper custom STON serialise/deserialise code to Text (handling both the string and runs). The question is, should this be in the general framework or just in your code ? STON is artificially limited to basic, cross-platform, model style classes.

In what use case do you encounter this ?

tinchodias commented 10 years ago

Hi,

Thanks for your fast answer. Now I understand the problem. My case is serialisation of a ClassComment system announcement. I guess it is a bug in Pharo, and easy to fix:

"Subscribe"
anns := OrderedCollection new.
SystemAnnouncer uniqueInstance weak
    when:  ClassCommented
    send: #add:
    to: anns.

"Now, copy a highlighted text from a Workspace into a class comment, and Accept."

"Check what was announced"
anns last newComment isText. "---> true" 
anns last classCommented comment isText. "---> false"

This shows that the announced ClassComment says that the newComment is a text, while in fact, if you ask to the class it was converted to a ByteString when it was applied. So I guess that it's clear that newComment should be a ByteString also here.

I will report an issue in Pharo, and meanwhile I can just convert to String before serialisation. It's fine for me.

tinchodias commented 10 years ago

Finally, I didn't report it as Pharo issue because maybe it was only in my needs. Anyway, I had again a case when I needed to materialize Texts. So I implemented it in Text class-side this way:

fromSton: stonReader
    "A Text can have references to other objects."

    ^ self streamContents: [ :stream |
        stonReader parseListDo: [ :each |
            | object |
            object:= 
                each isStonReference
                    ifFalse: [ each ]
                    ifTrue: [ stonReader resolveReference: each ].
            stream nextPut: object ] ]
svenvc commented 10 years ago

IMHO, the only correct solution is to treat Text (and RunArray) as normal objects and not as collections with respect to STON. They inherit the wrong behaviour for what they really are.

I tried the following (both for Text and RunArray):

instance side

stonOn: stonWriter
    stonWriter writeObject: self

fromSton: stonReader
    stonReader parseMapDo: [ :instVarName :value |
        self instVarNamed: instVarName put: value ]

class side

fromSton: stonReader
    ^ self new
        fromSton: stonReader;
        yourself

Then the following works correctly:

| text |
text := (Text string: 'I am bold' attribute: TextEmphasis bold), ' and I am normal text'.
(STON fromString: (STON toString: text)) = text.

text asMorph openInWindow.

But I am really not sure this should be a standard part of STON for all dialects.

svenvc commented 10 years ago

OK, I have included it anyway

https://github.com/svenvc/ston/commit/8815dff81b7d4110d095f0e7cb20acae5d4a4ada

tinchodias commented 10 years ago

Thanks, Sven. I try to avoid serialization of instances of Text by converting them to String, but it's good to count with the support.