Would be nice if Shift + Tab unindent a current edited line. Currently, it does nothing http://www.screenr.com/Vy0s
Created attachment 112862 [details] First try
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.
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 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 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.
(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.
> 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 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.
> >>> 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."?
> 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.
Created attachment 113762 [details] Here it is
Comment on attachment 113762 [details] Here it is Clearing flags on attachment: 113762 Committed r99372: <http://trac.webkit.org/changeset/99372>
All reviewed patches have been landed. Closing bug.