Closed Bug 139011 Opened 18 years ago Closed 16 years ago

Improve buglist colors further

Categories

(Bugzilla :: Query/Bug List, defect)

2.15
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: hwaara, Assigned: burnus)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
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
Depends on: 130276
So if this is really easy, can't someone who has the power Just Do It? :-)
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
Attachment #129859 - Flags: review+
Attachment #129859 - Flags: review+
> 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.)
Attachment #129859 - Attachment is obsolete: true
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);
+}
Target Milestone: --- → Bugzilla 2.18
Attachment #129867 - Attachment is obsolete: true
Attachment #129904 - Flags: review?(bbaetz)
Mine.
Assignee: endico → burnus
Comment on attachment 129904 [details] [diff] [review]
v2.1: Fix css_quota, it now actually works with &#xAE; 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.
Attachment #129904 - Flags: review+
> >+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?)
Status: NEW → ASSIGNED
Flags: approval?
Comment on attachment 129904 [details] [diff] [review]
v2.1: Fix css_quota, it now actually works with &#xAE; 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.
Attachment #129904 - Flags: review?(bbaetz) → review-
> 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)
Attachment #129904 - Attachment is obsolete: true
Attachment #129980 - Flags: review?
Comment on attachment 129980 [details] [diff] [review]
v2.2: As v2.1 but contains also t/008filter.t

Looks fine.
Attachment #129980 - Flags: review? → review+
Flags: approval?
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.
Attachment #129980 - Flags: review-
Attached patch v2.3 Add stub to 004template.t (obsolete) — Splinter Review
> 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 ...
Attachment #129980 - Attachment is obsolete: true
Attachment #130027 - Flags: review?(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)
Attachment #130027 - Flags: review?(justdave)
Attachment #130027 - Flags: review?(gerv)
Attachment #130027 - Flags: review+
Seems fine, although I'd call it css_class_quote rather than css_quote.

Gerv
> 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 %]
Attachment #130027 - Attachment is obsolete: true
Attachment #130219 - Flags: review?(justdave)
Attachment #130219 - Flags: review?(justdave) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.