Last Comment Bug 677010 - Show update channel on about: page
: Show update channel on about: page
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.6
Assigned To: Ian Neal
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-06 01:29 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-09-26 01:47 PDT (History)
3 users (show)
iann_bugzilla: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
fixed


Attachments
patch (5.36 KB, patch)
2011-08-06 01:29 PDT, Jens Hatlak (:InvisibleSmiley)
bugspam.Callek: review-
bugspam.Callek: feedback-
Details | Diff | Splinter Review
Similar to firefox (5.63 KB, patch)
2011-08-25 15:36 PDT, Ian Neal
bugspam.Callek: review+
neil: ui‑review+
Details | Diff | Splinter Review
Update channel in list (103.83 KB, image/png)
2011-08-29 14:52 PDT, Ian Neal
no flags Details
Update channel in logo container (104.93 KB, image/png)
2011-08-29 14:53 PDT, Ian Neal
no flags Details
Revised patch for checkin [Checked in: Comment 23] (5.51 KB, patch)
2011-08-30 06:16 PDT, Ian Neal
iann_bugzilla: review+
iann_bugzilla: ui‑review+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2011-08-06 01:29:47 PDT
Created attachment 551237 [details] [diff] [review]
patch

KaiRo mentioned on m.s.seamonkey that we should show the update channel on the about: page (which is where you get from Help/About SeaMonkey). I second that. We have more and more people that wonder why our betas identify themselves as finals. The approach taken here does not fully solve that (since you'll be shown "Update Channel: Beta" on a final, too) but it gives a hint.

KaiRo, please check if this is OK from an l10n POV.

Callek, please make sure this is escalated accordingly if you agree that this should be done ASAP (has l10n impact; next uplift to Aurora is August 16).
Comment 1 Justin Wood (:Callek) 2011-08-06 02:55:23 PDT
FWIW I had fully agreed with that suggestion/plan and would have looked into this as well this coming week, but glad that someone else took it up.
Comment 2 Justin Wood (:Callek) 2011-08-06 03:04:03 PDT
Comment on attachment 551237 [details] [diff] [review]
patch

Early comment, any local dev or distro could set a different channel at their choice, for example debian could do update-channel=deb-aurora, its all a configure arg.

So I would suggest instead if |!= "default"| we just use a default: case for the switch there.

(I might have more comments soon)
Comment 3 Justin Wood (:Callek) 2011-08-06 03:15:14 PDT
Comment on attachment 551237 [details] [diff] [review]
patch

Ok, looked at enough to know this won't work as is.

First the review "this is wrong" then my review "I'd like it this way", feel free to argue for an alternative for "I'd like it this way" ;-)


>diff --git a/suite/common/about.xhtml b/suite/common/about.xhtml
>--- a/suite/common/about.xhtml
>+++ b/suite/common/about.xhtml
>@@ -3,16 +3,18 @@
> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
>   "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd" [
> <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
> %brandDTD;
> <!ENTITY % aboutDTD SYSTEM "chrome://global/locale/about.dtd" >
> %aboutDTD;
> <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
> %globalDTD;
>+<!ENTITY % suiteAboutDTD SYSTEM "chrome://communicator/locale/about.dtd" >
>+%suiteAboutDTD;
> ]>
> 
> <!-- ***** BEGIN LICENSE BLOCK *****
>    - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>    -
>    - 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
>@@ -92,16 +94,45 @@
>       var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
>                                  .getService(Components.interfaces.nsIXULAppInfo)
>                                  .version;
> 
>       var version = document.getElementById("version");
> 
>       version.appendChild(document.createTextNode("&about.version; " + versionNum));
> 
>+      // insert channel name
>+      var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefBranch);
>+      var updateChannel;
>+      try {
>+        updateChannel = prefs.getCharPref("app.update.desiredChannel");
>+      } catch (e) {
>+        updateChannel = prefs.getCharPref("app.update.channel");
>+      }

Wrong here. This presumes that if you set the update Channel in about:config that it would work. It would change the UI in your code, but not what the app uses. Instead use:
Services.prefs.getDefaultBranch("");

and don't use desiredChannel (we don't support channel switching on our end; so its a no-op for us)

>+      // The default channel is used for custom-built releases that lack update capabilities.
>+      if (updateChannel != "default") {
>+        switch (updateChannel) {
>+          case "nightly":
>+            updateChannel = "&about.updateChannel.nightly;"
>+            break;
>+          case "aurora":
>+            updateChannel = "&about.updateChannel.aurora;"
>+            break;
>+          case "beta":
>+            updateChannel = "&about.updateChannel.beta;"
>+            break;
>+          case "release":
>+            updateChannel = "&about.updateChannel.release;"
>+            break;
>+        }
>+        version.appendChild(document.createTextNode(" (&about.updateChannel;" +
>+                                                    updateChannel + ")"));
>+      }

This *allows* us to pretty-up update-channels based on a particular channel name, but imo this is much more complicated than we need to have stuff here.

I would prefer an almost 1:1 copy of what Firefox uses here, (here being .xul, and .dtd)

