Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

[cmv4] Implement Tab key heuristics for multiple selections #7153

Merged
merged 2 commits into from
Mar 11, 2014

Conversation

njx
Copy link

@njx njx commented Mar 11, 2014

For #7022 - see the discussion in that issue as well as the comment at the beginning of Editor._handleTabKey() for a description of the exact heuristics involved.

Note that pretty much all the existing logic was rewritten, but the unit tests that were added in the first commit should verify that none of the existing desired behavior was broken in any of the single-select cases (aside from tweaks that were made to the existing behavior, as described in the issue thread).

Also note that I didn't change one bit of behavior that would be good to fix eventually: if you're immediately before the first non-whitespace character in a line and the line is improperly indented, hitting Tab just inserts an indent rather than autoindenting to the correct location. This is because we just rely on running CM's autoindent code in this case, and we don't want the autoindent to take effect if the line is already indented past the "correct" indentation - we want to just keep inserting tabs in that case. If we had a way to know whether we were past the "correct" indent level, we could fix this, but there isn't a good way to do that without implementing autoindent ourselves (which we could do by copying some of CM's code).

Narciso Jaramillo added 2 commits March 10, 2014 14:49
…election is a range in the middle of the line to insert tab/spaces at beginning instead of replacing selection.
@njx
Copy link
Author

njx commented Mar 11, 2014

@redmunds - also @dangoor might want to play around with this to see how it feels.

@redmunds
Copy link
Contributor

This seems like a bug:

  1. Select some whitespace at beginning of line (doesn't happen mid-line)
  2. Make a second selection that contains a newline (may contain part of line of first selection or not)
  3. Hit Tab

Result:
Indenting seems to be correct, but the first selection collapses to a cursor

Expected
All selections are maintained

@redmunds
Copy link
Contributor

This doesn't seem right, although behavior is the same as master:

  1. Select several full lines as a single, multiline selection
  2. Hit Tab

Results:
All lines remain completely selected except the first line. The selection of the first line starts with the first non-whitespace char regardless of whether first line had leading whitespace selected or not.

Expected:
All lines remain as complete line selections

@njx
Copy link
Author

njx commented Mar 11, 2014

Both of those are due to the behavior of CodeMirror's built-in indent logic: if either end of a selection is in the whitespace at the beginning of a line, it moves that end forward to the first non-whitespace char. This is useful when the selection is just a cursor in the whitespace, though it arguably makes more sense when we're doing smart autoindent as opposed to just inserting an indent level at the beginning of each line. (I wrote a comment about this behavior at Editor-test.js:1559.)

For reference, in the second case ST just inserts the whitespace before the beginning of the selection, so the start of the range gets pushed forward by the insertion (but doesn't move to the beginning of the non-whitespace content).

It wouldn't be that hard to replace CM's logic in this case, and make it so we keep the selection as the whole line if the range starts at position 0, but otherwise just insert the whitespace before the start of the selection (as ST does) and adjust the selection forward the appropriate number of characters. That said, the current behavior doesn't bother me all that much.

@njx
Copy link
Author

njx commented Mar 11, 2014

(Actually, to be clearer - I'd be inclined to leave this alone for now, and maybe file this as a separate bug to address later after cmv4 lands; if we change this behavior, we'd want to redo the general Indent/Unindent behavior as well, since those have the same behavior by virtue of using the CM algorithm.)

@redmunds
Copy link
Contributor

Looks good. Not 100% sure that all questions about behavior were resolved, but this is cmv4 branch, so merging.

redmunds added a commit that referenced this pull request Mar 11, 2014
[cmv4] Implement Tab key heuristics for multiple selections
@redmunds redmunds merged commit e94aabb into cmv4 Mar 11, 2014
@redmunds redmunds deleted the nj/issue-7022 branch March 11, 2014 21:10
@njx
Copy link
Author

njx commented Mar 11, 2014

OK - we can see if we get feedback from users on some of these behaviors.

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

Successfully merging this pull request may close these issues.

2 participants