I think that we need not use same shortcut key on all platforms. We should use a suited keyset for each platform.

Rob Campbell [:rc] (:robcee)

Reporter

Comment 14

•

12 years ago

I can chime in here with a few opinions: Ctrl-Space would be nice, but is apparently used on some systems for context menus. Alt-Space I don't know about, but probably not as comfortable or convenient as Ctrl. Cmd-Space (Mac Only) is Spotlight. ESC is what Xcode uses. It's a little unusual, and I've never gotten used to using that as the Dictionary lookup key in cocoa text widgets (like, Mail.app). Visual Studio does autocompletion automatically. I like the automatic suggestion idea. We could even begin autocompleting after the first character if we have some notion of current scope. We could complete variables and globals visible at the current level.

Paul Rouget [:paul]

Comment 15

•

12 years ago

What about just using Tab? If the character just before the cursor is a letter/number/underscore (\w) or a dot (\.), then you stop the propagation of the key event and start the completion.

Paul Rouget [:paul]

Comment 16

•

12 years ago

Here is the completion code I use in my jsterm extension: https://github.com/paulrouget/firefox-jsterm/blob/master/chrome/jsterm.js#L500

Tetsuharu OHZEKI [:tetsuharu] (UTC+9)

Comment 17

•

12 years ago

(In reply to Paul Rouget [:paul] from comment #16) > Here is the completion code I use in my jsterm extension: > https://github.com/paulrouget/firefox-jsterm/blob/master/chrome/jsterm. > js#L500 I seem Paul's jsterm completion is good as one of the solution. Full word completion with popup (like IDE) is more smart and typing will be easy. jsterm's completion is very simple. This may be more useful for some case than introducing the completion popup and invoke key for showing it.

Thaddee Tyl [:espadrine]

Comment 18

•

12 years ago

Ohzeki: Those completions are not exclusive. I really like Paul's system (which is close to zsh autocompletion) and I will implement something very similar. The great pro that zsh autocompletion features is modeless completion. This means that each key behaves exactly the same whatever the context. As a result, what I will implement is: - When we press tab in the right context, the popup appears. - Resuming typing makes it go away. This ensures that there is an easy way to dismiss the popup. Typing "Esc" should do that, too. - When we press tab twice, it starts cycling through all possible completions, and inserts the first possibility. - Typing shift+tab cycles back. - If there is a huge number of possible completions, the user will probably want to scroll through them, and click on the correct one when he finds it. This inserts the corresponding completion. All this was about UX. As for the system which provides autocompletion, I am going for a stack of "levels" of autocompletion. Each time we summon completion, it goes up the stack to gather all possible completions. 1. The first level (of lowest priority, which appears below all others) uses the same completion as the webconsole. Pros: it is dynamic and comprehensive (well, it will be comprehensive after the patch for bug 775812 [1] lands, and the patch for bug 774753 [2] gets created and lands ;) Cons: it doesn't see what isn't executed. In the scratchpad, we tend to write a bunch of code without executing it at first, so this is limited. 2. The second level (of highest priority) statically reads local and global variables in the script, using the Esprima parser (with some error tolerance that I have yet to write), and suggests completions based on that. Pros: it doesn't need to run the code. Cons: the Esprima parser doesn't run if there are too many errors. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=775812 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=774753

Rob Campbell [:rc] (:robcee)

Reporter

Comment 19

•

12 years ago

WIP. Filter on BLACKEAGLE?!

Assignee: nobody → thaddee.tyl

Status: NEW → ASSIGNED

Thaddee Tyl [:espadrine]

Comment 20

•

Attachment #653587 - Flags: review?(rcampbell)

Attachment #653587 - Flags: review?(mihai.sucan)

Victor Porof [:vporof][:vp]

Comment 35

•

12 years ago

Comment on attachment 653587 [details] [diff] [review] Autocompletion in the scratchpad. Review of attachment 653587 [details] [diff] [review]: ----------------------------------------------------------------- Robcee asked me to give you some feedback. So here I am. I have a quick glance and I may have missed some stuff. Feel free to ignore everything I say :) This is cool. Overall the functionality is pretty fluid indeed. The fact that there's a popup displayed on each keypress is something I really like. UX-wise, sometimes I was a bit confused on what key I should press in order to hide the popup. Here's some examples: * Autocompletion is case sensitive. I think it'd be a good idea to have the matching as fuzzy as possible, especially starting with making it case insensitive (I may be biased because I'm a terrible typer, trading precision for speed). Ideally, when I do a foo.BCAN i'd like it to show me "BEER_CAN_SOUNDS_LIKE_BACON" and "BREAKPOINT_CHANGE", but I can settle just for "window.com" showing "Components" instead of nothing. * Typing "window.get" and pressing TAB would autocomplete to "getComputedStyle" but still display the popup. I'd then need to type ESC or another key to hide it. I also think there's some duplicated behavior here. Generally you'd go to your preferred completion using the UP and DOWN keys, press TAB to complete and hide the popup. Currently TAB functions exactly like pressing UP and DOWN, which seems redundant. The fact that the initial entry in the popupbox is selected automatically is very good imo, but I feel like TAB should just autocomplete on what's currently selected and close the popup. * This was said previously in this bug, but I'll mention it too: the fact that UP/DOWN automatically changes the entered string with what's in the popup is not something I'd like to happen. UP/DOWN should just jump to the next element in the popup, and TAB would do one thing only: finish autocompleting and hide the popup. There's a certain degree of "instinct" built in when dealing with how autocompletion popups work, and while your proposed interaction is actually pretty damn fine, it'll require people detaching from what they learned to trust so far. For example my sentiment was something like "Ok, DOWN, DOWN... here it is. TAB. Oh? Um.. No, not that. DOWN, DOWN, DOWN. ESC. Hmm, ok then, I guess this is ok..". Here's a concrete example: - "typed windows.co" - pressed DOWN till I reached "controllers" - pressed TAB (by reflex) - the selection jumped to the first entry. That's definitely not what I was expecting. My intention was "write that and hide the popup", the effect was "select a different thing and don't hide the popup" which is the exact opposite :) * When only one completion option is available, the popup looks like a collapsed dropdown. Will the Orion update help us with this? *** There are a few places in which things aren't working as expected like trying to get properties of a global defined in a lazy getter (ex: without ever opening the Debugger, writing "DebuggerUI." gives me a "Parsing error: missing name after . operator"). I'm not sure if there's a followup for it, but it seems that the statical analysis is not finished yet. If I had a source containing "var foo = { bar: 42 }" doing "foo." wouldn't display anything in the popup. Same goes for arrays. You'd have to eval the source first. While this is generally ok, I think the Parser API is super powerful and we should make full use of it especially in these cases. One last thing: when relying on eval for the global scope or current context (or even when using the Parser API before any initial eval), having a bit more information than just the autocompletion string in the popup would be really awesome to have. For example, when displaying the function name in the popup, you could also show the params in on the right, when dealing with primitives (numbers, strings etc.) showing their value, or const if it's the case. This would be trivial to add, just regexp your way around the evaled values or dive deep into the abstract syntax tree from the Parser API. Take a look into the gist from comment #3, here's a screen: http://tinyurl.com/9gehfbn Can totally follow up for all of this. ::: browser/devtools/scratchpad/scratchpad.js @@ +409,5 @@ > + getSandbox: function SP_getSandbox() > + { > + // Evaluate the sandbox one to get all the autocompletion we can. > + this.evalForContext(""); > + return this.executionContext == SCRATCHPAD_CONTEXT_CONTENT ? The name of this function is a bit misleading. Either getSandboxForAutocompletion here (implying an automatic evalForContext beforehand), or just leave it as getSandbox and do a this.evalForContext("") manually where necessary. I'd go with the second option. ::: browser/devtools/scratchpad/test/browser_scratchpad_bug_660560_tab.js @@ +40,4 @@ > > EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow); > > + is(sp.getText(), " window.foo; ", Pressing TAB inside of a string tries to autocomplete? Ok, I think that's ok. ::: browser/devtools/scratchpad/test/browser_scratchpad_bug_762164_autocompletion.js @@ +41,5 @@ > + EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow); > + > + let popover = sp.editor._autocomplete.popover; > + is(popover.style.display, "block", > + "Autocompletion of window.F gives a popover"); Aren't you displaying the popup after a timeout(,0)? You're probably going to get into race conditions if that's the case. Fire an event to handle the autocompletion popover showing/hiding. ::: browser/devtools/sourceeditor/autocompletion.jsm @@ +16,5 @@ > + * It starts invisible (display: none). > + * > + * @param object document > + * The DOM document in which to add a popover. > + * Nit: don't add newlines between @params in comments. @@ +23,5 @@ > + * > + * @return object popover > + * The DOM element representing the popover. > + */ > +function createPopover(document, cssClass) { function createPopover(document, cssClass = "autocomplete") { Up to you. @@ +45,5 @@ > + * The Orion editor to target. > + * ed._mode must be "js". > + * ed._view must be a TextView. > + * ed._model must be a TextModel. > + * ed._undoStack must be an UndoStack. :) @@ +46,5 @@ > + * ed._mode must be "js". > + * ed._view must be a TextView. > + * ed._model must be a TextModel. > + * ed._undoStack must be an UndoStack. > + * ed.editorElement must be the XUL iframe containing the editor. Those are a lot of assertions. Maybe test for those in the constructor and throw an Error if something's not right. @@ +50,5 @@ > + * ed.editorElement must be the XUL iframe containing the editor. > + * > + * @param object options > + * Properties for tuning certain defaults: > + * - nbVisibleCompletions (defaults to 10): number of visible Nit: how about num instead of nb? It took me 4 seconds to figure out what nb means. @@ +58,5 @@ > + * - getSandbox (defaults to null): function to get a sandbox environment > + * to evaluate expressions for dynamic autocompletion purposes. > + */ > +function Autocompletion(ed, options = {}) { > + let cssClass = options.cssClass || "autocomplete"; ..and you could remove the || "autocomplete" here. @@ +59,5 @@ > + * to evaluate expressions for dynamic autocompletion purposes. > + */ > +function Autocompletion(ed, options = {}) { > + let cssClass = options.cssClass || "autocomplete"; > + this.nbVisibleCompletions = options.nbVisibleCompletions || 10; My comment about the magic 10 number applies here too. If you'll do something about it, make sure to update the comment for the constructor param. @@ +66,5 @@ > + this.document = ed.editorElement.contentDocument; > + this.popover = createPopover(this.document, cssClass); > + > + // The following will fire the autocompletion system on each character! > + this.editor.editorElement.addEventListener("keypress", function(e) { I'd avoid creating functions in the constructor. I think it's also a good practice to have the number of addEventListener equal to removeEventListener. Why not move this function in the prototype, and bind the |this| before adding it as a listener? @@ +68,5 @@ > + > + // The following will fire the autocompletion system on each character! > + this.editor.editorElement.addEventListener("keypress", function(e) { > + if (!this._insertingText) { > + if (e.keyCode == 13) { e.DOM_VK_RETURN instead of 13. @@ +71,5 @@ > + if (!this._insertingText) { > + if (e.keyCode == 13) { > + this._fireStaticAnalysis = true; > + } > + if (e.charCode > 0) { Nit: if (e.charCode) @@ +72,5 @@ > + if (e.keyCode == 13) { > + this._fireStaticAnalysis = true; > + } > + if (e.charCode > 0) { > + this.document.defaultView.setTimeout(this.displayCompletion.bind(this),0); setTimeout is the devil. But sometimes the devil is the only option. However, I think I understand the reasoning of waiting for a short delay before showing and populating the popup. In that case, why not use a longer delay? I don't think anyone would want such an incredibly snappy popup flashing around each time I type. Try out a timeout of 50ms (and put it in a const if it feels right). If that's not necessary, how about Services.tm.currentThread.dispatch({ run: yourFuncHere }, 1); instead of the timeout(,0)? @@ +76,5 @@ > + this.document.defaultView.setTimeout(this.displayCompletion.bind(this),0); > + } > + } > + }.bind(this)); > + this.editor.addEventListener("Selection", function(e) { Just like with keypress, move it in the prototype. And removeEventListener when unneeded. @@ +78,5 @@ > + } > + }.bind(this)); > + this.editor.addEventListener("Selection", function(e) { > + // If the distance between the old position and the new is bigger than 1, > + // the static analysis is worth updating. Heh, this is hacky. Why are you doing this on selection events? Not saying this is bad, just wanting to understand what you're trying to do. @@ +94,5 @@ > +Autocompletion.prototype = { > + > + // The following set the way in which the autocompletion system works. > + getSandbox: null, > + nbVisibleCompletions: 10, :) @@ +112,5 @@ > + _end: 0, > + _insertingText: false, > + _completion: null, > + // Static analysis would better be cached. > + _staticCandidates: null, Are the completions uncached when not present in the source code anymore? This concern is somewhat related to the selection events that trigger static analysis. @@ +145,5 @@ > + // The first child is a <select> element. > + let html = "<select>"; > + for (let i = 0; i < completions.length; i++) { > + // The first option gets selected by default. > + html += "<option " + (i == this._index? "selected='true'": "") + ">" + Nit: whitespace before ? and : @@ +149,5 @@ > + html += "<option " + (i == this._index? "selected='true'": "") + ">" + > + completions[i] + "</option>"; > + } > + html += "</select>"; > + this.popover.innerHTML = html; I'd avoid using innerHTML, I'm sure document.createElement would work better. Just make sure you're using the correct namespace. @@ +151,5 @@ > + } > + html += "</select>"; > + this.popover.innerHTML = html; > + // Track clicking on options. > + this.popover.firstChild.addEventListener("click", function() { You're creating a function each time when displaying the popover. And if you're going to do this on each key press, the gc may be stressed out a bit (just a bit). Again, can you move this definition in the prototype? @@ +160,5 @@ > + } > + this.editor._view.focus(); > + }.bind(this), false); > + this.popover.firstChild.size = > + Math.min(this.nbVisibleCompletions, completions.length); max-height and a scrollbar maybe? @@ +165,5 @@ > + > + // Positioning the popover. > + let coord = ed._view.getLocationAtOffset(ed._view.getCaretOffset()); > + coord.y += ed._view.getLineHeight(); > + ed._view.convert(coord, "document", "page"); Instead of all this getting of locations, caret offsets and convertions, would using a tooltip tied to the editor caret (by calling openPopup with an anchor) work better? You can make a custom tooltip item and add whatever you want to it: https://developer.mozilla.org/en-US/docs/XUL/tooltip On the other hand, maybe this is just trying to make things too elegant. @@ +180,5 @@ > + for (; line + " " !== linesOM[lineIndex].textContent; lineIndex++) {} > + coord.y = insideDoc.defaultView.scrollY + > + linesOM[lineIndex].getBoundingClientRect().top + > + linesOM[lineIndex].scrollHeight; > + // End of Bug 779642 fix. Custom tooltips may render this unneeded. If you have the time, it could be a good idea to try it out. @@ +189,5 @@ > + let dwidth = this.document.documentElement.clientWidth; > + let dheight = this.document.documentElement.clientHeight; > + if (coord.y + this.popover.offsetHeight > dheight) { > + this.popover.style.top = (coord.y - this.popover.offsetHeight + > + - ed._view.getLineHeight()) + "px"; Nit: + - How about just - @@ +211,5 @@ > + }, > + > + // Specific autocompletion-only keys. > + _keyBindings: function AC_keyBindings(e) { > + if (e.keyCode == 27) { // ESC key. e.DOM_VK_ESCAPE @@ +217,5 @@ > + if (this._cycling) { > + this.editor.setText("", this._start, this._end); > + } > + e.stopPropagation(); > + } else if (e.keyCode == 38) { // ↑ key. e.DOM_VK_UP @@ +223,5 @@ > + this.cycle(-1); > + e.stopPropagation(); > + e.preventDefault(); > + } > + } else if (e.keyCode == 40) { // ↓ key. e.DOM_VK_DOWN @@ +349,5 @@ > + * Autocompletion happens based on the following factors > + * (with increasing relevance): > + * > + * Level 0 = JS keywords. > + * Level 1 = dynamic lookup of available properties. Describe Level 2 here as well. @@ +385,5 @@ > + } > + let allStaticCandidates = this._staticCandidates; > + // Right now, we can only complete variables. > + if (identifier.indexOf(".") == -1 && identifier.indexOf("[") == -1 && > + // Thou shalt only complete what there is to complete. Nit: funky comment line in the middle of a conditional expression. Add it after { or before the if. @@ +398,5 @@ > + } > + staticCandidates.sort(function(a, b) { > + // Sort them according to nearest scope. > + return allStaticCandidates.get(b) - allStaticCandidates.get(a); > + }); This is cool. What would be even more awesome is to colorize or somehow delimit the candidates based on the scope. Just a suggestion, follow up for this if you think it's a good idea. @@ +400,5 @@ > + // Sort them according to nearest scope. > + return allStaticCandidates.get(b) - allStaticCandidates.get(a); > + }); > + candidates = candidates.concat(staticCandidates); > + completions = completions.concat(staticCandidates.map(function(candidate){ Nit: space before { @@ +414,5 @@ > + candidates = candidates.concat(sandboxCompletion.matches > + .filter(function(candidate) { > + // We are removing candidates from level 2. > + if (allStaticCandidates == null) return true; > + if (allStaticCandidates.has(candidate)) { Useless if. Are you planning on adding things here later? @@ +434,5 @@ > + "default", "delete", "do", "else", "export", "false", "finally", "for", > + "function", "get", "if", "import", "in", "instanceof", "let", "new", > + "null", "of", "return", "set", "super", "switch", "this", "true", "throw", > + "try", "typeof", "undefined", "var", "void", "while", "with", > + ]; Did you get those from https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Reserved_Words ? Seems that "each" is missing (maybe some other stuff too). I wouldn't worry too much about it though. @@ +435,5 @@ > + "function", "get", "if", "import", "in", "instanceof", "let", "new", > + "null", "of", "return", "set", "super", "switch", "this", "true", "throw", > + "try", "typeof", "undefined", "var", "void", "while", "with", > + ]; > + // This autocompletion is not meaningful when we type a property… Hmm, why? I'd like to have autocompletion when writing foo["bar @@ +437,5 @@ > + "try", "typeof", "undefined", "var", "void", "while", "with", > + ]; > + // This autocompletion is not meaningful when we type a property… > + if (identifier.indexOf(".") == -1 && identifier.indexOf("[") == -1 && > + // Thou shalt only complete what there is to complete. Again funky comment line in the middle of a conditional expression. @@ +537,5 @@ > + // Parameters are one level deeper than the function's name itself. > + argumentNames(subnode.params, store, stack.length + 1); > + } > + } > + deeper = nestedNodes(subnode, line, column); Should probably add tests for the supported subnode types too. @@ +551,5 @@ > + if (!deeper) { > + node = stack.pop(); > + index = indices.pop(); > + } > + } while (stack.length > 0 || (node && index < node.length) || !!deeper); This while loop is pretty dense. Probably a recursive approach would simplify things a bit. Not sure about the performance implications of that though. @@ +640,5 @@ > + */ > +function argumentNames(node, store, weight) { > + for (let i = 0; i < node.length; i++) { > + store.set(node[i].name, weight); > + } I didn't manage to make these show up in the completion popup. If they're working, should probably add a test for them too. @@ +654,5 @@ > + COMMENT : 4, // comment > + > + // Relevant character codes. > + // May be used as states. > + ETHER : 0, // ∅ A bit hacky. Probably no real-world implications though. @@ +779,5 @@ > +function isWhiteSpace(c) { > + return (c === 32) || (c === 0x0009) || (c === 0x000B) || > + (c === 0x000C) || (c === 0x00A0) || > + (c >= 0x1680 && > + [0x1680,0x180E,0x2000,0x2001,0x2002,0x2003,0x2004,0x2005,0x2006,0x2007,0x2008,0x2009,0x200A,0x202F,0x205F,0x3000,0xFEFF].indexOf(c) >= 0); Wouldn't just /\s/.match(c) or /[ \t\r\n]/ work? How many kinds of (JS) whitespace are there? @@ +795,5 @@ > + (c > 128 && c != 0xffff && c != 0x2028 && c != 0x2029) || > + // Letters (uppercase, lowercase) are identifier chars. > + (c > 0x40 && c < 0x5b) || (c > 0x60 && c < 0x7b) || > + // $ and _ are identifier chars. > + c == 0x5f || c == 0x24); Maybe /[_$a-zA-Z\xA0-\uFFFF]/ is more readable than all those ||s? PS: And if you'd need to test for a full identifier: ^[_$a-zA-Z\xA0-\uFFFF][_$a-zA-Z0-9\xA0-\uFFFF]*$ ::: browser/devtools/sourceeditor/source-editor-orion.jsm @@ +371,5 @@ > this.setMode(config.mode); > > + // The following needs the editor to have its mode set. > + this._autocomplete = new Autocompletion(this, { > + nbVisibleCompletions: 10, 10 is a beautiful number. Also magic. Put it in a const so everybody can admire it's grandeur. Or why not show all available completions, have a max-height styled popup and scroll when necessary. Performance? @@ +372,5 @@ > > + // The following needs the editor to have its mode set. > + this._autocomplete = new Autocompletion(this, { > + nbVisibleCompletions: 10, > + cssClass: "autocomplete", Nit: maybe use a default value for cssClass in the createPopover function in autocompletion.jsm? Up to you. @@ +524,5 @@ > return true; > } > > + // Do autocompletion. > + let previousChar = this.getText()[this.getCaretOffset() - 1]; I'm a bit concerned on the performance on getting the whole text in the editor on large files. Could just passing start and end params to getText based on the caret offset be better? @@ +566,5 @@ > + && /^[a-zA-Z0-9.$_]$/.test(previousChar)) { > + // We have the right to autocomplete now. > + this._autocomplete.cycle(-1); > + return true; > + } Might want to create a method handling this here and not duplicate the conditional. Maybe something like if (this._isAutocompletionAllowed()) { this._autocomplete.cycle() ... } Why autocomplete on shift+tab though? Maybe I actually want to unindent instead :)

Thaddee Tyl [:espadrine]

Comment 36

•

12 years ago

(In reply to Victor Porof [:vp] from comment #35) > * Autocompletion is case sensitive. I think it'd be a good idea to have the > matching as fuzzy as possible, especially starting with making it case > insensitive (I may be biased because I'm a terrible typer, trading precision > for speed). Ideally, when I do a foo.BCAN i'd like it to show me > "BEER_CAN_SOUNDS_LIKE_BACON" and "BREAKPOINT_CHANGE", but I can settle just > for "window.com" showing "Components" instead of nothing. Not a bad idea, but I feel it should really be a follow-up! > * Typing "window.get" and pressing TAB would autocomplete to > "getComputedStyle" but still display the popup. I'd then need to type ESC or > another key to hide it. As documented in a previous comment, this is desired behaviour. When autocompleting, just like in zsh, typing TAB selects the first candidate, and each successive TAB selects the next candidate. Selecting candidates in the dark, without the popup, would result in a suboptimal user experience. On the other hand, if there is only one candidate, the popup disappears after autocompletion. > * This was said previously in this bug, but I'll mention it too: the fact > that UP/DOWN automatically changes the entered string with what's in the > popup is not something I'd like to happen. UP/DOWN should just jump to the > next element in the popup, and TAB would do one thing only: finish > autocompleting and hide the popup. > > There's a certain degree of "instinct" built in when dealing with how > autocompletion popups work, and while your proposed interaction is actually > pretty damn fine, it'll require people detaching from what they learned to > trust so far. For example my sentiment was something like "Ok, DOWN, DOWN... > here it is. TAB. Oh? Um.. No, not that. DOWN, DOWN, DOWN. ESC. Hmm, ok then, > I guess this is ok..". > > Here's a concrete example: > - "typed windows.co" > - pressed DOWN till I reached "controllers" > - pressed TAB (by reflex) > - the selection jumped to the first entry. > That's definitely not what I was expecting. My intention was "write that and > hide the popup", the effect was "select a different thing and don't hide the > popup" which is the exact opposite :) This isn't really about jumping to the first entry. When cycling through candidates, TAB selects the next candidate. When on the last candidate, it goes back to the first candidate. There is absolutely no need to press TAB when you have selected "controllers", since it is already written down. > * When only one completion option is available, the popup looks like a > collapsed dropdown. Will the Orion update help us with this? > > *** > > There are a few places in which things aren't working as expected like > trying to get properties of a global defined in a lazy getter (ex: without > ever opening the Debugger, writing "DebuggerUI." gives me a "Parsing error: > missing name after . Parsing errors are not to be worried about. If I can't parse, I keep the data I already have. I didn't notice I left a dump() here, I'll remove it. > I'm not sure if there's a followup for it, but it seems that the statical > analysis is not finished yet. If I had a source containing "var foo = { bar: > 42 }" doing "foo." wouldn't display anything in the popup. Same goes for > arrays. You'd have to eval the source first. While this is generally ok, I > think the Parser API is super powerful and we should make full use of it > especially in these cases. That is entirely true, there is a lot to add still. Fetching scope variables is the most useful thing for now, and I really want to have a CSS autocompletion working too, so I'll start with that as a follow-up. > One last thing: when relying on eval for the global scope or current context > (or even when using the Parser API before any initial eval), having a bit > more information than just the autocompletion string in the popup would be > really awesome to have. > For example, when displaying the function name in the popup, you could also > show the params in on the right, when dealing with primitives (numbers, > strings etc.) showing their value, or const if it's the case. This would be > trivial to add, just regexp your way around the evaled values or dive deep > into the abstract syntax tree from the Parser API. Take a look into the gist > from comment #3, here's a screen: http://tinyurl.com/9gehfbn > Can totally follow up for all of this. I started with something that had this, but Paul convinced me against it. His driving argument was that flooding the editing workflow with too much information is detrimental to readability and usability. We can obviously discuss this as a follow-up, but it isn't part of this patch. > ::: browser/devtools/scratchpad/scratchpad.js > @@ +409,5 @@ > > + getSandbox: function SP_getSandbox() > > + { > > + // Evaluate the sandbox one to get all the autocompletion we can. > > + this.evalForContext(""); > > + return this.executionContext == SCRATCHPAD_CONTEXT_CONTENT ? > > The name of this function is a bit misleading. Either > getSandboxForAutocompletion here (implying an automatic evalForContext > beforehand), or just leave it as getSandbox and do a this.evalForContext("") > manually where necessary. I'd go with the second option. Ok, can do that. > ::: > browser/devtools/scratchpad/test/ > browser_scratchpad_bug_762164_autocompletion.js > @@ +41,5 @@ > > + EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow); > > + > > + let popover = sp.editor._autocomplete.popover; > > + is(popover.style.display, "block", > > + "Autocompletion of window.F gives a popover"); > > Aren't you displaying the popup after a timeout(,0)? You're probably going > to get into race conditions if that's the case. Fire an event to handle the > autocompletion popover showing/hiding. There's no race condition, it's all running sequentially, for simplicity's sake. > @@ +46,5 @@ > > + * ed._mode must be "js". > > + * ed._view must be a TextView. > > + * ed._model must be a TextModel. > > + * ed._undoStack must be an UndoStack. > > + * ed.editorElement must be the XUL iframe containing the editor. > > Those are a lot of assertions. Maybe test for those in the constructor and > throw an Error if something's not right. I'd rather not, because accessing TextView, TextModel, etc. would require a major refactoring of the source editor component to circumvent a long chain of circular dependencies which had me banging my head on the wall for a week. > @@ +50,5 @@ > > + * ed.editorElement must be the XUL iframe containing the editor. > > + * > > + * @param object options > > + * Properties for tuning certain defaults: > > + * - nbVisibleCompletions (defaults to 10): number of visible > > Nit: how about num instead of nb? It took me 4 seconds to figure out what nb > means. Sure! > @@ +72,5 @@ > > + if (e.keyCode == 13) { > > + this._fireStaticAnalysis = true; > > + } > > + if (e.charCode > 0) { > > + this.document.defaultView.setTimeout(this.displayCompletion.bind(this),0); > > setTimeout is the devil. But sometimes the devil is the only option. > > However, I think I understand the reasoning of waiting for a short delay > before showing and populating the popup. In that case, why not use a longer > delay? I don't think anyone would want such an incredibly snappy popup > flashing around each time I type. Try out a timeout of 50ms (and put it in a > const if it feels right). > > If that's not necessary, how about > Services.tm.currentThread.dispatch({ run: yourFuncHere }, 1); > instead of the timeout(,0)? The idea was more about dispatching the function in the next event loop cycle. So, importing services.jsm… > @@ +78,5 @@ > > + } > > + }.bind(this)); > > + this.editor.addEventListener("Selection", function(e) { > > + // If the distance between the old position and the new is bigger than 1, > > + // the static analysis is worth updating. > > Heh, this is hacky. Why are you doing this on selection events? > Not saying this is bad, just wanting to understand what you're trying to do. I don't want to run the static analysis too often, especially in places where it is likely to fail. When I am typing a newline, it is likely to work and to be useful, since we may change of scope. When I am putting the caret somewhere else, I want it to run, because the scope has very probably changed. > @@ +112,5 @@ > > + _end: 0, > > + _insertingText: false, > > + _completion: null, > > + // Static analysis would better be cached. > > + _staticCandidates: null, > > Are the completions uncached when not present in the source code anymore? > This concern is somewhat related to the selection events that trigger static > analysis. Static analysis is cached every time it succeeds. This avoids losing the valuable information that it provides every time parsing the code fails. > @@ +149,5 @@ > > + html += "<option " + (i == this._index? "selected='true'": "") + ">" + > > + completions[i] + "</option>"; > > + } > > + html += "</select>"; > > + this.popover.innerHTML = html; > > I'd avoid using innerHTML, I'm sure document.createElement would work > better. Just make sure you're using the correct namespace. Using innerHTML allows to have only one interaction with the DOM, and allows a much more concise manipulation of code. Please note that this part is bound to change heavily with the Orion update. > @@ +160,5 @@ > > + } > > + this.editor._view.focus(); > > + }.bind(this), false); > > + this.popover.firstChild.size = > > + Math.min(this.nbVisibleCompletions, completions.length); > > max-height and a scrollbar maybe? Not sure what you mean here? > @@ +165,5 @@ > > + > > + // Positioning the popover. > > + let coord = ed._view.getLocationAtOffset(ed._view.getCaretOffset()); > > + coord.y += ed._view.getLineHeight(); > > + ed._view.convert(coord, "document", "page"); > > Instead of all this getting of locations, caret offsets and convertions, > would using a tooltip tied to the editor caret (by calling openPopup with an > anchor) work better? You can make a custom tooltip item and add whatever you > want to it: https://developer.mozilla.org/en-US/docs/XUL/tooltip > > On the other hand, maybe this is just trying to make things too elegant. I don't know much about XUL tooltips, so they scare me. The simple calculations that I have right now work just fine. Maybe a follow-up? > @@ +180,5 @@ > > + for (; line + " " !== linesOM[lineIndex].textContent; lineIndex++) {} > > + coord.y = insideDoc.defaultView.scrollY + > > + linesOM[lineIndex].getBoundingClientRect().top + > > + linesOM[lineIndex].scrollHeight; > > + // End of Bug 779642 fix. > > Custom tooltips may render this unneeded. If you have the time, it could be > a good idea to try it out. Follow-up? > @@ +385,5 @@ > > + } > > + let allStaticCandidates = this._staticCandidates; > > + // Right now, we can only complete variables. > > + if (identifier.indexOf(".") == -1 && identifier.indexOf("[") == -1 && > > + // Thou shalt only complete what there is to complete. > > Nit: funky comment line in the middle of a conditional expression. Add it > after { or before the if. The comment refers to half of the condition. Having it after the { would make it apply to the first instruction inside the if, having it before would make it apply to the full condition, which it doesn't. I can remove the comment entirely if you want. > @@ +398,5 @@ > > + } > > + staticCandidates.sort(function(a, b) { > > + // Sort them according to nearest scope. > > + return allStaticCandidates.get(b) - allStaticCandidates.get(a); > > + }); > > This is cool. > What would be even more awesome is to colorize or somehow delimit the > candidates based on the scope. > Just a suggestion, follow up for this if you think it's a good idea. Yep! Lots of possible follow-up on autocompletion! > @@ +414,5 @@ > > + candidates = candidates.concat(sandboxCompletion.matches > > + .filter(function(candidate) { > > + // We are removing candidates from level 2. > > + if (allStaticCandidates == null) return true; > > + if (allStaticCandidates.has(candidate)) { > > Useless if. Are you planning on adding things here later? Huh. Not sure what that was supposed to be. Maybe a dump(). > @@ +434,5 @@ > > + "default", "delete", "do", "else", "export", "false", "finally", "for", > > + "function", "get", "if", "import", "in", "instanceof", "let", "new", > > + "null", "of", "return", "set", "super", "switch", "this", "true", "throw", > > + "try", "typeof", "undefined", "var", "void", "while", "with", > > + ]; > > Did you get those from > https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Reserved_Words > ? > Seems that "each" is missing (maybe some other stuff too). I wouldn't worry > too much about it though. No, it's from ES.next :) > @@ +435,5 @@ > > + "function", "get", "if", "import", "in", "instanceof", "let", "new", > > + "null", "of", "return", "set", "super", "switch", "this", "true", "throw", > > + "try", "typeof", "undefined", "var", "void", "while", "with", > > + ]; > > + // This autocompletion is not meaningful when we type a property… > > Hmm, why? > I'd like to have autocompletion when writing foo["bar Autocompleting the contents of strings with JS keywords seems like a stretch of the feature. > @@ +537,5 @@ > > + // Parameters are one level deeper than the function's name itself. > > + argumentNames(subnode.params, store, stack.length + 1); > > + } > > + } > > + deeper = nestedNodes(subnode, line, column); > > Should probably add tests for the supported subnode types too. I have a test which can only work if the static analysis goes through a lot of different subnode types. I can make it even more complex, to go through all subnode types, if you want. > @@ +551,5 @@ > > + if (!deeper) { > > + node = stack.pop(); > > + index = indices.pop(); > > + } > > + } while (stack.length > 0 || (node && index < node.length) || !!deeper); > > This while loop is pretty dense. Probably a recursive approach would > simplify things a bit. Not sure about the performance implications of that > though. You have no idea how much simpler the recursive algorithm was :) But yeah, the performance was abysmal. > @@ +640,5 @@ > > + */ > > +function argumentNames(node, store, weight) { > > + for (let i = 0; i < node.length; i++) { > > + store.set(node[i].name, weight); > > + } > > I didn't manage to make these show up in the completion popup. > If they're working, should probably add a test for them too. Can you give me the code you were writing and the position of your caret? I may have missed a possibility. > @@ +779,5 @@ > > +function isWhiteSpace(c) { > > + return (c === 32) || (c === 0x0009) || (c === 0x000B) || > > + (c === 0x000C) || (c === 0x00A0) || > > + (c >= 0x1680 && > > + [0x1680,0x180E,0x2000,0x2001,0x2002,0x2003,0x2004,0x2005,0x2006,0x2007,0x2008,0x2009,0x200A,0x202F,0x205F,0x3000,0xFEFF].indexOf(c) >= 0); > > Wouldn't just /\s/.match(c) or /[ \t\r\n]/ work? How many kinds of (JS) > whitespace are there? Unfortunately, in JS, regexes are not designed to work the way the JS parser works. All allowed ES whitespace characters are taken into account here. > @@ +795,5 @@ > > + (c > 128 && c != 0xffff && c != 0x2028 && c != 0x2029) || > > + // Letters (uppercase, lowercase) are identifier chars. > > + (c > 0x40 && c < 0x5b) || (c > 0x60 && c < 0x7b) || > > + // $ and _ are identifier chars. > > + c == 0x5f || c == 0x24); > > Maybe /[_$a-zA-Z\xA0-\uFFFF]/ is more readable than all those ||s? If I was to convert this to a regexp, while matching JS strictly, it would weight a lot more than that (see for example https://github.com/ariya/esprima/blob/master/esprima.js#L180). More information at http://qfox.nl/weblog/263. This being said, the line parser has no pretense of implementing ES completely, even though this is something that I'd like to do. It does take time to write a complete and fast tokenizer. > @@ +524,5 @@ > > return true; > > } > > > > + // Do autocompletion. > > + let previousChar = this.getText()[this.getCaretOffset() - 1]; > > I'm a bit concerned on the performance on getting the whole text in the > editor on large files. Could just passing start and end params to getText > based on the caret offset be better? That is one of the weakest perf optimization that I have done in a while, but sure. Testing shows that with this change, on the complete source code of jQuery pasted five times (36916 loc), this line of code used to take 3ms and now is no longer measurable :) > @@ +566,5 @@ > > + && /^[a-zA-Z0-9.$_]$/.test(previousChar)) { > > + // We have the right to autocomplete now. > > + this._autocomplete.cycle(-1); > > + return true; > > + } > > Might want to create a method handling this here and not duplicate the > conditional. Maybe something like > if (this._isAutocompletionAllowed()) { > this._autocomplete.cycle() > ... > } Sure! Obviously it should be more intelligent than a simple regex; making the line parser into a full-fledged tokenizer and putting it into shared/ would help here. > Why autocomplete on shift+tab though? Maybe I actually want to unindent > instead :) Yeah, sure. Except that neither tab nor shift-tab have any effect on indenting when the selection is empty.

Thaddee Tyl [:espadrine]

Comment 37

•

12 years ago

Attached patch Autocompletion in the scratchpad. (obsolete) — Details — Splinter Review

Taken Victor's comments into account.

Attachment #653587 - Attachment is obsolete: true

Attachment #653587 - Flags: review?(rcampbell)

Comment 45

•

12 years ago

Attached patch Autocompletion in the scratchpad. (obsolete) — Details — Splinter Review

I have taken Mihai's code review into account. I'd like a review from Rob…

Attachment #655520 - Attachment is obsolete: true

Attachment #655520 - Flags: review?(rcampbell)

Attachment #655910 - Flags: review?(rcampbell)

Rob Campbell [:rc] (:robcee)

Reporter

Comment 46

•

12 years ago

Comment on attachment 655910 [details] [diff] [review] Autocompletion in the scratchpad. Review of attachment 655910 [details] [diff] [review]: ----------------------------------------------------------------- need to spend more time digging through autocompletion.jsm. That's a big one. Impressive work so far! ::: browser/devtools/sourceeditor/autocompletion.jsm @@ +28,5 @@ > + let document = aDocument; > + let cssClass = aCssClass; > + let popover = document.createElement("div"); > + popover.classList.add("popover"); > + if (cssClass) { this is going to always be at least "autocomplete" is it not? @@ +31,5 @@ > + popover.classList.add("popover"); > + if (cssClass) { > + popover.classList.add(cssClass); > + } > + popover.style.position = "absolute"; are these necessary? Can't you just add the styles to the stylesheet? @@ +45,5 @@ > +/** > + * Autocompletion object. > + * This constructor creates the necessary DOM popup. > + * > + * @param object aEd could we call this aEditor? Use your words! :) @@ +62,5 @@ > +function Autocompletion(aEd, aOptions = {}) { > + this.numVisibleCompletions = aOptions.numVisibleCompletions || > + NUM_VISIBLE_COMPLETIONS; > + this.getSandbox = aOptions.getSandbox || null; > + if (this.getSandbox) { can this work without this.getSandbox being initialized? I wonder if this should just return early if (!this.getSandbox). @@ +81,5 @@ > +} > + > +Autocompletion.prototype = { > + > + // The following set the way in which the autocompletion system works. for once, I think we can remove a comment. Not really needed here. If you really want to have a comment, maybe describe what "getSandbox" is and what it's for. ::: browser/devtools/webconsole/WebConsoleUtils.jsm @@ +996,3 @@ > { > // Argument defaults. > + aOptions = aOptions || {matchProp: ""}; this is a bit of a puzzler. Why this change? Why did we need the aOptions.matchProp = aOptions.matchProp || "" check at all in the previous version? Can we not just set a default and not worry about it?

Thaddee Tyl [:espadrine]

•

12 years ago

also mentioned that we will need a "killswitch" pref. Suggest devtools.scratchpad.autocomplete.enabled = true/false to enable or disable the feature.

Thaddee Tyl [:espadrine]

Comment 51

•

12 years ago

(In reply to Mihai Sucan [:msucan] from comment #48) > Comment on attachment 655910 [details] [diff] [review] > Autocompletion in the scratchpad. > > Review of attachment 655910 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for the updated patch. I like the source editor changes. > > General comments: > > - While playing with Scratchpad I sometimes get the following in the error > console: > > Timestamp: 29/08/2012 18:14:48 > Error: TypeError: source is undefined > Source File: resource:///modules/devtools/autocompletion.jsm > Line: 729 > > - In the Style Editor after typing a bit, I get: > > Timestamp: 29/08/2012 18:24:35 > Error: TypeError: this._completion is null > Source File: resource:///modules/devtools/autocompletion.jsm > Line: 131 I'd love to have more information on these errors. I'm trying to reproduce them. > - In one of my previous comments I asked about the AutocompletePopup.jsm if > it could be reused. Any thoughts? After completely reviewing > Autocompletion.jsm it feels like there's quite some code duplication without > any obvious benefits. The popup could be reused, but I'd love this to be a follow-up. The look and feel of the AutocompletePopup is aesthetically incompatible with the source editor right now (although it does fit the web console). > - The approach you used for some methods where you changed argument names > then you do |let foo = aFoo| is not ideal. Yes, I wrote a fast macro to automate the change, but it wasn't ideal :) Consider it fixed. > - Please add source editor tests for all of the new API. Will do. > - lower the delay, try 100ms. I've put 100ms in the patch to come, and it does feel better than 200ms, while not being fast enough to show up while typing. > - in one of the comments above we talked about undoing to what I had before > starting autocompletion. I mentioned that Escape only closes the popup and > there's no way for me to get back to what I had in the editor before > completion changes. You suggested I use Ctrl-Z and I tried. After I press > Down several times to see the possible completions I decide to press Escape > and Ctrl-Z. The result is I have to Ctrl-Z for each of the items I went > through. One Ctrl-Z is not sufficient... This is a very bad regression! (and it is fixed, along with a couple other regressions I noticed, in the next patch.) > @@ +305,5 @@ > > + this.runCompleters(); > > + if (count == 0) { count = 1; } > > + if (count > 0) { > > + // We can start from the beginning. > > + this._index = count - 1; > > This is confusing. Why if (count == 0) { count = 1; } ? I changed that part to make it clearer that all we strive to avoid here is having a negative index. > @@ +565,5 @@ > > + subnode.type == "FunctionExpression" || > > + // Expressions, eg, (function(){…}()); > > + (subnode.callee && subnode.callee.type == "FunctionExpression")) { > > + if (subnode.callee) { subnode = subnode.callee; } > > + if (subnode.id) { store.set(subnode.id.name, stack.length); } > > Can you please put these on new lines? > > if (foo) { > bar(); > } > > ... to make the code easier to follow. Obviously I thought it made the code easier to read for me, but I changed it for you ;) > Maybe we also need an option to enable/disable autocompletion in the Source > Editor - since don't want to always enable completion. Default should be off. I agree with having the possibility to turn it off, but I believe it should be on by default. I strongly believe that features that are off by default are never seen by non-mozillians (and then, only a small percentage of mozillians). You'd be surprised how many people amongst mozilla employees I've taught how to enable chrome mode in the scratchpad. I'd be really sad to see this feature never get used (or get used only two years from now). I'm quite proud of how it turned out. > @@ +1884,5 @@ > > + * > > + * @param object aCoord > > + * An object with properties x and y. > > + */ > > + convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord) > > convertDocumentToView() ? not sure... It doesn't change the document at all, it just changes 'aCoord'. > @@ +38,5 @@ > > + > > +function editorLoaded() > > +{ > > + // JS autocompletion. > > + editor.setMode(SourceEditor.MODES.JAVASCRIPT); > > You can set the mode from editor.init() above. This is somewhat part of the test. The autocompletion mechanism should be flexible to changes in the editor's mode (to allow, in the future, for autocompleting JS in HTML). Added a comment. > @@ +65,5 @@ > > + editor.setText(before + after); > > + editor.setCaretOffset(before.length); > > + EventUtils.synthesizeKey("VK_TAB", {}, testWin); > > + is(editor.getText(), before + "bar" + after, > > + "Deep static JS autocompletion."); > > Is this the result of static JS analysis? Yes.