Links: 
* http://mxr.mozilla.org/comm-central/source/mozilla/browser/locales/en-US/chrome/browser/aboutDialog.dtd?mark=58-62#57
* http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.xul?mark=117-119#116
* http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js?mark=91-93#89
* http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.css#73
Comment 4 neil@parkwaycc.co.uk 2011-08-06 06:02:27 PDT
Comment on attachment 551237 [details] [diff] [review]
patch

I haven't looked at the Firefox code but this seems like something that should be done in a .properties file rather than a .dtd file.
Comment 5 Jens Hatlak (:InvisibleSmiley) 2011-08-06 06:54:41 PDT
(In reply to Justin Wood (:Callek) from comment #2)
> Early comment, any local dev or distro could set a different channel at
> their choice, for example debian could do update-channel=deb-aurora, its all
> a configure arg.

The way I wrote it, any update channel name (other than "default") that is not explicitly checked is reflected as-is.

(In reply to Justin Wood (:Callek) from comment #3)
> and don't use desiredChannel (we don't support channel switching on our end;
> so its a no-op for us)

I saw it was used in Toolkit code so I'd assume that it is an override that is checked first. Unless I'm wrong there, it doesn't matter whether we have channel switching or not: If a user can set it in about:config and it gets used, then about: should reflect that. Anyway, that's not for me to check anymore (see below).

> This *allows* us to pretty-up update-channels based on a particular channel
> name, but imo this is much more complicated than we need to have stuff here.

It allows for l10n of the defaults, which was my intention.

> I would prefer an almost 1:1 copy of what Firefox uses here, (here being
> .xul, and .dtd)

As you wish. You seem to have a pretty clear picture of what you'd like to see, so I think it's best if you code up what you like and discuss the rest with Neil. No point for me to stand in the way of multiple reviewers and their opinions. As long as this gets fixed some way, I'll be content.
Comment 6 Tony Mechelynck [:tonymec] 2011-08-06 07:27:53 PDT
Jens: I don't remember where or when, but I did see somewhere that changing the update channel in about:config is not enough to move to a different channel. IIRC, something else is required, and the Update Channel Selector extension https://addons.mozilla.org/en-US/seamonkey/addon/update-channel-selector/ (works also on Fx / Tb) does it.
Comment 7 Robert Kaiser 2011-08-06 07:52:45 PDT
Comment on attachment 551237 [details] [diff] [review]
patch

Actually, I quite like the idea of appending to the version, I just would only append the pure channel value, so that it says "version 2.3 beta" or "version 2.5a1 nightly" (even "version 2.4a2 default" is OK, IMHO).
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-08-06 07:56:01 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #7)
> Actually, I quite like the idea of appending to the version, I just would
> only append the pure channel value, so that it says "version 2.3 beta" or
> "version 2.5a1 nightly" (even "version 2.4a2 default" is OK, IMHO).

* if you are on the beta channel but run a release, it would still say (e.g.) "2.3 beta", which is just wrong
* "2.4a2 default" looks stupid, IMO (but we could e.g. put "custom" there)
Comment 9 Tony Mechelynck [:tonymec] 2011-08-06 08:50:21 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #8)
[...]
> * if you are on the beta channel but run a release, it would still say
> (e.g.) "2.3 beta", which is just wrong
[...]

IIUC, there's no other way to distinguish release and beta builds, since the release build _is_ the last beta, the only difference being that it is set to update from the release channel (but otherwise not recompiled), so it is not "just wrong", it is "the way it is". If you are on the beta channel but run a release, it must be assumed that what you run is actually a beta (the last one) because that's how we tell them apart.
Comment 10 Jens Hatlak (:InvisibleSmiley) 2011-08-06 09:22:55 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #5)
> (In reply to Justin Wood (:Callek) from comment #3)
> > and don't use desiredChannel (we don't support channel switching on our end;
> > so its a no-op for us)
> 
> I saw it was used in Toolkit code so I'd assume that it is an override that
> is checked first.

Correcting myself: Set app.update.desiredChannel to "beta" in SM 2.2 and checked for updates. Nothing. So the pref can indeed be ignored. Sorry for the confusion.

(In reply to Tony Mechelynck [:tonymec] from comment #9)
> IIUC, there's no other way to distinguish release and beta builds, since the
> release build _is_ the last beta, the only difference being that it is set
> to update from the release channel (but otherwise not recompiled), so it is
> not "just wrong", it is "the way it is". If you are on the beta channel but
> run a release, it must be assumed that what you run is actually a beta (the
> last one) because that's how we tell them apart.

Please re-think what you said from a user POV. If the page that comes up after you selected Help/About SeaMonkey says "2.3 beta" after you got and accepted an update prompt for 2.3 final, you *will* be confused. And as you said, there is *no way* to tell a release from a beta, but this bug should (IMO) provide an *indication* that you might be running a beta. If not, this bug fails its purpose (again IMO).
Comment 11 Tony Mechelynck [:tonymec] 2011-08-06 10:07:23 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #10)
[...]
> Please re-think what you said from a user POV. If the page that comes up
> after you selected Help/About SeaMonkey says "2.3 beta" after you got and
> accepted an update prompt for 2.3 final, you *will* be confused. And as you
> said, there is *no way* to tell a release from a beta, but this bug should
> (IMO) provide an *indication* that you might be running a beta. If not, this
> bug fails its purpose (again IMO).

Can you get an update prompt for a release if you're on the beta channel? I thought in that case you would already have the last beta (= the release build, but delivered on the beta channel before it was known that there would be no further beta for that version) so you wouldn't need to update it? And the next update you would get would be the first beta for the _next_ release?
Comment 12 Tony Mechelynck [:tonymec] 2011-08-06 10:17:36 PDT
...Conversely, if (unlike what I thought) you _do_ get an update prompt for the release while being on the beta channel, wouldn't that update, once installed, change the update channel to "release" or whatever that other channel is named?
Comment 13 Robert Kaiser 2011-08-06 10:47:12 PDT
After thinking about it some more, I'd bet happy if we'd find something to make nightly and aurora channels a bit more visible there, but it's probably best to go for the Firefox solution for displaying the channel.
Comment 14 Ian Neal 2011-08-25 15:36:03 PDT
Created attachment 555871 [details] [diff] [review]
Similar to firefox

This patch adds an extra line to the list with what the current update channel is in a similar way to that done by Firefox.
Comment 15 Justin Wood (:Callek) 2011-08-28 21:10:52 PDT
Comment on attachment 555871 [details] [diff] [review]
Similar to firefox

>diff --git a/suite/common/about.xhtml b/suite/common/about.xhtml
>@@ -61,16 +63,17 @@
>   <div id="aboutLogoContainer">
>     <a id="vendorURL" href="http://www.seamonkey-project.org/">
>       <img src="about:logo" alt="&brandShortName;"/>
>       <p id="version"></p>
>     </a>
>   </div>
> 
>   <ul id="aboutPageList">
>+    <li>&channel.description.start;<b id="currentChannel"></b>&channel.description.end;</li>
>     <li>&about.credits.beforeLink;<a href="about:credits">&about.credits.linkTitle;</a>&about.credits.afterLink;</li>

Nit: I would _MUCH_ prefer this in the Logo Container, under version as <p> instead of an <li>. But if you feel strongly I won't block on that to get this landed. After all you are the bikeshed painter today.
Comment 16 Ian Neal 2011-08-29 14:52:24 PDT
Created attachment 556678 [details]
Update channel in list

Screenshot of update channel in the list
Comment 17 Ian Neal 2011-08-29 14:53:09 PDT
Created attachment 556679 [details]
Update channel in logo container

Screenshot of update channel in the logo container
Comment 18 Ian Neal 2011-08-29 14:54:10 PDT
Comment on attachment 555871 [details] [diff] [review]
Similar to firefox

Requesting ui-review as to where is best for the update channel to show up in the about: page
Comment 19 Justin Wood (:Callek) 2011-08-29 14:55:49 PDT
Comment on attachment 556679 [details]
Update channel in logo container

Fwiw I would have expected CSS for similar size, position, and coloring as the Version string for this one.

But I don't necessarily mind the current patch if you don't like the concept of it in logo container. (I just feel its a better place for it)
Comment 20 neil@parkwaycc.co.uk 2011-08-29 16:04:56 PDT
Comment on attachment 555871 [details] [diff] [review]
Similar to firefox

>+    <li>&channel.description.start;<b id="currentChannel"></b>&channel.description.end;</li>
[Why does the channel need to be bold?]

>+      // Determine and display current channel.
>+      var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefService)
>+                            .getDefaultBranch("");
>+      var updateChannel;
>+      try {
>+        updateChannel = prefs.getCharPref("app.update.channel");
>+      } catch (ex) {
>+        updateChannel = "default";
>+      }
>+      var currentChannel = document.getElementById("currentChannel");
>+      currentChannel.appendChild(document.createTextNode(updateChannel));
[Don't use createTextNode, use textContent instead.]
If you write the default value in the XHTML
<b id="currentChannel">default</b>
Then you can write
try {
  document.getElementById("currentChannel").textContent =
      Components.classes["@mozilla.org/preferences-service;1"]
                .getService(Components.interfaces.nsIPrefService)
                .getDefaultBranch("");
                .getCharPref("app.update.channel");
} catch (ex) {
}
Or you may want to split that up to improve legibility.
Comment 21 neil@parkwaycc.co.uk 2011-08-30 01:29:52 PDT
Comment on attachment 555871 [details] [diff] [review]
Similar to firefox

[Actually, shouldn't we use <strong> rather than <b>?]
Comment 22 Ian Neal 2011-08-30 06:16:21 PDT
Created attachment 556817 [details] [diff] [review]
Revised patch for checkin [Checked in: Comment 23]

Carrying forward r/ui-r
Comment 23 Ian Neal 2011-08-30 06:22:54 PDT
Comment on attachment 556817 [details] [diff] [review]
Revised patch for checkin [Checked in: Comment 23]

http://hg.mozilla.org/comm-central/rev/88c8f88bf45c

Note You need to log in before you can comment on or make changes to this bug.