a bug in Glamour

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

a bug in Glamour

EstebanLM
Hi,
I noticed that in some complex updating between panels, using an external announcer, some of the update subscriptions were not removed when the browser is closed. I debugged a little and I founded that this implementation:

GLMUpdateAction>>unregisterFromAllAnnouncements
        announcerObjects ifNotNil: [
                announcerObjects do: [:each |
                        each unsubscribe: self ] ]

is bugged, because if announcerObjects are not previously computed (and in some cases that's what happens), the subscription is not removed.
This implementation (just using the accessor instead the direct object), solves the problem (but I don't know is it's a right fix, and it should be a fix in other place)

GLMUpdateAction>>unregisterFromAllAnnouncements
        self announcerObjects ifNotNil: [ :objects |
                objects do: [:each |
                        each unsubscribe: self ] ]

Cheers,
Esteban
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: a bug in Glamour

Tudor Girba
Excellent catch, Esteban!

I know about this problem since half a year, but I could not figure out where the problem was.

I opened a ticket:
http://code.google.com/p/moose-technology/issues/detail?id=492

Before integrating the fix, could you provide a test case that fails with the current implementation?

Cheers,
Doru


On 15 Jan 2011, at 14:29, Esteban Lorenzano wrote:

> Hi,
> I noticed that in some complex updating between panels, using an external announcer, some of the update subscriptions were not removed when the browser is closed. I debugged a little and I founded that this implementation:
>
> GLMUpdateAction>>unregisterFromAllAnnouncements
> announcerObjects ifNotNil: [
> announcerObjects do: [:each |
> each unsubscribe: self ] ]
>
> is bugged, because if announcerObjects are not previously computed (and in some cases that's what happens), the subscription is not removed.
> This implementation (just using the accessor instead the direct object), solves the problem (but I don't know is it's a right fix, and it should be a fix in other place)
>
> GLMUpdateAction>>unregisterFromAllAnnouncements
> self announcerObjects ifNotNil: [ :objects |
> objects do: [:each |
> each unsubscribe: self ] ]
>
> Cheers,
> Esteban
> _______________________________________________
> Moose-dev mailing list
> [hidden email]
> https://www.iam.unibe.ch/mailman/listinfo/moose-dev

--
www.tudorgirba.com

"In a world where everything is moving ever faster,
one might have better chances to win by moving slower."




_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: a bug in Glamour

Tudor Girba
I forgot to mention that right now we have two tests, but they do not capture the problem:
GLMBrowserTest>>testUnregisterAnnouncements
GLMBrowserTest>>testUnregisterAnnouncementsWhenRemovingPane

Cheers,
Doru


On 15 Jan 2011, at 14:41, Tudor Girba wrote:

> Excellent catch, Esteban!
>
> I know about this problem since half a year, but I could not figure out where the problem was.
>
> I opened a ticket:
> http://code.google.com/p/moose-technology/issues/detail?id=492
>
> Before integrating the fix, could you provide a test case that fails with the current implementation?
>
> Cheers,
> Doru
>
>
> On 15 Jan 2011, at 14:29, Esteban Lorenzano wrote:
>
>> Hi,
>> I noticed that in some complex updating between panels, using an external announcer, some of the update subscriptions were not removed when the browser is closed. I debugged a little and I founded that this implementation:
>>
>> GLMUpdateAction>>unregisterFromAllAnnouncements
>> announcerObjects ifNotNil: [
>> announcerObjects do: [:each |
>> each unsubscribe: self ] ]
>>
>> is bugged, because if announcerObjects are not previously computed (and in some cases that's what happens), the subscription is not removed.
>> This implementation (just using the accessor instead the direct object), solves the problem (but I don't know is it's a right fix, and it should be a fix in other place)
>>
>> GLMUpdateAction>>unregisterFromAllAnnouncements
>> self announcerObjects ifNotNil: [ :objects |
>> objects do: [:each |
>> each unsubscribe: self ] ]
>>
>> Cheers,
>> Esteban
>> _______________________________________________
>> Moose-dev mailing list
>> [hidden email]
>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>
> --
> www.tudorgirba.com
>
> "In a world where everything is moving ever faster,
> one might have better chances to win by moving slower."
>
>
>

--
www.tudorgirba.com

"There are no old things, there are only old ways of looking at them."




_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: a bug in Glamour

EstebanLM
In reply to this post by Tudor Girba
he, no... I was too optimistic, that solves just part of the problem, not completely :(
Now I'm debugging to try figure out what's happening... once I get it, I will provide a test :)

Cheers,
Esteban

El 15/01/2011, a las 10:41a.m., Tudor Girba escribió:

> Excellent catch, Esteban!
>
> I know about this problem since half a year, but I could not figure out where the problem was.
>
> I opened a ticket:
> http://code.google.com/p/moose-technology/issues/detail?id=492
>
> Before integrating the fix, could you provide a test case that fails with the current implementation?
>
> Cheers,
> Doru
>
>
> On 15 Jan 2011, at 14:29, Esteban Lorenzano wrote:
>
>> Hi,
>> I noticed that in some complex updating between panels, using an external announcer, some of the update subscriptions were not removed when the browser is closed. I debugged a little and I founded that this implementation:
>>
>> GLMUpdateAction>>unregisterFromAllAnnouncements
>> announcerObjects ifNotNil: [
>> announcerObjects do: [:each |
>> each unsubscribe: self ] ]
>>
>> is bugged, because if announcerObjects are not previously computed (and in some cases that's what happens), the subscription is not removed.
>> This implementation (just using the accessor instead the direct object), solves the problem (but I don't know is it's a right fix, and it should be a fix in other place)
>>
>> GLMUpdateAction>>unregisterFromAllAnnouncements
>> self announcerObjects ifNotNil: [ :objects |
>> objects do: [:each |
>> each unsubscribe: self ] ]
>>
>> Cheers,
>> Esteban
>> _______________________________________________
>> Moose-dev mailing list
>> [hidden email]
>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>
> --
> www.tudorgirba.com
>
> "In a world where everything is moving ever faster,
> one might have better chances to win by moving slower."
>
>
>
>
> _______________________________________________
> Moose-dev mailing list
> [hidden email]
> https://www.iam.unibe.ch/mailman/listinfo/moose-dev


_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: a bug in Glamour

Tudor Girba
Thanks a lot!

Doru



On 15 Jan 2011, at 14:45, Esteban Lorenzano wrote:

> he, no... I was too optimistic, that solves just part of the problem, not completely :(
> Now I'm debugging to try figure out what's happening... once I get it, I will provide a test :)
>
> Cheers,
> Esteban
>
> El 15/01/2011, a las 10:41a.m., Tudor Girba escribió:
>
>> Excellent catch, Esteban!
>>
>> I know about this problem since half a year, but I could not figure out where the problem was.
>>
>> I opened a ticket:
>> http://code.google.com/p/moose-technology/issues/detail?id=492
>>
>> Before integrating the fix, could you provide a test case that fails with the current implementation?
>>
>> Cheers,
>> Doru
>>
>>
>> On 15 Jan 2011, at 14:29, Esteban Lorenzano wrote:
>>
>>> Hi,
>>> I noticed that in some complex updating between panels, using an external announcer, some of the update subscriptions were not removed when the browser is closed. I debugged a little and I founded that this implementation:
>>>
>>> GLMUpdateAction>>unregisterFromAllAnnouncements
>>> announcerObjects ifNotNil: [
>>> announcerObjects do: [:each |
>>> each unsubscribe: self ] ]
>>>
>>> is bugged, because if announcerObjects are not previously computed (and in some cases that's what happens), the subscription is not removed.
>>> This implementation (just using the accessor instead the direct object), solves the problem (but I don't know is it's a right fix, and it should be a fix in other place)
>>>
>>> GLMUpdateAction>>unregisterFromAllAnnouncements
>>> self announcerObjects ifNotNil: [ :objects |
>>> objects do: [:each |
>>> each unsubscribe: self ] ]
>>>
>>> Cheers,
>>> Esteban
>>> _______________________________________________
>>> Moose-dev mailing list
>>> [hidden email]
>>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>>
>> --
>> www.tudorgirba.com
>>
>> "In a world where everything is moving ever faster,
>> one might have better chances to win by moving slower."
>>
>>
>>
>>
>> _______________________________________________
>> Moose-dev mailing list
>> [hidden email]
>> https://www.iam.unibe.ch/mailman/listinfo/moose-dev
>
>
> _______________________________________________
> Moose-dev mailing list
> [hidden email]
> https://www.iam.unibe.ch/mailman/listinfo/moose-dev

--
www.tudorgirba.com

"Not knowing how to do something is not an argument for how it cannot be done."


_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: a bug in Glamour

EstebanLM
In reply to this post by Tudor Girba
ok... it is more complicated than I thought... this is what I dug for now (sorry, this is large): 

1) the fix proposed originaly is still necesary. 
2) GLMBrowser>>unregisterFromAllAnnouncements needs to be defined this way: 

unregisterFromAllAnnouncements
super unregisterFromAllAnnouncements.
self panes do: [:each | 
each presentations unregisterFromAllAnnouncements ].
self transmissions do: [ :each | 
each destination pane unregisterFromAllAnnouncements ]

(this include panes in transmissions in the clean up)

3) this is the ugly part: there is an identity problem, because GLMUpdateActions are being copied a lot of times, to ensure the browser function. And unsubscribe: checks for identical objects (==) to remove from the annoncements. So... and THIS IS NOT THE SOLUTION, just a validation of the problem. I did: 

a) I added a method to Announcer:

Announcer>>unsubscribeNonIdentical: anObject
subscriptions ifNil: [ ^ self ].
subscriptions keysAndValuesDo: [ :class :actions |
subscriptions at: class put: (actions 
reject: [ :each | each receiver = anObject ]) ].
subscriptions keysAndValuesRemove: [ :class :actions |
actions isEmpty ]

(it just changes the check by == to a check by =)

b) assign an unique id, = and hash to GLMUpdateAction: 

