322693 - Create a mechanism to manage multiple custom skins

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P1)

Description

[Paul]

Now that we have (N) skins, we need a way to manage them.

It seems that users should have a choice of a skin with a possible system default.

Manage this under the user's "Prefs"

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

This isn't even too invasive. We should consider it for 2.22, I'd say.

Usage:
o Empty your skins/custom dir (this step is not required)
o Create a subdir of skins/custom, e.g. "skins/custom/My Happy Skin"
o Create empty copies of all skins/standard/*.css files in your new subdir of
  skins/custom
o Put overrides to the standard Bugzilla CSS into your CSS files -- this may be
  as simple as all files empty, and global.css containing
  body {background-color: green} (if you're inclined in such a way).

Select your new skin in your user preferences. Enjoy!

Comment 2

[Frédéric Buclin]

Comment on attachment 211392 [details] [diff] [review]
Patch

>+++ ./Bugzilla/User/Setting/Skin.pm	2006-02-09 20:46:41.809235200 +0100

Is this new module really required? Do you plan to add more methods/functions in the future?


>+use IO::Dir;

>+    my $dd = new IO::Dir($dirbase) || return $legal_values;
>+
>+    foreach ($dd->read()) {

Why using a different way from checksetup.pl to get directories? Maybe could we refactor this function in Util.pm, assuming this makes sense (Util::get_all_directories($parent_dir)).



+++ ./Bugzilla/User/Setting.pm	2006-02-09 22:08:47.592473600 +0100

>-    ($name && $values && $default_value)
>+    ($name && $default_value)

Is sanitycheck.cgi doing any check about the 'setting_value' table? I just want to make sure that adding no entry in this table won't break anything.



>+++ ./checksetup.pl	2006-02-09 20:34:33.461923200 +0100

>+# 2006-02-19 [e-mail] -- Bug 321556
>+add_setting ('Skin', undef, 'standard_skin');

s/Skin/skin/ probably.



>+++ ./template/en/default/filterexceptions.pl	2006-02-10 17:31:13.887290200 +0100

> 'global/header.html.tmpl' => [
>   'javascript', 
>   'style', 
>+  'userskin',

Didn't we talk about using variable_like_this? -> user_skin



>+++ ./template/en/default/global/setting-descs.none.tmpl	2006-02-10 08:37:10.475688000 

>+   # Don't use $terms.Bugzilla because "Classic Bugzilla" is the skin's name
>+   "standard_skin"                    => "Classic Bugzilla",
>+   "Skin"                             => "$terms.Bugzilla's general appearance (skin)",

if t/009bugwords.t would work correctly (it doesn't actually), it would complain about "Classic Bugzilla". Did you think about a workaround?


This is not an official review, just a first look at your patch. Better than nothing. ;)

Comment 3

[Max Kanat-Alexander]

Comment on attachment 211392 [details] [diff] [review]
Patch

>+    # We may create ourselves as a specialized sub-class
>+    my $subclass = $class . '::' . $setting_name;
>+    eval("require $subclass");
>+    $@ || ($class = $subclass);
>+

  Instead of this, add a column that tells us what type of setting the setting is. The column can be NULL by default, meaning that it's not any subclass. Then when we read the Setting, we can just load it in as the correct type.

  Ideally we'd have a way for the callers to choose the proper type, but I suppose it's easier for the callers this way.

>+++ ./template/en/default/global/setting-descs.none.tmpl	2006-02-10 08:37:10.475688000 +0100
>@@ -32,6 +32,9 @@
>    "post_bug_submit_action"           => "After changing $terms.abug",
>    "next_bug"                         => "Show next $terms.bug in my list",
>    "same_bug"                         => "Show the updated $terms.bug",
>+   # Don't use $terms.Bugzilla because "Classic Bugzilla" is the skin's name
>+   "standard_skin"                    => "Classic Bugzilla",
>+   "Skin"                             => "$terms.Bugzilla's general appearance (skin)",
>    "nothing"                          => "Do Nothing",
>                    } 
> %]

  I think we should add a hook to this file at the end, so that skin-makers can describe their own skins.

  In general, though, this looks really good! :-) I haven't tested it, though.

Comment 4

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

In discussions on the developers list a while back, we hashed out a Bugzilla CSS plan:

http://wiki.mozilla.org/Bugzilla:CSS_Plan

The plan was carefully designed to meet the needs of both installation admins and third-party skin developers and to make sure that the rules from all three skins (standard, third-party, and custom skins) are applied with the appropriate precedence.

In order to maximize Bugzilla's skinnability when implementing this feature, please make sure you do so in accordance with the plan.

Comment 5

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Updated patch. Three-tier cascade:

1. standard bz skin (persistent)
2. current preferred skin (or alternate, if switched in browser); standard
   or third-party (skins/contrib/*)
3. custom skin (skins/custom)

Usage:
o Create a subdir of skins/contrib, e.g. "skins/contrib/My Happy Skin"
o Create empty copies of all skins/standard/*.css files in your new subdir of
  skins/contrib
o Put overrides to the standard Bugzilla CSS into your CSS files.
o Maintain site-wide CSS overrides in skins/custom

Select your new skin in your user preferences. For single pages, you can use your browser to switch to an alternate skin, too. By locking down the user setting, the Bugzilla administrator may prevent both kinds of skin switching, setting-based as well as browser-based.


(In reply to comment #2)
> (From update of attachment 211392 [details] [diff] [review] [edit])
> >+++ ./Bugzilla/User/Setting/Skin.pm	2006-02-09 20:46:41.809235200 +0100
> 
> Is this new module really required? Do you plan to add more methods/functions
> in the future?

I think that's entirely possible.

> >+use IO::Dir;
> 
> Why using a different way from checksetup.pl to get directories? Maybe could we
> refactor this function in Util.pm, assuming this makes sense
> (Util::get_all_directories($parent_dir)).

Imo IO::Dir is the way to go, but here you go, using glob().

> +++ ./Bugzilla/User/Setting.pm  2006-02-09 22:08:47.592473600 +0100
> 
> >-    ($name && $values && $default_value)
> >+    ($name && $default_value)
> 
> Is sanitycheck.cgi doing any check about the 'setting_value' table? I just want
> to make sure that adding no entry in this table won't break anything.

It's not doing any checking currently. Plus, the standard check works the other way round (it'd check all setting_value rows whether there is a corresponding setting entry).

> s/Skin/skin/ probably.

It was necessary at the time to do the dynamic module loading. It has been obsoleted in the meantime, though.

> Didn't we talk about using variable_like_this? -> user_skin

Minor glitch fixed.

> if t/009bugwords.t would work correctly (it doesn't actually), it would
> complain about "Classic Bugzilla". Did you think about a workaround?

Uses the http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/template/en/default/search/search-advanced.html.tmpl#67 workaround now.

(In reply to comment #3)
>   Instead of this, add a column that tells us what type of setting the setting
> is. The column can be NULL by default, meaning that it's not any subclass. Then
> when we read the Setting, we can just load it in as the correct type.

Column added.

>   I think we should add a hook to this file at the end, so that skin-makers can
> describe their own skins.

Separate bug, please.

(In reply to comment #4)

The patch follows the plan very closely. Support for remembering browser-controlled skin switches may be added later.

Comment 6

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

Comment on attachment 211454 [details] [diff] [review]
Patch 1.2


>+            subclass      => {TYPE => 'varchar(32)', NOTNULL => 0},

As I'm not particularly clear on the ramifications of this subclass implementation, mkanat or someone else who has expertise with the user settings feature of Bugzilla should also review this part of the patch.


>+    foreach (glob(catdir($dirbase, '*'))) {
>+        next unless -d $_;

I don't share others' aversion to IO::Dir.  Actually, I like it better than glob.  But glob is also fine, so no need to change things around again.

A number of skin developers have expressed the desire to distribute skins as a single CSS file.  To accommodate this much simpler skin distribution mechanism, in addition to directories this code should support files as skins and handle them appropriately.


>+    [%# Set up the skin CSS cascade:
>+      #  1. Standard Bugzilla CSS file set (persistent)
>+      #  2. Standard Bugzilla CSS file set (selectable)
>+      #  3. All third-party "skin" CSS file sets (selectable)
>+      #  4. Custom Bugzilla CSS file set (persistent)
>+      # "Selectable" skin file sets may be either preferred or alternate.

Nit: CSS file -> stylesheet.

Also, it would help to have this description of the code interspersed with the code itself instead of (or in addition to) all in one block above the code.


>+    [% alternate = 'alternate ' IF user.settings.skin.is_enabled %]

It was hard to figure out how this variable was being used, and I had to refer back to it a number of times while perusing later code.  Part of the problem is that the variable means two very different things: a boolean value indicating whether or not alternate stylesheets are enabled and a string to prepend to the rel attribute when alternate stylesheets are enabled.

It would make this code more readable if instead of using this variable you just use the original "user.settings.skin.is_enabled" variable in boolean contexts and a literal string in string contexts, i.e.:

    [% IF user.settings.skin.is_enabled %]
      <link href="skins/standard/global.css"
            rel="[% "alternate " IF user_skin %]stylesheet"
            title="standard"
            type="text/css">
    [% END %]


>     <link href="skins/standard/global.css" rel="stylesheet" type="text/css">

Perhaps not this bug, but it would simplify the code if global.css were just the first item in the style_urls array instead of a hardcoded special case.


>+    [% IF alternate %]
>+      <link href="skins/standard/global.css"
>+            rel="[% alternate IF user_skin %]stylesheet"
>+            title="standard"
>+            type="text/css">
>+    [% END %]
>+    [% FOREACH style_url = style_urls %]
>+      <link href="[% style_url FILTER html %]"
>+            rel="stylesheet"
>+            type="text/css">
>+      [% IF alternate %]
>+        <link href="[% style_url FILTER html %]"
>+              rel="[% alternate IF user_skin %]stylesheet"
>+              title="standard"
>+              type="text/css">
>+      [% END %]
>+    [% END %]

Shouldn't the title of this skin be "Classic Bugzilla"?  Browser-based stylesheet selectors are going to display the title in their UI, so it would make sense for us to use the user-friendly term here (since we have it available).

Also, this code combines the first and the second items in the list of cascades, which makes it harder to read and understand what's going on here.  At the cost of some redundancy, it would be easier to grok this code if the two cascade levels were generated in separate code blocks, i.e.:

    [%# 1. standard Bugzilla stylesheet set (persistent) %]
    <link href="skins/standard/global.css" rel="stylesheet" type="text/css">
    [% FOREACH style_url = style_urls %]
      <link href="[% style_url FILTER html %]"
            rel="stylesheet"
            type="text/css">
    [% END %]

    [%# 2. standard Bugzilla stylesheet set (selectable) %]
    [% IF user.settings.skin.is_enabled %]
      <link href="skins/standard/global.css"
            rel="[% "alternate " IF user_skin %]stylesheet"
            title="Classic Bugzilla"
            type="text/css">
      [% FOREACH style_url = style_urls %]
        <link href="[% style_url FILTER html %]"
              rel="[% "alternate " IF user_skin %]stylesheet"
              title="Classic Bugzilla"
              type="text/css">
      [% END %]
    [% END %]


>+    [% FOREACH contrib_skin = user.settings.skin.legal_values %]
>+      [% NEXT IF contrib_skin == 'standard' %]
>+      [% NEXT UNLESS contrib_skin == user_skin OR alternate %]
>+      <link href="skins/contrib/[% contrib_skin FILTER url_quote %]/global.css"
>+            rel="[% alternate UNLESS contrib_skin == user_skin %]stylesheet"
>+            title="[% contrib_skin FILTER html %]"
>+            type="text/css">
>+    [% END %]
>+    [% FOREACH style_url = style_urls %]
>+      [% FOREACH contrib_skin = user.settings.skin.legal_values %]
>+        [% NEXT IF contrib_skin == 'standard' %]
>+        [% NEXT UNLESS contrib_skin == user_skin OR alternate %]
>+        [% IF style_url.match('^skins/standard/') %]
>+          [% contrib_skin = contrib_skin FILTER url_quote %]
>+          <link href="[% style_url.replace('^skins/standard/',
>+                                           "skins/contrib/$contrib_skin/") %]"
>+                rel="[% alternate UNLESS contrib_skin == user_skin %]stylesheet"
>+                title="[% contrib_skin FILTER html %]"
>+                type="text/css">
>+        [% END %]
>+      [% END %]
>+    [% END %]

This code is really hard to understand.  I had to read it several times carefully to get it, and then once I thought I understood it correctly I started to sketch out what I thought would be readability improvements only to realize halfway through that I hadn't understood the code at all!

So I went back and read it a few more times, and if I now understand it correctly, it does two independent things:

1. If the admin/user-specified skin is not the standard skin, it generates
   a stylesheet set for the admin/user-specified skin.
2. If alternate stylesheets are enabled, it generates alternate stylesheet
   sets for all contributed stylesheets.

As with the previous section, at the cost of some redundancy this code would be much more readable if these two tasks were separated into distinct code blocks, i.e.:

    [%# admin/user-specified stylesheet set %]
    [% IF user_skin != "standard" %]
      <link href="skins/contrib/[% user_skin FILTER url_quote %]/global.css"
            rel="stylesheet"
            title="[% user_skin FILTER html %]"
            type="text/css">
      [% FOREACH style_url = style_urls %]
        [% NEXT UNLESS style_url.match('^skins/standard/') %]
          [% user_skin = user_skin FILTER url_quote %]
          <link href="[% style_url.replace('^skins/standard/',
                                           "skins/contrib/$user_skin/") %]"
                rel="stylesheet"
                title="[% user_skin FILTER html %]"
                type="text/css">
      [% END %]
    [% END %]

    [%# third-party contributed "skin" stylesheet sets %]
    [% IF user.settings.skin.is_enabled %]
      [% FOREACH contrib_skin = user.settings.skin.legal_values %]
        [% NEXT IF contrib_skin == 'standard' || contrib_skin == user_skin %]
        <link href="skins/contrib/[% contrib_skin FILTER url_quote %]/global.css"
              rel="alternate stylesheet"
              title="[% contrib_skin FILTER html %]"
              type="text/css">
        [% FOREACH style_url = style_urls %]
          [% NEXT UNLESS style_url.match('^skins/standard/') %]
          [% contrib_skin = contrib_skin FILTER url_quote %]
          <link href="[% style_url.replace('^skins/standard/',
                                           "skins/contrib/$contrib_skin/") %]"
                rel="alternate stylesheet"
                title="[% contrib_skin FILTER html %]"
                type="text/css">
        [% END %]
      [% END %]
    [% END %]

Comment 7

[Marc Schumann [:Wurblzap]]

Thanks for the review! I'll look into your comments as soon as possible.

Comment 8

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

News:
- Skins as single file
- Readability improved quite a lot
- No special case for global.css any more; this hint contributed heavily
  to readability
- Nicer title for standard skin

Comment 9

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

Comment on attachment 216067 [details] [diff] [review]
Patch 1.3

>+    [% FOREACH style_url = style_urls %]
>+      [% IF user.settings.skin.is_enabled %]
>+        <link href="[% style_url FILTER html %]"
>+              rel="[% 'alternate ' IF user_skin %]stylesheet"
>+              title="[% setting_descs.standard FILTER html %]"
>+              type="text/css">
>+      [% END %]
>+    [% END %]

Nit: since this code does nothing unless user.settings.skin.is_enabled, the FOREACH loop should be inside the IF user.settings.skin.is_enabled conditional.


>+    [%# CSS cascade, part 3: Third-party stylesheet set (selectable).
>+      # All third-party skins are present if skin selection is enabled.
>+      # The admin-selected skin is always present.
>+      #%]
>+    [% FOREACH contrib_skin = user.settings.skin.legal_values %]
>+      [% NEXT IF contrib_skin == 'standard' %]
>+      [% NEXT UNLESS contrib_skin == user_skin
>+                  OR user.settings.skin.is_enabled %]
>+      [% contrib_skin = contrib_skin FILTER url_quote %]
>+      [% IF contrib_skin.match('\.css$') %]
>+        [%# 1st skin variant: single-file stylesheet %]
>+        <link href="[% "skins/contrib/$contrib_skin" %]"
>+              rel="[% 'alternate ' UNLESS contrib_skin == user_skin %]stylesheet"
>+              title="[% contrib_skin FILTER html %]"
>+              type="text/css">
>+      [% ELSE %]
>+        [%# 2nd skin variant: tylesheet set %]

Nit: tylesheet -> stylesheet

Otherwise this looks good, but someone with expertise in the Settings code should review the implementation of subclasses.  One question: is there anything in Bugzilla/User/Setting/ by default?

Comment 10

[Max Kanat-Alexander]

Comment on attachment 216067 [details] [diff] [review]
Patch 1.3

The Setting modifications look fine, but Setting/Skin.pm is entirely missing from this patch.

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.4

Nits fixed.
Patch includes Skin.pm now.

Comment 12

[Marc Schumann [:Wurblzap]]

Comment on attachment 224665 [details] [diff] [review]
Patch 1.4

Carrying forward r=myk.

Comment 13

[Bill Barry]

Pinging to see if we can get this in so it does not bitrot (hopefully it hasn't already). I am waiting on this to work on a patch for bug 330600.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 224665 [details] [diff] [review]
Patch 1.4

>+++ ./Bugzilla/User/Setting/Skin.pm	2006-03-23 21:37:33.187888000 +0100
> [snip]

>+sub legal_values {
>+    my ($self) = @_;
>+    my $dirbase = catdir('skins', 'contrib');

  That won't work under mod_perl, since all locations have to be absolute. The best thing to do would be to add these directories to Bugzilla::Constants::bz_locations.

>+    my $legal_values = ['standard'];

  That default should be in a constant, in this file.

  I already reviewed the rest of the patch, so I didn't look it over. But you might just want to make sure that all of our major changes lately haven't caused any bitrot.

Comment 15

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.5

Addressing comments.

Comment 16

[Marc Schumann [:Wurblzap]]

Comment on attachment 231072 [details] [diff] [review]
Patch 1.5

Carrying forward r=myk.

Comment 17

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.5.1

Unrotted.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 233633 [details] [diff] [review]
Patch 1.5.1

>+++ ./Bugzilla/DB/Schema.pm	2006-08-12 07:43:21.178171200 +0200
>@@ -1040,6 +1040,7 @@
>             default_value => {TYPE => 'varchar(32)', NOTNULL => 1},
>             is_enabled    => {TYPE => 'BOOLEAN', NOTNULL => 1,
>                               DEFAULT => 'TRUE'},
>+            subclass      => {TYPE => 'varchar(32)', NOTNULL => 0},

  Instead of putting NOTNULL => 0, just don't put NOTNULL at all.

>+++ ./Bugzilla/Install/Filesystem.pm	2006-08-12 07:43:22.039409600 +0200
>@@ -189,7 +190,7 @@
>     my %create_files = (
>         "$datadir/mail"    => { perms => $ws_readable },
>         'skins/.cvsignore' => { perms    => $owner_readable,
>-                                contents => ".cvsignore\ncustom\n" },
>+                                contents => ".cvsignore\ncustom\ncontrib\n" },

  If you want that to actually be updated for installations that are upgrading, you'll have to modify update_filesystem.

>+++ ./Bugzilla/User/Setting/Skin.pm	2006-07-28 08:31:23.394878000 +0200
>
>+use base qw{Bugzilla::User::Setting};

  Nit: We usually do "qw(" there. It's not critical, but if we ever have to do greps or search-and-replaces, it can be convenient to always do something the same way.

> my $legal_values = [@{(BUILTIN_SKIN_NAMES)}];

  Nit: You could just make that @legal_values, since you use it as an array more often than you use it as an arrayref.

>+    foreach (glob(catdir($dirbase, '*'))) {
>+        if (-d $_) {

  Nit: I'd rather see that variable be named than having it be $_.

>+            # Stylesheet set
>+            push(@$legal_values, basename($_));
>+        }
>+        else {
>+            if (/\.css$/) {

  Nit: Shouldn't that just be elsif?

>+=head1 SYNOPSIS
>+
>+Skin.pm is a class specialized for skins settings.

  Synopsis sections contain code demonstrations. This sentence description would be under DESCRIPTION.

>+=over 4

  Nit: I've recently discovered that "=over" is identical to "=over 4".

>+=item C<legal_values($setting_name)>

  That doesn't actually take an argument.

>+Returns:     A reference to an array containing all legal skins

  That's actually the "names of all legal skins", right?

>+++ ./template/en/default/global/header.html.tmpl	2006-07-14 08:28:38.497982400 +0200
>@@ -81,22 +84,86 @@

>      #  3. All third-party "skin" stylesheet sets (selectable)
>      #  4. Page-specific styles
>      #  5. Custom Bugzilla stylesheet set (persistent)

  I think third-party skins should override the "custom" skins. Otherwise people could make custom changes that would break any skins they imported, and I don't think that's their intention.

>+    [% IF user.settings.skin.value != 'standard' %]
>+      [% user_skin = user.settings.skin.value %]
>+    [% END %]

  This should go down lower, and should be the IF instead of the is_enabled check. (See my comment there.)

  Also, could we possibly simplify this code by making "custom" a skin, and making the skinsdir into $libdir/skins instead of $libdir/skins/contrib? That would also allow us to stop special-casing "standard" in Setting::Skin.

>+    [%# CSS cascade, part 2: Standard Bugzilla stylesheet set (selectable)
>+      # Present if skin selection is enabled.
>+      #%]
>+    [% IF user.settings.skin.is_enabled %]

  is_enabled isn't the right thing to check. is_enabled only means "can users set this or not." The admin could still select a custom skin that wasn't "standard," and just enforce it on every user.

>+      [% FOREACH style_url = style_urls %]
>+        <link href="[% style_url FILTER html %]"
>+              rel="[% 'alternate ' IF user_skin %]stylesheet"
>+              title="[% setting_descs.standard FILTER html %]"
>+              type="text/css">

  Wait...why is this an alternate? I thought that skins were just overrides to our standard CSS. That's what the comment above seems to specify.

>+++ ./template/en/default/global/setting-descs.none.tmpl	2006-05-12 09:54:49.580622100 +0200
>@@ -33,6 +33,9 @@
>    "post_bug_submit_action"           => "After changing $terms.abug",
>    "next_bug"                         => "Show next $terms.bug in my list",
>    "same_bug"                         => "Show the updated $terms.bug",
>+   # "Bugzilla" is to be part of the skin's name, so make sure it passes tests
>+   "standard"                         => "Classic B" _ "ugzilla",

  Use $terms.Bugzilla. This string can show up to users.


  I really want this feature in 3.0. I'll be more prompt about future reviews on this.

Comment 19

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.6

(In reply to comment #18)
>   Instead of putting NOTNULL => 0, just don't put NOTNULL at all.

Ok.

>   If you want that to actually be updated for installations that are upgrading,
> you'll have to modify update_filesystem.

In fact, we don't want to exclude skins/custom. Bug 321556 might be using the contrib dir.

>   Nit: We usually do "qw(" there. It's not critical, but if we ever have to do
> greps or search-and-replaces, it can be convenient to always do something the
> same way.

Sure.

>   Nit: You could just make that @legal_values, since you use it as an array
> more often than you use it as an arrayref.

Makes sense to me.

>   Nit: I'd rather see that variable be named than having it be $_.

It's a really short part, but ok.

>   Nit: Shouldn't that just be elsif?

Yeah. I probably missed that during testing.

>   Synopsis sections contain code demonstrations. This sentence description
> would be under DESCRIPTION.

Fixed.

>   Nit: I've recently discovered that "=over" is identical to "=over 4".

I assume what you're saying is that you'd like me to lose the 4?

> >+=item C<legal_values($setting_name)>
> 
>   That doesn't actually take an argument.

Yes, thanks.

> >+Returns:     A reference to an array containing all legal skins
> 
>   That's actually the "names of all legal skins", right?

Yup. Fixed.

>   I think third-party skins should override the "custom" skins. Otherwise
> people could make custom changes that would break any skins they imported, and
> I don't think that's their intention.

Overriding contrib skins is exactly the point of the custom dir. This way, the site admin has the last word.

>   This should go down lower, and should be the IF instead of the is_enabled
> check. (See my comment there.)

Maybe I don't understand you correctly here. We need to know whether user_skin is set only a few lines further down from where this code currently is.

>   Also, could we possibly simplify this code by making "custom" a skin, and
> making the skinsdir into $libdir/skins instead of $libdir/skins/contrib? That
> would also allow us to stop special-casing "standard" in Setting::Skin.

No. As said above, third-party skins should be sandwiched between standard and custom.
Plus, we'd still need to special-case "standard" because it should be the first step in the cascade, so that others can override it.

>   is_enabled isn't the right thing to check. is_enabled only means "can users
> set this or not." The admin could still select a custom skin that wasn't
> "standard," and just enforce it on every user.

No. What we're doing here is offering Bugzilla's standard style sheet a second time so that user's may select it from their front end. This is only needed if there is a possibility that the user has chosen a different skin.

>   Wait...why is this an alternate? I thought that skins were just overrides to
> our standard CSS. That's what the comment above seems to specify.

No. You can select which skin is sandwiched between "standard" and "custom". The code comments clearly mark cascade parts 2 and 3 as selectable.

>   Use $terms.Bugzilla. This string can show up to users.

It can show up to users, that's right. But the skin name should be "Classic Bugzilla" consistently accross installations. It shouldn't fluctuate with variable.html.tmpl customizations.

>   I really want this feature in 3.0. I'll be more prompt about future reviews
> on this.

Duly noted :)

Comment 20

[Max Kanat-Alexander]

> No. What we're doing here is offering Bugzilla's standard style sheet a second
> time so that user's may select it from their front end. This is only needed if
> there is a possibility that the user has chosen a different skin.

  Right. But there's also a possibility that the admin has chosen a different skin and forced it on every user, in which case "standard' should *still* be an option as an alternate stylesheet. So the thing to check is whether or not the current skin is "standard," not whether or not the user can set the skin.

> It can show up to users, that's right. But the skin name should be "Classic
> Bugzilla" consistently accross installations. It shouldn't fluctuate with
> variable.html.tmpl customizations.

  Admins who change $terms.Bugzilla *really* don't want the word "Bugzilla" to show up anywhere to end users. This shows up to end users. You could change the name to just "Classic," if you'd like, but displaying the word "Bugzilla" to users when the admin has changed $terms.Bugzilla is guaranteed to irritate admins.

Comment 21

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.7

(In reply to comment #20)
>   Right. But there's also a possibility that the admin has chosen a different
> skin and forced it on every user, in which case "standard' should *still* be an
> option as an alternate stylesheet. So the thing to check is whether or not the
> current skin is "standard," not whether or not the user can set the skin.

I don't think a user should be allowed to override an admin's decision. In the (imo very exotic) case an admin wants to allow this, why fix the third-party style sheet at all?

>   Admins who change $terms.Bugzilla *really* don't want the word "Bugzilla" to
> show up anywhere to end users. This shows up to end users. You could change the
> name to just "Classic," if you'd like, but displaying the word "Bugzilla" to
> users when the admin has changed $terms.Bugzilla is guaranteed to irritate
> admins.

Hrm, I can't see where any Given Name would irritate that much, and I'd've liked to have the word "Bugzilla" in Bugzilla's standard skin name, but yeah, ok, I think I can live with this compromise.

Comment 22

[Bill Barry]

(In reply to comment #21)
> Hrm, I can't see where any Given Name would irritate that much, and I'd've
> liked to have the word "Bugzilla" in Bugzilla's standard skin name, but yeah,
> ok, I think I can live with this compromise.

The management here where I work have said that "bugzilla" is too immature and that I should change the name to something more professional (I haven't done this as of yet, but if we are to provide it as a service to our clients I might have to). I guess the keyword here is management (go read Dilbert sometime).

Back to this bug though:

+  [% ELSIF error == "setting_subclass_invalid" %]
+    There is no such Setting subclass as
+    <code>[% subclass FILTER html %]</code>.
+

Should setting be uppercased there? It appears to be lowercase in every sentence elsewhere in that page.

Comment 23

[Marc Schumann [:Wurblzap]]

(In reply to comment #22)
> Should setting be uppercased there? It appears to be lowercase in every
> sentence elsewhere in that page.

I uppercased it because it refers to the Setting class. Other places refer to user settings afaics.

Comment 24

[Bill Barry]

(In reply to comment #23)
> (In reply to comment #22)
> > Should setting be uppercased there? It appears to be lowercase in every
> > sentence elsewhere in that page.
> 
> I uppercased it because it refers to the Setting class. Other places refer to
> user settings afaics.
> 

Hmm... yeah, that sounds good; maybe I just have a hard time reading it that way. Would it be easier to understand if written another way?

The class: <code>[% subclass FILTER html %]</code> is not a valid subclass of Setting.

The class Setting has no valid subclass <code>[% subclass FILTER html %]</code>.

<code>Setting :: [% subclass FILTER html %]</code> refers to an invalid structure.

The class <code>Setting :: [% subclass FILTER html %]</code> does not exist.

--
I merely want to avoid seeing bugs along the lines of "subclass is not a listed param but the error says I set it invalid."

Comment 25

[Max Kanat-Alexander]

Comment on attachment 235795 [details] [diff] [review]
Patch 1.7

  The patch is missing this line in Bugzilla::Install::DB:

  $dbh->bz_add_column('setting', 'subclass', {TYPE => 'varchar(32)'});

  Also, single-file skins are broken, because this line needs to be updated in Skin.pm:

> elsif (/\.css$/) {

  to:

  elsif ($direntry =~ /\.css$/) {

  Otherwise this works fine. So just add those on checkin and you're good.

Comment 26

[Max Kanat-Alexander]

docs bug filed as bug 351486. It just needs somebody to turn the docs into XML, now.

Comment 27

[Marc Schumann [:Wurblzap]]

Thanks for catching these two things, Max. The add_column thing seems to have gotten lost during an unrotting.

Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.50; previous revision: 1.49
done
Checking in Bugzilla/Install.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install.pm,v  <--  Install.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.69; previous revision: 1.68
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.17; previous revision: 1.16
done
Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/User/Setting.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting.pm,v  <--  Setting.pm
new revision: 1.8; previous revision: 1.7
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting/Skin.pm,v
done
Checking in Bugzilla/User/Setting/Skin.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User/Setting/Skin.pm,v  <--  Skin.pm
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.81; previous revision: 1.80
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/global/setting-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/setting-descs.none.tmpl,v  <--  setting-descs.none.tmpl
new revision: 1.9; previous revision: 1.8
done

Comment 28

[Max Kanat-Alexander]

Added to the release notes in bug 349423.