Change Announcement management in Calypso Tool

[guillep]

When the project tool is open in calypso, it subscribes to system announcements to refresh itself. But since calypso has no hook to know if a tool has been removed from the UI, we cannot unregister it, and it causes memory leaks.

We should maybe have our own announcer as in Iceberg.

[dionisiydk]

If you talking about Calypso tabs then there is a hook to subscribe and unsubscribe from environment. Look for example at ClyMethodCodeEditorTool

attachToSystem/#detachFromSystem methods.

2018-07-25 16:27 GMT+01:00 Guille Polito notifications@github.com:

When the project tool is open in calypso, it subscribes to system announcements to refresh itself. But since calypso has no hook to know if a tool has been removed from the UI, we cannot unregister it, and it causes memory leaks.

We should maybe have our own announcer as in Iceberg.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/demarey/cargo/issues/160, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxaoNVXxWq4UmisqSxlhxK14SmQ-t7oks5uKI53gaJpZM4VgTO5 .

[guillep]

Thanks @dionisydk, we will see if that helps us!

Thanks to that, I took a look and that let me find that we have to redefine isSimilarTo:, otherwise calypso tries to reuse the same tool (which was not really clear...). I wonder if that is really necessary or just an optimization, and then why not using #=.

[dionisiydk]

2018-07-25 18:47 GMT+01:00 Guille Polito notifications@github.com:

Thanks @dionisydk, we will see if that helps us!

Here it was again some conclusion about Calypso without any question to me.

Thanks to that, I took a look and that let me find that we have to redefine isSimilarTo:, otherwise calypso tries to reuse the same tool (which was not really clear...). I wonder if that is really

Yes, it is really necessary. It is related to the logic why new class editor is not opened when you select new method. (tabs are reconfigured after any selection change according to general state of browser and not just concrete pane).

About #=, I think it is not make sense to implement such method on morph. A lot of state is hidden inside. And it will require #hash method too.

necessary or just an optimization, and then why not using #=.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/demarey/cargo/issues/160#issuecomment-407838813, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxaoGQc56o_Uc6Ot877RgTAk0045PeFks5uKK8XgaJpZM4VgTO5 .

[guillep]

Thanks @dionisydk, we will see if that helps us! Here it was again some conclusion about Calypso without any question to me.

I'm sorry but stop saying that or I'll just ban you and not read your comments anymore. So, understand that your answer speed is far too slow for us: we are actively working on this 8 hours a day, we cannot wait 24 hours to get an answer. So yes, we read some code and class comments and we draw conclusions.

Also, understand that we just opened an issue because we needed to discuss about how to solve it. Cargo's calypso extensions were written before we arrived and several months ago: lots of stuff are/were not properly working, and we don't have the knowledge of it. We have to live with it, and you have to live with it too I think. So THIS is exactly the place to discuss this, there is no need for such remarks: the only thing that you provoke is that I react with an exclusively non-technical answer like this one, and I feel this is counter-productive.

So please, as soon as you have a description of Calypso's architecture (UI, model, interaction with queries, extension points, remotability), I'll gladly read it and review it.

[guillep]

Yes, it is really necessary. It is related to the logic why new class editor is not opened when you select new method. (tabs are reconfigured after any selection change according to general state of browser and not just concrete pane).

I don't get what you mean by 'reconfigure': you reuse the same tool objects all the time? I would expect that all but dirty tabs are just recreated... They are morphs, why reusing them? Is it just an optimization?

About #=, I think it is not make sense to implement such method on morph. A lot of state is hidden inside. And it will require #hash method too.

Ah, I did not see that ClyBrowserTool inherits from BorderedMorph. I would have expected that a tool creates a morph...

Question related to the issue: is #detachFromSystem called when the browser is closed or only when the tab is closed?

[dionisiydk]

2018-07-25 21:18 GMT+01:00 Guille Polito notifications@github.com:

Yes, it is really necessary. It is related to the logic why new class editor is not opened when you select new method. (tabs are reconfigured after any selection change according to general state of browser and not just concrete pane).

I don't get what you mean by 'reconfigure': you reuse the same tool objects all the time? I would expect that all but dirty tabs are just recreated... They are morphs, why reusing them? Is it just an optimization?

It is requirement. Users interact with opened tabs. For example you select something in class definition tab. Then you select new method. The class definition tab in that case should keep the current state. But it is not dirty in this example.

About #=, I think it is not make sense to implement such method on morph. A lot of state is hidden inside. And it will require #hash method too.

Ah, I did not see that ClyBrowserTool inherits from BorderedMorph. I would have expected that a tool creates a morph...

I would like to change it like you said and make these tools independent from tabs. So they will be reusable components without any relation to browser. Now they are tightly bound to tab and browser. It was easy to implement them this way.

Question related to the issue: is #detachFromSystem called when the browser is closed or only when the tab is closed?

In both cases. All components in browser have a chance to perform cleanup.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/demarey/cargo/issues/160#issuecomment-407881800, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxaoOqdEiRn8q_TAQ2l_JsNBhj5M0Bnks5uKNKbgaJpZM4VgTO5 .