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

Instances of Color deserialized as instances of Dictionary when being referenced in the STON serialization file #26

Closed jbrichau closed 5 years ago

jbrichau commented 5 years ago

Steps to reproduce the problem:

What happens? The system setting #\'Shortcut Reminder\'#textColor is deserialized as an instance of Dictionary rather than Color. As a result, the morph rendering breaks because it expects an instance of Color to be used as textColor and not an instance of Dictionary.

It seems it only happens because the value is a reference:

    StoredSetting {
        #settingNodeIdentifier : '#\'Shortcut Reminder\'#textColor',
        #realValue : @76
    },

The stored object at index 76 is indeed a Dictionary because that value was stored rather than the result of Color class>>fromSton:. It seems that the Color instance should be stored in the reference dictionary when reading the file. In other words: the result of the invocation of fromSton: should be stored.

jbrichau commented 5 years ago

Confirmed this by reverting to commit f685fb37e98e6ef79e122f57f06ca14d8024915d which is before the introduction of the Color class>fromSton: implementation. In that version, the scenario does not bug.

svenvc commented 5 years ago

Hmm, I do not fully understand, in my 7.0.3 with the latest STON, I get:

(Color r: 0.25 g: 0.5 b: 0.75) in: [ :color | STON toString: { color. color } ].
 "'[Color{#red:0.25,#green:0.5,#blue:0.75,#alpha:1.0},@2]'"
(Color r: 0.25 g: 0.5 b: 0.75) in: [ :color | STON fromString: (STON toString: { color. color }) ].

The second expression works.

AFAIU this works.

Do you see an error in STON or in STON usage ? Maybe the stored preference is incompatible with the loading STON ?

jbrichau commented 5 years ago

Yes, those snippets work. The problem only arises if the instance of Color is 'referenced' in the STON file.

This happens in the settings of Pharo. I do not have smaller reproducible scenario, but by debugging the store/load of the settings of a virgin Pharo image with latest STON, I got to this problem.

Yes, I make sure to remove the old settings file before testing this. I always do a 'store' and then a 'load', after which it goes wrong.

svenvc commented 5 years ago

The snippet is using a reference (the #== object is used twice, second time it is references using @2).

Can you upload the whole STON file ?

jbrichau commented 5 years ago

I will send it to you by email :)

svenvc commented 5 years ago

I can reproduce the error in a clean image.

However, even though STON is used to save the settings, it does so in mysterious ways.

In any case, I can do

(StartupPreferencesLoader preferencesVersionFolder / 'system-settings.ston') asFileReference readStreamDo: [ :in | STON fromStream: in ].

so basically, STON can reload the saved preferences. And as far as I can see the preferences are OK.

So we have to figure out what goes wrong where. I never looked at that code.

svenvc commented 5 years ago

Note: #testStoreAndLoadAllSystemSettings fails as well

jbrichau commented 5 years ago

I executed the code snippet in your previous message. In my understanding, the deserialization is not correct because the value of the setting should be an instance of Color rather than Dictionary.

settings-deserialize-debug

svenvc commented 5 years ago

Yes, I saw that as well, that is indeed wrong.

svenvc commented 5 years ago

I am obviously missing something, but referring to the same color does work:

(Color r: 0.25 g: 0.5 b: 0.75) in: [ :color | STON toString: { StoredSetting new settingNodeIdentifier: #One; realValue: color. StoredSetting new settingNodeIdentifier: #One; realValue: color } ].

And this can be read back:

(Color r: 0.25 g: 0.5 b: 0.75) in: [ :color | STON fromString: (STON toString: { StoredSetting new settingNodeIdentifier: #One; realValue: color. StoredSetting new settingNodeIdentifier: #One; realValue: color }) ].

I can't see where the Dictionary comes from ...

jbrichau commented 5 years ago

The dictionary is stored in the references index of the reader because the implementation of Color class>>fromSton: invokes parseSimpleValue, which calls parseMap who stores the dictionary...

Screenshot 2019-06-05 at 17 07 39

You can get the same debugger when putting the following code in STONReader>>storeReference: and loading the system-settings.ston I sent you.

...
    index = 76 ifTrue:[ self halt] .
...
svenvc commented 5 years ago

I used a Conditional Breakpoint to arrive at the same place.

But still, higher up the stack at #parseObject you can see that reference 75 will be set to the Color object itself.

I am still looking for a simpler, more isolated example separate from the settings.

jbrichau commented 5 years ago

Here is one:

| color1 color2 |
color1 := (Color r: 0.25 g: 0.5 b: 0.75 alpha: 10).
color2 := (Color red).
STON fromString: (STON toString: { StoredSetting new settingNodeIdentifier: #One; realValue: color1. StoredSetting new settingNodeIdentifier: #Two; realValue: color2 . StoredSetting new settingNodeIdentifier: #Three; realValue: color2. }).

You will see that the realValue of the setting #Three is the object of setting #Two.

jbrichau commented 5 years ago

More stripped down:

| color1 color2 |
color1 := (Color r: 0.25 g: 0.5 b: 0.75 alpha: 10).
color2 := (Color red).
STON fromString: (STON toString: { color1. color2 . color2 }).
svenvc commented 5 years ago

Super, I can work with that. Thank you!

svenvc commented 5 years ago

https://github.com/svenvc/ston/commit/3df1ea0a1dc49492e92727144f8ab3f1c56dd9d8 should fix the problem

jbrichau commented 5 years ago

I can confirm this is fixed. Thanks for the swift fix!