This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Repository navigation
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
[cmv4] Odd indent behavior with multiple cursors #7022
Description
Perhaps this is to be expected, but it's not what I expected.
To reproduce:
- Have at least one cursor in the middle of a line
- Add a cursor at the beginning of the line
- Press tab
- Four spaces added appropriately in the middles of the lines and at the beginning of a line
- Press the left arrow key
- All of the cursors are now back where they were with the 4 spaces following
- Press tab
- The two middle of the line cursors have no change, the cursor at the beginning of a line has the indentation removed
The beginning of the line behavior is consistent with pressing the tab key in the single select case. The middle of the line behavior is different because in the single select case another four spaces would be added.
Activity
njx commentedon Feb 27, 2014
I think in order to reproduce your steps, you have to do step 2 first and then step 1 (i.e., first cursor is at the left edge, second cursor is in the middle). The issue is that our indent code is only really looking at the primary selection in order to figure out what to do. There's a similar issue around soft tabs, which is partly related: #7002.
I agree that this is pretty low priority since it's probably not a really common case, but while fixing #7002 I can see if there's an obvious fix for this as well.
dangoor commentedon Feb 28, 2014
I definitely did it in the order listed... I had 2 cursors in the middles of lines and then added a 3rd at the beginning of another line.
If this proves to be non-trivial to fix, I think we just close it because it is such an edge case.
njx commentedon Mar 6, 2014
Oops, forgot to look at this with #7002 :) I'll look at it now.
njx commentedon Mar 6, 2014
Oh, I know why your case is different...the line that you added the cursor at the beginning of must have had no indentation to begin with.
njx commentedon Mar 7, 2014
This is generally tricky for the same reason that #7002 is tricky; the CM commands we currently use to indent operate on all the selections, not on an individual selection. However, for some cases where it makes sense, we could do the individual operations ourselves.
I'm thinking that (similar to #7002) it's better to generally apply the same operation to all selections anyway, rather than try to treat each selection individually, because it will be less confusing. So, my proposal is:
njx commentedon Mar 7, 2014
I'm thinking of modifying this slightly from the existing behavior. Currently, if you have a multi-line range selected, we add an indent level to the beginning of each line. However, if you have a range within a single line selected, and it's in the middle of the content, we replace the range with a tab (or spaces). That behavior has always felt a little weird to me (and the comments in
Editor._handleTab()suggest that it should only be doing that if the selection is a cursor rather than a range, though they don't specify what the desired behavior is in the range case). I'm tempted to make it so that it inserts a tab/spaces at the beginning of the selection rather than replacing it. (In other words, we would treat it like case 2 above, but with the insertion being at the beginning of the range.)@dangoor / @redmunds - any thoughts on this case?
redmunds commentedon Mar 8, 2014
I've always expected that behavior. I think it's more consistent to always indent (as opposed to replace in some cases).
njx commentedon Mar 8, 2014
@redmunds Ah ok - just to be clear, you mean that you would expect my proposed new behavior?
redmunds commentedon Mar 8, 2014
@njx Yes, I have always expected your new proposed behavior.
redmunds commentedon Mar 11, 2014
If I select a range in a single line and hit Tab, then an indent is inserted before the range, which is expected. But, if I hit Shift-Tab, then indent is removed from beginning of line -- I expected it to be removed from immediately before the range (same place it's inserted). This is useful when using Alt-drag to select a box of text, and then use Tab/Shift-Tab to move it to the desired indent level within some other text.
njx commentedon Mar 11, 2014
That makes sense, though it's new behavior. Does this sound reasonable for the behavior of Shift-Tab if a range is selected in the middle of a line:
(a) if there is a tab immediately before the selection, delete it
(b) otherwise do the standard Shift-Tab behavior (unindent whole line)
(a) if there are spaces before the range, delete backwards to the next virtual tab stop, or to the content, whichever is closer
(b) otherwise do the standard Shift-Tab behavior (unindent whole line)
2a is kind of like our original soft tab behavior for backspace in the middle of the line (which we removed). One issue is what we should do if there are multiple selections - do we look at each one individually and handle it separately (so you might get different deletion amounts for each selection), or do we find the minimal deletion across all selections? It gets kind of complicated (which is one reason I wanted to get rid of it originally :))
redmunds commentedon Mar 11, 2014
(Oops -- my comment was supposed to be in PR).
This only applies to a selection which does not contain a newline. Original comment was only about a range in a line, but may also apply to a cursor.
I, personally, think Shift-Tab should only remove indents between selection and previous non-whitespace char. This means stop when there are no more indents to remove, and don't remove indents from beginning of line. I don't like the idea of Shift-Tab removing indents from beginning of line when selection is in middle of line because Tab will never insert an indent at beginning of line (when selection is within line).
For multi-selection with different amounts of whitespace before each selection, I think each selection should be handled separately. This is how it works if you select a bunch of lines of text with different indention -- you keep hitting Shift-Tab and eventually everything will be left-aligned (each line is handed separately).
njx commentedon Mar 11, 2014
That makes sense (as long as the range is not in the whitespace at the beginning of a line). It was a little weird for Delete to do different things on each selection, but I think it's less weird for Shift-Tab.
If you're using spaces, and the space before one of the selections is less than a tab stop, should it just delete up to the previous content? That sounds right, although I bet in most cases you want to keep one space there...but then you can just type the space afterwards.
peterflynn commentedon Mar 11, 2014
Are the above statements intended to apply for the degenerate case of a cursor, or just to a selected range? For cursors, I think it's fairly common to have the cursor in the midst of the line's text when you hit Shift+Tab to unindent. Having it try to remove mid-line whitespace and no-op failing that would feel like a notable regression, IMO. But if we don't do that, will it fell weird for the behavior to differ for a cursor vs. a range?
dangoor commentedon Mar 11, 2014
+1 to what @peterflynn said. I'll add that since we changed
cmd-[andcmd-]to not be indent/outdent (a key combo I didn't realize my brain was programmed to use until we changed it), there would no longer be a way to change the indentation of a line when you're in the middle of the line.njx commentedon Mar 11, 2014
@peterflynn Good point. Also, if we make it work on cursors, but people are still expecting the Shift-Tab-to-unindent-line behavior, then they'll get weird cases where if they happen to be after a single space, it will delete the space instead of unindenting.
I'm tempted to say that we shouldn't change the Shift-Tab behavior now, since (even before these changes) Tab would insert a tab in the middle of a line and Shift-Tab wouldn't delete it, so it's not a new "inconsistency". @redmunds are you ok with that?
@dangoor - not sure what you mean; Cmd-[ and Cmd-] still seem to be indent/outdent in my build...maybe you have an extension that's conflicting?
redmunds commentedon Mar 11, 2014
@njx I'm ok with that.
dangoor commentedon Mar 11, 2014
@njx Oh, you're right. I didn't realize I had an extension doing that. That's definitely an argument for Brackets to know which things came from which extensions...
redmunds commentedon Mar 11, 2014
FBNC to @dangoor
njx commentedon Mar 11, 2014
Oops, I found a regression with my changes: if the cursor is at the beginning of a blank line, we used to autoindent to the correct level, but now we just insert a single indent. PR coming shortly.
njx commentedon Mar 12, 2014
OK, that last issue is fixed, so should be ready to FBNC again.
dangoor commentedon Mar 13, 2014
OK, the behavior is consistent now between the cursors in the middle of the line and the cursors at the beginning of the line.