Mihai Sucan [:msucan]

Comment 52

•

12 years ago

(In reply to Thaddee Tyl [:espadrine] from comment #51) > (In reply to Mihai Sucan [:msucan] from comment #48) > > Comment on attachment 655910 [details] [diff] [review] > > Autocompletion in the scratchpad. > > > > Review of attachment 655910 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Thank you for the updated patch. I like the source editor changes. > > > > General comments: > > > > - While playing with Scratchpad I sometimes get the following in the error > > console: > > > > Timestamp: 29/08/2012 18:14:48 > > Error: TypeError: source is undefined > > Source File: resource:///modules/devtools/autocompletion.jsm > > Line: 729 > > > > - In the Style Editor after typing a bit, I get: > > > > Timestamp: 29/08/2012 18:24:35 > > Error: TypeError: this._completion is null > > Source File: resource:///modules/devtools/autocompletion.jsm > > Line: 131 > > I'd love to have more information on these errors. I'm trying to reproduce > them. I tested the patch on Ubuntu 12.04. Please let me know if you need any specific information. > > - In one of my previous comments I asked about the AutocompletePopup.jsm if > > it could be reused. Any thoughts? After completely reviewing > > Autocompletion.jsm it feels like there's quite some code duplication without > > any obvious benefits. > > The popup could be reused, but I'd love this to be a follow-up. The look and > feel of the AutocompletePopup is aesthetically incompatible with the source > editor right now (although it does fit the web console). I leave the final decision to Rob. I incline to agree - a follow-up would be fine with me. Please make sure you file all of the suggested follow up bugs. > > - The approach you used for some methods where you changed argument names > > then you do |let foo = aFoo| is not ideal. > > Yes, I wrote a fast macro to automate the change, but it wasn't ideal :) > Consider it fixed. > > > - Please add source editor tests for all of the new API. > > Will do. Thank you! > > - lower the delay, try 100ms. > > I've put 100ms in the patch to come, and it does feel better than 200ms, > while not being fast enough to show up while typing. Uh oh. Rob, maybe you can come up with a detailed explanation of the UX we want for autocomplete? Down to how keyboard shortcuts should work in various cases. > > - in one of the comments above we talked about undoing to what I had before > > starting autocompletion. I mentioned that Escape only closes the popup and > > there's no way for me to get back to what I had in the editor before > > completion changes. You suggested I use Ctrl-Z and I tried. After I press > > Down several times to see the possible completions I decide to press Escape > > and Ctrl-Z. The result is I have to Ctrl-Z for each of the items I went > > through. One Ctrl-Z is not sufficient... > > This is a very bad regression! (and it is fixed, along with a couple other > regressions I noticed, in the next patch.) Good then! > > Maybe we also need an option to enable/disable autocompletion in the Source > > Editor - since don't want to always enable completion. Default should be off. > > I agree with having the possibility to turn it off, but I believe it should > be on by default. > > I strongly believe that features that are off by default are never seen by > non-mozillians (and then, only a small percentage of mozillians). You'd be > surprised how many people amongst mozilla employees I've taught how to > enable chrome mode in the scratchpad. > > I'd be really sad to see this feature never get used (or get used only two > years from now). I'm quite proud of how it turned out. Oh, sorry - I was misunderstood in this case. For Scratchpad users we definitely want this enabled by default - enable it from scratchpad.js. But, the default for the Source Editor component should be off (like syntax highlighting is off by default). Later, when we have CSS autocomplete support, we can enable it from the Style Editor as well (by default!). > > @@ +1884,5 @@ > > > + * > > > + * @param object aCoord > > > + * An object with properties x and y. > > > + */ > > > + convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord) > > > > convertDocumentToView() ? not sure... > > It doesn't change the document at all, it just changes 'aCoord'. convertDocumentCoordsToView() then? > > @@ +38,5 @@ > > > + > > > +function editorLoaded() > > > +{ > > > + // JS autocompletion. > > > + editor.setMode(SourceEditor.MODES.JAVASCRIPT); > > > > You can set the mode from editor.init() above. > > This is somewhat part of the test. The autocompletion mechanism should be > flexible to changes in the editor's mode (to allow, in the future, for > autocompleting JS in HTML). > > Added a comment. Oh, then as previously suggested please change modes and test for behavior differences between modes. > > @@ +65,5 @@ > > > + editor.setText(before + after); > > > + editor.setCaretOffset(before.length); > > > + EventUtils.synthesizeKey("VK_TAB", {}, testWin); > > > + is(editor.getText(), before + "bar" + after, > > > + "Deep static JS autocompletion."); > > > > Is this the result of static JS analysis? > > Yes. That wasn't obvious for me. Can you test something that makes it more obvious? How do I know the result doesn't come from, maybe, a list of known tokens in the document? Or maybe you can compare to how it works in different scopes when the same |foo| is typed. Thanks!

Thaddee Tyl [:espadrine]

Comment 53

•

12 years ago

(In reply to Mihai Sucan [:msucan] from comment #52) > (In reply to Thaddee Tyl [:espadrine] from comment #51) > > (In reply to Mihai Sucan [:msucan] from comment #48) > > > Comment on attachment 655910 [details] [diff] [review] > > > Autocompletion in the scratchpad. > > > > > > Review of attachment 655910 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Thank you for the updated patch. I like the source editor changes. > > > > > > General comments: > > > > > > - While playing with Scratchpad I sometimes get the following in the error > > > console: > > > > > > Timestamp: 29/08/2012 18:14:48 > > > Error: TypeError: source is undefined > > > Source File: resource:///modules/devtools/autocompletion.jsm > > > Line: 729 > > > > > > - In the Style Editor after typing a bit, I get: > > > > > > Timestamp: 29/08/2012 18:24:35 > > > Error: TypeError: this._completion is null > > > Source File: resource:///modules/devtools/autocompletion.jsm > > > Line: 131 > > > > I'd love to have more information on these errors. I'm trying to reproduce > > them. > > I tested the patch on Ubuntu 12.04. Please let me know if you need any > specific information. Can you confirm that you still get this? I can't seem to reproduce, but I might have fixed it. > > > Maybe we also need an option to enable/disable autocompletion in the Source > > > Editor - since don't want to always enable completion. Default should be off. > > > > I agree with having the possibility to turn it off, but I believe it should > > be on by default. > > > > I strongly believe that features that are off by default are never seen by > > non-mozillians (and then, only a small percentage of mozillians). You'd be > > surprised how many people amongst mozilla employees I've taught how to > > enable chrome mode in the scratchpad. > > > > I'd be really sad to see this feature never get used (or get used only two > > years from now). I'm quite proud of how it turned out. > > Oh, sorry - I was misunderstood in this case. > > For Scratchpad users we definitely want this enabled by default - enable it > from scratchpad.js. > > But, the default for the Source Editor component should be off (like syntax > highlighting is off by default). > > Later, when we have CSS autocomplete support, we can enable it from the > Style Editor as well (by default!). Ok. Done that, *and* the devtools.editor.autocomplete.enabled preference. Two birds with one stone. > > > @@ +1884,5 @@ > > > > + * > > > > + * @param object aCoord > > > > + * An object with properties x and y. > > > > + */ > > > > + convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord) > > > > > > convertDocumentToView() ? not sure... > > > > It doesn't change the document at all, it just changes 'aCoord'. > > convertDocumentCoordsToView() then? Deal.

Thaddee Tyl [:espadrine]

Comment 54

•

12 years ago

Comment 70

•

7 years ago

Per comment 69 + no answer from logger, closing this.

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

Flags: needinfo?(thaddee.tyl)

Resolution: --- → FIXED

Tim Nguyen :ntim

Updated

•

7 years ago

Assignee: thaddee.tyl → nobody

BMO Automation

Updated

•

6 years ago

Product: Firefox → DevTools

BMO Automation

Updated

•

5 years ago

Product: DevTools → DevTools Graveyard

WIP autocompletion for UX testing 12 years ago Thaddee Tyl [:espadrine] 16.74 KB, patch		Details \| Diff \| Splinter Review
Autocompletion in the scratchpad. 12 years ago Thaddee Tyl [:espadrine] 41.15 KB, patch		Details \| Diff \| Splinter Review
Autocompletion in the scratchpad. 12 years ago Thaddee Tyl [:espadrine] 42.32 KB, patch		Details \| Diff \| Splinter Review
Autocompletion in the scratchpad. 12 years ago Thaddee Tyl [:espadrine] 44.77 KB, patch		Details \| Diff \| Splinter Review
WIP Autocompletion in the scratchpad. 12 years ago Thaddee Tyl [:espadrine] 46.78 KB, patch		Details \| Diff \| Splinter Review