datenhahn / componentrenderer

A ComponentRenderer for the Vaadin Grid
Apache License 2.0
6 stars 8 forks source link

Caching generated components possible? #20

Open Juchar opened 8 years ago

Juchar commented 8 years ago

If I have a button to show the details row I want to be able to change the icon to "up"/"down" for showing/hiding details.

This does not work right now.

Example ComponentGenerator:

@Override
public Component getComponent(Object bean) {
    Button button = new Button();
    button.setWidth(2, Unit.EM);
    button.setHeight(2, Unit.EM);
    button.addStyleName(ValoTheme.BUTTON_ICON_ONLY);
    button.setIcon(VaadinIcons.CHEVRON_DOWN);
    button.addClickListener((Button.ClickListener) event -> {
        boolean detailsVisible = grid.isDetailsVisible(bean);
        event.getComponent().setIcon(detailsVisible ? VaadinIcons.CHEVRON_DOWN : VaadinIcons.CHEVRON_UP);
        grid.setDetailsVisible(bean, !detailsVisible);
    });
    return button;
}
Juchar commented 8 years ago

Ok, tracked this down a little bit. It comes from the fact that the generated property container calls the generator again and a new Button is created. I temporarily fixed this by managing a list myself that has the components for the rows and returning the correct button each time the generator gets called by myself. Is there some other way to circumvent this? Maybe you could implement such logic in the ComponentPropertyGenerator itself? Not regenerating a component if already done for a certain itemId?

datenhahn commented 8 years ago

Hi, thanks for your feedback, in that case you also have to track destructions of the components when scrolling, otherwise you will sum up components in memory (they don't use too much space though, the most important thing is, that they don't hang in the grid's component hierarchy, which already should be accomplished by the current componentrenderer code). Also you would need the possibility to update a component (see the food example, where the icon changes when you update the selection), So you need two generators, one to create the initial component, one which handles changing data. I would like to really think that through and test if there is a performance difference. Unfortunately currently right now I don't have much time, so I can only look into bugs, but I will keep it on the issue list for further investigation. If you want you can attach a gist to this ticket, so I can have a look at your current solution.

datenhahn commented 8 years ago

Regarding the classification as enhancement and not as bug: You already discovered, that you can do it by returning a new instance every time. (see the demo code, which exactly implements what you want to do)

https://github.com/datenhahn/componentrenderer/blob/master/componentrenderer-demo/src/main/java/de/datenhahn/vaadin/componentrenderer/demo/ViewComponents.java#L59-L86

https://github.com/datenhahn/componentrenderer/blob/master/componentrenderer-demo/src/main/java/de/datenhahn/vaadin/componentrenderer/demo/ClassicGridWithDecoratorTab.java#L72-L73

Disclaimer: just saw that my example always creates two images but only uses one, have to refactor that at some point.

datenhahn commented 8 years ago

Updated the title to reflect the new topic of the ticket

nloke commented 8 years ago

I don't think it make sense to have ComponentRenderer to cache. The logic should be in your own ComponentGenerator to return the same widget or create a new widget all the time.

I do have a patch for an issue where you decide to return the same widget where the widget will not be rendered properly when it got attached or detached due to tab switching for example.