Fix for cached component not rendered

[nloke]

This is to fix the case where for example : you have two tab sheets, with ComponentGrid that has two columns at least and you have the Component getComponent(T bean); method implementation to returned a cached component instead of a new component always.

When you switch the tab sheets, the cached component will be removed as unused component prematurely by another column that is not part of the same renderer

[nloke]

I think this addresses Issue #20

[nloke]

@datenhahn I am not sure what is the overall impact. Please take a look and see what can be done in this scenario.

[datenhahn]

Thanks for your comment, can you post me a short example of your tabsheet scenario, so I better understand, your use-case? It is not as easy as just removing the cleanup code. This would result in bloating your serverside user session with unused components (e.g. for a Grid with 100 000 rows if you slowly scroll through it you would have 100 000 components * no. of component-columns of components stored for the user.

One possibility I thought of would be to have some kind of 50 * no. of components cache, from which the components are retrieved and then just the value is set to them (some kind of serverside version of how the client side renderers work). Some extensive testing is necessary here to not produce situations where two times the same component is tried to be used on the client side.

If I add such feature it would not be for the existing code (that would completely break the renderer for most current users), but as a new option or class (e.g. a option "cached" or a CachedComponentRenderer).

You sparked my interest again in implementing that. I am quite busy currently, but I will try to find some time to play around with that in the next days/weeks. When I have a working approach, I will push it as a new branch so you can test it.

[datenhahn]

I was just looking at the code again. The destruction code you removed is meant for the case when you add/remove columns, the scrolling destruction code is in "destroyData(Object itemId)". I have to investigate again why it was necessary to immediately get rid of removed-column components (I think I remember something about that).

so it might be ok to not check for removed column-components, but it would not fix your use case (components still are destroyed on scrolling)

[nloke]

@datenhahn I am still doing the cleanup with the proposed changes. The only difference is each component renderer should cleanup its own unused widgets, other component should not interfere.

In the current implementation, it is doing a clean on every "jsonObject.getObject("d")" key, which in the example below, I have two keys, one for Column TestString (a nomal vaadin text renderer) and one which is component generator based.

When the first key (the TestString Column) is being processed, if (getColumn(key).getRenderer() == this) is false but it triggers a clean up on all the components within the row since itemID is a row data. TestString has no idea if other components that does not belong to it are still in used or not

So now when we process the second key if (getColumn(key).getRenderer() == this) is true and this is where it should decide if its own components needs to be destroyed.

For your reference, below is how I setup the grid and component renderer. This is my component generator

public class TestComponentGenerator implements ComponentGenerator<RowData> {
    private final Map<RowData, ComboBox> componentMap = Maps.newHashMap();

    @Override
    public Component getComponent(final RowData rowData) {
        ComboBox comboBox = componentMap.get(rowData);
        if (comboBox == null) {
            comboBox = create(rowData);
            componentMap.put(rowData, comboBox);
        }

       comboBox 
        return comboBox;
    }

   private ComboBox create(final RowData data) {
        final ComboBox comboBox = new ComboBox();
        comboBox.setWidth(150, Unit.PIXELS);
        comboBox.addItem("Combo Val 1");
        comboBox.setItemCaption("Combo Val 1", "Combo Val 1");
        comboBox.addItem("Combo Val 2");
        comboBox.setItemCaption("Combo Val  2", "Combo Val 2");

        comboBox.setValue(data.getComboVal());
   }
}

This is the Component Grid

        ComponentGrid grid= new ComponentGrid<>(RowData.class);
        grid.setSizeFull();
        grid.setHeightMode(HeightMode.ROW);
        grid.setSelectionMode(SelectionMode.MULTI);
        grid.setCaption("Grid Caption");

        grid.addComponentColumn("TestCombo", new TestComponentGenerator());

        grid.setColumns("TestString", "TestCombo");

        final Column stringColumn = grid.getColumn("TestString");
        attributeColumn.setHeaderCaption("String");
        attributeColumn.setExpandRatio(5);

        final Column comboColumn = grid.getColumn("TestCombo");
        operatorColumn.setHeaderCaption("Combo");
        operatorColumn.setExpandRatio(4);

       RowData bean = new RowData("Some Test Str",  "Combo Val 1");
       conditionGrid.getContainerDataSource().addItem(bean)

[nloke]

@datenhahn I updated my comment. please read the latest from github.

I do understand what you mean by the large data though and the concern but that is up to the developer themselves on how they want to implement the getComponent API? If they decide to use a map and bloat the memory usage, then they use it at their own risk... maybe they will know they have a small grid all the time and large data is not their concern?

In any case thanks for your input, I will probably change the implementation of my generator to create new components all the time and carry the state in the rowdata so that it will be repopulated with the proper state to support extra feature like read-only, enabled, bgcolour if some search filter is on. That was the reason why I "reused" the components as I wanted to preserve the state of the widget without having to modify the RowData to carry the extra states for the creation of the new component all the time.

Do consider the proposed code change however as it should not interfere with the clean up, just delegating the cleanup to each renderer itself. The proposed change can at least still help people who wants to reuse the components using getComponent() and they know they will only have a small grid at all times.

[datenhahn]

Now when I look at the code. It might be you actually found a bug (I have to investigate more thoroughly).

This part actually should be outside the for loop if I see correctly (I have to find some time to work a couple of hours on that without interruption). The plan was to loop through all columns of a row, track the used components and remove all others, because these others would belong to removed rows. It should not remove any components in use. Seems like I misplaced a curly bracket.

Can you try with this branch, if your problem still exists?

https://github.com/datenhahn/componentrenderer/tree/component_cleanup_fix

            // find all components, which are no longer in use for this item id
            Set<Component> itemIdComponents = components.get(itemId);

            if (itemIdComponents != null) {

                Set<Component> unusedComponents = new HashSet<>(itemIdComponents);
                unusedComponents.removeAll(componentsInUse);

                // remove unused components from current tracking
                components.get(itemId).removeAll(unusedComponents);

                // destroy unused components
                destroyComponents(unusedComponents);
            }

[datenhahn]

Ah sorry, I was confused by the change in indentation in the diff-view, now when I see the whole file, I see you had the same idea. The code should both do the same (your pull request and my branch)

[datenhahn]

It might be a couple of days before I release the fix, I first have to create a regression Testbench Test and review the changes again when I have more time.

[nloke]

@datenhahn Thanks. I looked at your code and I think both your way and my way works.

[datenhahn]

opened bug #32 and added code for reproducing the issue

[datenhahn]

@nloke I reworked the whole component tracking part, the Hashmap was actually unnecessary, as every renderer only tracks its own components, so per item and row only one component needs to be tracked. Thanks a lot for your input, version 1.0.3 is released

[nloke]

@datenhahn thanks for the release.