http://www.screenr.com/PZZs Next logical step would be inserting close bracket when the open one is typed. I.e.
Created attachment 113782 [details] First try Discussion has been started in https://bugs.webkit.org/show_bug.cgi?id=69986#c6 As you can see on 50th second of http://www.screenr.com/PZZs it doesn't yet work for: if (obj.y) return 42 I think it requires some static analyses that could be done some day. Source/WebCore/inspector/front-end/TextViewer.js: - if (handler && handler.call(this)) { + if (handler && handler()) { All handlers are bound, no need for "call" here.
I have no problems with the code, except for it introduces complexity. Adding Andrey who has been hacking the editing code, maybe he suggests something simpler.
There is some copy/paste between handleTabKeyPress and _insertNewLine: if (this._readOnly || this._dirtyLines) return false; var selection = this._getSelection(); if (!selection) return false; this.beginUpdates(); this._enterTextChangeMode(); var range = selection; if (range.startLine > range.endLine || (range.startLine === range.endLine && range.startColumn > range.endColumn)) range = new WebInspector.TextRange(range.endLine, range.endColumn, range.startLine, range.startColumn); ... this._exitTextChangeMode(range, newRange); this.endUpdates(); this._restoreSelection(newRange, true); We might want to refactor it.
> We might want to refactor it. Would you suggest a patch doing that?
Created attachment 113961 [details] Refactoring We used to revert selection range twice. From now on a selection start point is always ≤ the end point: handleTabKeyPress: - if (range.startLine > range.endLine || (range.startLine === range.endLine && range.startColumn > range.endColumn)) - range = new WebInspector.TextRange(range.endLine, range.endColumn, range.startLine, range.startColumn); _getSelection: - if (selection.anchorNode === selectionRange.startContainer && selection.anchorOffset === selectionRange.startOffset) - return new WebInspector.TextRange(start.line, start.column, end.line, end.column); - else - return new WebInspector.TextRange(end.line, end.column, start.line, start.column); + return new WebInspector.TextRange(start.line, start.column, end.line, end.column); None of the shortcuts work in readOnly mode, so I've moved readOnly check into _handleKeyDown. I haven't simplified the rest. I don't think I can.
Comment on attachment 113961 [details] Refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=113961&action=review > Source/WebCore/inspector/front-end/TextViewer.js:278 > + this._enterTextChangeMode(); this is a method of the TextEditorMainPanel class. did you mean to put the _insertNewLine() in that class instead of TextViewer? And IMO, it should be, nearby with the handleTab, handleUndo, and etc. > Source/WebCore/inspector/front-end/TextViewer.js:280 > + var lineBreak = this._textModel._lineBreak; accessing a private property "_lineBreak" of an object. introduce a getter for this? > Source/WebCore/inspector/front-end/TextViewer.js:311 > + var insertNewLine = this._insertNewLine.bind(this._mainPanel); ditto. move the new method to this._mainPanel, to be consistent with the code around? > Source/WebCore/inspector/front-end/TextViewer.js:327 > + if (this._mainPanel._readOnly) if (this.readOnly) > Source/WebCore/inspector/front-end/TextViewer.js:-1057 > - var range = selection; plz revert, see below > Source/WebCore/inspector/front-end/TextViewer.js:-1422 > - if (selection.anchorNode === selectionRange.startContainer && selection.anchorOffset === selectionRange.startOffset) plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top). This change will break the text selection by scrolling from bottom to top - I don't think we have a test for that :( repro steps: - select a long enough script, so that vertical scroll bar appears (or decrease the height of DevTools window) - go to the bottom of the document - left mouse click on any place at the bottom and start scrolling the document to the top (and selecting the text) by moving the mouse up - observe the text selection's end line&column changes while scrolling to the top would you mind putting a brief comment about this here? just to prevent folks from doing the same thing.
Thanks for the review! (In reply to comment #6) > plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top). I see it's broken, but I don't understand why. Would you mind pointing me to the code that changes selection on scrolling?
(In reply to comment #7) > Thanks for the review! > > (In reply to comment #6) > > plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top). > > I see it's broken, but I don't understand why. Would you mind pointing me to the code that changes selection on scrolling? Yeah, the this._restoreSelection() gets broken. Namely, the method window.getSelection().setBaseAndExtent() - it accepts an anchor node as a first argument, and a focus node as a second. In the test case I mentioned, we would call it with the anchor and focus nodes swapped, thus the restored selection would start from the topmost node instead of the one at the bottom that we started from. We can introduce a new class WebInspector.TextSelection and provide getters for anchorLine and focusLine along with startLine, endLine and etc. and force start <= end. This will remove the code duplication you were trying to deal with.
(In reply to comment #8) > We can introduce a new class WebInspector.TextSelection and provide getters for anchorLine and focusLine along with startLine, endLine and etc. and force start <= end. This will remove the code duplication you were trying to deal with. Looks like it could replace WebInspector.TextRange entirely, isn't it?
(In reply to comment #9) > (In reply to comment #8) > > We can introduce a new class WebInspector.TextSelection and provide getters for anchorLine and focusLine along with startLine, endLine and etc. and force start <= end. This will remove the code duplication you were trying to deal with. > > Looks like it could replace WebInspector.TextRange entirely, isn't it? We also had this idea before, but it was dropped due to the fact that TextRange is not a selection, and it does not have to know about "focus" and "anchor" terms.
Created attachment 114328 [details] Trying out WebInspector.TextSelection. Work in progress! I've created WebInspector.TextSelection. It allows us to save selection anchor/focus when editing (e.g., indenting http://www.screenr.com/Rcks). Unfortunately, it adds even more complexity. I'm not sure I'm on a right track.
(In reply to comment #6) > plz revert. we have to handle the selection's anchorNode being after the selection's focusNode (i.e. when scrolling from bottom to top). This change will break the text selection by scrolling from bottom to top - I don't think we have a test for that :( > > repro steps: > - select a long enough script, so that vertical scroll bar appears (or decrease the height of DevTools window) > - go to the bottom of the document > - left mouse click on any place at the bottom and start scrolling the document to the top (and selecting the text) by moving the mouse up > - observe the text selection's end line&column changes while scrolling to the top Fixed in the last patch.
Comment on attachment 114328 [details] Trying out WebInspector.TextSelection. Work in progress! View in context: https://bugs.webkit.org/attachment.cgi?id=114328&action=review > Source/WebCore/inspector/front-end/TextEditorModel.js:82 > + return this.anchorLine < this.focusLine ? this.anchorLine : this.focusLine; return Math.min(this.anchorLine, this.focusLine); > Source/WebCore/inspector/front-end/TextEditorModel.js:85 > + set startLine(lineNumber) Can you get rid of these setters? It is quite confusing having them. For example, if you call this setter, the subsequent call to the getter may not return the same value. > Source/WebCore/inspector/front-end/TextEditorModel.js:92 > + return this.anchorLine > this.focusLine ? this.anchorLine : this.focusLine; return Math.max(this.anchorLine, this.focusLine); > Source/WebCore/inspector/front-end/TextEditorModel.js:95 > + set endLine(lineNumber) ditto (remove?) > Source/WebCore/inspector/front-end/TextEditorModel.js:106 > + set startColumn(lineNumber) ditto (remove?) > Source/WebCore/inspector/front-end/TextEditorModel.js:119 > + set endColumn(lineNumber) ditto (remove?) > Source/WebCore/inspector/front-end/TextEditorModel.js:126 > + get linesCount() seems like a dead code. if so, plz remove > Source/WebCore/inspector/front-end/TextViewer.js:285 > + if (this._mainPanel._readOnly) this.readOnly > Source/WebCore/inspector/front-end/TextViewer.js:1123 > + if (lineNumber === selection.startLine) to get rid of the setters (see above): if (lineNumber === selection.anchorLine) newSelection.anchorColumn = Math.max(0, newSelection.anchorColumn - lineIndentLength); if (lineNumber === selection.focusLine) newSelection.focusColumn = Math.max(0, newSelection.focusColumn - lineIndentLength); > Source/WebCore/inspector/front-end/TextViewer.js:1128 > + newSelection.endColumn = Math.max(0, newSelection.endColumn - lineIndentLength); remove this, if you apply my comment above > Source/WebCore/inspector/front-end/TextViewer.js:1130 > + this._lastEditedRange = newSelection; _lastEditedRange -> _lastEditedSelection > Source/WebCore/inspector/front-end/TextViewer.js:1154 > + should we also add: if (this._lastEditedRange) this._textModel.markUndoableState(); and update the lastEditedRange at the end of this method? > Source/WebCore/inspector/front-end/TextViewer.js:1162 > + newSelection.anchorLine = --newSelection.focusLine; plz do one operation per one line: --newSelection.focusLine; newSelection.focusColumn += textEditorIndent.length; newSelection.anchorLine = newSelection.focusLine; newSelection.anchorColumn = newSelection.focusColumn; this is easier to understand > Source/WebCore/inspector/front-end/TextViewer.js:1475 > + var start = this._positionToSelection(range.anchorLine, range.anchorColumn); range -> selection
Created attachment 114606 [details] Remove TextSelection, add methods to TextRange. I don't like the previous patch. Setters were ugly. _lastEditedRange could be either TextSelection and TextRange. I do like the new one! I've got rid of TextSelection. Two times less classes, two times less complexity :)
By the way, should I use JSDoc comments? Most of the methods don't have them, but some do.
Comment on attachment 114606 [details] Remove TextSelection, add methods to TextRange. View in context: https://bugs.webkit.org/attachment.cgi?id=114606&action=review > Source/WebCore/inspector/front-end/TextEditorModel.js:39 > +WebInspector.TextRange = function(startLine, startColumn, endLine, endColumn, reversed) I had implemented this in the same way long ago. The (design) problem is that TextRange should not know about "anchor" and "focus" terms, because those are the terms of a selection.
(In reply to comment #16) > (From update of attachment 114606 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114606&action=review > > > Source/WebCore/inspector/front-end/TextEditorModel.js:39 > > +WebInspector.TextRange = function(startLine, startColumn, endLine, endColumn, reversed) > > I had implemented this in the same way long ago. The (design) problem is that TextRange should not know about "anchor" and "focus" terms, because those are the terms of a selection. Let's call it TextSelection then? I see the following problems with having both TextSelection and TextRange: 1. Convert a selection to a range before every _setText call. 2. WebInspector.TextEditorMainPanel._restoreSelection has to use "anchor" and "focus". I don't want to do something like: if (range instanceof WebInspector.TextSelection) var selection = range; else selection = range.toTextSelection();
AFAIU, TextRange has nothing to do neither with text selection nor with text selection range (i.e. window.getSelection() and window.getSelection().getRangeAt()). It just stands for a range of a text in the text model. Thus I suggest that we should not make it aware of anything related to the text selection. In other words, IMO this CL should not affect TextEditorModel.js at all. Maybe it could be done simply by introducing a static method like "normalizeTextRange" to ensure start <= end for a range? Maybe Pavel will chime in?
(In reply to comment #18) > AFAIU, TextRange has nothing to do neither with text selection nor with text selection range (i.e. window.getSelection() and window.getSelection().getRangeAt()). It just stands for a range of a text in the text model. Thus I suggest that we should not make it aware of anything related to the text selection. In other words, IMO this CL should not affect TextEditorModel.js at all. > > Maybe it could be done simply by introducing a static method like "normalizeTextRange" to ensure start <= end for a range? It would make http://www.screenr.com/Rcks impossible. Even though it's a minor issue, I would rather keep it working.
Created attachment 115789 [details] Add normalize method Andrey, I did as you said, except: > Maybe it could be done simply by introducing a static method like "normalizeTextRange" to ensure start <= end for a range? Why static? I've made it instance method.
Can I have a review, please? It is unpleasant to edit JS without this feature.
Andrey is on vacations, I'll pick it up. Did you fix everything he mentioned?
Comment on attachment 115789 [details] Add normalize method View in context: https://bugs.webkit.org/attachment.cgi?id=115789&action=review Looks good with a request for clarification on {} and a nit. Also, it would be nice to unindent upon closing bracket in case we produced the auto-indent (could be done in a separate change). > Source/WebCore/inspector/front-end/TextEditorModel.js:53 > + collapseToEnd: function() I would keep the text range immutable and return the new object from these methods. > Source/WebCore/inspector/front-end/TextEditorModel.js:116 > + get lineBreak() { { should be on the next line. > Source/WebCore/inspector/front-end/TextViewer.js:1060 > + range.normalize(); I.e. that would be var range = selection.normalize(); here. > Source/WebCore/inspector/front-end/TextViewer.js:-1059 > - range = new WebInspector.TextRange(range.endLine, range.endColumn, range.startLine, range.startColumn); As it actually was. > Source/WebCore/inspector/front-end/TextViewer.js:1168 > + newRange = this._setText(range, lineBreak + indent + lineBreak + currentIndent); What's happening here? Could you comment on this case? > Source/WebCore/inspector/front-end/TextViewer.js:1471 > + return new WebInspector.TextRange(start.line, start.column, end.line, end.column); So the selection is no longer normalized. It seems like restoreSelection still handles it ok.
Created attachment 117028 [details] Immutable TextRange (In reply to comment #23) > > Source/WebCore/inspector/front-end/TextViewer.js:1168 > > + newRange = this._setText(range, lineBreak + indent + lineBreak + currentIndent); > > What's happening here? Could you comment on this case? I've added a comment. Also, see http://www.screenr.com/PZZs at 0:04
Comment on attachment 117028 [details] Immutable TextRange View in context: https://bugs.webkit.org/attachment.cgi?id=117028&action=review > Source/WebCore/inspector/front-end/TextEditorModel.js:63 > + return new WebInspector.TextRange(this.startLine, this.startColumn, this.endLine, this.endColumn); "return this;" would work given that we consider range immutable. > Source/WebCore/inspector/front-end/TextViewer.js:1168 > + // {|} I would expect function foo() {|} to become function foo() { | } will it work?
Created attachment 117149 [details] Simplify normalize (In reply to comment #25) > (From update of attachment 117028 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117028&action=review > > > Source/WebCore/inspector/front-end/TextEditorModel.js:63 > > + return new WebInspector.TextRange(this.startLine, this.startColumn, this.endLine, this.endColumn); > > "return this;" would work given that we consider range immutable. Done. > > Source/WebCore/inspector/front-end/TextViewer.js:1168 > > + // {|} > > I would expect > function foo() {|} > > to become > > function foo() { > | > } > > will it work? Sure it will.
Comment on attachment 117149 [details] Simplify normalize Thanks for doing this.
Comment on attachment 117149 [details] Simplify normalize Clearing flags on attachment: 117149 Committed r101511: <http://trac.webkit.org/changeset/101511>
All reviewed patches have been landed. Closing bug.