semanticsoft / vaaclipse

Vaaclipse is a framework for building web applications using Eclipse 4 platform and Vaadin
Other
31 stars 13 forks source link

@PreDestroy never called #24

Closed ghost closed 11 years ago

ghost commented 11 years ago

Whenever a part is closed or removed from the stack the @PreDestroy method is not called. That is a huge potential memory leak.

ghost commented 11 years ago

See the contacts demo for examples.

semanticsoft commented 11 years ago

OK I will see tomorrow.

semanticsoft commented 11 years ago

There are nor correct removeGui, I will implement it when return to home.

ghost commented 11 years ago

I think we should consider replacing the general renderer approach for a better one. I will ask Tom to comment if his fx renderers will do the work as I think they are more closer to our nature.

semanticsoft commented 11 years ago

Renderer works ok. Now rerquired only add some lines of code - and this will be fixed. No reasone to do so complex work - becouse it destroy stable work off all renderer and require to rewrite all element renderers.

semanticsoft commented 11 years ago

The feature of current generic renderer - it is not coupled to vaadin. I think this renderer can be used in future as renderers for other toolkits.

ghost commented 11 years ago

I don't want to remove the actual work. I just want to consider the general stuff not the specific elements. Specific elemts will stay the same. I think in the future we will need to customize these general renderers to fit Vaadin needs, for the good of this project of course so the general part will become smaller and smaller.

Anyway it's just an idea. Let's see what Tom thinks. I've used those fx renderers to write a minimal Swing renderer and I walked away with good impressions. Who knows maybe you @semanticsoft like them too :)

As i said, just considering an alternative so I'm not sure if this is a good idea yet.

semanticsoft commented 11 years ago

General stuff is coupled with specific elements, becouse given element renderer is tuned to general rendering approach. It is realy unrealistic work to replace renderer - this work require to rewrite all project )

But I am interested in the opinion of Tom of course.

semanticsoft commented 11 years ago

Generic renderer not coupling with vaadin details, this is a reason why I have take Kai Todter's renderer as a base. Really, I think this is a strange to do hard work for every platform implementing rendering lifecycle when there are way to implement one or two variants of generic renderers (for example - one for desktop implementation and one for web).

semanticsoft commented 11 years ago

I am at home and now see on GenericPresentationEngine. There are one little thing must be done - when element isToBeRendered=false or element is removed from it parent - removeGui on it element must be called. Remove GUI now is not implemented in right way - it must recurcively dispose widgets. GenericRenderer.removeWidget must be renamed to disposeWidget and must take only one parameter - widget is to dispose. In this metod renderers can do any dispose work. Method removeGui of GenericPresentationEngine must dispose all MContribution-s in hierarchy of removed element (as in SWT renderer).

semanticsoft commented 11 years ago

Current GenericPresentationEngine and GenericRenderer ecosystem seems very clean if compare to SWT renderers. The number of methods that concrete renderer must implement is very few and their menaing is clean. All lifecycle is in GenericPresentationEngine, in SWT renderer every renderer itself process child elements.

Currently one thing must be done (except this issue) - the lazy load of stack elements. It is not hard thing and can be done without modification of generic engine.

ghost commented 11 years ago

Ok.

On Sun, Nov 25, 2012 at 3:48 PM, Rushan Gilmullin notifications@github.comwrote:

Current GenericPresentationEngine at GenericRenderer ecosystem seems very clean if compare to SWT renderers. The number of methods that concrete renderer must implement is very few and their menaing is clean. All lifecycle is in GenericPresentationEngine, in SWT renderer every renderer itself process child elements.

Currently one thing must be done (except this issue) - the lazy load of stack elements. It is not hard thing and can be done without modification of generic engine.

— Reply to this email directly or view it on GitHubhttps://github.com/semanticsoft/vaaclipse/issues/24#issuecomment-10694120.

ghost commented 11 years ago

I'm reworking a bit the contacts demo.

