216557 - Be able to specify the order of the columns in a bug list

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P4)

Description

[Jon]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686) Gecko/20030428 Galeon/1.3.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686) Gecko/20030428 Galeon/1.3.3

It would be nice if I could specify the order in which the columns appear in a
buglist.  For example, if I wanted the owner column last(right most column) or
the OS before the platform column I could specify that.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Frédéric Buclin]

*** Bug 236676 has been marked as a duplicate of this bug. ***

Comment 2

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 3

[Pascal Held]

I would implement this feature...

Comment 4

[Pascal Held]

Attachment: v1

I've put two select boxes to the column change page where you can select your columns. You can also change the order of the columns.

Comment 5

[timeless]

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

>Index: template/en/default/admin/params/bugfields.html.tmpl
>+  usecolumnssorting => "If this is activated you can sort the columns in the buglist." }
> %]

I'd rather "usecolumnsorting" (use column sorting)
>\ No newline at end of file

someone should fix this.

>Index: template/en/default/list/change-columns.html.tmpl
>+          <th style="text-align:center">selected columns</th>
>+          <th style="text-align:center">availible columns</th>

These should probably be "Selected columns" and "Available columns" (note the case and the corrected spelling)
>+            <select name="availible_columns" id="availible_columns" 

please fix the spelling everywhere :)


>Index: images/right.png

don't include binaries in patches, create attachments (e.g. a zip) for them

comments about spacing/whitespace/tabs/tailing space/etc. from the other bug apply here.

Comment 6

[Pascal Held]

Attachment: v2

I fixed the things from the last review.

Comment 7

[Pascal Held]

Attachment: Images for the patch

Comment 8

[Frédéric Buclin]

CC'ing pyrzak, because I'm going to ask him something in the coming review comments. :)

Comment 9

[Frédéric Buclin]

Comment on attachment 331918 [details] [diff] [review]
v2

>Index: Bugzilla/Config/BugFields.pm

>+  {
>+   name => 'usecolumnsorting',
>+   type => 'b',
>+   default => 1
>   } );

We don't need a parameter for that. This feature should always be available.



>Index: template/en/default/list/change-columns.html.tmpl

>-  # Contributor(s): Dave Lawrence <[e-mail]>
>+  # Contributor(s): Pascal Held <[e-mail]>
>+  #                 Dave Lawrence <[e-mail]>

Add yourself at the *end* of the list. You are not the primary author of the file.


>+[% IF Param("usecolumnsorting") %]
>+  [% PROCESS global/header.html.tmpl
>+    title = "Change Columns"
>+    javascript_urls = "change-columns.js"
>+    onload = "initChangeColumns()"
>+  %]
>+[% ELSE %]
>+  [% PROCESS global/header.html.tmpl
>+    title = "Change Columns"
>+  %]
>+[% END %]

Don't duplicate the code. Note that this won't be a problem anymore as I said we don't want a parameter to control this feature.


>+    var cc_all_fields=new Array();
>+    var cc_field_desc=new Object();

Do not use cc_*. These names make us think at the CC list of users in bugs, and this may confuse some of us. Maybe mv_* for "move"? Or even all_fields and fields_desc alone. Same remark for other cc_* names.


>+        <tr>
>+          <th style="text-align:center">Selected Columns</th>
>+          <th></th>
>+          <th style="text-align:center">Available Columns</th>
>+        </tr>

You should shift both fields. At the left available columns and at the right selected ones. This way we are consistent with other pages of the same kind (e.g. reclassification in editclassifications.cgi and group inheritance in editgroups.cgi). This also looks more logical to me.


>+          <td style="valign:center;text-align:center">
>+            <button type="button" name="select" value="select" onclick="cc_select()">
>+              <img src="images/left.png">
>+            </button>
>+            <br><br>
>+            <button type="button" name="deselect" value="deselect" onclick="cc_deselect()">
>+              <img src="images/right.png">
>+            </button>
>+            <br><br>
>+            <button type="button" name="up" value="up" onclick="cc_up()">
>+              <img src="images/up.png">
>+            </button>
>+            <br><br>
>+            <button type="button" name="down" value="down" onclick="cc_down()">
>+              <img src="images/down.png">
>+            </button>
>+          </td>