id 
^id ifNil: [ id = UUID new asString ]

= other 
self species = other species ifFalse: [ ^false ].
^self id = other id

hash 
^self species hash bitXor: self id hash

c) modify 

GLMUpdateAction>>unregisterFromAllAnnouncements
self announcerObjects 
do: [:each | each unsubscribeNonIdentical: self ]


and with all this changes... announcement un-suscription is working :P
this is the test who validates it: 

GLMUpdateAction>>testUnregisterAnnouncementsWhenUpdatingPane
| announcer browser  presentation |
announcer := Announcer new.
browser := GLMTabulator new.
browser row: #one.
browser transmit 
to: #one;
andShow: [ :presenter |
(presentation := presenter list)
updateOn: TestAnnouncement from:  [ announcer ];
display: [ :start | start to: 10 ] ].
browser startOn: 1.
presentation registerAnnouncements.  
browser unregisterFromAllAnnouncements.

self assert: (announcer instVarNamed: 'subscriptions') isEmpty.


So... this makes the un-suscription works, but the whole "id" thing is really-really ugly. So, as I don't know much of Glamour, maybe you can say me a real fix for this :)

Cheers,
Esteban

pd: I attached all this as a comment on issue http://code.google.com/p/moose-technology/issues/detail?id=492


El 15/01/2011, a las 10:43a.m., Tudor Girba escribió:

I forgot to mention that right now we have two tests, but they do not capture the problem:
GLMBrowserTest>>testUnregisterAnnouncements
GLMBrowserTest>>testUnregisterAnnouncementsWhenRemovingPane

Cheers,
Doru


On 15 Jan 2011, at 14:41, Tudor Girba wrote:

Excellent catch, Esteban!

I know about this problem since half a year, but I could not figure out where the problem was.

I opened a ticket:
http://code.google.com/p/moose-technology/issues/detail?id=492

Before integrating the fix, could you provide a test case that fails with the current implementation?

Cheers,
Doru


On 15 Jan 2011, at 14:29, Esteban Lorenzano wrote:

Hi,
I noticed that in some complex updating between panels, using an external announcer, some of the update subscriptions were not removed when the browser is closed. I debugged a little and I founded that this implementation:

GLMUpdateAction>>unregisterFromAllAnnouncements
announcerObjects ifNotNil: [
announcerObjects do: [:each |
each unsubscribe: self ] ]

is bugged, because if announcerObjects are not previously computed (and in some cases that's what happens), the subscription is not removed.
This implementation (just using the accessor instead the direct object), solves the problem (but I don't know is it's a right fix, and it should be a fix in other place)

GLMUpdateAction>>unregisterFromAllAnnouncements
self announcerObjects ifNotNil: [ :objects |
objects do: [:each |
each unsubscribe: self ] ]

Cheers,
Esteban
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev

--
www.tudorgirba.com

"In a world where everything is moving ever faster,
one might have better chances to win by moving slower."




--
www.tudorgirba.com

"There are no old things, there are only old ways of looking at them."




_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev


_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev