122154 - change array indexes to numeric and clean up query.atml javascript

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P1)

Description

[Christian Reis]

In the query.atml javascript, we use the product _name_ to index the component,
version and milestone arrays. This wastes HTML and also makes manipulation of
the contents a bit more involved.

There is also some cruft that can be removed from the code.

Comment 1

[Christian Reis]

Attachment: kiko v1

This patch includes restoreSelection() which is unused but is key to getting
83033 in. I can cut it out, but if it looks ok, leave it in as my 83033 patch
right now depends on it. Please. :-)

Comment 2

[Bradley Baetz (:bbaetz)]

Comment on attachment 66702 [details] [diff] [review]
kiko v1

>-function updateSelect(array, sel, target, sel_is_diff, single) {
>+  # - merge (boolean) determines if we are mergine in a diff or

s/mergine/merging/

r=bbaetz with that.

I played with it a bit, and it seemed identical.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

While you are at it, take a look at the version of the JavaScript code used in
the request tracker patch (bug 98801) and see if you can incorporate its changes
so that both sections of Bugzilla can use the same code base.

Comment 4

[Christian Reis]

Attachment: kiko_v3: s/merge/merging/

Comment 5

[Christian Reis]

Myk: are you proposing creating a separate .js file with the functions (merged
with your changes -- what are they?) so we can reuse them? There is a bug on
that filed.

If that could be done in a separate bug as soon as I have the blockers fixed I
would prefer it (post-2.16?) unless time is not a requirement. I fear it will
hamper both our patches acceptances since we will have to remod.

2r= anyone?

Comment 6

[Gervase Markham [:gerv]]

Please don't check this patch in until I've had a look at it, OK? :-) I hope to
do that tonight or tomorrow.

Gerv

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 66866 [details] [diff] [review]
kiko_v3: s/merge/merging/

Ok here's my review as promised.  This is just a quick glance at this code
without seeing it in all its glory, so I may have more to say later on.  I need
to apply this and see it in action before I stamp my r= on it. kiko what about
the landfill setup you promised me?  :)