I think only the left and right arrows should be in the middle (between the two select fields). The up and down arrows should be at the far right.



>Index: change-columns.js

I didn't look at this JS file at all as I'm not a JS expert. I will let pyrzak review it.


Now globally, I must say I like the UI *a lot* (yes, I applied your patch on my installation). My only concern is that if the user has turned off JS in his web browser, he no longer can select columns to display. I don't really care if he cannot reorder columns without JS but he should at least be able to select which ones to display. So a reasonable fallback is required. pyrzak, any idea how to do this? My idea was to have only one multi-select field (instead of the two above, but only when JS is off!), and only selected rows would be displayed. The user would have no control on the order of columns, but as I said, I don't really care.

Comment 10

[Frédéric Buclin]

Comment on attachment 331919 [details]
Images for the patch

These arrows are fine and correctly unzip them in images/. r=LpSolit assuming you created them yourself (legal issues).

Comment 11

[Pascal Held]

Yes I created them myself with incscape ;-)

Comment 12

[Pascal Held]

Attachment: v3

I fix all thinks from the review. Also the no-JS-Fallback. In this case the old UI will be show.

Comment 13

[Guy Pyrzak]

So as far as UI feedback.
1. Don't use inline style unless it is 100% necessary. Things like "display:none" are ok (some would disagree), but otherwise all style changes should be CSS driven. So R- on that
2. I personally prefer using document.getElementById. IMO It makes your code more readable and also causes your code not to break if people want to move your elements around on the page. So it generally makes your code more flexible, however, nothing is wrong with what you're doing so eh.
3. If we're going to make this not work without JS why are we being so meager in the implementation? I'd like to see a drag and drop UI using YUI (also just my opinion). But just my opinion, shouldn't block R+
4. I think this should work without JS. Think netflix! Have text boxes that indicate the order and figure out the right order on the back end. Not sure if this should block R+. In general we have tried to say if a feature CAN work without JS it should.
5. We like to avoid relying on images in general. We should also be using sprites and css so that it can be skinned changed. so R- on that also.
6. Missed a few semicolons.

Comment 14

[Guy Pyrzak]

I'm not sure if this is the new code or not, but moving the last column to the first column position causes all columns to be lost. I could debug this... but I don't want to.

Steps to reproduce. Select the last column. Move it to the first column, done on Firefox 2 on the mac.

Comment 15

[Frédéric Buclin]

Comment on attachment 331924 [details] [diff] [review]
v3

