[Pharo-project] Need code review
Hi guys I would like some other people to have a look at [Pharo-project] Hunting down nebraska [Pharo-project] MVC removal step2 investigations Can somebody have a look? Stef
Sorry to weak up so lately, but where the source code is accessible ? I spent few minutes on looking at: - PharoInBox - list of taks on gforge - list of issues on google and I haven't found where the code to review could be... Any hint? Alexandre On 13 Jun 2008, at 11:18, Stéphane Ducasse wrote:
Hi guys
I would like some other people to have a look at
[Pharo-project] Hunting down nebraska [Pharo-project] MVC removal step2 investigations
Can somebody have a look?
Stef
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
-- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
Stef wrote down subject lines from postings to this list http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000238.html http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000323.html Norbert On Fri, 2008-06-13 at 11:37 +0200, Alexandre Bergel wrote:
Sorry to weak up so lately, but where the source code is accessible ? I spent few minutes on looking at: - PharoInBox - list of taks on gforge - list of issues on google
and I haven't found where the code to review could be... Any hint?
Alexandre
On 13 Jun 2008, at 11:18, Stéphane Ducasse wrote:
Hi guys
I would like some other people to have a look at
[Pharo-project] Hunting down nebraska [Pharo-project] MVC removal step2 investigations
Can somebody have a look?
Stef
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000238.html
[Pharo-project] Hunting down nebraska
This change makes sense to me. Essentially reorganization. Alexandre -- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
ok I will harvest it. Stef On Jun 14, 2008, at 12:18 PM, Alexandre Bergel wrote:
http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000238.html
[Pharo-project] Hunting down nebraska
This change makes sense to me. Essentially reorganization.
Alexandre -- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
I am now working on the second change. It is a big one... Alexandre On 14 Jun 2008, at 12:27, Stéphane Ducasse wrote:
ok I will harvest it.
Stef
On Jun 14, 2008, at 12:18 PM, Alexandre Bergel wrote:
http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000238.html
[Pharo-project] Hunting down nebraska
This change makes sense to me. Essentially reorganization.
Alexandre -- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
-- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
yes if you could cut it into smaller ones it would be better. Stef On Jun 14, 2008, at 12:31 PM, Alexandre Bergel wrote:
I am now working on the second change. It is a big one...
Alexandre
On 14 Jun 2008, at 12:27, Stéphane Ducasse wrote:
ok I will harvest it.
Stef
On Jun 14, 2008, at 12:18 PM, Alexandre Bergel wrote:
http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000238.html
[Pharo-project] Hunting down nebraska
This change makes sense to me. Essentially reorganization.
Alexandre -- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
-- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000323.html
[Pharo-project] MVC removal step2 investigations
This is a big removal. I reviewed the code. Here are my comments: - Few methods in Browser are redefined using the hypothesis that a browser cannot be open in an MVC project. Methods that refer to PluggableButtonView are also removed. This make sense since this class can be considered as obsolete: a PluggableButtonView cannot be open anymore. - ChangeList: optionalButtonsView is removed. This makes sense since it opens MVC button (PluggableButtonView). This method is called nowhere - ChangeSorter>>openView: topView offsetBy: offset is removed. This method is called nowhere, and refers to MVC code Few questions: - Inspector>>openOn: anObject withEvalPane: withEval withLabel: label valueViewClass: valueViewClass seems to be called by FormInspectView class>>openOn: aFormDictionary withLabel: aLabel Don't you think that removing this method might be a problem? - Why have you removed PasteUpMorph>>open ? TestRunner me dit: - Avant le removal: 2117 run, 2108 passes, 2 expected failures, 7 failures, 0 errors, 0 unexpected passes - Après le removal: 2068 run, 2059 passes, 2 expected failures, 7 failures, 0 errors, 0 unexpected passes Je n'ai vu aucun problème après avoir chargé le removal. I favour an inclusion. Cheers, Alexandre -- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
Hi Alexandre,
thanks for this review.
On Monday 16 June 2008 09:34:26 Alexandre Bergel wrote:
> > http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000323.html
> >>> [Pharo-project] MVC removal step2 investigations
...
> Few questions:
> - Inspector>>openOn: anObject withEvalPane: withEval withLabel: label
> valueViewClass: valueViewClass seems to be called by FormInspectView
> class>>openOn: aFormDictionary withLabel: aLabel
> Don't you think that removing this method might be a problem?
FormInspectView class is removed
> - Why have you removed PasteUpMorph>>open ?
Here is it's code:
PasteUpMorph>>open
"Open a view on this WorldMorph."
MorphWorldView openOn: self.
MorphWorldView is a subclass of View (MVC) which is removed.
So PasteUpMorph>>open makes sense only in MVC context.
>
> Je n'ai vu aucun problème après avoir chargé le removal.
> I favour an inclusion.
I would only insist on the following point:
One big change concerns the Controller hierarchy which was
Controller #('model' 'view' 'sensor' 'lastActivityTime')
MouseMenuController #('redButtonMenu' 'redButtonMessages')
ScrollController #('scrollBar' 'marker' 'savedArea' 'menuBar' 'savedMenuBarArea')
ParagraphEditor #('paragraph' 'startBlock' 'stopBlock' 'beginTypeInBlock' '
and which becomes:
Controller #('model' 'sensor' 'lastActivityTime')
ParagraphEditor #('paragraph' 'startBlock' 'stopBlock' 'beginTypeInBlock' 'emphasisHere' 'initialText' 'selectionShowing' 'otherInterval' 'lastParentLocation')
as you can see, MouseMenuController and ScrollController are removed and ParagraphEditor is now a direct subclass of Controller.
Controller is now more simple because of 'view' instance variable removeal.
I think that 'lastActivityTime' can be removed too and I've also noticed more cleaning possibilities implied by this changeset.
Especially in class ScreenController which holds a lot of menu methods which are not used anymore
(world menu equivalent methods but in the context of MVC).
I will provide a changeset or a slice later.
alain
>
> Cheers,
> Alexandre
>
Ah ok..
I see now...
As I said, your removal makes fully sense to me...
Alexandre
On 16 Jun 2008, at 14:13, Alain Plantec wrote:
> Hi Alexandre,
> thanks for this review.
>
> On Monday 16 June 2008 09:34:26 Alexandre Bergel wrote:
>>> http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000323.html
>>>>> [Pharo-project] MVC removal step2 investigations
> ...
>> Few questions:
>> - Inspector>>openOn: anObject withEvalPane: withEval withLabel:
>> label
>> valueViewClass: valueViewClass seems to be called by FormInspectView
>> class>>openOn: aFormDictionary withLabel: aLabel
>> Don't you think that removing this method might be a problem?
> FormInspectView class is removed
>> - Why have you removed PasteUpMorph>>open ?
> Here is it's code:
> PasteUpMorph>>open
> "Open a view on this WorldMorph."
> MorphWorldView openOn: self.
> MorphWorldView is a subclass of View (MVC) which is removed.
> So PasteUpMorph>>open makes sense only in MVC context.
>>
>> Je n'ai vu aucun problème après avoir chargé le removal.
>> I favour an inclusion.
> I would only insist on the following point:
> One big change concerns the Controller hierarchy which was
> Controller #('model' 'view' 'sensor' 'lastActivityTime')
> MouseMenuController #('redButtonMenu' 'redButtonMessages')
> ScrollController #('scrollBar' 'marker' 'savedArea' 'menuBar'
> 'savedMenuBarArea')
> ParagraphEditor #('paragraph' 'startBlock' 'stopBlock'
> 'beginTypeInBlock' '
> and which becomes:
> Controller #('model' 'sensor' 'lastActivityTime')
> ParagraphEditor #('paragraph' 'startBlock' 'stopBlock'
> 'beginTypeInBlock' 'emphasisHere' 'initialText' 'selectionShowing'
> 'otherInterval' 'lastParentLocation')
>
> as you can see, MouseMenuController and ScrollController are removed
> and ParagraphEditor is now a direct subclass of Controller.
> Controller is now more simple because of 'view' instance variable
> removeal.
> I think that 'lastActivityTime' can be removed too and I've also
> noticed more cleaning possibilities implied by this changeset.
> Especially in class ScreenController which holds a lot of menu
> methods which are not used anymore
> (world menu equivalent methods but in the context of MVC).
> I will provide a changeset or a slice later.
>
> alain
>>
>> Cheers,
>> Alexandre
>>
>
--
_,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
Alexandre Bergel http://www.bergel.eu
^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
Ok I will try to harvest it today.
Stef
On Jun 16, 2008, at 2:23 PM, Alexandre Bergel wrote:
> Ah ok..
> I see now...
>
> As I said, your removal makes fully sense to me...
>
> Alexandre
>
>
> On 16 Jun 2008, at 14:13, Alain Plantec wrote:
>
>> Hi Alexandre,
>> thanks for this review.
>>
>> On Monday 16 June 2008 09:34:26 Alexandre Bergel wrote:
>>>> http://lists.gforge.inria.fr/pipermail/pharo-project/2008-June/000323.html
>>>>>> [Pharo-project] MVC removal step2 investigations
>> ...
>>> Few questions:
>>> - Inspector>>openOn: anObject withEvalPane: withEval withLabel:
>>> label
>>> valueViewClass: valueViewClass seems to be called by FormInspectView
>>> class>>openOn: aFormDictionary withLabel: aLabel
>>> Don't you think that removing this method might be a problem?
>> FormInspectView class is removed
>>> - Why have you removed PasteUpMorph>>open ?
>> Here is it's code:
>> PasteUpMorph>>open
>> "Open a view on this WorldMorph."
>> MorphWorldView openOn: self.
>> MorphWorldView is a subclass of View (MVC) which is removed.
>> So PasteUpMorph>>open makes sense only in MVC context.
>>>
>>> Je n'ai vu aucun problème après avoir chargé le removal.
>>> I favour an inclusion.
>> I would only insist on the following point:
>> One big change concerns the Controller hierarchy which was
>> Controller #('model' 'view' 'sensor' 'lastActivityTime')
>> MouseMenuController #('redButtonMenu' 'redButtonMessages')
>> ScrollController #('scrollBar' 'marker' 'savedArea' 'menuBar'
>> 'savedMenuBarArea')
>> ParagraphEditor #('paragraph' 'startBlock' 'stopBlock'
>> 'beginTypeInBlock' '
>> and which becomes:
>> Controller #('model' 'sensor' 'lastActivityTime')
>> ParagraphEditor #('paragraph' 'startBlock' 'stopBlock'
>> 'beginTypeInBlock' 'emphasisHere' 'initialText' 'selectionShowing'
>> 'otherInterval' 'lastParentLocation')
>>
>> as you can see, MouseMenuController and ScrollController are
>> removed and ParagraphEditor is now a direct subclass of Controller.
>> Controller is now more simple because of 'view' instance variable
>> removeal.
>> I think that 'lastActivityTime' can be removed too and I've also
>> noticed more cleaning possibilities implied by this changeset.
>> Especially in class ScreenController which holds a lot of menu
>> methods which are not used anymore
>> (world menu equivalent methods but in the context of MVC).
>> I will provide a changeset or a slice later.
>>
>> alain
>>>
>>> Cheers,
>>> Alexandre
>>>
>>
>
> --
> _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
> Alexandre Bergel http://www.bergel.eu
> ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
>
>
>
>
>
>
> _______________________________________________
> Pharo-project mailing list
> Pharo-project@lists.gforge.inria.fr
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>
participants (4)
-
Alain Plantec -
Alexandre Bergel -
Norbert Hartl -
Stéphane Ducasse