Smalltalk › Pharo › Pharo Smalltalk Developers

Issue 3247 breaks external packages

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

9 messages Options

Miguel Cobá

Issue 3247 breaks external packages

I am trying to run Magma and uses TextConstants at: idiom.

Before 3247 TextConstants was a Dictionary and the at: message works ok.
But now that it is a SharedPool there is no way to use the external code
without modify it before it works in Pharo 1.2. This change doesn't even
tries to present the same interface to acces the Text constants as
before. The TextConstants>>at: message in Pharo 1.2 is the Object>>at:
method that obviously doesn't work.

What is the reason of this change? Why is better than the dictionary (I
suppose that reifying it is a reason)?

What is the intended usage for code that uses TextConstants the
dictionary access way?
Specifically, code like this doesn't work anymore in Pharo1.2

self ~~ (TextConstants at: #DefaultTabsArray)
and: [ self ~~ (TextConstants at: #DefaultMarginTabsArray) ]

What should I do, I can't see a reopen status in the issue tracker.
Should I open a new ticket?

Cheers
--
Miguel Cobá
http://twitter.com/MiguelCobaMtz
http://miguel.leugim.com.mx

Stéphane Ducasse

Re: Issue 3247 breaks external packages

On Mar 15, 2011, at 5:58 AM, Miguel Cobá wrote:

> I am trying to run Magma and uses TextConstants at: idiom.
>
> Before 3247 TextConstants was a Dictionary and the at: message works ok.
> But now that it is a SharedPool there is no way to use the external code
> without modify it before it works in Pharo 1.2. This change doesn't even
> tries to present the same interface to acces the Text constants as
> before. The TextConstants>>at: message in Pharo 1.2 is the Object>>at:
> method that obviously doesn't work.

How could we do that?
If the system badly uses a global variable in all sort of different ways then
there is no way that we can propose the same interface.

Then TextConstants had different and bad use:
- storing constants
- storing volatile information about fonts and all sorts of registrations for fonts!!! in textConstants
"oh yes this is the dictionary so lets us mess there"
instead of using adequate classVariables in adequate classes (we should still clean this part).
TextSharedInformation below is for the moment containing all the crap.

> What is the reason of this change? Why is better than the dictionary (I
> suppose that reifying it is a reason)?

because TextConstants was the **last** global pool and it was like a jellyfish.
In fact when SharedPool were introduced by andreas (it was a good idea BTW)
TextConstants was left because it was touching a lot of code.

If we want to be able to get a bootstrappable core (and eliot and us want that)
then we should clean the mess.
Yes all the sharedPool are subclasses of sharedPool except TextConstants.... imagine the code to manage
that. Is it a class, a traits, a global is it a dictionary of may be this is textConstants.....

> What is the intended usage for code that uses TextConstants the
> dictionary access way?
> Specifically, code like this doesn't work anymore in Pharo1.2
>
> self ~~ (TextConstants at: #DefaultTabsArray)
> and: [ self ~~ (TextConstants at: #DefaultMarginTabsArray) ]

Using accessors to control access to defaultTabsArrat and DefaultMargin.... is the first steps.
Like that migration is easier.

Now I do not really understand why a database has to change that kind of text information.

>
> What should I do, I can't see a reopen status in the issue tracker.
> Should I open a new ticket?

Have a look in the SharedPool there is a classVariable

SharedPool subclass: #TextConstants
instanceVariableNames: ''
classVariableNames: 'BS BS2 Basal Bold CR Centered Clear CrossedX CtrlA CtrlB CtrlC CtrlD CtrlDigits CtrlE CtrlF CtrlG CtrlH CtrlI CtrlJ CtrlK CtrlL CtrlM CtrlN CtrlO CtrlOpenBrackets CtrlP CtrlQ CtrlR CtrlS CtrlT CtrlU CtrlV CtrlW CtrlX CtrlY CtrlZ Ctrla Ctrlb Ctrlc Ctrld Ctrle Ctrlf Ctrlg Ctrlh Ctrli Ctrlj Ctrlk Ctrll Ctrlm Ctrln Ctrlo Ctrlp Ctrlq Ctrlr Ctrls Ctrlt Ctrlu Ctrlv Ctrlw Ctrlx Ctrly Ctrlz DefaultBaseline DefaultFontFamilySize DefaultLineGrid DefaultMarginTabsArray DefaultMask DefaultRule DefaultSpace DefaultTab DefaultTabsArray ESC EndOfRun Enter Italic Justified LeftFlush LeftMarginTab RightFlush RightMarginTab Space Tab TextSharedInformation'
poolDictionaries: ''
category: 'Collections-Text'

Stef

>
> Cheers
> --
> Miguel Cobá
> http://twitter.com/MiguelCobaMtz
> http://miguel.leugim.com.mx
>
>
>
>

Miguel Cobá

Re: Issue 3247 breaks external packages

El mar, 15-03-2011 a las 09:07 +0100, Stéphane Ducasse escribió:

> On Mar 15, 2011, at 5:58 AM, Miguel Cobá wrote:
>
> > I am trying to run Magma and uses TextConstants at: idiom.
> >
> > Before 3247 TextConstants was a Dictionary and the at: message works ok.
> > But now that it is a SharedPool there is no way to use the external code
> > without modify it before it works in Pharo 1.2. This change doesn't even
> > tries to present the same interface to acces the Text constants as
> > before. The TextConstants>>at: message in Pharo 1.2 is the Object>>at:
> > method that obviously doesn't work.
>
> How could we do that?
> If the system badly uses a global variable in all sort of different ways then
> there is no way that we can propose the same interface.
>

Ok, I understand that, but also it was known that it would break code.
This is also understandable. and a price to pay to have this system
going forward. My point is the change was made, and no care for the
users was made in either:

- stating somewhere (maybe the list, a wiki, a faq) the consequences of
this change and the advantages over the old code.
- a way to migrate old code to the new code.

This, at least for someone that has never used shared pools, isn't
obvious. The issue tracker says something about importing the shared
pool in the class using it so it can access its values. But there are
package extensions that add methods to other classes that use
TextConstants, how is one to work out this case, adding TextConstant
sharedpool to the class (array in the case of magma that is augmented by
a couple of methods that uses TextConstants)

> Then TextConstants had different and bad use:
> - storing constants
> - storing volatile information about fonts and all sorts of registrations for fonts!!! in textConstants
> "oh yes this is the dictionary so lets us mess there"
> instead of using adequate classVariables in adequate classes (we should still clean this part).
> TextSharedInformation below is for the moment containing all the crap.
>

I understand.

> > What is the reason of this change? Why is better than the dictionary (I
> > suppose that reifying it is a reason)?
>
>
> because TextConstants was the **last** global pool and it was like a jellyfish.
> In fact when SharedPool were introduced by andreas (it was a good idea BTW)
> TextConstants was left because it was touching a lot of code.
>

Exactly, and this change let a lot of code in the cold without warning
or a migration path.

> If we want to be able to get a bootstrappable core (and eliot and us want that)
> then we should clean the mess.
> Yes all the sharedPool are subclasses of sharedPool except TextConstants.... imagine the code to manage
> that. Is it a class, a traits, a global is it a dictionary of may be this is textConstants.....
>
> > What is the intended usage for code that uses TextConstants the
> > dictionary access way?
> > Specifically, code like this doesn't work anymore in Pharo1.2
> >
> > self ~~ (TextConstants at: #DefaultTabsArray)
> > and: [ self ~~ (TextConstants at: #DefaultMarginTabsArray) ]
>
>
> Using accessors to control access to defaultTabsArrat and DefaultMargin.... is the first steps.
> Like that migration is easier.

Yes, I now, but this means that all the code using the at: message in
TextConstants is broken in the new Pharo1.2. Maybe this hasn't been
noted (at least for me it hasn't) because PharoCore 1.2 wasn't stable
but now that is stable people will try to load code that uses
TextConstants and if uses the at: message it won't work.

Don't get me wrong. I am for the progress of Pharo and this change is in
that direction but it appears to me that was without notice and without
a deprecation warning akin to the method deprecation. This was released
to the world without time for package to catch up and prepare either a
patch to the code or a PharoCompatibility package that copes with usage
of TextConstants.

>
> Now I do not really understand why a database has to change that kind of text information.
>

I don't know either because I'm not expert on magma but that is besides
the point.

Now what I will do is to add (after consulting with Chris) a Pharo
specific package to magma, or if that isn't possible, an add-on in
PharoTaskForces that is loaded after magma packages and overwrites the
methods using TextConstants with the Pharo specific shared pool
implementation. This I suppose will dirty the Magma package and I don't
like it so much.

So, dear Magma on Pharo users, the ConfigurationOfMagma isn't usable
yet. Please wait a couple of days more. :/

Cheers
--
Miguel Cobá
http://twitter.com/MiguelCobaMtz
http://miguel.leugim.com.mx

Henrik Sperre Johansen

Re: Issue 3247 breaks external packages

On 15.03.2011 17:39, Miguel Cobá wrote:

> El mar, 15-03-2011 a las 09:07 +0100, Stéphane Ducasse escribió:
>> On Mar 15, 2011, at 5:58 AM, Miguel Cobá wrote:
>>
>>> I am trying to run Magma and uses TextConstants at: idiom.
>>>
>>> Before 3247 TextConstants was a Dictionary and the at: message works ok.
>>> But now that it is a SharedPool there is no way to use the external code
>>> without modify it before it works in Pharo 1.2. This change doesn't even
>>> tries to present the same interface to acces the Text constants as
>>> before. The TextConstants>>at: message in Pharo 1.2 is the Object>>at:
>>> method that obviously doesn't work.
>> How could we do that?
>> If the system badly uses a global variable in all sort of different ways then
>> there is no way that we can propose the same interface.
>>
> Ok, I understand that, but also it was known that it would break code.
> This is also understandable. and a price to pay to have this system
> going forward. My point is the change was made, and no care for the
> users was made in either:
>
> - stating somewhere (maybe the list, a wiki, a faq) the consequences of
> this change and the advantages over the old code.
> - a way to migrate old code to the new code.
>
> This, at least for someone that has never used shared pools, isn't
> obvious. The issue tracker says something about importing the shared
> pool in the class using it so it can access its values. But there are
> package extensions that add methods to other classes that use
> TextConstants, how is one to work out this case, adding TextConstant
> sharedpool to the class (array in the case of magma that is augmented by
> a couple of methods that uses TextConstants)
>
>> Then TextConstants had different and bad use:
>> - storing constants
>> - storing volatile information about fonts and all sorts of registrations for fonts!!! in textConstants
>> "oh yes this is the dictionary so lets us mess there"
>> instead of using adequate classVariables in adequate classes (we should still clean this part).
>> TextSharedInformation below is for the moment containing all the crap.
>>
> I understand.
>
>>> What is the reason of this change? Why is better than the dictionary (I
>>> suppose that reifying it is a reason)?
>>
>> because TextConstants was the **last** global pool and it was like a jellyfish.
>> In fact when SharedPool were introduced by andreas (it was a good idea BTW)
>> TextConstants was left because it was touching a lot of code.
>>
> Exactly, and this change let a lot of code in the cold without warning
> or a migration path.
>
>> If we want to be able to get a bootstrappable core (and eliot and us want that)
>> then we should clean the mess.
>> Yes all the sharedPool are subclasses of sharedPool except TextConstants.... imagine the code to manage
>> that. Is it a class, a traits, a global is it a dictionary of may be this is textConstants.....
>>
>>> What is the intended usage for code that uses TextConstants the
>>> dictionary access way?
>>> Specifically, code like this doesn't work anymore in Pharo1.2
>>>
>>> self ~~ (TextConstants at: #DefaultTabsArray)
>>> and: [ self ~~ (TextConstants at: #DefaultMarginTabsArray) ]
>>
>> Using accessors to control access to defaultTabsArrat and DefaultMargin.... is the first steps.
>> Like that migration is easier.
> Yes, I now, but this means that all the code using the at: message in
> TextConstants is broken in the new Pharo1.2. Maybe this hasn't been
> noted (at least for me it hasn't) because PharoCore 1.2 wasn't stable
> but now that is stable people will try to load code that uses
> TextConstants and if uses the at: message it won't work.
>
> Don't get me wrong. I am for the progress of Pharo and this change is in
> that direction but it appears to me that was without notice and without
> a deprecation warning akin to the method deprecation. This was released
> to the world without time for package to catch up and prepare either a
> patch to the code or a PharoCompatibility package that copes with usage
> of TextConstants.
>
>
>> Now I do not really understand why a database has to change that kind of text information.
>>
> I don't know either because I'm not expert on magma but that is besides
> the point.
>
> Now what I will do is to add (after consulting with Chris) a Pharo
> specific package to magma, or if that isn't possible, an add-on in
> PharoTaskForces that is loaded after magma packages and overwrites the
> methods using TextConstants with the Pharo specific shared pool
> implementation. This I suppose will dirty the Magma package and I don't
> like it so much.
>
> So, dear Magma on Pharo users, the ConfigurationOfMagma isn't usable
> yet. Please wait a couple of days more. :/
>
>
> Cheers

How about:
<category: 'backwards compatability'
at: aSymbol
^self classPool at: aSymbol

at: aSymbol put: aValue
^self
deprecated: 'TextConstants is not a place to store your stuff'
on: '15 March 2011'
in: 'Pharo1.2'

Cheers,
Henry

Miguel Cobá

Re: Issue 3247 breaks external packages

Thanks Henrik, it is working great. Magma tests are running right now.

Now, what should we do. I have the changeset with this changes. Should I
open other ticket, referencing 3247, attach the CS so that is part of
PharoCore 1.2.1 or should we wait to see if someone else hits this
issue?

So, part of PharoCore 1.2.1 or part of ConfigurationOfMagma
dependencies?

I vote for part of 1.2.1, Stef, what do you think?

Again, thanks Henrik, for the solution.

Cheers

El mar, 15-03-2011 a las 18:00 +0100, Henrik Sperre Johansen escribió:

> On 15.03.2011 17:39, Miguel Cobá wrote:
> > El mar, 15-03-2011 a las 09:07 +0100, Stéphane Ducasse escribió:
> >> On Mar 15, 2011, at 5:58 AM, Miguel Cobá wrote:
> >>
> >>> I am trying to run Magma and uses TextConstants at: idiom.
> >>>
> >>> Before 3247 TextConstants was a Dictionary and the at: message works ok.
> >>> But now that it is a SharedPool there is no way to use the external code
> >>> without modify it before it works in Pharo 1.2. This change doesn't even
> >>> tries to present the same interface to acces the Text constants as
> >>> before. The TextConstants>>at: message in Pharo 1.2 is the Object>>at:
> >>> method that obviously doesn't work.
> >> How could we do that?
> >> If the system badly uses a global variable in all sort of different ways then
> >> there is no way that we can propose the same interface.
> >>
> > Ok, I understand that, but also it was known that it would break code.
> > This is also understandable. and a price to pay to have this system
> > going forward. My point is the change was made, and no care for the
> > users was made in either:
> >
> > - stating somewhere (maybe the list, a wiki, a faq) the consequences of
> > this change and the advantages over the old code.
> > - a way to migrate old code to the new code.
> >
> > This, at least for someone that has never used shared pools, isn't
> > obvious. The issue tracker says something about importing the shared
> > pool in the class using it so it can access its values. But there are
> > package extensions that add methods to other classes that use
> > TextConstants, how is one to work out this case, adding TextConstant
> > sharedpool to the class (array in the case of magma that is augmented by
> > a couple of methods that uses TextConstants)
> >
> >> Then TextConstants had different and bad use:
> >> - storing constants
> >> - storing volatile information about fonts and all sorts of registrations for fonts!!! in textConstants
> >> "oh yes this is the dictionary so lets us mess there"
> >> instead of using adequate classVariables in adequate classes (we should still clean this part).
> >> TextSharedInformation below is for the moment containing all the crap.
> >>
> > I understand.
> >
> >>> What is the reason of this change? Why is better than the dictionary (I
> >>> suppose that reifying it is a reason)?
> >>
> >> because TextConstants was the **last** global pool and it was like a jellyfish.
> >> In fact when SharedPool were introduced by andreas (it was a good idea BTW)
> >> TextConstants was left because it was touching a lot of code.
> >>
> > Exactly, and this change let a lot of code in the cold without warning
> > or a migration path.
> >
> >> If we want to be able to get a bootstrappable core (and eliot and us want that)
> >> then we should clean the mess.
> >> Yes all the sharedPool are subclasses of sharedPool except TextConstants.... imagine the code to manage
> >> that. Is it a class, a traits, a global is it a dictionary of may be this is textConstants.....
> >>
> >>> What is the intended usage for code that uses TextConstants the
> >>> dictionary access way?
> >>> Specifically, code like this doesn't work anymore in Pharo1.2
> >>>
> >>> self ~~ (TextConstants at: #DefaultTabsArray)
> >>> and: [ self ~~ (TextConstants at: #DefaultMarginTabsArray) ]
> >>
> >> Using accessors to control access to defaultTabsArrat and DefaultMargin.... is the first steps.
> >> Like that migration is easier.
> > Yes, I now, but this means that all the code using the at: message in
> > TextConstants is broken in the new Pharo1.2. Maybe this hasn't been
> > noted (at least for me it hasn't) because PharoCore 1.2 wasn't stable
> > but now that is stable people will try to load code that uses
> > TextConstants and if uses the at: message it won't work.
> >
> > Don't get me wrong. I am for the progress of Pharo and this change is in
> > that direction but it appears to me that was without notice and without
> > a deprecation warning akin to the method deprecation. This was released
> > to the world without time for package to catch up and prepare either a
> > patch to the code or a PharoCompatibility package that copes with usage
> > of TextConstants.
> >
> >
> >> Now I do not really understand why a database has to change that kind of text information.
> >>
> > I don't know either because I'm not expert on magma but that is besides
> > the point.
> >
> > Now what I will do is to add (after consulting with Chris) a Pharo
> > specific package to magma, or if that isn't possible, an add-on in
> > PharoTaskForces that is loaded after magma packages and overwrites the
> > methods using TextConstants with the Pharo specific shared pool
> > implementation. This I suppose will dirty the Magma package and I don't
> > like it so much.
> >
> > So, dear Magma on Pharo users, the ConfigurationOfMagma isn't usable
> > yet. Please wait a couple of days more. :/
> >
> >
> > Cheers
> How about:
> <category: 'backwards compatability'
> at: aSymbol
> ^self classPool at: aSymbol
>
> at: aSymbol put: aValue
> ^self
> deprecated: 'TextConstants is not a place to store your stuff'
> on: '15 March 2011'
> in: 'Pharo1.2'
>
> Cheers,
> Henry
>

--
Miguel Cobá
http://twitter.com/MiguelCobaMtz
http://miguel.leugim.com.mx

Stéphane Ducasse

Re: Issue 3247 breaks external packages

On Mar 15, 2011, at 6:30 PM, Miguel Cobá wrote:

> Thanks Henrik, it is working great. Magma tests are running right now.
>
> Now, what should we do. I have the changeset with this changes. Should I
> open other ticket, referencing 3247, attach the CS so that is part of
> PharoCore 1.2.1 or should we wait to see if someone else hits this
> issue?

open an issue and tage it for 1.2.1 and 1.3

We did not think that the solution was that easy else we would have done it.

>>>>>
>> How about:
>> <category: 'backwards compatability'
>> at: aSymbol
>> ^self classPool at: aSymbol
>>
>> at: aSymbol put: aValue
>> ^self
>> deprecated: 'TextConstants is not a place to store your stuff'
>> on: '15 March 2011'
>> in: 'Pharo1.2'
>>
>> Cheers,
>> Henry
>>
>
> --
> Miguel Cobá
> http://twitter.com/MiguelCobaMtz
> http://miguel.leugim.com.mx
>
>
>
>

Stéphane Ducasse

Re: Issue 3247 breaks external packages

In reply to this post by Miguel Cobá

On Mar 15, 2011, at 5:39 PM, Miguel Cobá wrote:

We are trying but it can take a lot of time to explain all the cruft that we face.

> - a way to migrate old code to the new code.
>
> This, at least for someone that has never used shared pools, isn't
> obvious. The issue tracker says something about importing the shared
> pool in the class using it so it can access its values. But there are
> package extensions that add methods to other classes that use
> TextConstants, how is one to work out this case, adding TextConstant
> sharedpool to the class (array in the case of magma that is augmented by
> a couple of methods that uses TextConstants)
>
>> Then TextConstants had different and bad use:
>> - storing constants
>> - storing volatile information about fonts and all sorts of registrations for fonts!!! in textConstants
>> "oh yes this is the dictionary so lets us mess there"
>> instead of using adequate classVariables in adequate classes (we should still clean this part).
>> TextSharedInformation below is for the moment containing all the crap.
>>
>
> I understand.
>
>>> What is the reason of this change? Why is better than the dictionary (I
>>> suppose that reifying it is a reason)?
>>
>>
>> because TextConstants was the **last** global pool and it was like a jellyfish.
>> In fact when SharedPool were introduced by andreas (it was a good idea BTW)
>> TextConstants was left because it was touching a lot of code.
>>
>
> Exactly, and this change let a lot of code in the cold without warning
> or a migration path.

So far this is the only impact with the problem reported by doru for fonts.

> Yes, I now, but this means that all the code using the at: message in
> TextConstants is broken in the new Pharo1.2.

I do not get the previous statements. All the system got recompiled and fixed.

> Maybe this hasn't been
> noted (at least for me it hasn't) because PharoCore 1.2 wasn't stable
> but now that is stable people will try to load code that uses
> TextConstants and if uses the at: message it won't work.
>
> Don't get me wrong. I am for the progress of Pharo and this change is in
> that direction but it appears to me that was without notice

this was not without notice. it was mentioned in the log (probably not a lot but
it took us some times and a lot of try and error before making it public).

> and without
> a deprecation warning akin to the method deprecation. This was released
> to the world without time for package to catch up and prepare either a
> patch to the code or a PharoCompatibility package that copes with usage
> of TextConstants.

There are situation where we cannot simply deprecate code.

>
>
>>
>> Now I do not really understand why a database has to change that kind of text information.
>>
>
> I don't know either because I'm not expert on magma but that is besides
> the point.
>
> Now what I will do is to add (after consulting with Chris) a Pharo
> specific package to magma, or if that isn't possible, an add-on in
> PharoTaskForces that is loaded after magma packages and overwrites the
> methods using TextConstants with the Pharo specific shared pool
> implementation. This I suppose will dirty the Magma package and I don't
> like it so much.
>
> So, dear Magma on Pharo users, the ConfigurationOfMagma isn't usable
> yet. Please wait a couple of days more. :/
>
>
> Cheers
> --
> Miguel Cobá
> http://twitter.com/MiguelCobaMtz
> http://miguel.leugim.com.mx
>
>
>
>

Miguel Cobá

Re: Issue 3247 breaks external packages

In reply to this post by Stéphane Ducasse

El mar, 15-03-2011 a las 18:34 +0100, Stéphane Ducasse escribió:

> On Mar 15, 2011, at 6:30 PM, Miguel Cobá wrote:
>
> > Thanks Henrik, it is working great. Magma tests are running right now.
> >
> > Now, what should we do. I have the changeset with this changes. Should I
> > open other ticket, referencing 3247, attach the CS so that is part of
> > PharoCore 1.2.1 or should we wait to see if someone else hits this
> > issue?
>
> open an issue and tage it for 1.2.1 and 1.3

Done, issue 3820. Includes the fix and is ready to integrate in 1.2.1
and 1.3

Cheers
--
Miguel Cobá
http://twitter.com/MiguelCobaMtz
http://miguel.leugim.com.mx

Stéphane Ducasse

Re: Issue 3247 breaks external packages

great!

On Mar 15, 2011, at 7:24 PM, Miguel Cobá wrote:

> El mar, 15-03-2011 a las 18:34 +0100, Stéphane Ducasse escribió:
>> On Mar 15, 2011, at 6:30 PM, Miguel Cobá wrote:
>>
>>> Thanks Henrik, it is working great. Magma tests are running right now.
>>>
>>> Now, what should we do. I have the changeset with this changes. Should I
>>> open other ticket, referencing 3247, attach the CS so that is part of
>>> PharoCore 1.2.1 or should we wait to see if someone else hits this
>>> issue?
>>
>> open an issue and tage it for 1.2.1 and 1.3
>
> Done, issue 3820. Includes the fix and is ready to integrate in 1.2.1
> and 1.3
>
> Cheers
> --
> Miguel Cobá
> http://twitter.com/MiguelCobaMtz
> http://miguel.leugim.com.mx
>
>
>
>