vaadin-component-factory / selection-grid-flow

Add the functionality to Vaadin Grid to focus on a particular row and column and select a range of rows with SHIFT/CTRL Click
https://incubator.app.fi/selection-grid-flow-demo/
Apache License 2.0
4 stars 8 forks source link

ScrollToItem corrupts tree data #53

Open andersforsell opened 8 months ago

andersforsell commented 8 months ago

With Vaadin 24.3.3 and selection-grid-flow version 3.0.3 I sometimes see problems that the tree data is corrupted, for example child nodes are empty.

The below code reproduces the problem if you click the button and then expand the tree data, see also snapshot.

public class MainView extends VerticalLayout {
    public MainView() {
      SelectionTreeGrid<String> treeGrid = new SelectionTreeGrid<>();
    treeGrid.addHierarchyColumn(String::toString);
    TreeData<String> treeData = new TreeData<>();
    for (int i = 1; i <= 10; i++) {
        String topLevelItem = "Item " + i;
        treeData.addItem(null, topLevelItem);
        for (int j = 1; j <= 3; j++) {
            String subItem = "Subitem " + (i * 10 + j);
            treeData.addItem(topLevelItem, subItem);
            for (int k = 1; k <= 3; k++) {
                String subSubItem = "Subsubitem " + (i * 100 + j * 10 + k);
                treeData.addItem(subItem, subSubItem);
            }
        }
    }
    treeGrid.setDataProvider(new TreeDataProvider<>(treeData));
    Button button = new Button("Select Item", e -> {
      treeGrid.scrollToItem("Subsubitem 512");
      treeGrid.select("Subsubitem 512");
    });
    add(treeGrid, button);
    }
}

Screenshot 2024-02-09 133039

TatuLund commented 8 months ago

@andersforsell The scrollToItem implementation in SelectionTreeGrid is based on the workaround of missing feature in TreeGrid. This missing feature actually has now been implemented in Vaadin 24. There is now scrollToIndex(int... indeces) method. Most likely fix to your observed problem is to refactor SelcetionTreeGrid implementation to use that as scrollToIndex(int... indeces) will also activate loading of the rows data as per needed.

andersforsell commented 8 months ago

@TatuLund I'm not sure it is due to the scrollToItem method because I see problems with this code as well:

@Route
public class MainView extends VerticalLayout {

    public MainView() {
      TreeGrid<String> treeGrid = new SelectionTreeGrid<>();
    treeGrid.addHierarchyColumn(String::toString);
    TreeData<String> treeData = new TreeData<>();
    for (int i = 1; i <= 10; i++) {
        String topLevelItem = "Item " + i;
        treeData.addItem(null, topLevelItem);
        for (int j = 1; j <= 3; j++) {
            String subItem = "Subitem " + (i * 10 + j);
            treeData.addItem(topLevelItem, subItem);
            for (int k = 1; k <= 3; k++) {
                String subSubItem = "Subsubitem " + (i * 100 + j * 10 + k);
                treeData.addItem(subItem, subSubItem);
            }
        }
    }
    treeGrid.setDataProvider(new TreeDataProvider<>(treeData));
    Button button = new Button("Select Item", e -> {
      treeGrid.expand("Item 5");
      treeGrid.expand("Subitem 51");
      treeGrid.select("Subsubitem 512");
    });

    add(treeGrid, button);
    }
}

Note that I'm only using the TreeGrid interface but somehow the SelectionTreeGrid implementation affects how items are fetched and displayed. Switching to the TreeGrid implementation does not show the problem.

TatuLund commented 8 months ago

Note, 3.0.3 is not the latest, 3.0.4 is, and there I fixed something like you described. Can you update and check if the problem disappears.

https://github.com/vaadin-component-factory/selection-grid-flow/commit/6f6521fe03b24a70f80fc967587cabc25a88a772

andersforsell commented 8 months ago

I tested with 3.0.4 but its unfortunately the same problem.

andersforsell commented 8 months ago

@TatuLund the problem is fixed when I changed the _getItemOverriden function in helpers.js to better correspond to the overridden _getItem :

export function _getItemOverriden(idx, el) {
      if (idx >= this._flatSize) {
        return;
      }

      el.index = idx;

      const { item } = this._dataProviderController.getFlatIndexContext(idx);
      if (item) {
        this.__updateLoading(el, false);
        this._updateItem(el, item);
        if (this._isExpanded(item)) {
          this._dataProviderController.ensureFlatIndexHierarchy(idx);
        }
      } else {
        this.__updateLoading(el, true);
        this._dataProviderController.ensureFlatIndexLoaded(idx);
      }    

    /** focus when get item if there is an item to focus **/
    if (this._rowNumberToFocus > -1) {
        if (idx === this._rowNumberToFocus) {
            const row = Array.from(this.$.items.children).filter(
                (child) => child.index === this._rowNumberToFocus
            )[0];
            if (row) {
                this._focus();
            }
        }
    }
}
TatuLund commented 8 months ago

I can't reproduce the issues demonstrated by your code samples with version 3.0.4. Can you check browser console log if you see the warnings reported by this ticket https://github.com/vaadin-component-factory/selection-grid-flow/issues/52

If yes, you are still having the old JavaScript, then you need to do full frontend clean by running mvn clean-frontend first and delete fronted/generated folder.

TatuLund commented 8 months ago

Also your proposed fix looks to be essentially the same as already in version 3.0.4.

andersforsell commented 8 months ago

Yes, you are right, it must have been the old JavaScript when I was testing.

Can you please release and publish the 3.0.4 version on Maven Central?

TatuLund commented 8 months ago

Can you please release and publish the 3.0.4 version on Maven Central?

Version 3.0.4 has been published in Vaadin's add-on repository already.

TatuLund commented 8 months ago

Duplicate of https://github.com/vaadin-component-factory/selection-grid-flow/issues/52