636416 - Use the standard value-controller javascript to control the fields on the search page

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Max Kanat-Alexander]

Right now we have two sets of javascript for showing/hiding field values--one used by show_bug.cgi for the "value controller" system, and another used by query.cgi to limit down the Product, Component, etc. boxes. We should combine these systems into one system, namely a modification of the "value controller" system (because it is considerably more flexible, the data it uses--ids--takes up considerably less space than the names of field values, and also it can [and does] preserve the sort order of the existing values in the arrays).

This may require modifying the value-controller system so that it can:

(1) Work off of a data structure instead of one event handler per field value (this would be better for show_bug too, in any case).

(2) Coalesce multiple duplicate values into one value.

Comment 1

[Max Kanat-Alexander]

Attachment: Work In Progress

Here's some work in progress. This currently works with the classification field to control the product field. Now I just have to make the Product field control the other three fields.

Comment 2

[Max Kanat-Alexander]

Attachment: WIP 2

This is a more-advanced work-in-progress patch. Currently it doesn't properly always narrow down the Component/etc. fields when I select a new Classification.

Comment 3

[Max Kanat-Alexander]

Attachment: v1

Okay, this works, and it's very speedy with a small number of products. However, with 2000 products (and about 50,000 components) it's currently VERY slow.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 515019 [details] [diff] [review]
v1

Okay, further testing shows that this may actually be just as fast as our current code, which is also VERY slow when you have that many products/components/milestones/versions. 

At the very least, the new code should be theoretically easier to optimize. The slowness (on non-Gecko browsers) is coming from replaceChild, and I'm experimenting with ways to make it faster (although that may just depend on browsers making replaceChild faster).

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Okay, further testing shows that this may actually be just as fast as our
> current code, which is also VERY slow when you have that many
> products/components/milestones/versions. 

  Further testing shows that this new code is in fact slightly faster than our existing code. :-)

Comment 6

[Max Kanat-Alexander]

  glob: I would really love a review on this code whenever you can do it.

Comment 7

[Frédéric Buclin]

This is too invasive for 4.2, right?

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> This is too invasive for 4.2, right?

  It may help fix a bug--the problem where your Target Milestone selection disappears when you go back. If we can confirm it fixes that bug, then it should go into 4.2.

Comment 9

[:glob ✱]

i haven't forgotten about this review; i've been working through it, sorry for the delays.  bug 584742 is painful, and i'd really like to see a fix for that in 4.2.

Comment 10

[:glob ✱]

Comment on attachment 515019 [details] [diff] [review]
v1

r=glob

this looks and functions well.

Comment 11

[Max Kanat-Alexander]

Thank you so much, glob! You are a hero. :-)

Comment 12

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified query.cgi
modified js/field.js
modified js/util.js
modified template/en/default/search/field.html.tmpl
modified template/en/default/search/form.html.tmpl                             
modified template/en/default/search/search-advanced.html.tmpl
modified template/en/default/search/search-create-series.html.tmpl
modified template/en/default/search/search-report-graph.html.tmpl
modified template/en/default/search/search-report-table.html.tmpl
Committed revision 7907.

Comment 13

[Edmund Yan]

I think there is a bug with glob's change.

query.cgi:63 in get_product_values()
-----
-    if ($field eq 'version') {
+    if ($field eq 'versions') {

Or at least when I make this change on my local trunk install, versions are correctly sorted.  Is this a revert or should I submit a patch to fix the bug?.

On a side note, I believe this was a blocker for bug 463894, which has now been resolved in trunk.

Comment 14

[Edmund Yan]

Sorry for the double post, but while we are here, I believe $field in get_product_values() is always the plural case.  So any conditionals should switch to the plural case.

eq 'version' --> eq 'versions'
eq 'component' --> eq 'components'

Comment 15

[Frédéric Buclin]

(In reply to Edmund Yan from comment #14)
> Sorry for the double post, but while we are here, I believe $field in
> get_product_values() is always the plural case.  So any conditionals should
> switch to the plural case.
> 
> eq 'version' --> eq 'versions'
> eq 'component' --> eq 'components'

No, the only error is that $field =~ s/s$//; should come before if ($field eq 'version'). I fixed that right now:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified query.cgi
Committed revision 7947.