RESOLVED FIXED 70181
Web Inspector: Unindent edited text by pressing Shift + Tab
https://bugs.webkit.org/show_bug.cgi?id=70181
Summary Web Inspector: Unindent edited text by pressing Shift + Tab
Nikita Vasilyev
Reported 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
Attachments
First try (1.77 KB, patch)
2011-10-28 07:47 PDT, Nikita Vasilyev
pfeldman: review-
Indent/unindent multiple lines (4.17 KB, patch)
2011-11-04 22:27 PDT, Nikita Vasilyev
pfeldman: review-
Here it is (4.36 KB, patch)
2011-11-05 15:01 PDT, Nikita Vasilyev
no flags
Nikita Vasilyev
Comment 1 2011-10-28 07:47:52 PDT
Created attachment 112862 [details] First try
Pavel Feldman
Comment 2 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.
Nikita Vasilyev
Comment 3 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
Nikita Vasilyev
Comment 4 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.
Pavel Feldman
Comment 5 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.
Nikita Vasilyev
Comment 6 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.
Pavel Feldman
Comment 7 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.
Pavel Feldman
Comment 8 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.
Nikita Vasilyev
Comment 9 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."?
Pavel Feldman
Comment 10 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.
Nikita Vasilyev
Comment 11 2011-11-05 15:01:38 PDT
Created attachment 113762 [details] Here it is
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2011-11-06 00:23:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.