From 3116c16d54d0dbadfc2a865293c0518c5973c2ab Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Tue, 6 Jan 2009 22:09:35 -0500 Subject: [PATCH 1/5] Revert "fixed :open completions for slow computers. We now yield" This reverts commit b32cfe76ff5f5a22e868eb8ab97191b12561e16e. --- common/content/completion.js | 37 +++++++++---------------------- common/content/liberator.js | 1 - common/content/ui.js | 43 +++++++++++++----------------------- 3 files changed, 26 insertions(+), 55 deletions(-) diff --git a/common/content/completion.js b/common/content/completion.js index 058ef027..c27734fc 100644 --- a/common/content/completion.js +++ b/common/content/completion.js @@ -528,9 +528,7 @@ CompletionContext.prototype = { cancelAll: function () { - // Kris: contextList gives undefined. And why do we have contexts and contextList? - // I am not too much of a fan of a huge API for classes - for (let [,context] in Iterator(this.top.contexts)) + for (let [,context] in Iterator(this.contextList)) { if (context.cancel) context.cancel(); @@ -1578,7 +1576,6 @@ function Completion() //{{{ context.filterFunc = null; context.cancel = function () services.get("autoCompleteSearch").stopSearch(); context.compare = null; - // TODO: shouldn't this timer be deleted by context.cancel? let timer = new Timer(50, 100, function (result) { context.incomplete = result.searchResult >= result.RESULT_NOMATCH_ONGOING; context.completions = [ @@ -1586,28 +1583,16 @@ function Completion() //{{{ for (i in util.range(0, result.matchCount)) ]; }); - context.generate = function () { - let minItems = context.minItems || 1; - services.get("autoCompleteSearch").stopSearch(); - services.get("autoCompleteSearch").startSearch(context.filter, "", context.result, { - onSearchResult: function (search, result) - { - context.result = result; - timer.tell(result); - if (result.searchResult <= result.RESULT_SUCCESS) - timer.flush(); - } - }); - - let end = Date.now() + 5000; - while (context.incomplete && context.completions.length < minItems && Date.now() < end) - liberator.threadYield(false, true); - - // context.message = "More results..."; // very very strange, if I enable this line, completions don't work; - services.get("autoCompleteSearch").stopSearch(); - - return context.completions; - } + services.get("autoCompleteSearch").stopSearch(); + services.get("autoCompleteSearch").startSearch(context.filter, "", context.result, { + onSearchResult: function onSearchResult(search, result) + { + context.result = result; + timer.tell(result); + if (result.searchResult <= result.RESULT_SUCCESS) + timer.flush(); + } + }); }, macro: function macro(context) diff --git a/common/content/liberator.js b/common/content/liberator.js index e7085102..dd5d396c 100644 --- a/common/content/liberator.js +++ b/common/content/liberator.js @@ -1362,7 +1362,6 @@ const liberator = (function () //{{{ callback.call(self); }, - // TODO: interruptable not used? threadYield: function (flush, interruptable) { let mainThread = services.get("threadManager").mainThread; diff --git a/common/content/ui.js b/common/content/ui.js index 36a1f144..728c4897 100644 --- a/common/content/ui.js +++ b/common/content/ui.js @@ -362,7 +362,7 @@ function CommandLine() //{{{ else idx = this.selected + 1; break; - case this.RESET: // TODO: never used for now + case this.RESET: idx = null; break; default: @@ -386,37 +386,22 @@ function CommandLine() //{{{ tab: function tab(reverse) { autocompleteTimer.flush(); - // Check if we need to run the completer. - let numElementsNeeded; - if (this.selected == null) - numElementsNeeded = reverse ? 100000 : 1; // better way to specify give me all items than setting to a very large number? - else - numElementsNeeded = reverse ? this.selected : this.selected + 2; // this.selected is zero-based - if (this.context.waitingForTab || this.wildIndex == -1) - { this.complete(true, true); - } - else if (this.context.incomplete && numElementsNeeded > this.items.length) - { - for (let [, context] in Iterator(this.context.contexts)) - { - if (context.incomplete && context.generate) - { - // regenerate twice as many items as needed, as regenerating - // to often looks visually bad - context.minItems = numElementsNeeded * 2; // TODO: document minItems, or find a better way - context.regenerate = true; - context._generate(); // HACK - } - if (this.items.length >= numElementsNeeded) - break; - } - } - if (this.items.length == 0) - return liberator.beep(); + // Would prefer to only do this check when no completion + // is available, but there are complications. + if (this.items.length == 0 || this.context.incomplete) + { + // No items. Wait for any unfinished completers. + let end = Date.now() + 5000; + while (this.context.incomplete && /* this.items.length == 0 && */ Date.now() < end) + liberator.threadYield(true, true); + + if (this.items.length == 0) + return liberator.beep(); + } switch (this.wildtype.replace(/.*:/, "")) { @@ -1619,9 +1604,11 @@ function CommandLine() //{{{ { autocompleteTimer.reset(); + // liberator.dump("Resetting completions..."); if (completions) { completions.context.cancelAll(); + completions.wildIndex = -1; completions.previewClear(); } From 61d3fae8b46174e9af3c725b2f01e13a1a99db4c Mon Sep 17 00:00:00 2001 From: Doug Kearns Date: Wed, 7 Jan 2009 01:04:16 +1100 Subject: [PATCH 2/5] Use self as the returned object from all creation functions. --- common/content/commands.js | 18 +++++++------- common/content/completion.js | 2 +- common/content/events.js | 16 ++++++------ common/content/io.js | 48 ++++++++++++++++++------------------ common/content/modes.js | 3 ++- 5 files changed, 44 insertions(+), 43 deletions(-) diff --git a/common/content/commands.js b/common/content/commands.js index 7b70abae..e8a0a50f 100644 --- a/common/content/commands.js +++ b/common/content/commands.js @@ -307,7 +307,7 @@ function Commands() //{{{ completion.setFunctionCompleter(commands.get, [function () ([c.name, c.description] for (c in commands))]); }); - var commandManager = { + const self = { // FIXME: remove later, when our option handler is better OPTION_ANY: 0, // can be given no argument or an argument of any type, @@ -796,7 +796,7 @@ function Commands() //{{{ // TODO: Vim allows commands to be defined without {rep} if there are {attr}s // specified - useful? - commandManager.add(["com[mand]"], + self.add(["com[mand]"], "List and define commands", function (args) { @@ -903,11 +903,11 @@ function Commands() //{{{ bang: true, completer: function (context) completion.userCommand(context), options: [ - [["-nargs"], commandManager.OPTION_STRING, + [["-nargs"], self.OPTION_STRING, function (arg) /^[01*?+]$/.test(arg), ["0", "1", "*", "?", "+"]], - [["-bang"], commandManager.OPTION_NOARG], - [["-count"], commandManager.OPTION_NOARG], - [["-complete"], commandManager.OPTION_STRING, + [["-bang"], self.OPTION_NOARG], + [["-count"], self.OPTION_NOARG], + [["-complete"], self.OPTION_STRING, function (arg) arg in completeOptionMap || /custom,\w+/.test(arg), function (context) [[k, ""] for ([k, v] in Iterator(completeOptionMap))]] ], @@ -930,7 +930,7 @@ function Commands() //{{{ ] }); - commandManager.add(["comc[lear]"], + self.add(["comc[lear]"], "Delete all user-defined commands", function () { @@ -938,7 +938,7 @@ function Commands() //{{{ }, { argCount: "0" }); - commandManager.add(["delc[ommand]"], + self.add(["delc[ommand]"], "Delete the specified user-defined command", function (args) { @@ -956,7 +956,7 @@ function Commands() //{{{ //}}} - return commandManager; + return self; }; //}}} diff --git a/common/content/completion.js b/common/content/completion.js index 058ef027..dd7574ba 100644 --- a/common/content/completion.js +++ b/common/content/completion.js @@ -1265,7 +1265,7 @@ function Completion() //{{{ ////////////////////// PUBLIC SECTION ////////////////////////////////////////// /////////////////////////////////////////////////////////////////////////////{{{ - let self = { + const self = { setFunctionCompleter: function setFunctionCompleter(funcs, completers) { diff --git a/common/content/events.js b/common/content/events.js index 8d132e56..b94183de 100644 --- a/common/content/events.js +++ b/common/content/events.js @@ -612,7 +612,7 @@ function Events() //{{{ { try { - eventManager[method](event); + self[method](event); } catch (e) { @@ -758,7 +758,7 @@ function Events() //{{{ ////////////////////// PUBLIC SECTION ////////////////////////////////////////// /////////////////////////////////////////////////////////////////////////////{{{ - var eventManager = { + const self = { feedingKeys: false, @@ -1723,7 +1723,7 @@ function Events() //{{{ } }; //}}} - window.XULBrowserWindow = eventManager.progressListener; + window.XULBrowserWindow = self.progressListener; window.QueryInterface(Ci.nsIInterfaceRequestor) .getInterface(Ci.nsIWebNavigation) .QueryInterface(Ci.nsIDocShellTreeItem).treeOwner @@ -1732,21 +1732,21 @@ function Events() //{{{ .XULBrowserWindow = window.XULBrowserWindow; try { - getBrowser().addProgressListener(eventManager.progressListener, Ci.nsIWebProgress.NOTIFY_ALL); + getBrowser().addProgressListener(self.progressListener, Ci.nsIWebProgress.NOTIFY_ALL); } catch (e) {} - eventManager.prefObserver.register(); + self.prefObserver.register(); liberator.registerObserver("shutdown", function () { - eventManager.destroy(); - eventManager.prefObserver.unregister(); + self.destroy(); + self.prefObserver.unregister(); }); window.addEventListener("keypress", wrapListener("onKeyPress"), true); window.addEventListener("keydown", wrapListener("onKeyUpOrDown"), true); window.addEventListener("keyup", wrapListener("onKeyUpOrDown"), true); - return eventManager; + return self; }; //}}} diff --git a/common/content/io.js b/common/content/io.js index fcf48181..9da85767 100644 --- a/common/content/io.js +++ b/common/content/io.js @@ -114,10 +114,10 @@ function IO() //{{{ function joinPaths(head, tail) { - let path = ioManager.getFile(head); + let path = self.getFile(head); try { - path.appendRelativePath(ioManager.expandPath(tail, true)); // FIXME: should only expand env vars and normalise path separators + path.appendRelativePath(self.expandPath(tail, true)); // FIXME: should only expand env vars and normalise path separators if (path.exists() && path.normalize) path.normalize(); } @@ -385,14 +385,14 @@ function IO() //{{{ liberator.registerObserver("load_completion", function () { - completion.setFunctionCompleter([ioManager.getFile, ioManager.expandPath], + completion.setFunctionCompleter([self.getFile, self.expandPath], [function (context, obj, args) { context.quote[2] = ""; completion.file(context, true); }]); }); - var ioManager = { + const self = { MODE_RDONLY: 0x01, MODE_WRONLY: 0x02, @@ -413,7 +413,7 @@ function IO() //{{{ // Firefox's CWD - see // https://bugzilla.mozilla.org/show_bug.cgi?id=280953 getCurrentDirectory: function () { - let dir = ioManager.getFile(cwd.path); + let dir = self.getFile(cwd.path); // NOTE: the directory could have been deleted underneath us so // fallback to Firefox's CWD @@ -433,7 +433,7 @@ function IO() //{{{ } else { - let dir = ioManager.getFile(newdir); + let dir = self.getFile(newdir); if (!dir.exists() || !dir.isDirectory()) { @@ -444,7 +444,7 @@ function IO() //{{{ [cwd, oldcwd] = [dir, this.getCurrentDirectory()]; } - return ioManager.getCurrentDirectory(); + return self.getCurrentDirectory(); }, getRuntimeDirectories: function (specialDirectory) @@ -489,10 +489,10 @@ function IO() //{{{ } else { - let expandedPath = ioManager.expandPath(path); + let expandedPath = self.expandPath(path); if (!isAbsolutePath(expandedPath) && !noCheckPWD) - file = joinPaths(ioManager.getCurrentDirectory().path, expandedPath); + file = joinPaths(self.getCurrentDirectory().path, expandedPath); else file.initWithPath(expandedPath); } @@ -536,7 +536,7 @@ function IO() //{{{ readDirectory: function (file, sort) { if (typeof file == "string") - file = ioManager.getFile(file); + file = self.getFile(file); else if (!(file instanceof Ci.nsILocalFile)) throw Cr.NS_ERROR_INVALID_ARG; // FIXME: does not work as expected, just shows undefined: undefined @@ -567,7 +567,7 @@ function IO() //{{{ let toCharset = "UTF-8"; if (typeof file == "string") - file = ioManager.getFile(file); + file = self.getFile(file); else if (!(file instanceof Ci.nsILocalFile)) throw Cr.NS_ERROR_INVALID_ARG; // FIXME: does not work as expected, just shows undefined: undefined @@ -595,14 +595,14 @@ function IO() //{{{ let charset = "UTF-8"; // Can be any character encoding name that Mozilla supports if (typeof file == "string") - file = ioManager.getFile(file); + file = self.getFile(file); else if (!(file instanceof Ci.nsILocalFile)) throw Cr.NS_ERROR_INVALID_ARG; // FIXME: does not work as expected, just shows undefined: undefined if (mode == ">>") - mode = ioManager.MODE_WRONLY | ioManager.MODE_CREATE | ioManager.MODE_APPEND; + mode = self.MODE_WRONLY | self.MODE_CREATE | self.MODE_APPEND; else if (!mode || mode == ">") - mode = ioManager.MODE_WRONLY | ioManager.MODE_CREATE | ioManager.MODE_TRUNCATE; + mode = self.MODE_WRONLY | self.MODE_CREATE | self.MODE_TRUNCATE; if (!perms) perms = 0644; @@ -624,7 +624,7 @@ function IO() //{{{ if (isAbsolutePath(program)) { - file = ioManager.getFile(program, true); + file = self.getFile(program, true); } else { @@ -701,9 +701,9 @@ lookup: } if (res > 0) // FIXME: Is this really right? Shouldn't we always show both? - var output = ioManager.readFile(stderr) + "\nshell returned " + res; + var output = self.readFile(stderr) + "\nshell returned " + res; else - output = ioManager.readFile(stdout); + output = self.readFile(stdout); // if there is only one \n at the end, chop it off if (output && output.indexOf("\n") == output.length - 1) @@ -752,11 +752,11 @@ lookup: // no need (actually forbidden) to add: js < Date: Wed, 7 Jan 2009 01:11:06 +1100 Subject: [PATCH 3/5] s/retVal/ret/ in buffer.followDocumentRelationship. --- common/content/buffer.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/common/content/buffer.js b/common/content/buffer.js index add1c14a..0205966a 100644 --- a/common/content/buffer.js +++ b/common/content/buffer.js @@ -1082,12 +1082,12 @@ function Buffer() //{{{ return false; } - let retVal = followFrame(window.content); - if (!retVal) - // only loop through frames if the main content didnt match - retVal = Array.some(window.content.frames, followFrame); + let ret = followFrame(window.content); + if (!ret) + // only loop through frames if the main content didn't match + ret = Array.some(window.content.frames, followFrame); - if (!retVal) + if (!ret) liberator.beep(); }, From 4d45627f8860fcc6fbb226bef3036bf76f2a48e8 Mon Sep 17 00:00:00 2001 From: Doug Kearns Date: Wed, 7 Jan 2009 14:48:15 +1100 Subject: [PATCH 4/5] Comment on 'new' in HACKING. --- HACKING | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/HACKING b/HACKING index 16d2d9c7..57d5f947 100644 --- a/HACKING +++ b/HACKING @@ -8,7 +8,7 @@ happy to accept your patches. First of all, all new features and all user-visible changes to existing features need to be documented. That means editing the appropriate help files -and adding a NEWS enty where appropriate. When editing the NEWS file, you +and adding a NEWS entry where appropriate. When editing the NEWS file, you should add your change to the top of the list of changes. If your change alters an interface (key binding, command) and is likely to cause trouble, prefix it with 'IMPORTANT:', otherwise, place it below the other 'IMPORTANT' @@ -116,9 +116,11 @@ We try to be quite consistent, but of course, that's not always possible. CompletionContext() could return a cached object. What's thesnowdog's opinion and why do you, Kris, think it's better/cleaner? - I don't think it's better/cleaner, it just seemed to be a concensus. + I don't think it's better/cleaner, it just seemed to be a consensus. --Kris + I don't like unnecessary use of 'new', I don't like 'new'. --djk + == Testing/Optimization == TODO: Add some information here about testing/validation/etc. From 102605556d31858eced69bdd16f412af42ef0c24 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Tue, 6 Jan 2009 23:12:51 -0500 Subject: [PATCH 5/5] Fix rapid succession when busy. Don't wait for all contexts to complete when processing --- common/content/completion.js | 9 ++++++- common/content/events.js | 2 +- common/content/liberator.xul | 1 + common/content/ui.js | 47 +++++++++++++++++++++++------------- 4 files changed, 40 insertions(+), 19 deletions(-) diff --git a/common/content/completion.js b/common/content/completion.js index c27734fc..21cce3d3 100644 --- a/common/content/completion.js +++ b/common/content/completion.js @@ -155,7 +155,9 @@ function CompletionContext(editor, name, offset) //{{{ /** * @property {Object} A map of all contexts, keyed on their names. * Names are assigned when a context is forked, with its specified - * name appended, after a '/', to its parent's name. + * name appended, after a '/', to its parent's name. May + * contain inactive contexts. For active contexts, see + * {@link #contextList}. */ this.contexts = { "/": this }; /** @@ -638,6 +640,11 @@ CompletionContext.prototype = { for (let type in this.selectionTypes) this.highlight(0, 0, type); + /** + * @property {[CompletionContext]} A list of active + * completion contexts, in the order in which they were + * instantiated. + */ this.contextList = []; this.offset = 0; this.process = []; diff --git a/common/content/events.js b/common/content/events.js index 8d132e56..0408e157 100644 --- a/common/content/events.js +++ b/common/content/events.js @@ -1014,7 +1014,7 @@ function Events() //{{{ if (event.metaKey) modifier += "M-"; - if (event.type == "keypress") + if (/^key/.test(event.type)) { if (event.charCode == 0) { diff --git a/common/content/liberator.xul b/common/content/liberator.xul index b8003cfd..3f6ae0ca 100644 --- a/common/content/liberator.xul +++ b/common/content/liberator.xul @@ -85,6 +85,7 @@ the terms of any one of the MPL, the GPL or the LGPL.