Created attachment 39507 [details] [review]
Patch to enable support for multi-valued properties.

Patch is shaping up.  Here are some comments:

* Throughout the code, there are still minor style issues, like spaces after
for, foreach, and if and before the open parenthesis.

* In Hit.cs, use ICollection everywhere instead of IList

* I know I said otherwise in an earlier review, but I am thinking now that
returning null from the Hit indexer might be better than the empty array.  A
large chunk of the patch is changing a ton of existing code from doing "if
(hit["Foo"] == null)" to "if (hit["Foo"].Count == 0)" for no real benefit.  It's
probably simpler to do null checks before foreach loops in the many fewer places
where that is used presently than changing all the null checks.  We can always
change it later if we have to, but I don't think we will.

* In both SetValue() functions if key is null or trims to "" we should probably
throw an exception rather than silently exiting.

* Also in SetValue(), there is no need to do properties.Contains(key).  Just do
the Remove(), it doesn't throw an exception if it isn't there.

* This is totally a nitpicky thing, but when dealing with a string "value" is a
good name, but when dealing with an ICollection I'd prefer if they were named
"values" for clarity.

* In ReplaceValue() you're doing a check that is essentially "if (this [key] ==
null)" but your implementation of the indexer never allows null to be returned.
 If you change the thing I suggested above that'll be fixed. :)  But as it is
now, it'll never work as long as it is previously unset.

* I don't like some of the naming.  I suggest renaming GetValuesAsString() as
GetValuesAsArray() and rely on the fact that they're always dealt with as
strings and that it returns string[] to get that idea across.

* Similarly, I'd rename GetValuesAsDelimitedString() to GetValuesAsString() and
have two versions: one in which you specify the delimiter and one which defaults
to a delimiter of ", ".  I guess this is sort of what
Beagle.Util.StringFu.GetValuesAsString() does?  This stuff seems kinda messy.

* Rename IsKeyContains() to just KeyContains().

* In Tiles/Template.cs, does it make sense to switch the Template over to using
ICollections instead of strings?

* I don't understand the use of Beagle.Util.StringFu.Escape() and UnEscape() in
Template.cs.  Why not just pass "," into UnEscape() and have it do the work of
whatever Escape() is doing?

* Almost everywhere I think you should probably use GetValuesAsDelimitedString()
instead of GetValuesAsString()[0].  Lets just try to get this as right the first
time so that we don't have to keep going back and cleaning up these things.  The
only thing that might give you trouble here are email addresses in the mail backend.

* You have a FIXME in TileContact.cs which says "a Hit cannot have two icons". 
I agree, but if you're going to enforce that I'd rather see it done with a
Debug.Assert() rather than just accessing the 0th element.  Maybe you could have
an overload of GetValuesAsDelimitedString() which enforces the number (or
maximum number) of elements allowed.

* I don't like the conversion from a char to a byte in StringFu.cs.  A char is
two bytes in .net and a byte is one, so you can overflow if you use a big enough
byte.  The conversion also uses single chars, which is kind of limiting. 
Normally you want a delimiter to be ", " and not just ','.  Or maybe you want it
to be "<br/>" for HTML.  So I think the escaping needs to be rethought here.

* Lose the static StringBuilder.

* I think SplitDelimitedString() will probably have null passed in very
infrequently.  It should probably either throw a ArgumentNullException or just
return "new string[0]".  Ie, lose the static emptyArray.

* Why is the code commented out in ExtractContent.cs?  Is that leftover from
something else?

* In GaimLogQueryable, it's probably good to enforce a single "fixme:file" element.

* The mail backend should be looked at, particularly because of the way it
parses email addresses.  That's the prime candidate to take advantage of the
multiple values per key thing right now.

oops, some typos:

* Throughout the code, there are still minor style issues, like spaces after
for, foreach, and if and before the open parenthesis.

Of course I mean that you don't have these spaces and should add them.  Also
around mathematical operators (ie, "a + b" good, "a+b" bad)

And one other thing: I suggest renaming UnEscape() to Unescape().  Easier to
type and read, and is more consistent with other code, I think.

"* Why is the code commented out in ExtractContent.cs?  Is that leftover from
something else?"

--- EEEEEK!! Sorry, actually was testing some .doc optimization.  Thanks for
your "(e)beagle-eyed" patch-review. ;-)

"* In Tiles/Template.cs, does it make sense to switch the Template over to using
ICollections instead of strings?"

--  I just had a sneak-preview of the TilesXXX code that use Template[] stuff. 
In most cases we are just using Hit[] directly than Template[] stuff.  Moreover,
the indexer in Template is used to hold-beautified-or-other-forms-of-strings
only.  So, I feel its better to have strings than ICollection in Template.cs.

Joe: What do you say?

Ok, more little thoughts...

If we use ICollection, we will have to update Tiles/templates-*.html accordingly.

And, we will have to provide all sort of conversion methods from ICollection to
String(s) just like Hit.cs, which is kind of replication, since, methods in
Hit.cs are directly accessible in Template class and also template class is
being used for just "presentation" purpose, I think its good to leave it as it
is to use strings.

If any one wants to have more control on the way multiple-values of a property
are rendered, he can directly use the indexer Hit[] and convert them the way he
wants and store it in the Template[] for rundering purposes.

Hmm.. 

As we are providing GetValuesAsArray() that returns a string[], I think we can
avoid "Escaping" and "Unescaping" delimiter string in GetValuesAsString(), which
will be purely for "displaying" purpose and not for data manipulation.

Also, marking the current patch as obsolete.

Created attachment 45230 [details] [review]
updated multi-valued-property patch

This is now fixed in CVS.