Support grid selection with Ctrl+Shift+Arrow

[Ocarthon]

Implements #593

Adds the function to select all cells in one direction using the shortcut Ctrl+Shift+Arrow

[fflorent]

I am really happy to see that this feature is getting implemented, thanks for that 🙏

In Libreoffice, CTRL-SHIFT-<arrow key> selects elements before a blank cell, or when a blank cell is selected until it selects a filled cell, as shown in the below video:

recording.mp4

@paulfitz What do you think of this behavior? Is this desirable in Grist?

@Ocarthon Same question as for Paul :). And would you like to implement this? (otherwise I would say it's ok to merge as is)
Please note that I don't have any authority of this subject (I am a contributor just like you), so don't hesitate to tell your opinion ;-).

And if I am not bothering you too much with my questions, @Ocarthon, how did you find this issue? Is this because you wished the feature or because you looked for issues with the good-first-issue tag and were willing to make your first contribution? (I ask this question to get feedback about the usefulness of this tag)

That's all with my questions. Thanks in advance 😁

[Ocarthon]

@fflorent oh i did not even consider this, but it makes sense to do it this way (especially since Libreoffice and Excel do it this way). When i initially tested in excel, i just used an empty table.

I implemented this functionality (hopefully correct :) ) but please review my changes. I don't think the function names are the best, but that was the best i did come up with.

And yes, i did this because it was tagged as a good first issue. I want to personally use grist and maybe / hopefully add more features. And instead of learning a codebase by reading it, having a concrete task atleast produces some value :)

[paulfitz]

Thanks a lot for chipping in @Ocarthon. Would you be up for writing some tests? As a model, https://github.com/gristlabs/grist-core/blob/main/test/nbrowser/SelectionSummary.ts is an example of a fairly recent test that includes selection.

[Ocarthon]

Test for Shift and Ctrl+Shift selection added :)

Please Review

[georgegevoian]

Thanks for opening the PR @Ocarthon.

Performance does take a big hit in tables with large numbers of rows. I suspect the problem is with the checking of empty cells; the selection is changed with a step value of 1, which can be expensive when there are thousands of rows needed to step through in the direction of the new selection.

One idea to try would be deferring changing the selection until the index of the first row/column containing a blank value is known. Then, you only have to make one call to change the selection, and you'd only have visited each field at most once when checking for empty values (in search of the first blank index).

[Ocarthon]

@georgegevoian Thanks for the review and suggestions!

I deferred the actual selection now to only one shift with the required step count. A crude test with ~3000 Cells went from an initial ~700ms to around 20ms. I hope this is good enough.

[georgegevoian]

@georgegevoian Thanks for the review and suggestions!

I deferred the actual selection now to only one shift with the required step count. A crude test with ~3000 Cells went from an initial ~700ms to around 20ms. I hope this is good enough.

It's a big improvement. I optimized things a bit further in the code below. It should also handle a few subtle corner cases, like when the index of the selection end is less than the start; in that scenario, shift-selecting in the opposite direction was selecting more than it should.

With the code below, I'm seeing ~90ms when selecting 1.68 million cells. If it's not too much trouble, could you copy these changes over to your branch? Thanks!

GridView.prototype._shiftSelectUntilContent = function(type, direction, selectObs, maxVal) {
  const selection = {
    colStart: this.cellSelector.col.start(),
    colEnd: this.cellSelector.col.end(),
    rowStart: this.cellSelector.row.start(),
    rowEnd: this.cellSelector.row.end(),
  };

  const steps = this._stepsToContent(type, direction, selection, maxVal);
  if (steps > 0) { this._shiftSelect(direction * steps, selectObs, type, maxVal); }
}

