Hi!

� I submitted a little fix where the UI is updated and the example is working.� 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?
-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...
-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)

- 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.

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