On Aug 11, 2010, at 11:05 PM, Guillermo Polito wrote:

> Hi!
>
> � I submitted a little fix where the UI is updated and the example is working.

How?
Because the model should not rely on the UI as it was before

> �I expect you don't get upset for touching it.
>
> By the way, I have some extra comments:
>
> -Why there are MessageList and RecentMessageList? �Why not only one of them?

Because you can reuse MessageList in other circumstances

> -Why MethodReferenceWithSource is called like that? �It can contain also Class references... �Otherwise, maybe there is a better name for that class. �Maybe something like RecentChangeNotification or something like that...

This has nothing to do with notification. This is just an extension of MethodReference

> -There is no RecentMessageList example or similar!!!! �I had to browse the code and guess that "RecentMessageList new openInWorld" should open it :).


> - All the asDictionaryByXXX are very similar... �Maybe you can build a asDictionaryBy (or groupedBy: as Stephane said) parametrized like:
>
> groupBy: groupBlock valueBy: valueBlock
> � � | result tempList |
> � � result := Dictionary new.
> � � tempList := self methodReferenceList copy sort: [:m1 :m2 | m2 timeStamp <= m1 timeStamp].
> � � tempList do: [:each |
> � � � � | key value |
> � � � � key := groupBlock value each.
> � � � � value := valueBlock value: each.
> � � � � (result includesKey: key)
> � � � � � � ifFalse: [result at: key put: OrderedCollection new].
> � � � � (result at: key) add: value].
> � � ^(Association key: result value: tempList)

probably

> - And that reminds me: �Why does that method return an association? �So you can return multiple values? :( �Thats not nice.. �Maybe you can make the RecentMessageList stateful and let it have a sorted collection with the sorted elements. �Maybe playing with that let you manage the speed problems.
>
> - And maybe you should look at SettingBrowser>>treeMorphIn:. �There's the way to build a MorphTreeMorph binded to a list, so you can update the view sending the message #changed to the model.

Yes that part I do not know

>
> Cheers,
> Guille
>
> On Tue, Aug 10, 2010 at 8:11 AM, St�phane Ducasse <stephane.ducasse@inria.fr> wrote:
> Hi benjamin
>
> Thanks for your code!!!!
>
> I did a review of your latest code: Here are some points that it would be nice to address
> I fixed what I saw and republished
>
>
> 0 - all the classes should have comments
>
> MessageList
> ----------
> 1- � � �MessageList example does not work
> � � � �Can you create some Unit tests to cover the expected behavior of the MessageList
>
> 2- MessageList>>browsers
> � � � �I removed it
>
> 3- recategorized the methods
>
> 4- I removed all the "comment10"
>
> 5- what I level
> � � � �Can you comment it?
>
> 6- for asDictionaryByClass
> � � � �why don't you use groupedBy: or similar methods?
> � � � �I'm not sure that I would call them asDictionaryBy.... since you return an association?
>
>
> � � � �Why don't you call them groupedBySelectors, groupedByClasses, groupedByPackages
> � � � �then why do you return an Association and not a dictionary.
>
> 7- Can you add oneline method comments to method?
>
>
> 8- Could you introduce an explicit instance variable representing the environment in use and initialize
> � � � �it with Smalltalk globals because like that we can specify in which environment the set is working.
>
> 9- openInWorldWithADictionarySelector: sucks :)
> � � � �the name is not really cool
> � � � �then you bind the core with the UI
> � � � �so you should define this method as an extension of RecentSubmission-UI and may be
>
> 10- You update all the browsers open but this should not be your role.
> As a core domain object you should not do this kind of behavior. The UI widget should register to event
> or notification and update themselves when you change.
>
> � � � �updateView
> � � � � � � � �browsers ifNotNilDo: [:collection | collection do: [:each | each updateView]]
>
>
> MessageListAssociation
> --------------------
> � � � �why do you need this guy?
> � � � �Just to print the elements?
> � � � �This is a UI concerns. The UI should wrap the model and provide a specific printed version
> � � � �or the MessageListAssociation should be explained in the class comment
>
>
> MethodReferenceWithSource
> ------------------------
> � � � �We should probably move some behavior to MethodReference
>
>
> NullTextStyler
> ------------
> � � � �I categorized all the methods following SHStyler categories
>
> RecentMessageList
> ----------------
> � � � �I cleaned the singleton part.
> � � � �You do not need
> � � � � � � � �UniqueInstance ifNotNil: [:u | UniqueInstance releaseAll].
>
> � � � �Again the model should not be referencing the ui
> � � � � � � � �releaseAll
> � � � � � � � � � � � �super initialize.
> � � � � � � � � � � � �MessageListBrowser allInstancesDo: [:each | each messageList = self ifTrue:[each delete]].
> � � � � � � � � � � � �browser:= nil.
> � � � � � � � � � � � �self methodReferenceList: OrderedCollection new.
>
>
> � � � �Why the setting is registration is done in the core and not at the level of the widget?
>
>
> � � � �- at startUp: you should not create the instance since uniqueInstance is lazzily accessed.
> � � � �However you should register to the SystemChangeNotifier to get the notification.
>
>
>
>
> MessageListBrowser
> -----------------
> Instead of having
>
> MessageListBrowser >>byDateAscendingOn: aMessageList
>
> � � � �^self
> � � � � � � � �on: aMessageList
> � � � � � � � �withADictionarySelector: #asDictionaryByDateAscending.
>
>
> MessageListBrowser >>byDateAscendingOn: aMessageList
>
> � � � �^self
> � � � � � � � �on: aMessageList
> � � � � � � � �withADictionarySelector: aMessageList asDictionaryByDateAscendingSelector
>
> and
>
> MessageList>> asDictionaryByDateAscendingSelector
> � � � �^ #asDictionaryByDateAscending
>
> => encapsulate information from model to UI.
> => I did it
>
>
>
> So for now this is enough :)
>
>
> Stef
>
> On Aug 1, 2010, at 3:00 AM, Benjamin Van Ryseghem wrote:
>
> > Gofer new
> > � � � squeaksource: 'PharoTaskForces';
> > � � � package: 'RecentSubmissions';
> > � � � load.
>
>
> _______________________________________________
> Pharo-users mailing list
> Pharo-users@lists.gforge.inria.fr
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-users
>
> _______________________________________________
> Pharo-users mailing list
> Pharo-users@lists.gforge.inria.fr
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-users


_______________________________________________
Pharo-users mailing list
Pharo-users@lists.gforge.inria.fr
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-users