'Find-as-you-type'-search in dartdoc/apidoc.

'Find-as-you-type'-search in dartdoc/apidoc.

Committed: https://code.google.com/p/dart/source/detail?r=11508

[Lasse Reichstein Nielsen, 2012-08-20 13:07:58]

Looks odd to me, but ok if it works.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/client-...
File pkg/dartdoc/client-live-nav.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/client-...
pkg/dartdoc/client-live-nav.dart:37: buildNavigation(List libraries) {
Better, but List of what?

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/dartdoc...
File pkg/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/dartdoc...
pkg/dartdoc/dartdoc.dart:618: <div class="drop-down" id="drop-down"></div>
Why both id and class? Are there any other elements with class drop-down?

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/dartdoc...
pkg/dartdoc/dartdoc.dart:699: // Ignore.
I still don't like this.
It's really a bad idea to ignore an exception without checking what caused it.
Maybe it isn't "file already exists"?

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/mirrors...
File pkg/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/mirrors...
pkg/dartdoc/mirrors/dart2js_mirror.dart:1232: _constructorName =
_name.substring(dollarPos+1);
Spaces around '+'.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
File pkg/dartdoc/search.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:31: * substring within a [text]. The matched [text] is
split into the [prefixText],
What variable does [text] refer to?

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:32: * [infixText], and [suffixText], where [infixText]
is the part matching the
Why not just call it "matchText"? And perhaps "preMatchText"/"postMatchText".
Are they always needed, or could you just store the position of the match and
compute them dynamically if needed?

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:60: * Is `true` iff [searchText] matches the full [text]
case-sensitively.
Drop the quoting around 'true'. If you want something different, write it
[:true:].

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:156: String infixText = text.substring(offset,
offset+searchText.length);
Spaces around '+'. Ditto next line.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:163: int compareBools(bool a, bool b) {
Document that it considers true bigger than false.
And that it doesn't handle null.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:172: else return 0;
Aren't numbers comparable? I.e., a.compareTo(b)?

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:175: int resultComparator(Result a, Result b) {
That's a lot of work to do in a sort comparison function.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:210: result =
a.type.toLowerCase().compareTo(b.type.toLowerCase());
This is particularly worrisome. You can have a quadratic number of comparisons
in a sort (worst case) and doing case conversion on both comparands seems likely
to explode.

Then again, we should probably have "String.compareNoCase" to handle that, and
we don't.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:220: DivElement dropdown;
Is this a global variable? That isn't even private?!?
Ick. 
Argh.
Ok, but still!!!

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:281: StringMatch memberMatch = obtainMatch(emptyText,
member[NAME]);
Line too long.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:324: }
These huge blobs of complex code would be great as separate functions.
Preferably with a meaningful name and good documentation.
I.e., I have no idea what happens here, and I can't read the code.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:492: if (event.keyCode == 70/*F*/ && event.ctrlKey ||
I prefer hex for keycodes. I.e., 0x46.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/static/...
File pkg/dartdoc/static/styles.css (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/static/...
pkg/dartdoc/static/styles.css:153: }
empty lines between declarations.

[Johnni Winther, 2012-08-23 10:19:50]

PTAL

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/client-...
File pkg/dartdoc/client-live-nav.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/client-...
pkg/dartdoc/client-live-nav.dart:37: buildNavigation(List libraries) {
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Better, but List of what?

It is JSON, so List<Map<String,...?...>>, currently. I'll put types on all
for-each loop variables instead so expected type is documented.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/dartdoc...
File pkg/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/dartdoc...
pkg/dartdoc/dartdoc.dart:618: <div class="drop-down" id="drop-down"></div>
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Why both id and class? Are there any other elements with class drop-down?

I just dislike referring to 'id' in CSS and 'class' in single-point queries, so
'id' for element identity and 'class' for its style.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/dartdoc...
pkg/dartdoc/dartdoc.dart:699: // Ignore.
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> I still don't like this.
> It's really a bad idea to ignore an exception without checking what caused it.
> Maybe it isn't "file already exists"?

There seems to be a solution to 3914 underway.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/mirrors...
File pkg/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/mirrors...
pkg/dartdoc/mirrors/dart2js_mirror.dart:1232: _constructorName =
_name.substring(dollarPos+1);
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Spaces around '+'.

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
File pkg/dartdoc/search.dart (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:31: * substring within a [text]. The matched [text] is
split into the [prefixText],
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> What variable does [text] refer to?

To the property, that is the getter 'text'.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:32: * [infixText], and [suffixText], where [infixText]
is the part matching the
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Why not just call it "matchText"? And perhaps "preMatchText"/"postMatchText".
> Are they always needed, or could you just store the position of the match and
> compute them dynamically if needed?

They are used extensively so I prefer creating them once.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:60: * Is `true` iff [searchText] matches the full [text]
case-sensitively.
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Drop the quoting around 'true'. If you want something different, write it
> [:true:].

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:156: String infixText = text.substring(offset,
offset+searchText.length);
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Spaces around '+'. Ditto next line.

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:163: int compareBools(bool a, bool b) {
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Document that it considers true bigger than false.
> And that it doesn't handle null.

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:172: else return 0;
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Aren't numbers comparable? I.e., a.compareTo(b)?

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:175: int resultComparator(Result a, Result b) {
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> That's a lot of work to do in a sort comparison function.

Yes, but we have a lot of situations to handle!

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:210: result =
a.type.toLowerCase().compareTo(b.type.toLowerCase());
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> This is particularly worrisome. You can have a quadratic number of comparisons
> in a sort (worst case) and doing case conversion on both comparands seems
likely
> to explode.
> 
> Then again, we should probably have "String.compareNoCase" to handle that, and
> we don't.

Yes, I looked for that!

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:220: DivElement dropdown;
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Is this a global variable? That isn't even private?!?
> Ick. 
> Argh.
> Ok, but still!!!

This is a web page and the DOM is global!

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:281: StringMatch memberMatch = obtainMatch(emptyText,
member[NAME]);
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> Line too long.

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:324: }
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> These huge blobs of complex code would be great as separate functions.
> Preferably with a meaningful name and good documentation.
> I.e., I have no idea what happens here, and I can't read the code.

Refactored into several methods.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/search....
pkg/dartdoc/search.dart:492: if (event.keyCode == 70/*F*/ && event.ctrlKey ||
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> I prefer hex for keycodes. I.e., 0x46.

Done.

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/static/...
File pkg/dartdoc/static/styles.css (right):

https://chromiumcodereview.appspot.com/10829361/diff/3001/pkg/dartdoc/static/...
pkg/dartdoc/static/styles.css:153: }
On 2012/08/20 13:07:58, Lasse Reichstein Nielsen wrote:
> empty lines between declarations.

Done.