On Sun, Nov 25, 2012 at 3:49 PM, Sopot Çela sopotcela@gmail.com wrote:

Ok.

On Sun, Nov 25, 2012 at 3:48 PM, Rushan Gilmullin < notifications@github.com> wrote:

Current GenericPresentationEngine at GenericRenderer ecosystem seems very clean if compare to SWT renderers. The number of methods that concrete renderer must implement is very few and their menaing is clean. All lifecycle is in GenericPresentationEngine, in SWT renderer every renderer itself process child elements.

Currently one thing must be done (except this issue) - the lazy load of stack elements. It is not hard thing and can be done without modification of generic engine.

— Reply to this email directly or view it on GitHubhttps://github.com/semanticsoft/vaaclipse/issues/24#issuecomment-10694120.

semanticsoft commented 11 years ago

Now all ok, but I commit a little later, becouse want check one thing. Now I take a pause on match Barca-Levante, it must be very interesting )

ghost commented 11 years ago

Ok. I just went out too. On Nov 25, 2012 9:07 PM, "Rushan Gilmullin" notifications@github.com wrote:

Now all ok, but I commit a little later, becouse want check one thing. Now I take a pause on match Barca-Levante, it must be very interesting )

— Reply to this email directly or view it on GitHubhttps://github.com/semanticsoft/vaaclipse/issues/24#issuecomment-10697663.

semanticsoft commented 11 years ago

Implemented: https://github.com/semanticsoft/vaaclipse/commit/2e1614660cc35a1856993e0bfc9c5317c72ae5b2

Now all cleanup works ok. There are some things in rendering engine that I want implement yet, will be done tomorrow.

tomsontom commented 11 years ago

I guess I came too late to the party. I've never looked at Kais stuff you are using to implement your renderers, all I know is that his starting point have been the SWT-Renderer.

Now some infos about the renderer implemented as the base for my JavaFX rendering. Here are the most important facts:

So how does such a renderer then look like.

The JavaFX ones now look like this:

So all you need to do most in the toolkit specific renderer is to implement the required interface (WMenuBar,WPerspectiveStack,WSash) for your UI-Technology and all the other stuff is handled by the generic one.

Anyways reimplementing ontop of our renderers should not be a disruptive thing if you decide in future that the generic framework written us is the better fit.

I always said that although the renderers are written with no UI-tech-dependency our companies main focus is and stays JavaFX as long as we don't have to make a compromise we are happy to add stuff needed for other UI-technologies, the design of all stuff is made for it so why I can't see a reason why your vaadin renderes should not use it.

Naturally everyone is biased but I really think that our approach is the better one compared to the one the Eclipse-Team and Kai have been following but like I said I'm naturally biased.

semanticsoft commented 11 years ago

@tomsontom Thanks for your comment (I have notice it only now). Now I look at your renderer and see that it is really much more easy implement the renderer with your rendering framework in the case that start new work. You provide additional abstraction layer over the standart widgets so implementing this interfaces allow using your renderer implementation for this elements. If I will wirte renderer for any tookit in future, I will take your framework as a base. But unfortunally I detect your renderer too late and I think currently there are no real need to rebase vaadin renderer. I also do simplificate the renderer cycle if compare to SWT renderer and if I understand write the cycle is seemalar to your (so rebase renderer is not big problem).

semanticsoft commented 11 years ago

@tomsontom pleasy comment with more details about toBeRendered issue. If I understand correctly only toBeRendered property define to be rendered given element or not to be rendered, so there are no hardcoded hooks.

tomsontom commented 11 years ago

The problem with the SWT renderering framework is that the responsibility between PartRenderingEngine and *Renderer is not 100% clear because the PRE also listens for element removals who IMO is the task of the renderer, to only task of the PRE is to detect that an element has toBeRendered=true and hand over to the appropriate renderer.

I agree with you that there's no immediate need for you to switch to our rendering engine only wanted to make sure you know about it and if you decide later for whatever reason Kais engine is not good enough anymore.

