139011 - Improve buglist colors further

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Håkan Waara]

One thing that would be great if it was added to bugzilla (at least on
mozilla.org), was if resolved bugs would get a "greyed-out" sort of "disabled"
color; just like critical or blocker bugs become sparkling red. :-)

I have a bookmark, "Today's bugs", and when I go to that query, I often see some
new major bugs, some already resolved (as invalid or duplicate or whatever), and
as you can see, it would be really useful if I could quickly note (by color)
when a bug is already resolved.

Comment 1

[Gervase Markham [:gerv]]

The template in CVS does the following:

  <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF
(bug.groupset && !usebuggroups) %]">

This bug would therefore be a request to add bz_[% bug.status %] to this list.
Apart from the bulking of the HTML, I don't see why we shouldn't.

Gerv

Comment 2

[Håkan Waara]

So if this is really easy, can't someone who has the power Just Do It? :-)

Comment 3

[GavinS]

Attachment: Add the bug status to the css classes in a buglist

A patch to implement the fix as described by Gerv in comment#1.

Notes about this patch: 

1) Blocking bug#130276 still applies, which is why I'm not going to request a
review of this patch (i.e. thought is needed before adding more css classes)

2) This is inconsistent with the change in bug#100639, another indication that
bug#130276 is a good thing

3) This patch suffers from the same problem as described in bug#207044
comment#7 (spaces in bug status would cause css to not work properly)

4) If the status is not included in your buglist, then the code should either
a) check for this, and not include an empty 'bz_' css class, or b) always get
the status from the DB, so the css class can be specified, even if it is not a
displayable column. The current severity and priority css classes suffer from
this as well, I think

Comment 4

[Tobias Burnus]

Attachment: Add resolution and bug_status, add css_quota filter

> 1) Blocking bug#130276 still applies, which is why I'm not going to request a

> review of this patch (i.e. thought is needed before adding more css classes)
In my opinion this bug should block bug 130276 (a meta bug) not the other way
round.

> 2) This is inconsistent with the change in bug#100639, another indication
that
> bug#130276 is a good thing
Hmm, I think they are quite independent.

> 3) This patch suffers from the same problem as described in bug#207044
> comment#7 (spaces in bug status would cause css to not work properly)
I added a css_quote filter, which changes ' ' to '_' and css encodes the other
characters which might appear (like a e.g. '&').

> 4) If the status is not included in your buglist, then the code should either

> a) check for this, and not include an empty 'bz_' css class, or b) always get

> the status from the DB, so the css class can be specified, even if it is not
a
> displayable column.
My patch always gets the data from the database, I regards this by the way as
better solution than a. See also bug 213079 which fixed this for severity or
priority. (This is the reason for making attachment 129859 [details] [diff] [review] as obsolete.)

Comment 5

[Tobias Burnus]

