Closed Bug 842913 Opened 11 years ago Closed 11 years ago

Rename winstripe->windows, pinstripe->osx, gnomestripe->linux

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
To improve the situation for new contributors we should rename the themes to match their respective platforms.

For /browser we have three themes:
winstripe -> windows
pinstripe -> osx
gnomestripe -> linux

For /toolkit we have five themes:
winstripe -> windows
pinstripe -> osx
gnomestripe -> linux
faststripe is windows + faststripe (no native theme components)
pmstripe -> os2

faststripe is the only one that doesn't have a clearly better name, but maybe 'fast' is sufficient? I have left it unchanged in this patch, as we could probably fix it in a follow-up bug or if someone has a better idea. I don't think 'fast' is a particularly good name since it indirectly implies the other themes are slow, which they're not ;)

Blair, can you review the toolkit changes?
Matt, can you review the browser changes?
Boris, can you double check the layout/reftest change?
Attachment #715885 - Flags: review?(mnoorenberghe+bmo)
Attachment #715885 - Flags: review?(bzbarsky)
Attachment #715885 - Flags: review?(bmcbride)
Comment on attachment 715885 [details] [diff] [review]
Patch

Review of attachment 715885 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming the |hg move|s are included in the same changeset.

Flagging Dave for super-review since this change crosses modules and to make sure he's aware.  Make sure to give people notice on the mailing lists before landing.
Attachment #715885 - Flags: superreview?(dtownsend+bugmail)
Attachment #715885 - Flags: review?(mnoorenberghe+bmo)
Attachment #715885 - Flags: review+
Comment on attachment 715885 [details] [diff] [review]
Patch

"gnomestripe" is more accurate than "linux" since it's a theme for GTK.

