Closed Bug 677010 Opened 13 years ago Closed 13 years ago

Show update channel on about: page

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.5-, seamonkey2.6 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.6
Tracking Flags:
Tracking Status
seamonkey2.5 - ---
seamonkey2.6 --- fixed

People

(Reporter: InvisibleSmiley, Assigned: iannbugzilla)

Details

Attachments

(3 files, 2 obsolete files)

Update channel in list
103.83 KB, image/png
Details
Update channel in logo container
104.93 KB, image/png
Details
Revised patch for checkin [Checked in: Comment 23]
5.51 KB, patch
iannbugzilla
: review+
iannbugzilla
: ui-review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
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).
Attachment #551237 - Flags: review?(neil)
Attachment #551237 - Flags: feedback?(kairo)
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 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)
Attachment #551237 - Flags: feedback-
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
Attachment #551237 - Flags: review?(neil)
Attachment #551237 - Flags: review-
Attachment #551237 - Flags: feedback?(kairo)
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.
(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.
Assignee: jh → nobody
Status: ASSIGNED → NEW
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 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).
(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)
(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.
(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).
(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?
...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?
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.
Attached patch Similar to firefox (obsolete) — — Splinter Review 
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.
Assignee: nobody → iann_bugzilla
Attachment #551237 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #555871 - Flags: review?(bugspam.Callek)
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.
Attachment #555871 - Flags: review?(bugspam.Callek) → review+
Attached image Update channel in list — 
Screenshot of update channel in the list
Screenshot of update channel in the logo container
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
Attachment #555871 - Flags: ui-review?(neil)
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 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 on attachment 555871 [details] [diff] [review]
Similar to firefox

[Actually, shouldn't we use <strong> rather than <b>?]
Attachment #555871 - Flags: ui-review?(neil) → ui-review+
Carrying forward r/ui-r
Attachment #555871 - Attachment is obsolete: true
Attachment #556817 - Flags: ui-review+
Attachment #556817 - Flags: review+
Comment on attachment 556817 [details] [diff] [review]
Revised patch for checkin [Checked in: Comment 23]

http://hg.mozilla.org/comm-central/rev/88c8f88bf45c
Attachment #556817 - Attachment description: Revised patch for checkin → Revised patch for checkin [Checked in: Comment 23]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
status-seamonkey2.6: --- → fixed
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.6
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: