When user press ctrl+a combination selection will be correct

[YaroslavOvdii]

@sofiiakvasnevska

Issue

https://github.com/Fliplet/fliplet-studio/issues/6108

Description

When user press ctrl+a combination selection will be correct

Screenshots/screencasts

https://share.getcloudapp.com/YEuAEeEq

Backward compatibility

This change is fully backward compatible.

[tonytlwu]

Could we not be more specific like this in all this new code? The current format is more complex to read, understand and maintain.

Suggested change

	return [left, right, top, bottom];
	return {
	top: top,
	right: right,
	bottom: bottom,
	left: left
	};

[YaroslavOvdii]

@tonytlwu We could do it in this function, yes, but not for this function because such format needed for the handsontable library.

[YaroslavOvdii]

@tonytlwu also about this return we decided to use this format:

1. It is easy to expand functionality for this feature when we decide to look for data in diagonal.
2. This return used in a sequence of the functions we have written and we are expecting an array to be a result of this function. So if we change it to the object we will have to transform it back to the array after that

[tonytlwu]

1. What do you mean by deciding to look for data in diagonal? Can you give me code examples of what it would roughly look in one method compared to another?
2. I think if we use .top and other properties, it's essentially only in this line https://github.com/Fliplet/fliplet-widget-data-source/pull/75/files#diff-58f9b9f8183cba2a53b4728f2df11588R369 that it needs to be converted to an array format (because handsontable needs it). I don't like the fact that all these small functions are written in a format handsontable dictates when it can just be a helper function that transforms these in the moment right before handsontable needs them. (We don't necessarily have to change the format now, as I'd like to see what you mean by point 1 first to decide.)

[YaroslavOvdii]

What do you mean by deciding to look for data in diagonal? Can you give me code examples of what it would roughly look in one method compared to another?

By diagonal I mean that now we look for data on top, left, bottom and right to decide should we what range we should select. But if we will want to look for data at top-left, top-right bottom-left and bottom-right in the current function we will simply add these cells to the array but with the suggested function, we will need to add an extra object key and search for it in next functions.
For example changes with our function:

  function getNearestCells(cellPosition) {
    var leftShift = cellPosition[1] - 1;
    var topShift = cellPosition[0] - 1;
    var left = [cellPosition[0], leftShift < 0 ? cellPosition[1] : leftShift];
    var right = [cellPosition[0], cellPosition[1] + 1];
    var top = [topShift < 0 ? cellPosition[0] : topShift, cellPosition[1]];
    var bottom = [cellPosition[0] + 1, cellPosition[1]];
    var topLeft = [topShift < 0 ? cellPosition[0] : topShift, leftShift < 0 ? cellPosition[1] : leftShift];
    var topRight = [cellPosition[0] + 1, cellPosition[1]];

    return [left, right, top, bottom, topLeft, topRight ];
  }

Changes with an object for top value only.

  function getNearestCells(cellPosition) {
    var leftShift = cellPosition[1] - 1;
    var topShift = cellPosition[0] - 1;
    var left = [cellPosition[0], leftShift < 0 ? cellPosition[1] : leftShift];
    var right = [cellPosition[0], cellPosition[1] + 1];
    var top = [topShift < 0 ? cellPosition[0] : topShift, cellPosition[1]];
    var bottom = [cellPosition[0] + 1, cellPosition[1]];
    var topLeft = [topShift < 0 ? cellPosition[0] : topShift, leftShift < 0 ? cellPosition[1] : leftShift];
    var topRight = [cellPosition[0] + 1, cellPosition[1]];

    return {
      left: left,
      right: right,
      top: top,
      bottom: bottom,
      topLeft: topLeft,
      topRight: topRight
    };
  }

  function getDataCoordinates(tableData, cellPosition, processedCells) {
    if (!processedCells) {
      processedCells = [];
      processedCells[cellPosition[0]] = [];
      processedCells[cellPosition[0]][cellPosition[1]] = true;
    }

    var cellsGroup = getNearestCells(cellPosition);

    cellsGroup['currentCell'] = cellPosition;

    var processedRow;
    var processedCell;

    if (tableData[cellsGroup.top] !== null) {
      processedRow = processedCells[cellsGroup.top[0]];

      if (!processedRow) {
        processedCells[cellsGroup.top[0]] = [];
      }

      processedCell = processedCells[cellsGroup.top[0]][cellsGroup.top[1]];

      if (!processedCell) {
        processedCells[cellsGroup[cell][0]][cellsGroup[cell][1]] = false;
      }
      
      getDataCoordinates(tableData, cellsGroup.top, )
    }

    // And for every key in the object, we will need such if
}

Example with for ... in :

  function getDataCoordinates(tableData, cellPosition, processedCells) {
    if (!processedCells) {
      processedCells = [];
      processedCells[cellPosition[0]] = [];
      processedCells[cellPosition[0]][cellPosition[1]] = true;
    }

    var cellsGroup = getNearestCells(cellPosition);

    cellsGroup['currentCell'] = cellPosition;

    for (var cell in cellsGroup) {
      var hasData = tableData[cellsGroup[cell][0]][cellsGroup[cell][1]] !== null;
      var processedRow = processedCells[cellsGroup[cell][0]];

      if (!processedRow) {
        processedCells[cellsGroup[cell][0]] = [];
      }

      var processedCell = processedCells[cellsGroup[cell][0]][cellsGroup[cell][1]];

      if (!processedCell) {
        processedCells[cellsGroup[cell][0]][cellsGroup[cell][1]] = false;
      }

      if (hasData && (!processedRow || !processedCell)) {
        processedCells[cellsGroup[cell][0]][cellsGroup[cell][1]] = true;
        getDataCoordinates(tableData, cell, processedCells);
      }      
    }

[tonytlwu]

@YaroslavOvdii I think I'm understanding this a little better now. Much of the confusion is created by the [0], [1] notations and ambiguous naming, which made it difficult to understand.

Am I right in assuming that getNearestCells() takes the position of one cell and returns a range of cells, e.g. from A1 to E5? If so, then:

1. I still recommend using objects and properties instead of an array of numbers, which is unclear.
2. The range would be defined by either:
   1. { topLeft, topRight, bottomRight, bottomLeft } or
   2. { top, left, columns, rows } Preferred

[YaroslavOvdii]

@tonytlwu

Am I right in assuming that getNearestCells() takes the position of one cell and returns a range of cells, e.g. from A1 to E5? If so, then:

No, it is not a range. It's simply coordinates of the four cells around one cell. And we won't be able to refuse from the [0] and [1] because the cell position is an array [rowIndex, colIndex] and we needed this coordinates to find the value of the cell in the tableData array.
I'll try to remade as you suggest.

[tonytlwu]

@YaroslavOvdii I see. Then I think { top, left, bottom, right } is the right way to go. If "top left" was needed, I'd just take the x coordinate of one and y coordinate of the other when it is needed, without needing to provide additional properties.

[YaroslavOvdii]

@tonytlwu added changes as suggested.

[tonytlwu]

Suggested change

	var cellsGroup = getNearestCells(cellPosition);
	var nearestCells = getNearestCells(cellPosition);

[tonytlwu]

Why would there be a currentCell property? I don't see it in the getNearestCells() function.

[YaroslavOvdii]

@tonytlwu
currentCell needed for when our selected cell is without a data on it row or col.

[tonytlwu]

@YaroslavOvdii Let's just add it into getNearestCells(). The currentCell property is 100% always added at this point, and it's good for the nearest cells to have a reference of which cell was the starting point.

[tonytlwu]

Am I right in assuming that the process of selecting all cells is a recursive process, as it's indicated in this line?

If so, how efficient is it if we want to select all 100K rows and 100 columns?

[YaroslavOvdii]

@tonytlwu You were right about large amount of data. This solution couldn't handle it. I've created a new PR with a new solution and tested it on a large data source 60k rows and it works fine.

I'll close this PR because it is not needed anymore.

[tonytlwu]

The plural for index is indices.

Suggested change

	var rowIndexes = [];
	var rowIndices = [];