(In reply to comment #13)
> So as far as UI feedback.
> 1. Don't use inline style unless it is 100% necessary. Things like
> "display:none" are ok (some would disagree), but otherwise all style changes
> should be CSS driven. So R- on that

I agree, move style outside templates, even for display:none.


> 2. I personally prefer using document.getElementById.


I also agree. That's the usual way to do it.


> in the implementation? I'd like to see a drag and drop UI using YUI

I always thought that drag & drop was less intuitive and less discoverable. I wouldn't block r+ on this.


> 4. I think this should work without JS. Think netflix! Have text boxes that
> indicate the order and figure out the right order on the back end.

Huh? Which UI do you have in mind? A mockup, please.


> 5. We like to avoid relying on images in general. We should also be using
> sprites and css so that it can be skinned changed. so R- on that also.

Arrows are fine and are perfectly understandable. I like them.


My r- is about the first two comments, which are perfectly valid.

Comment 16

[Guy Pyrzak]

Attachment: Netflix Example

Here is what netflix does. In this case it is specifying the order of the movies i will receive. I'm not saying I like it more than what is there right now (in the current mock) but here is an example of how others have handed ordering without javascript.

Comment 17

[Guy Pyrzak]

(In reply to comment #15)
> > 5. We like to avoid relying on images in general. We should also be using
> > sprites and css so that it can be skinned changed. so R- on that also.
> 
> Arrows are fine and are perfectly understandable. I like them.
> 
> 
> My r- is about the first two comments, which are perfectly valid.
> 

If we're going to use 4 small images on one page we should be using sprites to help with download speed/performance see this website for why: http://www.websiteoptimization.com/speed/tweak/css-sprites/

see this website to make them:
http://spritegen.website-performance.org/

If we want to use images, we should make sure it doesn't effect performance. 

Comment 18

[Frédéric Buclin]

(In reply to comment #16)
> Here is what netflix does.

This UI has already been suggested in bug 206425, but justdave didn't like it, see bug 206425 comment 18. He prefered the "move up", "move down" idea. Now even if we take your mockup for browsers without JS, this means having two completely distinct UI, with and without JS, for a page which is not accessed very often. Do we really want to go in this direction and maintain two UI?


(In reply to comment #17)
> If we're going to use 4 small images on one page we should be using sprites to
> help with download speed/performance

We *could* do it, but it's not *required* IMO. There is a huge difference between pages on e.g. AOL.com with tens of icons on a single page and Bugzilla with only a few icons. This perf improvement could/should be done in a later bug if we see any perf benefit. The current contribution is very valuable.

Comment 19

[Guy Pyrzak]

(In reply to comment #18)
> (In reply to comment #16)
> > Here is what netflix does.
> 
> This UI has already been suggested in bug 206425, but justdave didn't like it,
> see bug 206425 comment 18. He prefered the "move up", "move down" idea. Now
> even if we take your mockup for browsers without JS, this means having two
> completely distinct UI, with and without JS, for a page which is not accessed
> very often. Do we really want to go in this direction and maintain two UI?
That was all I wanted to hear. I honestly wasn't a huge fan of the 2 ui option, but I wanted to make sure it was discussed and discounted b/c we are deploying a feature that will not work without JS. Seems like this is the case and I'm happy with the decision. Thanks.

As far as the performance tweak, fine by me, I'm never sure which performance enhancements are preferred vs not. The images should probably be applied via CSS though so they can be skinned, not all people will want red arrows. 

Comment 20

[Pascal Held]

Attachment: v4

- I add the missing semicolons in the JS
- I removed the style tags (I see I don't need them)
- I changed the access to the fields to document.getElementById...

Comment 21

[Pascal Held]

I didn't know how to move the images to css. Can somebody help me? I think I have to move the image files to "/skins/standard/" but how to reference them by css and not by html?

Comment 22

[Guy Pyrzak]

Here is an example of the css to put buggy as the background of an image.
.button_exmple{
background-image:url(https://bugzilla.mozilla.org/attachment.cgi?id=300417);
background-repeat:no-repeat;
}

Comment 23

[Pascal Held]

Attachment: v4-2

sorry, I had a problem with the last patch

Comment 24

[Pascal Held]

Attachment: v5

I've moved the images to css.

Comment 25

[Pascal Held]

Attachment: images in css directory

The same Images, but now in the css directory

Comment 26

[Frédéric Buclin]

Comment on attachment 332329 [details]
images in css directory

These images must go into skins/standard/global/

Comment 27

[Frédéric Buclin]

Comment on attachment 332327 [details] [diff] [review]
v5

I don't know what's wrong with this patch, but the order I choose is ignored. The buglist is displayed exactly the same way as before.

Also, it seems you removed the ability to store columns with the saved search itself, so that each saved search can have its own list of columns to display. This feature must be restored.

Comment 28

[Frédéric Buclin]

Comment on attachment 332327 [details] [diff] [review]
v5

>Index: change-columns.js

One more thing. This JS file must be in js/. There should be no JS file in the bugzilla root directory.

Comment 29

[Pascal Held]

Attachment: Images - v3

I put them to skins/standard/global

Comment 30

[Pascal Held]

Attachment: v6

I removed the no-js-interface and modify the other so it can be used without js.

Comment 31

[Pascal Held]

Attachment: v7

I fixed the bugs.

Comment 32

[Frédéric Buclin]

Comment on attachment 332890 [details]
Images - v3

Those are fine, r=LpSolit

Comment 33

[Frédéric Buclin]

err... sorry, but there is a problem with Bugzilla right now. All my review comment has been lost somewhere in the air. And it seems we cannot edit attachments anymore for some unknown reason. I will try to repeat what I said in my review comment in a moment.

Comment 34

[Frédéric Buclin]

Comment on attachment 332895 [details] [diff] [review]
v7

OK, my review comments have been lost because I disabled JS while testing your patch. And Bugzilla is unable to edit attachments when JS is off! :(

So I will try to remember what I wrote a few minutes ago: (*sigh*)



>Index: skins/standard/global.css


>+#select_button {
>+    background-image:url(global/right.png);
>+    background-repeat:no-repeat;
>+    background-position: center center;
>+    width: 30px;
>+}

Wouldn't it be easier to have a single block for common rules (background-repeat, background-position and width) and only specify background-image individually? This would let us avoid some rules duplication.



>Index: template/en/default/list/change-columns.html.tmpl

>+          <select name="available_columns" id="available_columns" 
>+             size="15" multiple="multiple" onchange="updateView();"
>+              style="display:none"> 
>+          </select>

Why is this list populated by JS? Why not populating it directly in the template? Is there any special reason for this?


>+          <select name="selected_columns" id="selected_columns" 
>+              size="15" multiple="multiple" onchange="updateView();">
>+            [% FOREACH column = collist %]
>+                <option value="[% column %]" selected="selected">
>+                [% (field_descs.${column} || column) FILTER html %]
>+              </option>
>+            [% END %]
>+            [% FOREACH column = masterlist %]
>+              [% IF lsearch(collist, column) == -1 %]
>+                <option value="[% column %]">
>+                  [% (field_descs.${column} || column) FILTER html %]
>+                </option>
>+              [% END %]
>+            [% END %]
>+          </select>

When JS is turned off, you get this single list with currently displayed columns at the top, followed by remaining available columns below them. This means they no longer appear in the order specified by @masterlist. Is that intentional? Is this the desired behavior?
Also, when you hit F5 on Firefox to refresh the page, this list disappears. Could you check this and fix it?


>+<script type="text/javascript">
>+  document.getElementById("avail_header").style.display="";
>+  document.getElementById("available_columns").style.display="";
>+  document.getElementById("select_button").style.display="";
>+  document.getElementById("deselect_button").style.display="";
>+  document.getElementById("up_button").style.display="";
>+  document.getElementById("down_button").style.display="";
>+</script>

Couldn't it be possible to have this in e.g. initChangeColumns() to not have all this JS code in the template?



>Index: colchange.cgi

>+        if (defined $cgi->param("selected_columns")) {

Now that the template passes "selected_columns" to colchange.cgi, the "column_foo" parameters are no longer in use, and the code relative to them, right before this line above, should go away.



>Index: js/change-columns.js

I'm not a JS expert; I will let pyrzak review it.

I tested your patch and it's working fine. The reasons of my r- are because your patch doesn't apply cleanly on today's source code due to a recent checkin in the template, and because you forgot to remove obsolete code from colchange.cgi. Also, please reply to my questions above to make sure we understand correctly what's going on. pyrzak, could you give a 2nd look, JS-oriented?

Comment 35

[Guy Pyrzak]

Comment on attachment 332895 [details] [diff] [review]
v7

The indentation is off for the first 3 functions. 

You seem to repeat the same code for looping over the select. You should change that to have a function like getSelectedOptions or something like that.

you've got a lot of loops in the updateView function, none with breaks. You should break in those ifs b/c you only care about the first time that it is caught.

Lastly in update View you call getElementById too often. Store the element in an object and reference it like you do in other functions.

Fix those and I'll take another look at the JS.

Comment 36

[Pascal Held]

> Wouldn't it be easier to have a single block for common rules
yes ;-) it's fixed.

> Why is this list populated by JS? Why not populating it directly in the
> template? Is there any special reason for this?
If JS is off, all fields are in the "selected fields" box. If JS is on I can simple move the not selected ones to the "available box". If I would fill the "available box" also in the template, I have some dublicates.


> When JS is turned off, you get this single list with currently displayed
> columns at the top, followed by remaining available columns below them. This
> means they no longer appear in the order specified by @masterlist. Is that
> intentional? Is this the desired behavior?
Yes that is my intention. If JS is disabled and you change the columns the old order will not be mixed. 
You can also change the order of the columns, if you only add one column a time. This new column will be show at the right side.


> Also, when you hit F5 on Firefox to refresh the page, this list disappears.
> Could you check this and fix it?
I can't reproduce :-(

> Couldn't it be possible to have this in e.g. initChangeColumns() to not have
> all this JS code in the template?
yes, it's done.

Comment 37

[Pascal Held]

Attachment: v8

Comment 38

[Frédéric Buclin]

Comment on attachment 334254 [details] [diff] [review]
v8

>Index: template/en/default/list/change-columns.html.tmpl

>+<script type="text/javascript">
>+  var all_fields=new Array();
>+  var field_desc=new Object();
>+</script>

Where is this code used? Old stuff?

Comment 39

[Pascal Held]

yes, I fergot to delete, sorry. I will do it with the next patch, but I will wait for further remarks.

Comment 40

[Frédéric Buclin]

(In reply to comment #36)
> > Also, when you hit F5 on Firefox to refresh the page, this list disappears.
> > Could you check this and fix it?
> I can't reproduce :-(

This happens to me all the time, even with your new patch. I'm using Firefox 3.0.1 on Linux. pyrzak, can you reproduce?

Comment 41

[Frédéric Buclin]

Comment on attachment 334254 [details] [diff] [review]
v8

The patch is broken. I cannot add new columns to the list (the "move_select" button has no effect).

Also, your patch doesn't pass tests:

t/005no_tabs.........NOK 7
#   Failed test 'colchange.cgi contains tabs --WARNING'

Comment 42

[Pascal Held]

Attachment: v9

I solved the problem with the tests and the select button.

Comment 43

[Pascal Held]

> Also, when you hit F5 on Firefox to refresh the page, this list disappears.
> Could you check this and fix it?

OK, I see the problem. It's because Firefox don't use the original form data on reload. That means on a normal load the form is filled like the form if JS is off, but on a F5 reload firefox use the current values and runs the JS with this data, so the list disappears. 
I have no Idea to solve this, can anybody help?

Comment 44

[Frédéric Buclin]

Comment on attachment 334269 [details] [diff] [review]
v9

>Index: template/en/default/list/change-columns.html.tmpl

>-    <input id="nosplitheader" type="radio" name="splitheader" value="0"
>-      [%+ "checked='checked'" IF NOT splitheader %]>
>-    <label for="nosplitheader">
>-      Normal headers (prettier)
>-    </label>
>-    <br>
>-
>-    <input id="splitheader" type="radio" name="splitheader" value="1"
>-      [%+ "checked='checked'" IF splitheader %]>
>-    <label for="splitheader">
>-      Stagger headers (often makes list more compact)
>-    </label>


Seems to work fine now. But I just realized you deleted radio buttons related to the "splitheader" parameter, which affects how column headers are displayed. You have to bring them back. r=LpSolit for the current code, but you still have to bring back the parameter, and pyrzak still has to review the JS part of the patch.

Now a technical tip: when you want to request review from several reviewers at once, you can write e.g. "LpSolit, pyrzak" in the requestee field, and Bugzilla will automatically convert this into two review requests, one per requestee. Magic, isn't it? ;)

Comment 45

[Pascal Held]

Attachment: v10

I find a workaround for the reload problem. I use the onunload event to restore the original data. I hope that this is ok

Comment 46

[Frédéric Buclin]

Comment on attachment 334271 [details] [diff] [review]
v10

Read again my last comment about splitheader.

Comment 47

[Pascal Held]

Attachment: v11

I add the splitheaders, sorry for the spam

Comment 48

[Frédéric Buclin]

Comment on attachment 334272 [details] [diff] [review]
v11

>Index: template/en/default/list/change-columns.html.tmpl

>+            [% FOREACH column = collist %]
>+                <option value="[% column %]" selected="selected">

Write [% column FILTER html %]. Without the filtering, I'm able to do XSS attacks by clicking this URL:

https://localhost/bugzilla/colchange.cgi?rememberedquery=helpwanted&selected_columns="></select><script>javascript:alert("Die")</script>

You also have to remove the corresponding entry from filterexceptions.pl:

'list/change-columns.html.tmpl' => [
  'column', 
],


>+                <option value="[% column %]">

Same here. Use FILTER html.


>+          <select name="available_columns" id="available_columns" 
>+             size="15" multiple="multiple" onchange="updateView();"
>+              style="display:none"> 
>+          </select>

The indentation is incorrect. size= and style= should be aligned with name=.


>+          <input class="image_button" type="button" id="select_button" name="select" onclick="move_select()" style="display:none">
>+          <br><br>
>+          <input class="image_button" type="button" id="deselect_button" name="deselect" onclick="move_deselect()" style="display:none">

Split these long lines, for instance between id= and name=, and align name= with class=. Do the same with the other two buttons.



>@@ -61,7 +101,7 @@
>       Stagger headers (often makes list more compact)
>     </label>
>   </p>
>-
>+  
>   [% IF saved_search %]
>     <p>
>       <input type="hidden" name="saved_search"
>@@ -72,7 +112,7 @@
>         for search '[% saved_search.name FILTER html %]'</label>
>     </p>
>   [% END %]
>-
>+  
>   <p>
>     <input type="submit" id="change" value="Change Columns">
>   </p>
>@@ -86,4 +126,3 @@
> </form>
> 
> [% PROCESS global/footer.html.tmpl %]
>-

Remove this part from your patch. You add whitespaces which have nothing to do here.



>Index: colchange.cgi

>-        foreach my $i (@masterlist) {
>-            if (defined $cgi->param("column_$i")) {
>-                push @collist, $i;
>-            }
>+        if (defined $cgi->param("selected_columns")) {
>+            @collist = $cgi->param("selected_columns");
>         }

Currently, items added to @collist are validated (they must belong to @masterlist). But now items are no longer validated, allowing me to do XSS attacks as above.

Comment 49

[Pascal Held]

Attachment: v12

I fixed the things from the last review.

Comment 50

[Frédéric Buclin]

Comment on attachment 334672 [details] [diff] [review]
v12

>Index: colchange.cgi
>+            foreach my $field ($cgi->param("selected_columns")) {
>+                foreach (@masterlist) {
>+                    if ($_ eq $field) {
>+                        push(@collist,$field);
>+                        last;
>+                    }
>+                }
>             }

To avoid two nested loops, you should convert @masterlist to a hash:

my %legal_list = map { $_ => 1 } @masterlist;
@collist = grep { exists $legal_list{$_} } $cgi->param("selected_columns");

Otherwise, looks good and works fine. r=LpSolit with the fix above.

Comment 51

[Guy Pyrzak]

Comment on attachment 334672 [details] [diff] [review]
v12

>### Eclipse Workspace Patch 1.0
>#P bugzilla-sort-columns
>Index: skins/standard/global.css
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v
>retrieving revision 1.48
>diff -u -r1.48 global.css
>--- skins/standard/global.css	3 Aug 2008 13:28:39 -0000	1.48
>+++ skins/standard/global.css	20 Aug 2008 11:08:06 -0000
>@@ -438,4 +438,24 @@
>     background: white;
> }
> 
>+.image_button {
>+    background-repeat:no-repeat;
>+    background-position: center center;
>+    width: 30px;
>+}
> 
>+#select_button {
>+    background-image:url(global/right.png);
>+}
>+
>+#deselect_button {
>+    background-image:url(global/left.png);
>+}
>+
>+#up_button {
>+    background-image:url(global/up.png);
>+}
>+
>+#down_button {
>+    background-image:url(global/down.png);
>+}

NIT: spaces after all the colons;

>\ No newline at end of file
>Index: template/en/default/list/change-columns.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/change-columns.html.tmpl,v
>retrieving revision 1.17
>diff -u -r1.17 change-columns.html.tmpl
>--- template/en/default/list/change-columns.html.tmpl	8 Aug 2008 03:17:24 -0000	1.17
>+++ template/en/default/list/change-columns.html.tmpl	20 Aug 2008 11:08:09 -0000
>@@ -16,12 +16,16 @@
>   # Rights Reserved.
>   #
>   # Contributor(s): Dave Lawrence <[e-mail]>
>+  #                 Pascal Held <[e-mail]>
>   #%]
> 
> [% PROCESS global/variables.none.tmpl %]
> 
> [% PROCESS global/header.html.tmpl
>   title = "Change Columns"
>+  javascript_urls = "js/change-columns.js"
>+  onload = "initChangeColumns()"
>+  onunload = "unload()";
> %]
> 
>Index: template/en/default/global/header.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v
>retrieving revision 1.57
>diff -u -r1.57 header.html.tmpl
>--- template/en/default/global/header.html.tmpl	30 Jul 2008 11:13:50 -0000	1.57
>+++ template/en/default/global/header.html.tmpl	20 Aug 2008 11:08:09 -0000
>@@ -223,7 +223,7 @@
>   # but set the onload attribute in the DEFAULT directive above.
>   #%]
> 
>-  <body onload="[% onload %]"
>+  <body onload="[% onload %]" onunload="[% onunload %]"
>         class="[% urlbase.replace('^https?://','').replace('/$','').replace('[-~@:/.]+','-') %]
>                [% FOREACH class = bodyclasses %]
>                  [% ' ' %][% class FILTER css_class_quote %]
This is not going to work for Bugzilla. Please use YUI or something like that to attach the onunload to only the change col page and not to the header file. We don't want to have this kind of hook in global/header.html.tmpl at all b/c using onunload will break bf_cache, which is super bad and gets us yelled at. I would prefer not to use the onunload at all if possible, but if not please only use it on this page.

>Index: js/change-columns.js
>===================================================================
>RCS file: js/change-columns.js
>diff -N js/change-columns.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ js/change-columns.js	1 Jan 1970 00:00:00 -0000
>@@ -0,0 +1,144 @@
>+/*# The contents of this file are subject to the Mozilla Public
>+  # License Version 1.1 (the "License"); you may not use this file
>+  # except in compliance with the License. You may obtain a copy of
>+  # the License at http://www.mozilla.org/MPL/
>+  #
>+  # Software distributed under the License is distributed on an "AS
>+  # IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
>+  # implied. See the License for the specific language governing
>+  # rights and limitations under the License.
>+  #
>+  # The Original Code is the Bugzilla Bug Tracking System.
>+  #
>+  # The Initial Developer of the Original Code is Pascal Held.
>+  #
>+  # Contributor(s): Pascal Held <[e-mail]>
>+  #
>+*/
>+
>+function initChangeColumns() {
>+    var av_select=document.getElementById("available_columns");
>+    var sel_select=document.getElementById("selected_columns");
>+    document.getElementById("avail_header").style.display="";
>+    document.getElementById("available_columns").style.display="";
>+    document.getElementById("select_button").style.display="";
>+    document.getElementById("deselect_button").style.display="";
>+    document.getElementById("up_button").style.display="";
>+    document.getElementById("down_button").style.display="";
>+    switch_options(sel_select,av_select,false);
>+    sel_select.selectedIndex=-1;
>+    updateView();
>+}
>+
>+function switch_options(from_box, to_box, selected) {
>+    for (var i=0;i<from_box.options.length;i++) {
NIT: not following the style guide please put spaces around the semicolons for more info about the style check out man perlstyle

>+        var opt=from_box.options[i];
spaces around opperators

>+        if (opt.selected==selected) {
spaces here too..
>+            var newopt=new Option(opt.text,opt.value,opt.defaultselected,opt.selected);
>+            to_box.options[to_box.options.length]=newopt;
>+            from_box.options[i]=null;
>+            i=i-1;
the previous 4 lines too.

>+        }
>+        
>+    }
>+}
>+
>+function move_select() {
>+    var av_select=document.getElementById("available_columns");
>+    var sel_select=document.getElementById("selected_columns");
>+    switch_options(av_select,sel_select,true);
>+    updateView();
>+}
>+

these too. I'm not going to list them anymore but make sure you have spaces around operators and check out the perlstyle.

Comment 52

[Pascal Held]

Attachment: v13

- I removed the nested loops.
- I removed the onunload hook and add JS code in the JS file which register the onunload event. I didn't know an other way to avoid the reload problem
- I fixed the style errors

Comment 53

[Guy Pyrzak]

Comment on attachment 337004 [details] [diff] [review]
v13

whomever contributes this into the code please put display:none into image_button. Also replace display none elsewhere with the class .bz_default_hidden{ display:none }, just like in show_bug.css

Comment 54

[Frédéric Buclin]

(In reply to comment #53)
> image_button. Also replace display none elsewhere with the class
> .bz_default_hidden{ display:none }, just like in show_bug.css

Note that .bz_default_hidden is not in global.css.

Pascal, could you update your patch to 1) remove display:none from the template and add it to the image_button class instead, and 2) move .bz_default_hidden from show_bug.css into global.css and use it at the two other places where the image_button class is not already in use (<div id="avail_header" style="display:none"> and <select name="available_columns" ...>)? That's the final change requested for your patch. :)

Good job!

Comment 55

[Pascal Held]

Attachment: v14 - final

the last changes ;-)

Comment 56

[Frédéric Buclin]

Comment on attachment 337831 [details] [diff] [review]
v14 - final

Looks good to me. Thanks! r=LpSolit

Comment 57

[Frédéric Buclin]

Checking in colchange.cgi;
/cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v  <--  colchange.cgi
new revision: 1.65; previous revision: 1.64
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/js/change-columns.js,v
done
Checking in js/change-columns.js;
/cvsroot/mozilla/webtools/bugzilla/js/change-columns.js,v  <--  change-columns.js
initial revision: 1.1
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.50; previous revision: 1.49
done
Checking in skins/standard/show_bug.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/show_bug.css,v  <--  show_bug.css
new revision: 1.8; previous revision: 1.7
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/global/down.png,v
done
Checking in skins/standard/global/down.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global/down.png,v  <--  down.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/global/left.png,v
done
Checking in skins/standard/global/left.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global/left.png,v  <--  left.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/global/right.png,v
done
Checking in skins/standard/global/right.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global/right.png,v  <--  right.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/global/up.png,v
done
Checking in skins/standard/global/up.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global/up.png,v  <--  up.png
initial revision: 1.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.116; previous revision: 1.115
done
Checking in template/en/default/list/change-columns.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/change-columns.html.tmpl,v  <--  change-columns.html.tmpl
new revision: 1.18; previous revision: 1.17
done

Comment 58

[Cédric Corazza]

Am I wrong to suppose there is a missing opening <center> tag here: https://bugzilla.mozilla.org/attachment.cgi?id=337831&action=diff#mozilla/webtools/bugzilla/template/en/default/list/change-columns.html.tmpl_sec2
(see line 91)?

Comment 59

[Frédéric Buclin]

(In reply to comment #58)
> Am I wrong to suppose there is a missing opening <center> tag

Yes, you are right. There should be one right before <table>. Either that or remove </center>. Pascal?

Comment 60

[Pascal Held]

I think we can remove the </center> tag. Should I post a new patch or do you remove without?

Comment 61

[Frédéric Buclin]

(In reply to comment #60)
> I think we can remove the </center> tag. Should I post a new patch or do you
> remove without?

I removed it.

Checking in template/en/default/list/change-columns.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/change-columns.html.tmpl,v  <--  change-columns.html.tmpl
new revision: 1.19; previous revision: 1.18
done

Comment 62

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.