>+function updateSelect(array, sel, target, merging) {
>         
>-    var i, comp;
>+    var i, item;
> 
>-    [%# if single, even if it's a diff (happens when you have nothing
>-        selected and select one item alone), skip this. %]
>-    if (!single) {
>+    [%# If we have no versions/components/milestones %]
>+    if (array.length < 1) {
>+        target.options.length = 0;
>+        return;

JS strict warning here for not returning a value.  maybe |return false;| ?

>+    }
>+
>+    [%# if multiple items selected, or if we are merging %]
>+    if (merging || sel.length > 1) {
>         [%# array merging/sorting in the case of multiple selections %]
>-        if (sel_is_diff) {        
>+        if (merging) {        
>             [%# merge in the current options with the first selection %]
<SNIP>
>     [%# load elements of list into select %]
>-    for (i = 0; i < comp.length; i++) {
>-        target.options[i] = new Option(comp[i], comp[i]);
>+    for (i = 0; i < item.length; i++) {
>+        target.options[i] = new Option(item[i], item[i]);
>     }

Please add a |return true;| right in here to avoid a strict warning (combined
with the one above).

> }
> 
>@@ -185,6 +208,36 @@
>     return ret;
> }
> 
>+[%# Returns an array of indexes.
>+  #    - control: select control from which to find selections
>+  #    - findall: boolean, dumping all options if all or just the selected 
>+  #     indexes. %]

Please align "indexes" with "findall" above it.

>+function getSelection(control, findall) {
>+    var ret = new Array();
>+
>+    if ((!findall) && (control.selectedIndex == -1)) {
>+        return ret;
>+    }
>+
>+    for (var i=0; i<control.length; i++) {
>+        if (findall || control.options[i].selected) {
>+            ret[ret.length] = i;
>+        }
>+    }
>+    return ret;
>+}
>+
>+[%# Selects items in control that have index defined in sel
>+  #    - control: SELECT control to be restored
>+  #    - sel: array of indexes in select form control %]
>+function restoreSelection(control, sel) {
>+    for (var s in sel) {
>+        control.options[sel[s]].selected = 1;

use true, not 1.

>+    }
>+}
>+
>+
>+
> [%# selectProduct reads the selection from f.product and updates
>   # f.version, component and target_milestone accordingly.
>   #     - f: a form containing product, component, varsion and
>@@ -202,7 +255,7 @@
>   #       changed, and optimize for additions. %]
> function selectProduct(f) {
>     [%# this is to avoid handling events that occur before the form
>-       itself is ready, which happens in buggy browsers. %]
>+       itself is ready, which could happen in buggy browsers. %]

Ultra-nit: please align "itself" with "this" above it

>     if ((!f) || (!f.product)) {
>         return;
>     }
>@@ -224,46 +277,35 @@
>     first_load = 0;
> 
>     [%# - sel keeps the array of products we are selected. 
>-        - is_diff says if it is a full list or just a list of products that
>-          were added to the current selection.
>-        - single indicates if a single item was selected %]
>+        - merging says if it is a full list or just a list of products that
>+          were added to the current selection. %]
>+    var merging;

Since |merging| is a bool and is assumed false unless explicitly set to true,
go ahead and initialize this as false.

>     var sel = Array();
>-    var is_diff = 0;
>-    var single;
> 
>     [%# if nothing selected, pick all %]
>     if (f.product.selectedIndex == -1) {
>-        for (var i = 0 ; i < f.product.length ; i++) {
>-            sel[sel.length] = f.product.options[i].value;
>-        }
>-        single = 0;
>+        sel = getSelection( f.product, 1 );

Nit: Can we kill the spaces here in the function call?

>     } else {
>-        for (i = 0 ; i < f.product.length ; i++) {
>-            if (f.product.options[i].selected) {
>-                sel[sel.length] = f.product.options[i].value;
>-            }
>-        }
>-
>-        single = (sel.length == 1);
>+        sel = getSelection( f.product, 0 );

Nit: Can we kill the spaces here in the function call?

> 
>-        [%# save last_sel before we kill it %]
>+        [%# save last_sel for the next invocation of selectProduct() %]
>         var tmp = last_sel;
>         last_sel = sel;
>     
>         [%# this is an optimization: if we have added components, no need
>            to remerge them; just merge the new ones with the existing
>            options. %]
>-        if ((tmp) && (tmp.length < sel.length)) {
>+        if ((tmp.length > 0) && (tmp.length < sel.length)) {
>             sel = fake_diff_array(sel, tmp);
>-            is_diff = 1;
>+            merging = 1;

Please use |merging = true;| instead.  No need to use integers when dealing
with booleans.

>         }
>     }
> 
>     [%# do the actual fill/update %]
>-    updateSelect(cpts, sel, f.component, is_diff, single);
>-    updateSelect(vers, sel, f.version, is_diff, single);
>+    updateSelect(cpts, sel, f.component, merging);
>+    updateSelect(vers, sel, f.version, merging);
>     if (usetms) {
>-        updateSelect(tms, sel, f.target_milestone, is_diff, single);
>+        updateSelect(tms, sel, f.target_milestone, merging);
>     }
> }
>

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Christian-

My changes are minor.  I genericize the JavaScript routines to handle arbitrary
product/component form fields rather than a single one, and I put the code into
a separate file.  Yes, this can be done in a separate bug, but take a look if
you get a chance and see if you can integrate it.
 

Comment 9

[Gervase Markham [:gerv]]

I asked you to wait to make sure you weren't changing bits of this template I
cared about. Fortunately, I find that you aren't :-) Apart from the fact that we
should probably avoid calling Param('usetargetmilestone') once for each product
by using a local variable, I'm happy with this. I haven't reviewed it closely
enough to give it r=, though, as caillon seems to be on the case, and I have
other work :-)

Gerv

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 66866 [details] [diff] [review]
kiko_v3: s/merge/merging/

More comments.	:)  The entire block below should be better written as:

  if (merging) {
    ...
  }
  else if (sel.length > 1) {
    ...
  }
  else {
    ...
  }

>+    if (merging || sel.length > 1) {
>         [%# array merging/sorting in the case of multiple selections %]
>-        if (sel_is_diff) {        
>+        if (merging) {        
>             [%# merge in the current options with the first selection %]
>-            comp = merge_arrays(array[sel[0]], target.options, 1);
>+            item = merge_arrays(array[sel[0]], target.options, 1);
> 
>             [%# merge the rest of the selection with the results %]
>             for (i = 1 ; i < sel.length ; i++) {
>-                comp = merge_arrays(array[sel[i]], comp, 0);
>+                item = merge_arrays(array[sel[i]], item, 0);
>             }
>         } else {
>             [%# here we micro-optimize for two arrays to avoid merging with a
>                 null array %]
>-            comp = merge_arrays(array[sel[0]],array[sel[1]], 0);
>+            item = merge_arrays(array[sel[0]],array[sel[1]], 0);
> 
>             [%# merge the arrays. not very good for multiple selections. %]
>             for (i = 2; i < sel.length; i++) {
>-                comp = merge_arrays(comp, array[sel[i]], 0);
>+                item = merge_arrays(item, array[sel[i]], 0);
>             }
>         }
>     } else {
>         [%# single item in selection, just get me the list %]
>-        comp = array[sel[0]];
>+        item = array[sel[0]];
>     }
>-
>+    

And why are you adding whitespace up there?  Hmmmmmm?  :)

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

This blocks a blocker so this is a blocker.

Comment 12

[Christian Reis]

Attachment: kiko_v5: fix gerv's request and caillon's comments. THIS IS READY (yeah right)

(yes, there have been hidden versions)

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 67022 [details] [diff] [review]
kiko_v5: fix gerv's request and caillon's comments.  THIS IS READY (yeah right)

r=caillon, though I don't like the loops starting with different values. 
They're commented fine and for the sake of getting 2.16 out in a timely fashion
I won't hold review for it because it's debatable how we should do that.  It's
not a big deal though and I've talked with kiko about this and he's promised me
to do that right later on for 2.18.

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 67022 [details] [diff] [review]
kiko_v5: fix gerv's request and caillon's comments.  THIS IS READY (yeah right)

OK, this works. Note that gerv's comment about Param('foo') being expensive is
not true - its just a global hash.

Given that, I'd prefer that the 'var tms;' is removed entirely, and the
conditionals turned into template conditionals- thats one of the reasons we use
templates. However, since thats part of the current code, I won't make you
change it. You may want to do so as part of one of your followup patches,
though.

It appears to work, so r=bbaetz.

Comment 15

[Christian Reis]

Attachment: kiko_cleanup_v1: remove usetms as per bbaetz' comment

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 67123 [details] [diff] [review]
kiko_cleanup_v1: remove usetms as per bbaetz' comment

r=gerv.

Gerv

Comment 17

[Christian Reis]

Comment on attachment 67022 [details] [diff] [review]
kiko_v5: fix gerv's request and caillon's comments.  THIS IS READY (yeah right)

kiko_v5 was checked in, thanks bbaetz

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 67123 [details] [diff] [review]
kiko_cleanup_v1: remove usetms as per bbaetz' comment

r= justdave

Comment 19

[Bradley Baetz (:bbaetz)]

Comment on attachment 67123 [details] [diff] [review]
kiko_cleanup_v1: remove usetms as per bbaetz' comment

Theres still a: var tmp = new Array(); line there unconditionally (just below
the frist hunk on the diff)

Comment 20

[Christian Reis]

Attachment: kiko_cleanup_v2: fix per bbaetz' smart catch.

Sorry.

Comment 21

[Christian Reis]

Attachment: kiko_cleanup_v3: final version (uh-huh)

Sorry. That patch had more mumbo jumbo from bug 83033 stuck in it. This patch
is clean.

Comment 22

[Bradley Baetz (:bbaetz)]

Comment on attachment 67345 [details] [diff] [review]
kiko_cleanup_v3: final version (uh-huh)

r=bbaetz

Comment 23

[Jacob Steenhagen]

Comment on attachment 67345 [details] [diff] [review]
kiko_cleanup_v3: final version (uh-huh)

Looks good to me
r=jake

Comment 24

[Jacob Steenhagen]

Checking in template/default/query/query.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/query/query.atml,v  <-- 
query.atml
new revision: 1.4; previous revision: 1.3
done