Pharo issue 13063

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Pharo issue 13063

Stephan Eggermont-3
Can someone review my fix?

https://pharo.fogbugz.com/default.asp?13063&

Stephan
_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pharo issue 13063

Johan Brichau-2
Stupid question maybe, but how can we see the changes?

Johan

> On 21 Mar 2014, at 11:44, Stephan Eggermont <[hidden email]> wrote:
>
> Can someone review my fix?
>
> https://pharo.fogbugz.com/default.asp?13063&
>
> Stephan
> _______________________________________________
> seaside-dev mailing list
> [hidden email]
> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pharo issue 13063

Stephan Eggermont-3
In reply to this post by Stephan Eggermont-3
Johan wrote:
>Stupid question maybe, but how can we see the changes?

Not so stupid, it is a platform specific fix in a somewhat seaside
related part.

The slice is in the Pharo30 inbox.

So, preferably:
- download a latest vm from ci
- download a latest 30  image (with seaside loaded, or load seaside in a plain image)
- open monticello
- open the pharo30 inbox repository
- type 13063 in the package filter
- select merge to see the change
- merge it, and try

Stephan _______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pharo issue 13063

Johan Brichau-2
Ok, your fix seems correct but...

wtf? why is there an explicit check for WARenderCanvas necessary in the NEC package ?

That has nothing to do with your fix, of course.
Unless there is a better way to specialize this implementation and include the code in the Pharo-Development package of Seaside?

Johan

On 21 Mar 2014, at 13:56, Stephan Eggermont <[hidden email]> wrote:

> Johan wrote:
>> Stupid question maybe, but how can we see the changes?
>
> Not so stupid, it is a platform specific fix in a somewhat seaside
> related part.
>
> The slice is in the Pharo30 inbox.
>
> So, preferably:
> - download a latest vm from ci
> - download a latest 30  image (with seaside loaded, or load seaside in a plain image)
> - open monticello
> - open the pharo30 inbox repository
> - type 13063 in the package filter
> - select merge to see the change
> - merge it, and try
>
> Stephan _______________________________________________
> seaside-dev mailing list
> [hidden email]
> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev

_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pharo issue 13063

Johan Brichau-2
Hi Stephan,

I don't know if now is the time for it, but we might want to take the opportunity to improve the code of NEC on this.

I have the impression the check for the presence of WAHtmlCanvas|WARenderCanvas should actually occur in NECVarTypeGuesser>>getClassFromTypeSuggestingName:
Next, frameworks (like Seaside) would need a place to plugin such type-guessing improvements. For that we could add a dictionary to NECVarTypeGuesser where frameworks can register a 'varname->class' mapping.

Afaik, a small change that is worthwhile for future code changes and a chance for frameworks to improve the code completion (I guess this is used for code completion? )

But, as I said, we can also take this to Pharo4 if necessary...

thanks for catching the bug ;-)
Johan

On 21 Mar 2014, at 20:54, Johan Brichau <[hidden email]> wrote:

> Ok, your fix seems correct but...
>
> wtf? why is there an explicit check for WARenderCanvas necessary in the NEC package ?
>
> That has nothing to do with your fix, of course.
> Unless there is a better way to specialize this implementation and include the code in the Pharo-Development package of Seaside?
>
> Johan
>
> On 21 Mar 2014, at 13:56, Stephan Eggermont <[hidden email]> wrote:
>
>> Johan wrote:
>>> Stupid question maybe, but how can we see the changes?
>>
>> Not so stupid, it is a platform specific fix in a somewhat seaside
>> related part.
>>
>> The slice is in the Pharo30 inbox.
>>
>> So, preferably:
>> - download a latest vm from ci
>> - download a latest 30  image (with seaside loaded, or load seaside in a plain image)
>> - open monticello
>> - open the pharo30 inbox repository
>> - type 13063 in the package filter
>> - select merge to see the change
>> - merge it, and try
>>
>> Stephan _______________________________________________
>> seaside-dev mailing list
>> [hidden email]
>> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
>

_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pharo issue 13063

Stephan Eggermont-3
In reply to this post by Stephan Eggermont-3
Johan wrote:
>I don't know if now is the time for it, but we might want to take the opportunity to improve the code of NEC on this.

Good idea, and I think this is probably the patch that should be in Pharo 3.
I hadn’t looked deeper into the problem, just eliminated class references.

>I have the impression the check for the presence of WAHtmlCanvas|WARenderCanvas should actually occur in >NECVarTypeGuesser>>getClassFromTypeSuggestingName:

Yes, I agree. And taking a look at some Seaside code, canvas seems to be an alias for html too.

>Next, frameworks (like Seaside) would need a place to plugin such type-guessing improvements. For that we could add a >dictionary to NECVarTypeGuesser where frameworks can register a 'varname->class' mapping.

That would allow simple mappings from html and canvas to WAHtmlCanvas, yes. Would something more
complex be needed to guess based on the receiver?

>Afaik, a small change that is worthwhile for future code changes and a chance for frameworks to improve the code >completion (I guess this is used for code completion? )

I guess so too...

>But, as I said, we can also take this to Pharo4 if necessary…

AFAIK it’s code most recently handled by Esteban and Marcus. They are on the critical path
to releasing Pharo 3, and this is a design improvement that needs a bit documentation.
On the other hand it has a decent set of tests, so we can just resolve 13063 and open a new
issue to do the design improvement.

>thanks for catching the bug ;-)

Yeah, nice one.

Stephan


_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pharo issue 13063

philippeback
I continued working on the plugins and extension menus

I noticed that there were no NautiliusIcons anymore, but Smalltalk ui icons.

All of these  plugin and extensions mechanisms are quite powerful :-)

Phil



On Fri, Mar 21, 2014 at 10:59 PM, Stephan Eggermont <[hidden email]> wrote:
Johan wrote:
>I don't know if now is the time for it, but we might want to take the opportunity to improve the code of NEC on this.

Good idea, and I think this is probably the patch that should be in Pharo 3.
I hadn’t looked deeper into the problem, just eliminated class references.

>I have the impression the check for the presence of WAHtmlCanvas|WARenderCanvas should actually occur in >NECVarTypeGuesser>>getClassFromTypeSuggestingName:

Yes, I agree. And taking a look at some Seaside code, canvas seems to be an alias for html too.

>Next, frameworks (like Seaside) would need a place to plugin such type-guessing improvements. For that we could add a >dictionary to NECVarTypeGuesser where frameworks can register a 'varname->class' mapping.

That would allow simple mappings from html and canvas to WAHtmlCanvas, yes. Would something more
complex be needed to guess based on the receiver?

>Afaik, a small change that is worthwhile for future code changes and a chance for frameworks to improve the code >completion (I guess this is used for code completion? )

I guess so too...

>But, as I said, we can also take this to Pharo4 if necessary…

AFAIK it’s code most recently handled by Esteban and Marcus. They are on the critical path
to releasing Pharo 3, and this is a design improvement that needs a bit documentation.
On the other hand it has a decent set of tests, so we can just resolve 13063 and open a new
issue to do the design improvement.

>thanks for catching the bug ;-)

Yeah, nice one.

Stephan


_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev


_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev