Issue 492 in moose-technology: Glamour browsers do not release all subscriptions to announcer objects

Status: New
Owner: [hidden email]
Labels: Type-Defect Priority-High Component-Glamour Milestone-4.3

New issue 492 by [hidden email]: Glamour browsers do not release all  
subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

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

Comment #1 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

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 :)

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

Comment #2 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Good work.

I am not sure I understand the id part, though. Why do you need to update  
hash? And shouldn't you need the copy method as well?

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

Comment #3 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Oh, that was just to test the removing of the subscribers using #= instead  
#==.
The #hash is because I always define hash when I define #=, is just a  
tick :)
The #id is just to provide a unique identifier during the tests... we  
should find a better way to make this removal really work.

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

Comment #4 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

I still do not understand. GLMUpdateAction is not registered to any  
announcement. So, do you actually need the extensions of GLMUpdateAction  
after all?

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

Comment #5 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

I am not exactly sure what the test tests, given that the presentation is  
not the presentation that gets in the actual pane.

Try this in the test:
self assert: (presentation = browser panes first presentations first).

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

Comment #6 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Another thing, could you take a look at this test:

GLMUpdateMorphicTest>>testAnnouncerUnregistration

I do not understand why this one passes without the fix.

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

Comment #7 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

GLMUpdateAction is getting into the announcement because you do:

in registerInPresentatio:

...announcerObject
     on: self announcement
     do: [ ... ]

then, the block is registered, and the way you can later unsubscribe is by  
using the BlockClosure>>receiver... which is the GLMUpdateAction object :)




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

Comment #8 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Right, I always get confused with this part of announcement, because I only  
think of the announcerObject :).

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

Comment #9 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

So, now... the test tries to catch the "living moment" where a browser  
chains events, to test unregistration in a realistic case. Maybe not the  
best piece of software, but it worked :)
Interesting, looks like

presentation

and

browser transmissions first  destination pane presentations first

are not == (presentation are copied)

which is de reason why when I try to unsubscribe later, there are not the  
same objects. Ok, some way, this is what happens several times when browser  
is running. Maybe the test should be named  
#testUnregisterAnnouncementsAfterCopy or something...


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

Comment #10 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

and well... #testAnnouncerUnregistration seems to be testing the  
announcement unsubscription in a series of events (changing selected  
entities and general unregistration). I don't know why that test does not  
catch our problem. I presume that changing entities does not "fires" the  
presentation update mechanism who finalizes in a copy, I really don't know  
why :)

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

Comment #11 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

I integrated the changes, but then I had to revert them because it crashed  
the whole image when opening a MoosePanel on a model. Strange.

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

Updates:
        Owner: ---
        Cc: estebanlm tudor.girba

Comment #12 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

I would need your help on this one. Esteban, would you have time to give a  
hand?

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

Comment #13 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Yes, I'm willing to help fix this... last weeks it was really full of work,  
but now I'm having some time :)

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

Comment #14 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

How would you like to proceed? I would suggest to take a look and ask  
questions and I will answer.

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

Updates:
        Labels: -Milestone-4.3

Comment #15 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

It looks like we are not making progress here. With great regret I push it  
for later :)

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

Comment #16 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Yes, I'm sorry, worked a little on it last saturday, but no progress...  
yet, it is in my priority todo list.
Sorry for the delay...

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

Updates:
        Labels: Milestone-4.4

Comment #17 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Thanks to Henrik, it seems that we might have found the problem. All tests  
are passing. One problem was in  
UpdateAction>>unregisterFromAllAnnouncements.

It used to be like this:
unregisterFromAllAnnouncements
        [self announcerObjects do: [:each |
                each unsubscribe: self ]] on: Error do: [:e | ]

This meant that if there were an error inside the loop with one announcer,  
the rest of announcers would not have been unregistered from.

So, the new implementation fixes the problem:
unregisterFromAllAnnouncements
        self announcerObjects do: [:each |
                [each unsubscribe: self] on: Error do: [:e | e resume]]

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

Comment #18 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

Issue 589 has been merged into this issue.

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

Comment #19 on issue 492 by [hidden email]: Glamour browsers do not  
release all subscriptions to announcer objects
http://code.google.com/p/moose-technology/issues/detail?id=492

It looks like this issue is still around :(

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