[PATCH] returning an array of possible defaults

[gtdphp developers]

I've got a patch to handle noun types returning an *array* of possible
default values, in addition to the current tip which only handles a
single default value. It also tweaks the suggest callback to handle
arrays too. While labs.toolness.com is down (error 500s all day),
there's nowhere obvious for me to post it, so I've posted it belowfor
review. I think it will address Gary Hodgson's ticket 358.

Regards,
Andrew

diff -r 3b57c4c9ae22 ubiquity/modules/parser/parser.js
--- a/ubiquity/modules/parser/parser.js Sun Nov 23 16:05:55 2008 +0800
+++ b/ubiquity/modules/parser/parser.js Sun Nov 23 18:46:11 2008 +0000
@@ -225,7 +225,6 @@
} else {
this._parsingsList = newSuggs;
}
-
this.refreshSuggestionList( query, context );
},

@@ -404,24 +403,50 @@
},

fillMissingArgsWithDefaults: function() {
- let newSentence = this.copy();
- let defaultValue;
+ let newSentences=[this.copy()];
+ let defaultsArray=[];
+ let gotArrayOfDefaults=false;
+ let defaultsSoFar={};
for (let argName in this._verb._arguments) {
if (!this._argSuggs[argName]) {
+ let defaultValue;
+ let thisDefault={name:argName};
let missingArg = this._verb._arguments[argName];
if (missingArg.default) {
defaultValue = this._makeSugg(missingArg.default);
} else if (missingArg.type.default) { // Argument value from
nountype default
- // TODO note this doesn't allow a nounType to return more
than one item from
- // its default() method.
defaultValue = missingArg.type.default();
} else { // No argument
defaultValue = {text:"", html:"", data:null, summary:""};
}
- newSentence.setArgumentSuggestion(argName, defaultValue);
+ let numDefaults=defaultValue.length;
+ if (numDefaults && gotArrayOfDefaults || numDefaults===1) {
+ // we've already used an array of values for a previous
modifier,
+ // so just use first default for this modifier
+ defaultValue=defaultValue[0];
+ numDefaults=0;
+ }
+ if (numDefaults) {
+ // first time we've seen multiple defaults, so create an array of
sentences
+ gotArrayOfDefaults=true;
+ for (let i=0;i<numDefaults;i++) {
+ if (i) {
+ newSentences[i]=this.copy();
+ for (let arg in defaultsSoFar) {
+ newSentences[i].setArgumentSuggestion(arg,defaultsSoFar[arg]);
+ }
+ }
+ newSentences[i].setArgumentSuggestion(argName,defaultValue[i]);
+ }
+ } else {
+ for (let sen in newSentences) {
+ newSentences[sen].setArgumentSuggestion
(argName,defaultValue);
+ }
+ defaultsSoFar[argName]=defaultValue;
+ }
}
}
- return newSentence;
+ return newSentences;
},

