Bug 70181 - Web Inspector: Unindent edited text by pressing Shift + Tab
: Web Inspector: Unindent edited text by pressing Shift + Tab
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nikita Vasilyev
:
Depends on: 69986
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-15 14:11 PDT by Nikita Vasilyev
Modified: 2011-11-06 00:23 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2011-10-15 14:11:27 PDT
Would be nice if Shift + Tab unindent a current edited line. Currently, it does nothing http://www.screenr.com/Vy0s
Comment 1 Nikita Vasilyev 2011-10-28 07:47:52 PDT
Created attachment 112862 [details]
First try
Comment 2 Pavel Feldman 2011-10-31 06:53:30 PDT
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.
Comment 3 Nikita Vasilyev 2011-11-04 22:27:21 PDT
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
Comment 4 Nikita Vasilyev 2011-11-04 22:44:43 PDT
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.
Comment 5 Pavel Feldman 2011-11-05 08:58:14 PDT
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.
Comment 6 Nikita Vasilyev 2011-11-05 10:55:08 PDT
(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.
Comment 7 Pavel Feldman 2011-11-05 11:22:26 PDT
> 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 Pavel Feldman 2011-11-05 11:32:14 PDT
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.
Comment 9 Nikita Vasilyev 2011-11-05 13:12:48 PDT
> >>> 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 Pavel Feldman 2011-11-05 13:52:21 PDT
> 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 Nikita Vasilyev 2011-11-05 15:01:38 PDT
Created attachment 113762 [details]
Here it is
Comment 12 WebKit Review Bot 2011-11-06 00:23:33 PDT
Comment on attachment 113762 [details]
Here it is

Clearing flags on attachment: 113762

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