Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: the cursor cannot exit the table using up and down arrow keys #975

Closed
wants to merge 0 commits into from

Conversation

asjqkkkk
Copy link
Contributor

Before:

2024-11-28.210148.mp4

Aftre:

2024-11-28.210409.mp4

At present, it has only resolved the issue of the cursor jumping out with the up and down arrow keys; there are other issues, such as: using arrow keys to move from other nodes into a table causing the cursor to disappear. These may need to be addressed through other pull requests.

@CLAassistant
Copy link

CLAassistant commented Nov 28, 2024

CLA assistant check
All committers have signed the CLA.

? selection.start.offset
: target.delta!.length;
try {
final nextNode = _getNextNode(inTableNodes, 0, -1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use _getPreviousNode instead of _getNextNode. Also, it would be better to refactor the function _getPreviousNode to add named parameters.

Node? _getPreviousNode(
  Iterable<Node> nodes, {
  int colDiff = 0,
  int rowDiff = 0,
}) {}
final previousNodeInSameColumn = _getPreviousNode(
      inTableNodes,
      rowDiff: 1,
    );
CommandShortcutEventHandler _upInTableCellHandler = (editorState) {
  final inTableNodes = _inTableNodes(editorState);
  final selection = editorState.selection;
  if (selection == null ||
      !_hasSelectionAndTableCell(inTableNodes, selection)) {
    return KeyEventResult.ignored;
  }

  final previousNodeInSameColumn = _getPreviousNode(
    inTableNodes,
    rowDiff: 1,
  );
  if (previousNodeInSameColumn != null &&
      _nodeHasTextChild(previousNodeInSameColumn)) {
    final target = previousNodeInSameColumn.childAtIndexOrNull(0);
    final delta = target?.delta;
    if (target == null || delta == null) {
      return KeyEventResult.ignored;
    }
    final offset = delta.length.clamp(
      0,
      selection.start.offset,
    );
    editorState.selectionService.updateSelection(
      Selection.single(
        path: target.path,
        startOffset: offset,
      ),
    );
  }

  return KeyEventResult.handled;
};

final off = target.delta!.length > selection!.start.offset
? selection.start.offset
: target.delta!.length;
try {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please help remove force-unwrap operators (!) as well.

@LucasXu0
Copy link
Collaborator

Hi, @asjqkkkk. Please check the CI. You need to format the code.

@asjqkkkk
Copy link
Contributor Author

Hi @LucasXu0 , I've changed _getNextNode to _getPreviousNode in _upInTableCellHandler, and refactor the function _getPreviousNode to add named parameters

The direction operations of the table have four: up, down, left, and right, and basically, each move will only shift by one cell. The parameters [colDiff+rowDiff] are like x and y coordinates that can represent the four directions, so a single method might be sufficient to obtain the next Node. In this case, the [getPreviousNode] method seems a bit redundant. Additionally, if we were to use [getNextNode and getPreviousNode], there should theoretically be some input restrictions on the parameters [colDiff+rowDiff], such as they cannot be negative numbers. However, if we add these restrictions, it seems unnecessary to use the two int values of [colDiff+rowDiff]. Although [colDiff+rowDiff] can cleverly express directions, it increases the difficulty of reading the code, and it does not impose any input restrictions. In comparison, I might prefer to use an enumeration method to express directions.

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Dec 1, 2024

@asjqkkkk Agreed. Using an enum can improve readability. I have another suggestion: using getCellAtPosition to get the cell from the table. I was refactoring the table code and using these 4 functions to get the previous/next focusable node.

https://github.com/AppFlowy-IO/AppFlowy/pull/6831/files#diff-d3499f42f1d6695407a5ac49d5f3d47ef66979a9db5a03540f80c55eeac0ada4R352

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

Attention: Patch coverage is 65.11628% with 15 lines in your changes missing coverage. Please review.

Project coverage is 72.17%. Comparing base (09f4b17) to head (3e9557f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...omponent/table_block_component/table_commands.dart 65.11% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #975      +/-   ##
==========================================
- Coverage   72.24%   72.17%   -0.07%     
==========================================
  Files         318      318              
  Lines       15127    15152      +25     
==========================================
+ Hits        10928    10936       +8     
- Misses       4199     4216      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants