Bug 71625 – Web Inspector: Preserve an indentation level when inserting a new line

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Nikita Vasilyev]

http://www.screenr.com/PZZs

Next logical step would be inserting close bracket when the open one is typed. I.e.

[Nikita Vasilyev]

Created attachment 113782 [details]
First try

Discussion has been started in https://bugs.webkit.org/show_bug.cgi?id=69986#c6

As you can see on 50th second of http://www.screenr.com/PZZs it doesn't yet work for:

if (obj.y)
    return 42

I think it requires some static analyses that could be done some day.


Source/WebCore/inspector/front-end/TextViewer.js:
-        if (handler && handler.call(this)) {
+        if (handler && handler()) {

All handlers are bound, no need for "call" here.

[Pavel Feldman]

I have no problems with the code, except for it introduces complexity. Adding Andrey who has been hacking the editing code, maybe he suggests something simpler.

[Nikita Vasilyev]

There is some copy/paste between handleTabKeyPress and _insertNewLine:

        if (this._readOnly || this._dirtyLines)
            return false;

        var selection = this._getSelection();
        if (!selection)
            return false;

        this.beginUpdates();
        this._enterTextChangeMode();

        var range = selection;
        if (range.startLine > range.endLine || (range.startLine === range.endLine && range.startColumn > range.endColumn))
            range = new WebInspector.TextRange(range.endLine, range.endColumn, range.startLine, range.startColumn);

        ...

        this._exitTextChangeMode(range, newRange);
        this.endUpdates();
        this._restoreSelection(newRange, true);

We might want to refactor it.

[Pavel Feldman]

> We might want to refactor it.

Would you suggest a patch doing that?

[Nikita Vasilyev]

Created attachment 113961 [details]
Refactoring

We used to revert selection range twice. From now on a selection start point is always ≤ the end point:

handleTabKeyPress:
-        if (range.startLine > range.endLine || (range.startLine === range.endLine && range.startColumn > range.endColumn))
-            range = new WebInspector.TextRange(range.endLine, range.endColumn, range.startLine, range.startColumn);

_getSelection:
-        if (selection.anchorNode === selectionRange.startContainer && selection.anchorOffset === selectionRange.startOffset)
-            return new WebInspector.TextRange(start.line, start.column, end.line, end.column);
-        else
-            return new WebInspector.TextRange(end.line, end.column, start.line, start.column);
+        return new WebInspector.TextRange(start.line, start.column, end.line, end.column);

None of the shortcuts work in readOnly mode, so I've moved readOnly check into _handleKeyDown.

I haven't simplified the rest. I don't think I can.

[Andrey Adaikin]

Comment on attachment 113961 [details]
Refactoring

View in context: https://bugs.webkit.org/attachment.cgi?id=113961&action=review

> Source/WebCore/inspector/front-end/TextViewer.js:278
> +        this._enterTextChangeMode();

this is a method of the TextEditorMainPanel class. did you mean to put the _insertNewLine() in that class instead of TextViewer? And IMO, it should be, nearby with the handleTab, handleUndo, and etc.

> Source/WebCore/inspector/front-end/TextViewer.js:280
> +        var lineBreak = this._textModel._lineBreak;

accessing a private property "_lineBreak" of an object. introduce a getter for this?

> Source/WebCore/inspector/front-end/TextViewer.js:311
> +        var insertNewLine = this._insertNewLine.bind(this._mainPanel);

ditto. move the new method to this._mainPanel, to be consistent with the code around?

> Source/WebCore/inspector/front-end/TextViewer.js:327
> +        if (this._mainPanel._readOnly)

if (this.readOnly)

> Source/WebCore/inspector/front-end/TextViewer.js:-1057
> -        var range = selection;

plz revert, see below

> Source/WebCore/inspector/front-end/TextViewer.js:-1422
> -        if (selection.anchorNode === selectionRange.startContainer && selection.anchorOffset === selectionRange.startOffset)

plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top). This change will break the text selection by scrolling from bottom to top - I don't think we have a test for that :(

repro steps:
- select a long enough script, so that vertical scroll bar appears (or decrease the height of DevTools window)
- go to the bottom of the document
- left mouse click on any place at the bottom and start scrolling the document to the top (and selecting the text) by moving the mouse up
- observe the text selection's end line&column changes while scrolling to the top

would you mind putting a brief comment about this here? just to prevent folks from doing the same thing.

[Nikita Vasilyev]

Thanks for the review!

(In reply to comment #6)
> plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top).

I see it's broken, but I don't understand why. Would you mind pointing me to the code that changes selection on scrolling?

[Andrey Adaikin]

(In reply to comment #7)
> Thanks for the review!
> 
> (In reply to comment #6)
> > plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top).
> 
> I see it's broken, but I don't understand why. Would you mind pointing me to the code that changes selection on scrolling?

Yeah, the this._restoreSelection() gets broken. Namely, the method window.getSelection().setBaseAndExtent() - it accepts an anchor node as a first argument, and a focus node as a second. In the test case I mentioned, we would call it with the anchor and focus nodes swapped, thus the restored selection would start from the topmost node instead of the one at the bottom that we started from.

We can introduce a new class WebInspector.TextSelection and provide getters for anchorLine and focusLine along with startLine, endLine and etc. and force start <= end. This will remove the code duplication you were trying to deal with.

[Nikita Vasilyev]

(In reply to comment #8)
> We can introduce a new class WebInspector.TextSelection and provide getters for anchorLine and focusLine along with startLine, endLine and etc. and force start <= end. This will remove the code duplication you were trying to deal with.

Looks like it could replace WebInspector.TextRange entirely, isn't it?

[Andrey Adaikin]

(In reply to comment #9)
> (In reply to comment #8)
> > We can introduce a new class WebInspector.TextSelection and provide getters for anchorLine and focusLine along with startLine, endLine and etc. and force start <= end. This will remove the code duplication you were trying to deal with.
> 
> Looks like it could replace WebInspector.TextRange entirely, isn't it?

We also had this idea before, but it was dropped due to the fact that TextRange is not a selection, and it does not have to know about "focus" and "anchor" terms.

[Nikita Vasilyev]

Created attachment 114328 [details]
Trying out WebInspector.TextSelection. Work in progress!

I've created WebInspector.TextSelection. It allows us to save selection anchor/focus when editing (e.g., indenting http://www.screenr.com/Rcks). Unfortunately, it adds even more complexity. I'm not sure I'm on a right track.

[Nikita Vasilyev]

(In reply to comment #6)

> plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top). This change will break the text selection by scrolling from bottom to top - I don't think we have a test for that :(
> 
> repro steps:
> - select a long enough script, so that vertical scroll bar appears (or decrease the height of DevTools window)
> - go to the bottom of the document
> - left mouse click on any place at the bottom and start scrolling the document to the top (and selecting the text) by moving the mouse up
> - observe the text selection's end line&column changes while scrolling to the top

Fixed in the last patch.

[Andrey Adaikin]

Comment on attachment 114328 [details]
Trying out WebInspector.TextSelection. Work in progress!

View in context: https://bugs.webkit.org/attachment.cgi?id=114328&action=review

> Source/WebCore/inspector/front-end/TextEditorModel.js:82
> +        return this.anchorLine < this.focusLine ? this.anchorLine : this.focusLine;

return Math.min(this.anchorLine, this.focusLine);

> Source/WebCore/inspector/front-end/TextEditorModel.js:85
> +    set startLine(lineNumber)

Can you get rid of these setters? It is quite confusing having them. For example, if you call this setter, the subsequent call to the getter may not return the same value.

> Source/WebCore/inspector/front-end/TextEditorModel.js:92
> +        return this.anchorLine > this.focusLine ? this.anchorLine : this.focusLine;

return Math.max(this.anchorLine, this.focusLine);

> Source/WebCore/inspector/front-end/TextEditorModel.js:95
> +    set endLine(lineNumber)

ditto (remove?)

> Source/WebCore/inspector/front-end/TextEditorModel.js:106
> +    set startColumn(lineNumber)

ditto (remove?)

> Source/WebCore/inspector/front-end/TextEditorModel.js:119
> +    set endColumn(lineNumber)

ditto (remove?)

> Source/WebCore/inspector/front-end/TextEditorModel.js:126
> +    get linesCount()

seems like a dead code. if so, plz remove

> Source/WebCore/inspector/front-end/TextViewer.js:285
> +        if (this._mainPanel._readOnly)

this.readOnly

> Source/WebCore/inspector/front-end/TextViewer.js:1123
> +            if (lineNumber === selection.startLine)

to get rid of the setters (see above):

if (lineNumber === selection.anchorLine)
    newSelection.anchorColumn = Math.max(0, newSelection.anchorColumn - lineIndentLength);
if (lineNumber === selection.focusLine)
    newSelection.focusColumn = Math.max(0, newSelection.focusColumn - lineIndentLength);

> Source/WebCore/inspector/front-end/TextViewer.js:1128
> +            newSelection.endColumn = Math.max(0, newSelection.endColumn - lineIndentLength);

remove this, if you apply my comment above

> Source/WebCore/inspector/front-end/TextViewer.js:1130
> +        this._lastEditedRange = newSelection;

_lastEditedRange -> _lastEditedSelection

> Source/WebCore/inspector/front-end/TextViewer.js:1154
> +

should we also add:
  if (this._lastEditedRange)
        this._textModel.markUndoableState();
and update the lastEditedRange at the end of this method?

> Source/WebCore/inspector/front-end/TextViewer.js:1162
> +            newSelection.anchorLine = --newSelection.focusLine;

plz do one operation per one line:

--newSelection.focusLine;
newSelection.focusColumn += textEditorIndent.length;
newSelection.anchorLine = newSelection.focusLine;
newSelection.anchorColumn = newSelection.focusColumn;

this is easier to understand

> Source/WebCore/inspector/front-end/TextViewer.js:1475
> +        var start = this._positionToSelection(range.anchorLine, range.anchorColumn);

range -> selection

[Nikita Vasilyev]

Created attachment 114606 [details]
Remove TextSelection, add methods to TextRange.

I don't like the previous patch. Setters were ugly. _lastEditedRange could be either TextSelection and TextRange.

I do like the new one! I've got rid of TextSelection. Two times less classes, two times less complexity :)

[Nikita Vasilyev]

By the way, should I use JSDoc comments? Most of the methods don't have them, but some do.

[Andrey Adaikin]

Comment on attachment 114606 [details]
Remove TextSelection, add methods to TextRange.

View in context: https://bugs.webkit.org/attachment.cgi?id=114606&action=review

> Source/WebCore/inspector/front-end/TextEditorModel.js:39
> +WebInspector.TextRange = function(startLine, startColumn, endLine, endColumn, reversed)

I had implemented this in the same way long ago. The (design) problem is that TextRange should not know about "anchor" and "focus" terms, because those are the terms of a selection.

[Nikita Vasilyev]

(In reply to comment #16)
> (From update of attachment 114606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114606&action=review
> 
> > Source/WebCore/inspector/front-end/TextEditorModel.js:39
> > +WebInspector.TextRange = function(startLine, startColumn, endLine, endColumn, reversed)
> 
> I had implemented this in the same way long ago. The (design) problem is that TextRange should not know about "anchor" and "focus" terms, because those are the terms of a selection.

Let's call it TextSelection then?

I see the following problems with having both TextSelection and TextRange:

1. Convert a selection to a range before every _setText call.

2. WebInspector.TextEditorMainPanel._restoreSelection has to use "anchor" and "focus". I don't want to do something like:

if (range instanceof WebInspector.TextSelection)
    var selection = range;
else
    selection = range.toTextSelection();

[Andrey Adaikin]

AFAIU, TextRange has nothing to do neither with text selection nor with text selection range (i.e. window.getSelection() and window.getSelection().getRangeAt()). It just stands for a range of a text in the text model. Thus I suggest that we should not make it aware of anything related to the text selection. In other words, IMO this CL should not affect TextEditorModel.js at all.

Maybe it could be done simply by introducing a static method like "normalizeTextRange" to ensure start <= end for a range?

Maybe Pavel will chime in?

[Nikita Vasilyev]

(In reply to comment #18)
> AFAIU, TextRange has nothing to do neither with text selection nor with text selection range (i.e. window.getSelection() and window.getSelection().getRangeAt()). It just stands for a range of a text in the text model. Thus I suggest that we should not make it aware of anything related to the text selection. In other words, IMO this CL should not affect TextEditorModel.js at all.
> 
> Maybe it could be done simply by introducing a static method like "normalizeTextRange" to ensure start <= end for a range?

It would make http://www.screenr.com/Rcks impossible. Even though it's a minor issue, I would rather keep it working.

[Nikita Vasilyev]

Created attachment 115789 [details]
Add normalize method

Andrey, I did as you said, except:

> Maybe it could be done simply by introducing a static method like "normalizeTextRange" to ensure start <= end for a range?

Why static? I've made it instance method.

[Nikita Vasilyev]

Can I have a review, please? It is unpleasant to edit JS without this feature.

[Pavel Feldman]

Andrey is on vacations, I'll pick it up. Did you fix everything he mentioned?

[Pavel Feldman]

Comment on attachment 115789 [details]
Add normalize method

View in context: https://bugs.webkit.org/attachment.cgi?id=115789&action=review

Looks good with a request for clarification on {} and a nit. Also, it would be nice to unindent upon closing bracket in case we produced the auto-indent (could be done in a separate change).

> Source/WebCore/inspector/front-end/TextEditorModel.js:53
> +    collapseToEnd: function()

I would keep the text range immutable and return the new object from these methods.

> Source/WebCore/inspector/front-end/TextEditorModel.js:116
> +    get lineBreak() {

{ should be on the next line.

> Source/WebCore/inspector/front-end/TextViewer.js:1060
> +        range.normalize();

I.e. that would be var range = selection.normalize(); here.

> Source/WebCore/inspector/front-end/TextViewer.js:-1059
> -            range = new WebInspector.TextRange(range.endLine, range.endColumn, range.startLine, range.startColumn);

As it actually was.

> Source/WebCore/inspector/front-end/TextViewer.js:1168
> +            newRange = this._setText(range, lineBreak + indent + lineBreak + currentIndent);

What's happening here? Could you comment on this case?

> Source/WebCore/inspector/front-end/TextViewer.js:1471
> +        return new WebInspector.TextRange(start.line, start.column, end.line, end.column);

So the selection is no longer normalized. It seems like restoreSelection still handles it ok.

[Nikita Vasilyev]

Created attachment 117028 [details]
Immutable TextRange

(In reply to comment #23)
> > Source/WebCore/inspector/front-end/TextViewer.js:1168
> > +            newRange = this._setText(range, lineBreak + indent + lineBreak + currentIndent);
> 
> What's happening here? Could you comment on this case?

I've added a comment. Also, see http://www.screenr.com/PZZs at 0:04

[Pavel Feldman]

Comment on attachment 117028 [details]
Immutable TextRange

View in context: https://bugs.webkit.org/attachment.cgi?id=117028&action=review

> Source/WebCore/inspector/front-end/TextEditorModel.js:63
> +            return new WebInspector.TextRange(this.startLine, this.startColumn, this.endLine, this.endColumn);

"return this;" would work given that we consider range immutable.

> Source/WebCore/inspector/front-end/TextViewer.js:1168
> +            // {|}

I would expect
function foo() {|}

to become

function foo() {
    |
}

will it work?

[Nikita Vasilyev]

Created attachment 117149 [details]
Simplify normalize

(In reply to comment #25)
> (From update of attachment 117028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117028&action=review
> 
> > Source/WebCore/inspector/front-end/TextEditorModel.js:63
> > +            return new WebInspector.TextRange(this.startLine, this.startColumn, this.endLine, this.endColumn);
> 
> "return this;" would work given that we consider range immutable.

Done.

> > Source/WebCore/inspector/front-end/TextViewer.js:1168
> > +            // {|}
> 
> I would expect
> function foo() {|}
> 
> to become
> 
> function foo() {
>     |
> }
> 
> will it work?

Sure it will.

[Pavel Feldman]

Comment on attachment 117149 [details]
Simplify normalize

Thanks for doing this.

[WebKit Review Bot]

Comment on attachment 117149 [details]
Simplify normalize

Clearing flags on attachment: 117149

Committed r101511: <http://trac.webkit.org/changeset/101511>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.