Bug 70181 - Web Inspector: Unindent edited text by pressing Shift + Tab
: Web Inspector: Unindent edited text by pressing Shift + Tab
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 69986
:
  Show dependency treegraph
 
Reported: 2011-10-15 14:11 PST by
Modified: 2011-11-06 00:23 PST (History)


Attachments
First try (1.77 KB, patch)
2011-10-28 07:47 PST, Nikita Vasilyev
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
Indent/unindent multiple lines (4.17 KB, patch)
2011-11-04 22:27 PST, Nikita Vasilyev
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
Here it is (4.36 KB, patch)
2011-11-05 15:01 PST, Nikita Vasilyev
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-15 14:11:27 PST
Would be nice if Shift + Tab unindent a current edited line. Currently, it does nothing http://www.screenr.com/Vy0s
------- Comment #1 From 2011-10-28 07:47:52 PST -------
Created an attachment (id=112862) [details]
First try
------- Comment #2 From 2011-10-31 06:53:30 PST -------
(From update of attachment 112862 [details])
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.
------- Comment #3 From 2011-11-04 22:27:21 PST -------
Created an attachment (id=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
------- Comment #4 From 2011-11-04 22:44:43 PST -------
(From update of attachment 113751 [details])
>+                lineIndentLength = line.match(new RegExp("^ {1," + indent.length +"}"))[0].length;

I should have moved the regexp out of the loop.
------- Comment #5 From 2011-11-05 08:58:14 PST -------
(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.
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.
------- Comment #6 From 2011-11-05 10:55:08 PST -------
(In reply to comment #5)
> (From update of attachment 113751 [details] [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.
------- Comment #7 From 2011-11-05 11:22:26 PST -------
> 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.
------- Comment #8 From 2011-11-05 11:32:14 PST -------
(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.
>> 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.
------- Comment #9 From 2011-11-05 13:12:48 PST -------
> >>> 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."?
------- Comment #10 From 2011-11-05 13:52:21 PST -------
> 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.
------- Comment #11 From 2011-11-05 15:01:38 PST -------
Created an attachment (id=113762) [details]
Here it is
------- Comment #12 From 2011-11-06 00:23:33 PST -------
(From update of attachment 113762 [details])
Clearing flags on attachment: 113762

Committed r99372: <http://trac.webkit.org/changeset/99372>
------- Comment #13 From 2011-11-06 00:23:38 PST -------
All reviewed patches have been landed.  Closing bug.