After testing (http://www.physik.fu-berlin.de/~tburnus/test2/) the css_quote
part and after re-reading bug#207044 comment#7, I'm not sure whether css_quote
works or not. I think it should do url_quote + replacing ' ' by '_'.
Moreover Mozilla doesn't seem to be able to handle those, see bug 216293.

I think I should use this:

+sub css_quote {
+    my ($toencode) = (@_);
+    $toencode =~ s/ /_/g;
+    return url_quote($toencode);
+}

Comment 6

[Tobias Burnus]

Attachment: v2.1: Fix css_quota, it now actually works with ® names, ' ' => '_'

Comment 7

[Tobias Burnus]

Mine.

Comment 8

[Christian Reis]

Comment on attachment 129904 [details] [diff] [review]
v2.1: Fix css_quota, it now actually works with ® names, ' ' => '_'

> 
> Quotes characters so that they may be included as part of a url.
>+
>+=item C<css_quote($val)>
>+
>+Quotes characters so that they may be used as CSS class names. Spaces
>+are replaced by underscores.

Doesn't it make sense to say it quotes a string? Anyway, I see there are other
comments in the same line in this file, so it's a nit.

>+  <tr class="bz_[% bug.bug_severity FILTER css_quote %]
>+             bz_[% bug.priority FILTER css_quote %]
>+             bz_[% bug.bug_status FILTER css_quote %]
>+             bz_[% bug.resolution FILTER css_quote %]
>+             [%+ "bz_secure" IF bug.isingroups %]">

I considered recommending using a FOREACH here and applying all possible styles
to the row, but that would incur in style changes according to what columns are
being displayed, which isn't a good idea.

r=kiko on this change, and I think it shouldn't conflict with the current
blocker bug -- it *allows* styling to be done, and doesn't change the default
style in any way.

Comment 9

[Tobias Burnus]

> >+Quotes characters so that they may be used as CSS class names. Spaces
> >+are replaced by underscores.
> Doesn't it make sense to say it quotes a string? Anyway, I see there are other
> comments in the same line in this file, so it's a nit.

Well, strings sounds better, but they are quoted character by character.
I don't care, but I can change this word before checking in. (justdave?)

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 129904 [details] [diff] [review]
v2.1: Fix css_quota, it now actually works with ® names, ' ' => '_'

You need to fix the template compile test to define a stub for the new
css_quote filter or it's going to fail tests on tinderbox when you check it in.

Comment 11

[Tobias Burnus]

Attachment: v2.2: As v2.1 but contains also t/008filter.t

> You need to fix the template compile test to define a stub for the new
> css_quote filter or it's going to fail tests on tinderbox when you check it
in.
I changed the t/ directory -- but I missed to include it in the diff :-(
(I hate modified trees. They make diffing a hassle)

Comment 12

[Christian Reis]

Comment on attachment 129980 [details] [diff] [review]
v2.2: As v2.1 but contains also t/008filter.t

Looks fine.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 129980 [details] [diff] [review]
v2.2: As v2.1 but contains also t/008filter.t

actually, t/Support/Template.pm is what I was thinking of.  But the 008filter.t
was a good catch, too.

Comment 14

[Tobias Burnus]

Attachment: v2.3 Add stub to 004template.t

> actually, t/Support/Template.pm is what I was thinking of.  But the
008filter.t
> was a good catch, too.
Hmm, the latter I needed in order to pass runtests, but for whatever reason I
didn't need to fix 004template.t, even without it passed.
I wonder why ...

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 130027 [details] [diff] [review]
v2.3 Add stub to 004template.t

r=justdave

requesting a peek from Gerv due to adding a new "approved filter" to the filter
test.  (I think it's okay, but better to have another set of eyes)

Comment 16

[Gervase Markham [:gerv]]

Seems fine, although I'd call it css_class_quote rather than css_quote.

Gerv

Comment 17

[Tobias Burnus]

Attachment: Change css_quote to css_class_quote

> Seems fine, although I'd call it css_class_quote rather than css_quote.
Ok, changed. Interdiff (only -/+ lines):
-	     css_quote => \&Bugzilla::Util::css_quote ,
+	     css_class_quote => \&Bugzilla::Util::css_class_quote ,
-			      css_quote
+			      css_class_quote
-sub css_quote {
+sub css_class_quote {
-=item C<css_quote($val)>
+=item C<css_class_quote($val)>
-	     css_quote => sub { return $_ } ,
+	     css_class_quote => sub { return $_ } ,
-    return 1 if $directive =~ /FILTER\ (html|csv|js|url_quote|css_quote|
+    return 1 if $directive =~ /FILTER\ (html|csv|js|url_quote|css_class_quote|

-      <col class="bz_[% id FILTER css_quote %]_column">
+      <col class="bz_[% id FILTER css_class_quote %]_column">
-  <tr class="bz_[% bug.bug_severity FILTER css_quote %]
-	      bz_[% bug.priority FILTER css_quote %]
-	      bz_[% bug.bug_status FILTER css_quote %]
-	      bz_[% bug.resolution FILTER css_quote %]
+  <tr class="bz_[% bug.bug_severity FILTER css_class_quote %]
+	      bz_[% bug.priority FILTER css_class_quote %]
+	      bz_[% bug.bug_status FILTER css_class_quote %]
+	      bz_[% bug.resolution FILTER css_class_quote %]

Comment 18

[Tobias Burnus]

FIXED

/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v
new revision: 1.232; previous revision: 1.231
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v
new revision: 1.9; previous revision: 1.8
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v
new revision: 1.11; previous revision: 1.10
/cvsroot/mozilla/webtools/bugzilla/t/004template.t,v
new revision: 1.26; previous revision: 1.25
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v
new revision: 1.6; previous revision: 1.5
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v
new revision: 1.7; previous revision: 1.6
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v
new revision: 1.13; previous revision: 1.12