GridView.prototype._stepsToContent = function (type, direction, selection, maxVal) {
  const {colStart, colEnd, rowStart, rowEnd} = selection;
  let selectionData;

  if (type === selector.ROW && direction > 0) {
    if (colEnd + 1 > maxVal) { return 0; }

    selectionData = this._selectionData({colStart: colEnd, colEnd: maxVal, rowStart, rowEnd});
  } else if (type === selector.ROW && direction < 0) {
    if (colEnd - 1 < 0) { return 0; }

    selectionData = this._selectionData({colStart: 0, colEnd, rowStart, rowEnd});
  } else if (type === selector.COL && direction > 0) {
    if (rowEnd + 1 > maxVal) { return 0; }

    selectionData = this._selectionData({colStart, colEnd, rowStart: rowEnd, rowEnd: maxVal});
  } else if (type === selector.COL && direction < 0) {
    if (rowEnd - 1 > maxVal) { return 0; }

    selectionData = this._selectionData({colStart, colEnd, rowStart: 0, rowEnd});
  }

  const {fields, rowIndices} = selectionData;
  if (type === selector.ROW && direction < 0) {
    // When moving selection left, we step through fields in reverse order.
    fields.reverse();
  }
  if (type === selector.COL && direction < 0) {
    // When moving selection up, we step through rows in reverse order.
    rowIndices.reverse();
  }

  const colValuesByIndex = {};
  for (const field of fields) {
    const displayColId = field.displayColModel.peek().colId.peek();
    colValuesByIndex[field._index()] = this.tableModel.tableData.getColValues(displayColId);
  }

  let steps = 0;

  if (type === selector.ROW) {
    const hasEmptyValuesInLastCol = this._hasEmptyValuesInCol(colEnd, rowIndices, colValuesByIndex);
    const hasEmptyValuesInNextCol = this._hasEmptyValuesInCol(colEnd + direction, rowIndices, colValuesByIndex);
    const shouldStopOnEmptyValue = !hasEmptyValuesInLastCol && !hasEmptyValuesInNextCol;
    for (let i = 1; i < fields.length; i++) {
      const hasEmptyValues = this._hasEmptyValuesInCol(fields[i]._index(), rowIndices, colValuesByIndex);
      if (hasEmptyValues && shouldStopOnEmptyValue) {
        return steps;
      } else if (!hasEmptyValues && !shouldStopOnEmptyValue) {
        return steps + 1;
      }

      steps += 1;
    }
  } else {
    const hasEmptyValuesInLastRow = this._hasEmptyValuesInRow(rowIndices[0], colValuesByIndex);
    const hasEmptyValuesInNextRow = this._hasEmptyValuesInRow(rowIndices[1], colValuesByIndex);
    const shouldStopOnEmptyValue = !hasEmptyValuesInLastRow && !hasEmptyValuesInNextRow;
    for (let i = 1; i < rowIndices.length; i++) {
      const hasEmptyValues = this._hasEmptyValuesInRow(rowIndices[i], colValuesByIndex);
      if (hasEmptyValues && shouldStopOnEmptyValue) {
        return steps;
      } else if (!hasEmptyValues && !shouldStopOnEmptyValue) {
        return steps + 1;
      }

      steps += 1;
    }
  }

  return steps;
}

GridView.prototype._selectionData = function({colStart, colEnd, rowStart, rowEnd}) {
  const fields = [];
  for (let i = colStart; i <= colEnd; i++) {
    const field = this.viewSection.viewFields().at(i);
    if (!field) { continue; }

    fields.push(field);
  }

  const rowIndices = [];
  for (let i = rowStart; i <= rowEnd; i++) {
    const rowId = this.viewData.getRowId(i);
    if (!rowId) { continue; }

    rowIndices.push(this.tableModel.tableData.getRowIdIndex(rowId));
  }

  return {fields, rowIndices};
}

GridView.prototype._hasEmptyValuesInCol = function(colIndex, rowIndices, colValuesByIndex) {
  return rowIndices.some(rowIndex => {
    const value = colValuesByIndex[colIndex][rowIndex];
    return value === null || value === undefined || value === '' || value === 'false';
  });
}

GridView.prototype._hasEmptyValuesInRow = function(rowIndex, colValuesByIndex) {
  return Object.values(colValuesByIndex).some((colValues) => {
    const value = colValues[rowIndex];
    return value === null || value === undefined || value === '' || value === 'false';
  });
}

[Ocarthon]

Hey @georgegevoian, thank you for the input

Your suggestion is incorporated now. I just hat the change the empty row / col logic, as all cells have to the empty and not just atleast one.

Tbh i would have though loading all the possible cells beforehand would take a big performance hit, but i was wrong 😃

[georgegevoian]

Thanks @Ocarthon.

Just have a few more comments. Will merge once resolved.

[georgegevoian]

Hey @georgegevoian, thank you for the input

Your suggestion is incorporated now. I just hat the change the empty row / col logic, as all cells have to the empty and not just atleast one.

Tbh i would have though loading all the possible cells beforehand would take a big performance hit, but i was wrong 😃

It's a bit heavier on memory usage. There's a way to ask a column for its value at a specific row, but I suspect it's a bit slower than building the entire list of values upfront.

The changes to the logic are ok with me. I actually just checked how other products handle this, and it seems like they only consider the row/column of the cursor when checking for empty values, which is something we could do as well (and in hindsight, would've been easier for us).

@fflorent do you have a preference on how the feature should work when shift-selecting multiple rows/columns? I set up a preview of the old behavior at https://grist-ctrl-shift-select.fly.dev/ and the new behavior at https://grist-ctrl-shift-arrow.fly.dev/.

[georgegevoian]

Thanks @Ocarthon!

[fflorent]

@fflorent do you have a preference on how the feature should work when shift-selecting multiple rows/columns? I set up a preview of the old behavior at https://grist-ctrl-shift-select.fly.dev/ and the new behavior at https://grist-ctrl-shift-arrow.fly.dev/.

@georgegevoian Even if that differs from the implementation in LibreOffice Calc or Excel, both are fine for me, I don't have a strong opinion on this :).

[fflorent]

Thanks @Ocarthon BTW! :)