getMatchScores: function() {
@@ -463,7 +488,6 @@
let newSen = new NLParser.ParsedSentence(this._verb, {},
this._matchScore);
this._parsedSentences = [newSen];
for (let argName in this._verb._arguments) {
- let argSuggs = [];
if (argStrings[argName] && argStrings[argName].length > 0) {
// If argument is present, try the noun suggestions based both
on
// substituting pronoun...
@@ -497,7 +521,10 @@
let self = this;
// Callback function for asynchronously generated suggestions:
let callback = function(newSugg) {
- self.addArgumentSuggestion(argName, newSugg);
+ if (newSugg.length) {
+ for each (let eachsug in newSugg)
self.addArgumentSuggestion(argName, eachsug);
+ } else
+ self.addArgumentSuggestion(argName, newSugg);
// send a notifcation to let the UI know to update the suggestion
list
Observers.notify(self, "ubiq-suggestions-updated", "");
};
@@ -589,7 +616,8 @@
}

for each( let sen in this._parsedSentences) {
- parsedSentences.push(sen.fillMissingArgsWithDefaults());
+ let newSentences=sen.fillMissingArgsWithDefaults();
+ for each (newOne in newSentences) parsedSentences.push(newOne);
}

return parsedSentences;

[gtdphp developers]

This is only a partial solution, so far.

One very minor problem - the line 5 lines from the end of the patch
should have been
for each (let newOne in newSentences) parsedSentences.push
(newOne);
(the let was missing)

But one major problem: this patch allows the number of suggestions to
grow beyond NLParser.MAX_SUGGESTIONS - does anyone have any quick
suggestion on where to fix that?

The extra suggestions are inaccessible, because the key-handler
prevents one going beyond the NLParser.MAX_SUGGESTIONS entry in the
list, even if there are more items. That's caused by
NLParser.Parser.getNumSuggestions, which returns
Math.min(NLParser.MAX_SUGGESTIONS,this._suggestionList.length)

And on a tangentially related subject, can we arrange things so that
the MAX_SUGGESTIONS constant is just specified in one place, and is
globally visible to parser.js, nountypes.js and user scripts (and is
maybe even a user-specified option, rather than a constant).
Currently, it gets specified in each of them (well, actually, in
nountypes.js, the value 5 is just hard-coded twice, rather than
specified as a constant and re-used).

Regards,
Andrew

[Fernando Takai]

Andrew,

I just fixed #423 that addressed that - now, with the latest tip
everything is fine. (only five suggestions appear and only five can be
selected)

--
Fernando "Takai"
http://flickr.com/photos/supeertakai
http://fernandotakai.jaiku.com

Get Ubiquity: https://wiki.mozilla.org/Labs/Ubiquity/

[gtdphp developers]

> I just fixed #423 that addressed that - now, with the latest tip
> everything is fine. (only five suggestions appear and only five can be
> selected)

Thanks Fernando. I think I just broke that bit again, with my patch :-
( But that gives me a good lead on how to fix it again.

And I need to work out how to deal with multiple default values for
one verb, if several verbs are to be suggested. I guess it's better to
have just one default value for each verb, and then add other
suggestions onto the end of the list if there's still room. I'll have
a play around with that, now.

Andrew

[gtdphp developers]

OK, I think I've fixed the issues. Patched against the current tip,
copied below for review. I'll upload it to trac ticket 385 once
labs.toolness.com is back up. Once again, the patch's purpose is to

handle noun types returning an *array* of possible default values, in
addition to the current tip which only handles a
single default value. It also tweaks the "suggest" callback to handle
arrays too.

Andrew


diff -r e87b6c7efdcc ubiquity/modules/parser/parser.js
--- a/ubiquity/modules/parser/parser.js Sun Nov 23 14:17:37 2008 -0200
+++ b/ubiquity/modules/parser/parser.js Mon Nov 24 14:55:49 2008 +0000

@@ -225,7 +225,6 @@
} else {
this._parsingsList = newSuggs;
}
-
this.refreshSuggestionList( query, context );
},

@@ -282,6 +281,7 @@
this._verb = verb;
this._argSuggs = argumentSuggestions;
}
+ this.duplicateDefaultMatchScore=100;
this.verbMatchScore = verbMatchScore;
this.frequencyScore = 0; // not yet tracked
this.argMatchScore = 0;
@@ -404,28 +404,59 @@

+ // reduce the match score so that multiple entries with the
+ // same verb are only shown if there are no other verbs
+ newSentences[i].duplicateDefaultMatchScore=
+ this.duplicateDefaultMatchScore/(i+1);

+ }
+ newSentences[i].setArgumentSuggestion(argName,defaultValue[i]);
+ }
+ } else {
+ for (let sen in newSentences) {
+ newSentences[sen].setArgumentSuggestion
(argName,defaultValue);
+ }
+ defaultsSoFar[argName]=defaultValue;
+ }
}
}
- return newSentence;
+ return newSentences;
},

