Issue 492 in moose-technology: Glamour browsers do not release all subscriptions to announcer objects
Status: New Owner: tudor.gi...@gmail.com Labels: Type-Defect Priority-High Component-Glamour Milestone-4.3
New issue 492 by tudor.gi...@gmail.com: 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
Comment #1 on issue 492 by esteba...@gmail.com: 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 :)
Comment #2 on issue 492 by tudor.gi...@gmail.com: 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?
Comment #3 on issue 492 by esteba...@gmail.com: 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.
Comment #4 on issue 492 by tudor.gi...@gmail.com: 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?
Comment #5 on issue 492 by tudor.gi...@gmail.com: 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).
Comment #6 on issue 492 by tudor.gi...@gmail.com: 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.
Comment #7 on issue 492 by esteba...@gmail.com: 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 :)
Comment #8 on issue 492 by tudor.gi...@gmail.com: 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 :).
Comment #9 on issue 492 by esteba...@gmail.com: 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...
Comment #10 on issue 492 by esteba...@gmail.com: 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 :)
Comment #11 on issue 492 by tudor.gi...@gmail.com: 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.
Updates: Owner: --- Cc: estebanlm tudor.girba
Comment #12 on issue 492 by tudor.gi...@gmail.com: 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?
Comment #13 on issue 492 by esteba...@gmail.com: 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 :)
Comment #14 on issue 492 by tudor.gi...@gmail.com: 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.
Updates: Labels: -Milestone-4.3
Comment #15 on issue 492 by tudor.gi...@gmail.com: 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 :)
Comment #16 on issue 492 by esteba...@gmail.com: 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...
Updates: Labels: Milestone-4.4
Comment #17 on issue 492 by tudor.gi...@gmail.com: 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]]
Comment #18 on issue 492 by tudor.gi...@gmail.com: 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.
Comment #19 on issue 492 by tudor.gi...@gmail.com: 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 :(
Updates: Labels: -Milestone-4.4
Comment #20 on issue 492 by tudor.gi...@gmail.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
(No comment was entered for this change.)
Comment #21 on issue 492 by esteba...@gmail.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
looks like adding a #makeWeak send to GLMUpdateAction>>#registerInPresentation makes the bahavior better (I don't know if it corrects the problem, but it reduces the announcement subscription propagation a lot).
GLMUpdateAction>>#registerInPresentation self announcerObjects do: [: announcerObject | announcerObject notNil ifTrue: [ (announcerObject on: self announcement send: #actOn: to: self) makeWeak. "we remember the announcer object to be able to unregister from it when the presentation goes away" self presentation registeredAnnouncers add: announcerObject ] ]
Comment #22 on issue 492 by esteba...@gmail.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
anyway, there are still internal announcements who uses the on:do: form which can not be maked as weak (because of the ephemerons issue), and we need to check how to clean those... well, maybe tomorrow :)
Updates: Labels: Milestone-4.5
Comment #23 on issue 492 by tu...@tudorgirba.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
Please commit the makeWeak fix. This is needed. It would be great to have a test that at least reveals the problem ... but we do not have it right now.
Regarding the internal announcements, these should not be problematic because they do not link to the actual model (hopefully :)
Updates: Labels: -Milestone-4.5 Milestone-4.6
Comment #24 on issue 492 by jannik.l...@gmail.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
(No comment was entered for this change.)
Updates: Status: Fixed
Comment #25 on issue 492 by tu...@tudorgirba.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
This is now fixed. Hopefully :)
this is really cool, great Tudor!
sorry I didn't find time to help a little more with this... but well, at least I can help by testing it :)
cheers, Esteban
El 14/07/2011, a las 7:15a.m., moose-technology@googlecode.com escribió:
Updates: Status: Fixed
Comment #25 on issue 492 by tu...@tudorgirba.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
This is now fixed. Hopefully :)
Moose-dev mailing list Moose-dev@iam.unibe.ch https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Hi Esteban,
Testing this would be very helpful :)
Cheers, Doru
On 14 Jul 2011, at 14:37, Esteban Lorenzano wrote:
this is really cool, great Tudor!
sorry I didn't find time to help a little more with this... but well, at least I can help by testing it :)
cheers, Esteban
El 14/07/2011, a las 7:15a.m., moose-technology@googlecode.com escribió:
Updates: Status: Fixed
Comment #25 on issue 492 by tu...@tudorgirba.com: Glamour browsers do not release all subscriptions to announcer objects http://code.google.com/p/moose-technology/issues/detail?id=492
This is now fixed. Hopefully :)
Moose-dev mailing list Moose-dev@iam.unibe.ch https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Moose-dev mailing list Moose-dev@iam.unibe.ch https://www.iam.unibe.ch/mailman/listinfo/moose-dev
-- www.tudorgirba.com
"Every thing should have the right to be different."
-
Esteban Lorenzano -
moose-technology＠googlecode.com -
Tudor Girba