semanticsoft commented 11 years ago

My approach is different from yours in polymorphic layer. I look at renderers as on platfrom-specific entities and all non platfrom specfifc issues is moved to engine itself. So I have a thick engine and thin renderers (so implementors can it implement in easy way). In opposite, your engine is thin and all logic is moved to renderers; polymorphic platfrom-specific layer is located in different plafrom widgets. Your approach let to provide for implementors more specialized widget-specific api and this is strong feature of this approach. The downside of this approach is the existing yet another abstraction layer with possible integration issues. For example, when added new platfrom widgets (it is very hard to write universal interfaces from first attempt, without testing with different implementations, so possibly there are need some changes). This integration issues can cause problems because I have no access to repository of your renderer (yes, it is opensource, but maintained by commercial company that's may not like intervention in his buisness; if I do fork it is hard to support fork).

tomsontom commented 11 years ago

Using libraries always has this problem whether a commerical company is maintain it or not stays the same. So this argument makes nosense IMO. At least the license allows you fork and go on if the main people are behind it do something you don't agree with or stop investment.

If you look at the widget interfaces you'll notice that they are more or less already predefined by the Application-Model-Elements but you are right there's currently no guarantee that you don't have to update your code in future because we'll change this interface which I consider an SPI and not an API. To make the API-abstraction as small as possible all simple attributes are pushed to the widget implementations through DI so only the structural ones are predefined.

Anyways - like I stated before - I can completely understand that there's currently no real appeal for you to switch to our rendering framework.

ghost commented 11 years ago

Rushan when I originally invited Tom to comment it was for us to have a complete view of what is around. I mean that I also think that for the moment we should not do any changes. But it's a good thing to see alternatives. If, for whatever reasons, we decide to change something we know that there are these other renderers around, even though they might not fit our needs never ever.

What you are worried that will happen with the fx renderers has already happened with the renderers we are using. Kai's not maintaining it anymore and you and I are maintaining our version of it so this is not an argument.

Again in our project the members decide and currently it is two of us. Neither I nor you want to move and we have other priorities (bootstrap, packaging, full multiuser support etc.)

semanticsoft commented 11 years ago

@scela Kai do just protype, currently code is completely rewrited and very far from his code. And then we have some deep changes with Kai's approach, so they are completely uncompatible today.

@tomsontom Yes, it is not princiapial thing, I just imagine possible problems. I admir your work and I think your doing greate work generilizing renderer infrastructure. It is will be great if renderer providers doesn't write bicycles and can use general code. Yes, I already have implement vaadin renderer, but there are things that I have not imlemented and I can use your bundles for it. Forexample, key bindings. I wanted to do decoupling key bindings from jface 2 day later, but fortunately remembed how you tell me about your binding bundles so I going use it in future.

tomsontom commented 11 years ago

Just a small note what I think is wrong in the SWT-Renderers from Eclipse.org and Generic ones from Kai is that the PRE is also dealing with children modifications. I think the responsibility for this has to be in the renderers because then you can easily implement multi-value changes which is currently handled by multiple render calls which from a performance point of view is not a good idea.

It also has the problem that only children additions are handled appropriately but e.g. not adding trims, ...

semanticsoft commented 11 years ago

@tomsontom thanks for your note. I will take it into account if will be refactor engine in future... And there are yet one benefit - dealing with leazy elements can be don without interop with engine, engine doesn't know about this issue (now in SWT there are hardcode for this, but even remove hardcode and intoduce isLazy property into renderer's contract this is not nice).

Currently in vaadin renderer child addition and child to be rendererd is processed with same renderer's method - addChildGui (and removeChildGui for removing). This method is add the child's gui to parent widget hierarchy. There are no difference between TBR and children added in case of modifying widget hierarchy - so it can be processed with same methods. More, it is possible remove processContent method and create initial widget with addChildGui method, but of cource this is not capable from perfomance issue (becouse addChildGui do calculations how to modify parent's widget).