Excellent hunting!
I think this is a conceptual bug in the current Pharo implementation.
Anyway, I fixed it now (and added a comment):
MOAbstractGraphLayout>>childrenFor: aNode except: aNodeCollection
^(self childrenFor: aNode)
reject: [:each | aNodeCollection includes: each]
"we are explicitly not using the default Collection>>difference:
behavior here because we want to preserve the order of the collection"
Thanks,
Doru
On 16 May 2012, at 18:22, Ben Coman wrote:
> This discussion originates from Mondrian on Moose 4.7 where layouts which
> previously were exactly reproducible now randomly alternate between two
> arrangements. I have cross-posted since upstream behaviour of
> Collection>>difference: has changed between Pharo 1.3 and 1.4.
>
> The issue in Mondrian has been isolated so that it can be observed using the
> attached changeset 'isolate-unstable-972.2.cs' against a fresh Moose 4.7
> image downloaded a few moments ago.
> After loading the cs, open a Transcript and then <Generate> the following
> script in Modrian Easel a dozen times.
> view shape label.
> view nodes: #(1 2 3 4 ).
> view edgesFromAssociations: { 1 -> 2. 1 ->3 . 3 -> 4}.
> view treeLayout
>
> At some point you will see a change in the order that nodes are drawn between
> runs as reflected in lines B2 and A2 shown below...
> ------------------------------RUN 1
> B1>an OrderedCollection(a MONode model: 2 a MONode model: 3)
> B2>an OrderedCollection(a MONode model: 3 a MONode model: 2)
> A2>a MONode model: 1
> A2>a MONode model: 3
> A2>a MONode model: 4
> A2>a MONode model: 2
> ------------------------------RUN 2
> B1>an OrderedCollection(a MONode model: 2 a MONode model: 3)
> B2>an OrderedCollection(a MONode model: 2 a MONode model: 3)
> A2>a MONode model: 1
> A2>a MONode model: 2
> A2>a MONode model: 3
> A2>a MONode model: 4
> ------------------------------END
>
>
> The cause of this is a change in behaviour of Collection>>#difference:
> between Pharo 1.3 & 1.4. This can be observed by executing the following in
> Workspace...
> 20 timesRepeat:
> [
> one := TextMorph new newContents: '1'.
> two := TextMorph new newContents: '2'.
> collection := (OrderedCollection with: one with: two).
> diff := collection difference: OrderedCollection new.
> Transcript crShow: diff first text, diff second text.
> ]
>
> In Pharo 1.3 the result never deviates from '12'.
> In Pharo 1.4 the result is sometimes '21'.
>
> Pharo 1.3 has...
> Collection>>difference: aCollection
> "Answer the set theoretic difference of two collections."
> ^ self reject: [:each | aCollection includes: each]
>
> Pharo 1.4 has...
> Collection>>difference: aCollection
> "Answer the set theoretic difference of two collections."
> | set|
> set := self asSet.
> aCollection do: [ :each|
> set remove: each ifAbsent: []].
> ^ self species withAll: set asArray
>
>
> Your thoughts?
>
> cheers -ben
>
>
> Tudor Girba wrote:
>> Could it be a Pharo issue, because I think it happened since we moved to
>> Pharo 1.4.
>>
>> Doru
>>
>>
>> On 15 May 2012, at 21:51, Alexandre Bergel wrote:
>>
>>
>>
>>> Well… this happens since the changes in the collection hierarchy a few
>>> weeks ago.
>>> The tree layout should indeed preserver the order. I spent some time but I
>>> could not see where the problem came from. I should spent some more time on
>>> this.
>>>
>>> Alexandre
>>> --
>>> _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
>>> Alexandre Bergel
>>> http://www.bergel.eu
>>>
>>> ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
>>>
>>>
>>>
>>> On May 13, 2012, at 11:21 PM, Tudor Girba wrote:
>>>
>>>
>>>
>>>> Thanks, Alex.
>>>>
>>>> But, it is not good in this case to fix the tests because the rendering
>>>> should remain the same :(.
>>>>
>>>> For example, in the MetaBrowser I rely on the fact that the tree rendering
>>>> remains the same to highlight the currently selected property.
>>>>
>>>> We need to investigate this issue closer because it is an important bug.
>>>>
>>>> Cheers,
>>>> Doru
>>>>
>>>>
>>>> On 14 May 2012, at 04:04, Alexandre Bergel wrote:
>>>>
>>>>
>>>>
>>>>>>> I've seen you committed in the Mondrian rep about the rubber band drag
>>>>>>> and drop facilities. This is indeed an interesting feature. However, I
>>>>>>> feel a bit more work is necessary. Here are my suggestions:
>>>>>>> - can you make sure all the Mondrian tests are green. Apparently
>>>>>>> MORoot>>resetFormCacheResursively disappeared.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Do you mean MONode>>resetFormCacheResursively which was renamed in
>>>>>> Mondrian-Core-AlexandreBergel.79 ?
>>>>>> I've searched back a few Mondrian-Core mcz files and not found
>>>>>> MORoot>>resetFormCacheResursively.
>>>>>> I fixed a few remaining references to this in Mondrian-Core-BenComan.80
>>>>>> et al.
>>>>>>
>>>>>>
>>>>>>
>>>>> Ok, I've produced a new version of Mondrian that includes your changes.
>>>>>
>>>>>
>>>>>
>>>>>> I fixed MOLayoutTest>>testTreeLayout failure in
>>>>>> Mondrian-Tests-BenComan.101.
>>>>>> There is one remaining failure that I can't work out. TestRunner cannot
>>>>>> open a debugger on it and after inserting 'self halt' it steps through
>>>>>> to the end of the method without a failure. Makes no sense to me. I
>>>>>> need to leave this one to others.
>>>>>>
>>>>>>
>>>>>>
>>>>> The tree layout seems to randomly order the nodes. Type the following
>>>>> script in an easel:
>>>>> -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>>>>> view shape rectangle withText.
>>>>> view nodes: #(1 2 3 4 ).
>>>>> view edgesFromAssociations: { 1 -> 2. 1 ->3 . 3 -> 4}.
>>>>> view treeLayout
>>>>> -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>>>>>
>>>>> Times to times it produces:
>>>>> <Screen Shot 2012-05-13 at 10.02.08 PM.png>
>>>>>
>>>>> times to times it produces:
>>>>> <Screen Shot 2012-05-13 at 10.02.16 PM.png>
>>>>>
>>>>> A few weeks ago, it had always produced the same rendering, and tests had
>>>>> been written accordingly. Now it varies, so tests have to be adjusted. I
>>>>> fixed this.
>>>>> The test is now green.
>>>>>
>>>>>
>>>>>>> - add new tests that capture your change of MOCanvas>>mouseUp: ,
>>>>>>> MOCanvas>>mouseOver:, and possibly for MOCanvas>>drawOn:. Those methods
>>>>>>> are central to Mondrian, they need to be robust.
>>>>>>>
>>>>>>>
>>>>>> I will now start thinking about some tests for these.
>>>>>>
>>>>>>
>>>>>>
>>>>> Cool!
>>>>>
>>>>> Alexandre
>>>>>
>>>>>
>>>>>
>>>>>>> Let me know how it goes. Currently the tests of Mondrian are yellow.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Alexandre
>>>>>>>
>>>>>>>
>>>>>>> On 8 May 2012, at 16:42,
>>>>>>>
>>>>>>> [email protected]
>>>>>>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> See <http://hudson.moosetechnology.org/job/moose-latest-dev/972/>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Moose-dev mailing list
>>>>>>>>
>>>>>>>>
>>>>>>>> [email protected]
>>>>>>>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> _______________________________________________
>>>>>> Moose-dev mailing list
>>>>>>
>>>>>> [email protected]
>>>>>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>>>>>
>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> Moose-dev mailing list
>>>>>
>>>>> [email protected]
>>>>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>>>>
>>>>>
>>>>>
>>>> --
>>>>
>>>> www.tudorgirba.com
>>>>
>>>>
>>>> "Don't give to get. Just give."
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Moose-dev mailing list
>>>>
>>>> [email protected]
>>>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>>>
>>>>
>>>>
>>> _______________________________________________
>>> Moose-dev mailing list
>>>
>>> [email protected]
>>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>>
>>>
>>>
>>
>> --
>>
>> www.tudorgirba.com
>>
>>
>> Innovation comes in least expected form.
>> That is, if it is expected, it already happened.
>>
>>
>> _______________________________________________
>> Moose-dev mailing list
>>
>> [email protected]
>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>
>>
>>
>>
>
> 'From Pharo1.4 of 18 April 2012 [Latest update: #14438] on 16 May 2012 at
> 11:15:20 pm'!
>
> !MOAbstractRegularTreeLayout methodsFor: 'hook' stamp: 'BenComan 5/16/2012
> 23:14'!
> doExecute: node
> | rootNodes btcdebug|
> alreadyLayoutedNodes := OrderedCollection new.
> rootNodes := self rootNodesFor: node nodes.
> nodesByLayer := OrderedCollection new.
>
> self flag: #btcdebug.
> "Regarding [email protected] emails 'Jenkins build is
> still unstable: moose-latest-dev #972'
> To isolate the issue, open a Transcript then generate the
> following in Mondrian Easel twenty times
> view shape label.
> view nodes: #(1 2 3 4 ).
> view edgesFromAssociations: { 1 -> 2. 1 ->3 . 3 -> 4}.
> view treeLayout.
> "
> btcdebug := rootNodes first.
> Transcript cr; crShow: '------------------------------'.
> Transcript crShow: 'B1>', (self childrenFor: btcdebug)
> asString.
> Transcript crShow: 'B2>', (self computeChildrenFor: btcdebug)
> asString.
>
> self
> layout: rootNodes
> atPoint: self leftGap @ self topGap
> atLayer: 1.
> self isLayered ifTrue: [
> self rearrangeByLayers: node ]! !
>
>
> !MOAbstractVerticalTreeLayout methodsFor: 'hook-private' stamp: 'BenComan
> 5/16/2012 23:02'!
> layout: aNodeCollection atPoint: aPoint atLayer: aNumber
> | treeSize childrenPosition x y middleOfTree |
> aNodeCollection isEmpty ifTrue: [ ^ 0 ].
> x := aPoint x.
> y := aPoint y.
> alreadyLayoutedNodes addAll: aNodeCollection.
> self atLayer: aNumber add: aNodeCollection.
> aNodeCollection do: [ :each |
> self flag: #btcdebug. Transcript crShow: 'A2>', each asString.
> childrenPosition := y + each height + self verticalGap.
> treeSize := each width
> max: (self layout: (self computeChildrenFor: each)
> atPoint: x @ childrenPosition atLayer: aNumber + 1).
> middleOfTree := x + (treeSize / 2.0) - (each width / 2.0).
> each translateTo: middleOfTree @ y.
> x := x + treeSize + self horizontalGap ].
> ^ x - aPoint x - self horizontalGap! !
>
> _______________________________________________
> Moose-dev mailing list
> [email protected]
> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
--
www.tudorgirba.com
"We cannot reach the flow of things unless we let go."
