Bug 70181 – Web Inspector: Unindent edited text by pressing Shift + Tab

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

[Nikita Vasilyev]

Would be nice if Shift + Tab unindent a current edited line. Currently, it does nothing http://www.screenr.com/Vy0s

[Nikita Vasilyev]

Created attachment 112862 [details]
First try

[Pavel Feldman]

Comment on attachment 112862 [details]
First try

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

> Source/WebCore/inspector/front-end/TextViewer.js:1062
> +            range.startColumn -= WebInspector.settings.textEditorIndent.get().length;

This can go negative, no?

> Source/WebCore/inspector/front-end/TextViewer.js:1063
> +            var newRange = this._setText(range, "");

Unindent should only be available for indented line.

[Nikita Vasilyev]

Created attachment 113751 [details]
Indent/unindent multiple lines

It should fix https://bugs.webkit.org/show_bug.cgi?id=70182 as well.

Screencast: http://www.screenr.com/KgZs

[Nikita Vasilyev]

Comment on attachment 113751 [details]
Indent/unindent multiple lines

>+                lineIndentLength = line.match(new RegExp("^ {1," + indent.length +"}"))[0].length;

I should have moved the regexp out of the loop.

[Pavel Feldman]

Comment on attachment 113751 [details]
Indent/unindent multiple lines

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

> Source/WebCore/inspector/front-end/TextViewer.js:1065
> +            if (range.isEmpty()) {

Lets apply indent when there is multiline selection only (Eclipse model). I.e.
if (range.linesCount())
    newRange = this.indentLines(range);
else
    ...

> Source/WebCore/inspector/front-end/TextViewer.js:1079
> +    indentLines: function(range)

Should be called _indentLines (since is private to this file).

> Source/WebCore/inspector/front-end/TextViewer.js:1083
> +        if (this._lastEditedRange)

It looks like you should set lastEditedRange to the newRange in the end of this method.

> Source/WebCore/inspector/front-end/TextViewer.js:1096
> +    unindentLines: function(range)

_unindentLines

> Source/WebCore/inspector/front-end/TextViewer.js:1099
> +            this._textModel.markUndoableState();

Same comment on the undoable state, should set lastEditedRange in the end.

> Source/WebCore/inspector/front-end/TextViewer.js:1105
> +        for (var lineNumber = range.startLine; lineNumber <= range.endLine; lineNumber++) {

Again, I would use Eclipse model here: go through lines, check whether line starts with indent. If all lines do, trim them. Otherwise do nothing.

[Nikita Vasilyev]

(In reply to comment #5)
> (From update of attachment 113751 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113751&action=review
> 
> > Source/WebCore/inspector/front-end/TextViewer.js:1065
> > +            if (range.isEmpty()) {
> 
> Lets apply indent when there is multiline selection only (Eclipse model). I.e.

I've implemented a model used by IntelliJ IDEA. I prefer it over Eclipse one. I indent a line very often, but I can hardly imagine a need to insert an indent in the middle of the line.

Also, Eclipse does indent a single line when, and only when, its fully selected. Otherwise it replaces selected text with an indent. I've found it a bit confusing.

> > Source/WebCore/inspector/front-end/TextViewer.js:1105
> > +        for (var lineNumber = range.startLine; lineNumber <= range.endLine; lineNumber++) {
> 
> Again, I would use Eclipse model here: go through lines, check whether line starts with indent. If all lines do, trim them. Otherwise do nothing.

Seems like only Eclipse does that. I have nothing agains it though, so I can do it.

[Pavel Feldman]

> Seems like only Eclipse does that. I have nothing agains it though, so I can do it.

I have no strong preference for Eclipse over IDEA, I just want us to be consistent and to stick to either of them. I just checked Cloud 9 and they seem to do what you suggest. Let me go through your change once again.

[Pavel Feldman]

Comment on attachment 113751 [details]
Indent/unindent multiple lines

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

>>> Source/WebCore/inspector/front-end/TextViewer.js:1065
>>> +            if (range.isEmpty()) {
>> 
>> Lets apply indent when there is multiline selection only (Eclipse model). I.e.
>> if (range.linesCount())
>>     newRange = this.indentLines(range);
>> else
>>     ...
> 
> I've implemented a model used by IntelliJ IDEA. I prefer it over Eclipse one. I indent a line very often, but I can hardly imagine a need to insert an indent in the middle of the line.
> 
> Also, Eclipse does indent a single line when, and only when, its fully selected. Otherwise it replaces selected text with an indent. I've found it a bit confusing.

Sounds good, lets keep it isEmpty

>>> Source/WebCore/inspector/front-end/TextViewer.js:1105
>>> +        for (var lineNumber = range.startLine; lineNumber <= range.endLine; lineNumber++) {
>> 
>> Again, I would use Eclipse model here: go through lines, check whether line starts with indent. If all lines do, trim them. Otherwise do nothing.
> 
> Seems like only Eclipse does that. I have nothing agains it though, so I can do it.

Again, sounds good, lets leave the behavior you suggest.

> Source/WebCore/inspector/front-end/TextViewer.js:1111
> +                lineIndentLength = line.match(new RegExp("^ {1," + indent.length +"}"))[0].length;

Consider the following case:
WebInspector.settings.textEditorIndent === "\t" -> indent.length === 1

Following line is selected:
\s\s\s\s\tFoo

You'll remove just one space from the first line. It sounds like we need to define tab size for that. I am fine with setting it to 4 for now.

[Nikita Vasilyev]

> >>> Source/WebCore/inspector/front-end/TextViewer.js:1105
> >>> +        for (var lineNumber = range.startLine; lineNumber <= range.endLine; lineNumber++) {
> >> 
> >> Again, I would use Eclipse model here: go through lines, check whether line starts with indent. If all lines do, trim them. Otherwise do nothing.
> > 
> > Seems like only Eclipse does that. I have nothing agains it though, so I can do it.
> 
> Again, sounds good, lets leave the behavior you suggest.

I'm not clear on this one. Both solutions are fine to me. Should we keep the behavior I've already implemented or "go through lines, check whether line starts with indent. If all lines do, trim them. Otherwise do nothing."?

[Pavel Feldman]

> I'm not clear on this one. Both solutions are fine to me. Should we keep the behavior I've already implemented or "go through lines, check whether line starts with indent. If all lines do, trim them. Otherwise do nothing."?

Go ahead and attach it then.

[Nikita Vasilyev]

Created attachment 113762 [details]
Here it is

[WebKit Review Bot]

Comment on attachment 113762 [details]
Here it is

Clearing flags on attachment: 113762

Committed r99372: <http://trac.webkit.org/changeset/99372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.