getMatchScores: function() {

- return [this.frequencyMatchScore,
+ return [this.duplicateDefaultMatchScore,
+ this.frequencyMatchScore,
this.verbMatchScore,
this.argMatchScore];
},
@@ -463,7 +494,6 @@

let newSen = new NLParser.ParsedSentence(this._verb, {},
this._matchScore);
this._parsedSentences = [newSen];
for (let argName in this._verb._arguments) {
- let argSuggs = [];
if (argStrings[argName] && argStrings[argName].length > 0) {
// If argument is present, try the noun suggestions based both
on
// substituting pronoun...

@@ -497,7 +527,11 @@

let self = this;
// Callback function for asynchronously generated suggestions:
let callback = function(newSugg) {
- self.addArgumentSuggestion(argName, newSugg);
+ if (newSugg.length) {
+ for each (let eachsug in newSugg)

+ self.addArgumentSuggestion(argName, eachsug);

+ } else
+ self.addArgumentSuggestion(argName, newSugg);
// send a notifcation to let the UI know to update the suggestion
list
Observers.notify(self, "ubiq-suggestions-updated", "");
};

@@ -589,7 +623,7 @@

}

for each( let sen in this._parsedSentences) {
- parsedSentences.push(sen.fillMissingArgsWithDefaults());

+ parsedSentences=parsedSentences.concat
(sen.fillMissingArgsWithDefaults());
}

return parsedSentences;

[Blair McBride]

Looks good in general. I have a few small issues mostly relating to code
style:

* When checking to see if a value is an array, use: if(val instanceof
Array) {}

* Use spaces around comparison and assignment operators: val = 5 (as
opposed to val=5)

* If an 'if' statement has {} braces, then the 'else' statement should
too (there's an instance where this isn't the case, near the bottom of
the patch)


The last 2 points are to keep with the JavaScript style guide:
https://developer.mozilla.org/en/JavaScript_style_guide
(Unfortunately, not all of Ubiquity's code follows this yet.)


- Blair

> + if (numDefaults&& gotArrayOfDefaults || numDefaults===1) {

[gtdphp developers]

> I have a few small issues mostly relating to code style:

Ooh, thanks for that - I was going to ask about coding style.

> When checking to see if a value is an array, use:
> if(val instanceof Array) {}

There's something very odd going on with that. I did try it
originally, and had to use ".length" as a fallback. In one of the
places where I use ".length" to test, it's because "val instanceof
Array" simply wasn't behaving properly. I think it's the one around
line 419 of parser.js:

> defaultValue = missingArg.type.default();
> ...
> +  let numDefaults = defaultValue.length;

> +  if (numDefaults && gotArrayOfDefaults || numDefaults===1) {

For some peculiar reason that I just couldn't fathom, a test of
"defaultValue instanceof Array" *never* returns true, even when it is
an array. Very confusing. Any ideas as to why that might happen?

How are other people debugging javascript in the tip currently? I
would use Firebug+Chromebug, but (on Windows) Chromebug won't work
with 3.1beta, and Ubiquity tip *only* works on 3.1beta.

Regards,
Andrew

[gtdphp developers]

Hurray, trac's back! I'll move patch discussions onto trac 359 ("No
modifier suggestion given until first letter pressed"), as this patch
should address that trac ticket: http://labs.toolness.com/trac/ticket/359

[gtdphp developers]

I've still got the problem with instanceof Array.

Even if I hard-code my default function like this, to return an array
of two suggestions, a and b:

'default': function() {
return [CmdUtils.makeSugg('a'),CmdUtils.makeSugg('b')];
}

(a straightforward indisputable Array, right?)

then when I test it in
NLParser.ParsedSentence.fillMissingArgsWithDefaults , then instanceof
Array returns false! Its typeof is Object. It's got a length of 2.
But it's not an instanceof Array!

Am I going mad, or just missing something blindingly obvious? Is it
because the array has been generated in a sandbox? Is it because the
array is returned from a vanilla javascript function, but I'm testing
instanceof within a Javscript 1.7 module? Has the Array object been
modified somewhere in Ubiquity?

Andrew

[gtdphp developers]

OK, I think I've found the problem with instanceof Array.

defaultValue.indexOf() is not a function, when tested in
NLParser.ParsedSentence.fillMissingArgsWithDefaults

Whereas an array created within that function *does* have the indexOf
() method.

So defaultValue does not conform to the Array prototype *in the form
that parser.js recognises it*, and I think that's because parser.js is
a javascript 1.7 module, but my ubiquity command script is just an
ordinary javascript 1.5 file.

So, what do we do about that? It does kind of kill instanceof testing
here, doesn't it? Or can we / should we force ubiquity scripts to
conform to javascript 1.7?

Andrew

[Blair McBride]

Hmm, that's really odd (I seem to say that a lot) ... but I guess it
makes sense. And if that is indeed the case, then I don't see any way to
make "instanceof Array" work.

I had the smart idea of constructing a new array using arr.splice(0) -
before realizing you'd need to know if its an array first anyway, which
defeats the purpose. Still, it would be interesting to know if that worked.

We should hopefully soon have a new binary component (nsIUbiquity)
working flawlessly for both Firefox 3.0 and 3.1, which will allow us to
use JS 1.7 in commands. Its needed to work around a limitation in the
sandbox API Ubiquity currently uses, whereby commands only execute as JS
1.5. But for now, I think we have to assume that nsIUbiquity isn't
available, and things still need to work with the commands running as JS
1.5.

Another way around it would be to change CmdUtils.CreateCommand() to
wrap the 'default' function, so that it always returns an array. It
should be able to use "instanceof Array" there, as that's still in the
sandbox (running as JS 1.5). That way commands can use the old
(non-array) and new (array) method, but the parser always only gets the
new method (ie, only arrays).

Of course, a lot of this may change once the SafeBox architecture gets
setup (see http://www.toolness.com/wp/?p=356).


- Blair

[Blair McBride]

Er, sorry, ignore my paragraph about CmdUtils.CreateCommand() - no idea
how I managed to jump from nouns to commands.

- Blair

[Blair McBride]

I recently added Utils.isArray() to deal with this issue - it checks if
the object's constructor name is "Array". Anyway, I'll get Jono to look
over this before I merge it in - he understands the parser code much
better than I do.

- Blair