"winstripe" is the base theme in toolkit, e.g. gnomestripe and pmstripe override it where they want to. This is more confusing when the former is called "windows", less so when it has a custom name (although the current name doesn't really cut it as it obviously references Windows). We could also stop making it the base theme.
Attachment #715885 - Flags: review-
Comment on attachment 715885 [details] [diff] [review]
Patch

Unsetting review request pending us being sure we're doing this.
Attachment #715885 - Flags: review?(bzbarsky)
(In reply to Dão Gottwald [:dao] from comment #2)
> "gnomestripe" is more accurate than "linux" since it's a theme for GTK.

In practice it's our Linux theme - I don't see how this distinction is important to note in the directory name.

> "winstripe" is the base theme in toolkit, e.g. gnomestripe and pmstripe
> override it where they want to. This is more confusing when the former is
> called "windows", less so when it has a custom name (although the current
> name doesn't really cut it as it obviously references Windows). We could
> also stop making it the base theme.

Again, I don't think that's reason enough to block this change. We use the Windows theme as the base for others in toolkit/ - that's easy enough to note in comments/documentation (and indeed is already noted in toolkit/themes/Makefile.in).
Comment on attachment 715885 [details] [diff] [review]
Patch

I approve of the general idea - naming by platform rather than by codename is a clarity win for new contributors, and the existing codenames aren't particularly meaningful.

I don't think we necessarily need to make the change for toolkit and browser at the same time, if that coordination proves to be too difficult (I'm optimistic it won't be, though).
Attachment #715885 - Flags: feedback+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > "gnomestripe" is more accurate than "linux" since it's a theme for GTK.
> 
> In practice it's our Linux theme - I don't see how this distinction is
> important to note in the directory name.

The distinction provides clearness (presumably a goal of this bug). The theme has actual GTK dependencies.

> > "winstripe" is the base theme in toolkit, e.g. gnomestripe and pmstripe
> > override it where they want to. This is more confusing when the former is
> > called "windows", less so when it has a custom name (although the current
> > name doesn't really cut it as it obviously references Windows). We could
> > also stop making it the base theme.
> 
> Again, I don't think that's reason enough to block this change. We use the
> Windows theme as the base for others in toolkit/ - that's easy enough to
> note in comments/documentation (and indeed is already noted in
> toolkit/themes/Makefile.in).

Clearness, again. Comment 0 speaks about new contributors and the fact that winstripe is used on more platforms than just Windows has surprised people again and again. In fact this is the only actual new-contributor confusion due to our present structure that I've witnessed. If anything we should make this clearer. Nobody reads documentation in makefiles.
(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #4)
> > (In reply to Dão Gottwald [:dao] from comment #2)
> > > "gnomestripe" is more accurate than "linux" since it's a theme for GTK.
> > 
> > In practice it's our Linux theme - I don't see how this distinction is
> > important to note in the directory name.
> 
> The distinction provides clearness (presumably a goal of this bug). The
> theme has actual GTK dependencies.

Whether it has GTK dependencies or not our linux builds ship with this theme. If I'm a developer wanting to change how the linux build looks I shouldn't need to know that I am using the gnome/gtk theme (maybe I use kde!), I just need to know this is what we ship to our linux users.

> > > "winstripe" is the base theme in toolkit, e.g. gnomestripe and pmstripe
> > > override it where they want to. This is more confusing when the former is
> > > called "windows", less so when it has a custom name (although the current
> > > name doesn't really cut it as it obviously references Windows). We could
> > > also stop making it the base theme.
> > 
> > Again, I don't think that's reason enough to block this change. We use the
> > Windows theme as the base for others in toolkit/ - that's easy enough to
> > note in comments/documentation (and indeed is already noted in
> > toolkit/themes/Makefile.in).
> 
> Clearness, again. Comment 0 speaks about new contributors and the fact that
> winstripe is used on more platforms than just Windows has surprised people
> again and again. In fact this is the only actual new-contributor confusion
> due to our present structure that I've witnessed. If anything we should make
> this clearer. Nobody reads documentation in makefiles.

Yes this is confusing. I'm not sure that these name changes make it any more confusing though. I do wonder how much point there is keeping linux as overrides to windows anymore, you probably have more intelligent things to say about that than me though.

There are probably other things we could do to help alleviate this confusion. Maybe a comment at the top of every css file saying that it is overriding the windows file? Whatever I think it's a separate discussion.
Attachment #715885 - Flags: superreview?(dtownsend+bugmail) → superreview+
my 2 cents, "gtklinux" would be clearer than "gnomestripe" and clearly pointing the fact it has gtk dependencies (in future one may even create a base "linux" theme and have gtklinux, qtlinux, whateverlinux derived themes).
(In reply to Dave Townsend (:Mossop) from comment #7)
> > The distinction provides clearness (presumably a goal of this bug). The
> > theme has actual GTK dependencies.
> 
> Whether it has GTK dependencies or not our linux builds ship with this
> theme. If I'm a developer wanting to change how the linux build looks I
> shouldn't need to know that I am using the gnome/gtk theme (maybe I use
> kde!), I just need to know this is what we ship to our linux users.

If you're a developer wanting to work on this theme, you'd better know about the baked-in GTK dependencies. They're not hidden in the build system or widget code, they're right there in the theme code.

We've had internal and external, paid and unpaid contributors hack on Qt ports. Who can guarantee we'll never have one?

"Linux" is also inaccurate because you can build with GTK on other platforms.

> > Clearness, again. Comment 0 speaks about new contributors and the fact that
> > winstripe is used on more platforms than just Windows has surprised people
> > again and again. In fact this is the only actual new-contributor confusion
> > due to our present structure that I've witnessed. If anything we should make
> > this clearer. Nobody reads documentation in makefiles.
> 
> Yes this is confusing. I'm not sure that these name changes make it any more
> confusing though. I do wonder how much point there is keeping linux as
> overrides to windows anymore, you probably have more intelligent things to
> say about that than me though.

I don't think it's useful anymore for gnomestripe. faststripe and pmstripe might be a different story, but I don't know what the former is used for (I expect it to be dead code) and the latter isn't code we maintain.

> There are probably other things we could do to help alleviate this
> confusion. Maybe a comment at the top of every css file saying that it is
> overriding the windows file? Whatever I think it's a separate discussion.

I'm bringing it up here because this bug wants to reduce confusion at the cost of churning a lot of code. We'd better do it right. If we stick with GTK overriding Win(stripe), I'd rather stick with a custom name for the time being or even go in the opposite direction entirely and call it "base".
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Dave Townsend (:Mossop) from comment #7)
> > > The distinction provides clearness (presumably a goal of this bug). The
> > > theme has actual GTK dependencies.
> > 
> > Whether it has GTK dependencies or not our linux builds ship with this
> > theme. If I'm a developer wanting to change how the linux build looks I
> > shouldn't need to know that I am using the gnome/gtk theme (maybe I use
> > kde!), I just need to know this is what we ship to our linux users.
> 
> If you're a developer wanting to work on this theme, you'd better know about
> the baked-in GTK dependencies. They're not hidden in the build system or
> widget code, they're right there in the theme code.

So you'll spot them as soon as you start working on the theme? That sounds like a good thing to me.

> We've had internal and external, paid and unpaid contributors hack on Qt
> ports. Who can guarantee we'll never have one?
> 
> "Linux" is also inaccurate because you can build with GTK on other platforms.

But we don't. Do we support others doing that?
(In reply to Dave Townsend (:Mossop) from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > If you're a developer wanting to work on this theme, you'd better know about
> > the baked-in GTK dependencies. They're not hidden in the build system or
> > widget code, they're right there in the theme code.
> 
> So you'll spot them as soon as you start working on the theme? That sounds
> like a good thing to me.

Is it? You'll also spot Windows dependencies as you hack winstripe. This bug's point was to make this clearer upfront.

> > We've had internal and external, paid and unpaid contributors hack on Qt
> > ports. Who can guarantee we'll never have one?
> > 
> > "Linux" is also inaccurate because you can build with GTK on other platforms.
> 
> But we don't. Do we support others doing that?

We support it by automatically picking gnomestripe when you're building with MOZ_WIDGET_TOOLKIT=gtk2.
(In reply to Dão Gottwald [:dao] from comment #6)
> The distinction provides clearness (presumably a goal of this bug). The
> theme has actual GTK dependencies.

Our whole code base does. If someone isn't aware that Firefox is a GTK app, I don't think this particular directory's name is going to help them (and in any case we have code review to ensure that). If we end up with a QT theme in the future (seems unlikely) we can revisit the naming then.
 
> Clearness, again. Comment 0 speaks about new contributors and the fact that
> winstripe is used on more platforms than just Windows has surprised people
> again and again. In fact this is the only actual new-contributor confusion
> due to our present structure that I've witnessed. If anything we should make
> this clearer. Nobody reads documentation in makefiles.

I agree; as you note though, this is an existing problem that I don't think this particular change makes any worse, so it's kind of orthogonal to this bug.
We don't "support" (in any real sense of the word) non-Linux GTK builds.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > The distinction provides clearness (presumably a goal of this bug). The
> > theme has actual GTK dependencies.
> 
> Our whole code base does. If someone isn't aware that Firefox is a GTK app,
> I don't think this particular directory's name is going to help them

We're talking about contributors interested in hacking our themes, right? I don't think I see how GTK dependencies in other random parts of our code base matter here.

> (and in any case we have code review to ensure that).

Code review (hopefully) ensures that any kind of new-contributors confusion doesn't result in broken code landing. I don't think this is a helpful argument given this bug's goal.

> If we end up with a QT theme
> in the future (seems unlikely) we can revisit the naming then.

Or we could pick a more accurate name now.

> > Clearness, again. Comment 0 speaks about new contributors and the fact that
> > winstripe is used on more platforms than just Windows has surprised people
> > again and again. In fact this is the only actual new-contributor confusion
> > due to our present structure that I've witnessed. If anything we should make
> > this clearer. Nobody reads documentation in makefiles.
> 
> I agree; as you note though, this is an existing problem that I don't think
> this particular change makes any worse, so it's kind of orthogonal to this
> bug.

It /should/ make it better, as that's this bug's goal.
(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #12)
> > (In reply to Dão Gottwald [:dao] from comment #6)
> > > The distinction provides clearness (presumably a goal of this bug). The
> > > theme has actual GTK dependencies.
> > 
> > Our whole code base does. If someone isn't aware that Firefox is a GTK app,
> > I don't think this particular directory's name is going to help them
> 
> We're talking about contributors interested in hacking our themes, right? I
> don't think I see how GTK dependencies in other random parts of our code
> base matter here.
> 
> > (and in any case we have code review to ensure that).
> 
> Code review (hopefully) ensures that any kind of new-contributors confusion
> doesn't result in broken code landing. I don't think this is a helpful
> argument given this bug's goal.
> 
> > If we end up with a QT theme
> > in the future (seems unlikely) we can revisit the naming then.
> 
> Or we could pick a more accurate name now.

We already build gnomestripe for qt builds. What would you suggest be an accurate name right now that would remain accurate if Qt happened to spin off?
(In reply to Dave Townsend (:Mossop) from comment #15)
> > > If we end up with a QT theme
> > > in the future (seems unlikely) we can revisit the naming then.
> > 
> > Or we could pick a more accurate name now.
> 
> We already build gnomestripe for qt builds.

Well, if the GTK theme bits work there (and I don't know if they do; does anybody use these builds?), then only because some compatibility layer is taking care of them.

> What would you suggest be an
> accurate name right now that would remain accurate if Qt happened to spin
> off?

"gtk" or -- if people are crazy about the somewhat flawed linux reference -- "gtklinux"
The goal of this bug is to give our themes names that are representative of their use. "gnomestripe" is our Linux theme, and the fact that it's optimized for GTK is a detail that isn't relevant to people who are unfamiliar enough with our theming as to need to derive meaning from the directory name.
In other words, the theme directory name should answer "which files do I edit to change the appearance on Linux?"; it does not need to answer "which widget toolkit is the Firefox theme optimized for?".
Does anyone here truly believe that people would have trouble finding "the linux theme" if it were in a directory named gtk? Linux is as much jargon as GTK is. People use Mint, Ubuntu, Android (also Linux but not at all targeted by toolkit/themes/gnomestripe/).
(In reply to Jared Wein [:jaws] from comment #0)
> faststripe is the only one that doesn't have a clearly better name, but
> maybe 'fast' is sufficient? I have left it unchanged in this patch, as we
> could probably fix it in a follow-up bug or if someone has a better idea. I
> don't think 'fast' is a particularly good name since it indirectly implies
> the other themes are slow, which they're not ;)

windows-nonative ?

But we should find out if its dead code or not (like Dao, I expect it is). And if it is, remove it. Either way, followup fodder.


Also, +1 to everything comment 17 and comment 18 said.

(In reply to Dão Gottwald [:dao] from comment #19)
> Does anyone here truly believe that people would have trouble finding "the
> linux theme" if it were in a directory named gtk?

Honestly, yes.
Attachment #715885 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > Does anyone here truly believe that people would have trouble finding "the
> > linux theme" if it were in a directory named gtk?
> 
> Honestly, yes.

Care to elaborate? Note the quotation marks around "the linux theme". The idea that people would always look out for "the linux theme" is flawed for both novice and expert perspectives, as (1) Linux is becoming an increasingly meaningless term these days for people not into kernels and (2) we maintain a GTK theme that doesn't work for all flavours of Linux but does work on some OSes that aren't Linux (whether we produce builds for them doesn't matter; we deliberately outsource this task).
(In reply to Dão Gottwald [:dao] from comment #21)
> The idea that people would always look out for "the linux theme" is flawed for
> both novice and expert perspectives, as (1) Linux is becoming an
> increasingly meaningless term these days for people not into kernels

I don't think this is true. "Linux" is the correct analog to "Windows" and "Mac", pretty much universally.

I think we've discussed this enough; what we name the directory is not critically important enough to merit this much bike-shedding. Let's go with the names in the current patch (though for pinstripe, I'd probably prefer "mac" to "osx").
Comment on attachment 715885 [details] [diff] [review]
Patch

Reflagging Boris for review on the /layout/reftest changes based on comment #22.
Attachment #715885 - Flags: review?(bzbarsky)
Comment on attachment 715885 [details] [diff] [review]
Patch

r=me on that bit
Attachment #715885 - Flags: review?(bzbarsky) → review+
Let's go with linux_gtk then. I think this addresses people's concern that new contributors would be unable to find the linux theme.

And file a bug to remove windows as the base theme in toolkit, as otherwise the clarity this bug seemingly adds just cements the real and not just hypothetical new-contributor confusion there.

I agree on mac vs. osx.
(In reply to Dão Gottwald [:dao] from comment #25)
> Let's go with linux_gtk then. I think this addresses people's concern that
> new contributors would be unable to find the linux theme.
> 
> And file a bug to remove windows as the base theme in toolkit, as otherwise
> the clarity this bug seemingly adds just cements the real and not just
> hypothetical new-contributor confusion there.

Let me step in for a moment here.

Gavin is spot-on in comment 22. This is prime bikeshedding fodder, but at the end of the day the exact name doesn't really matter. Neither perfection nor unanimous approval is the goal, but incremental improvement is. I think Gavin's made a fair appraisal of the issues raised here, so let's respect the decision and move on. I think we all win from moving fast with clear decisions instead of getting embroiled in extended debates.

Also, I've previously talked with Gavin about this, and we're clear to proceed with "osx" instead of "mac" (i.e., land as-is). For exactly the same reasons! :)

I don't think there's any need to block on a bug for removing windows as a base toolkit theme, but happy to discuss it in a new bug. Coincidentally, I've been thinking related thoughts, and bug 844412 is a small step in that direction.

Jared: Land away.
Thanks a lot for doing this!
I disagree with the sentiment that when there's a concern (in this case based on my experience seeing new contributors touch this stuff) and a change is requested, that's bikeshedding. If the name doesn't really matter to you, the change could as well have been made. It would have been a simple search-and-replace task.

In the spirit of incremental improvements, I will now file a new bug on renaming linux to linux_gtk.
https://hg.mozilla.org/mozilla-central/rev/7318b2b26843
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Blocks: 844578
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1361705282.1361705552.24912.gz#err0

Preprocessor.Error: ('', 0, 'FILE_NOT_FOUND', 'e:\\builds\\slave\\c-cen-t-w32-dbg\\build\\mozilla\\toolkit\\themes\\winstripe\\help\\Makefile.in')
configure: error: /e/builds/slave/c-cen-t-w32-dbg/build/mozilla/configure failed for mozilla

Missed one occurence of winstripe
http://hg.mozilla.org/mozilla-central/file/195e706140d1/toolkit/toolkit-makefiles.sh#l1350
Attachment #717617 - Flags: review?(bmcbride) → review+
You need to log in before you can comment on or make changes to this bug.