464784 - What's New page

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Rafael Ebron (:rebron)]

Need What's New page to show what's new in the product and to message changes to the user.

Comment 1

[Tyler Downer [He/Him]]

Not OS specific.

Comment 2

[Emre Birol]

Attachment: Proposed patch 1

Proposed patch to show "What's New" tab that will contain the page.

Content and localization are missing bits. Requesting a generic review.

Comment 3

[Joey Minta]

Comment on attachment 348119 [details] [diff] [review]
Proposed patch 1

Concept looks ok, except for the version-reuse comment at the end.

> window.addEventListener("load", function(e) {
>-    let tabmail = document.getElementById('tabmail');
>-    if (tabmail) {
>-      tabmail.registerTabType(mailTabType);
>-      tabmail.openFirstTab();
>-    }
>   }, false
> );
If you're removing all the logic here, just remove the the listener call too.

> Components.utils.import("resource://gre/modules/folderUtils.jsm");
>+Components.utils.import("resource://gloda/modules/gloda.js");
I don't think this belongs in the patch.

>+// a tab to show the "what's new" page to the user
>+// at the very first start of upgraded TB 
>+var whatsnewTabType = {
>+  name: "whatsNew",
>+  perTabPanel: "iframe",
>+  modes: {
>+    whatsNew: {
>+      type: "whatsNew",
Trailing comma.

>+  openTab: function (aTab) {
Can you give all these functions name to make debugging easier?

>+    aTab.panel.contentWindow.addEventListener("load",
>+      function(e) {}, false);
This is another empty listener...

>+    //aTab.panel.setAttribute("src",
>+    // "chrome://activity/locale/activity.properties");
Obviously this shouldn't stay.

You probably also want a maxTabs attribute, according to tabmail.xml.

>+    
>+  // initialize tabmail system
>+  let tabmail = document.getElementById('tabmail');
>+  if (tabmail)
>+  {
>+    tabmail.registerTabType(mailTabType);
>+    tabmail.openFirstTab();
>+  }
>+  
>+  // Show the "what's new" tab to the user
>+  if (gShowWhatsNewTab)
>+  {
>+    let whatsNewTab = document.getElementById("tabmail");
This is exactly the same as the tabmail variable above, so you should probably just remove this and add the if (gShowWhatsNewtab) check inside the if (tabmail) check.

>   try {
>-
>+    
Oops. extra whitespace.

>     threadPaneUIVersion = gPrefBranch.getIntPref("mailnews.ui.threadpane.version");
> 
>     if (threadPaneUIVersion < 7)
>     {
>+      // show the what's new tab when done with the pane
>+      gShowWhatsNewTab = true;
>+      
I don't think we can use < 7 here, since nightly/alpha users will miss out (since their version was already bumped). We probably need to bump the version another notch and check that.

Comment 4

[Magnus Melin [:mkmelin]]

Why in a tab an not on the start page where we normally display these things? (Just use it as the first run page.)

Comment 5

[Mark Banner (:standard8)]

What Magnus comments about in comment 4 is something we already have in the code base:

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#497

See the startPageUrlPref pref function. Specifically we are currently able to show a different start page on upgrade in the pane just by changing the url redirects on the web site.

If we really do want to show it in a new tab, then we should base the detection code on startPageUrlPref, re-use the preference option(s) and strip out the unnecessary options in startPageUrlPref.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

> Why in a tab an not on the start page where we normally display these things?
> (Just use it as the first run page.)

This is going to be a better system for upgrade pages for us.  For the TB3 release it immediately lets people see that we have tabs in the UI and how they work.  We'll have a bit more space to use than the message reader window.  I see moving away from using the entire message reader space for the start page as a good future step.  

Things like bug 452440 are things it would be better to do in the message reader area; much better than always showing the same page (unless someone changes the pref).  I think we should still allow people to customize what shows up in the message reader area, but (as mentioned in that bug) I'd like to have more of a generated content area than (what is for most) a static page.

Comment 7

[Mark Banner (:standard8)]

So it sounds like:

- Show current start page in message reader view.
- If first start, open additional tab with mailnews.start_page.welcome_url (aka first run)
- If upgraded milestone, open additional tab with mailnews.start_page.override_url

Keep the prefs the same and handle redirects on the server (currently they all redirect to the same url I believe, even though the values of the prefs are different).

Comment 8

[Rafael Ebron (:rebron)]

Does this page need to be a local page or can it reside on the server?  My preference is to host this page on the server.

Comment 9

[Mark Banner (:standard8)]

The preferences are currently all set up to be hosted on (or via redirects from) mozillamessaging.com. That would definitely be the best way as we can always update it for style etc.

Comment 10

[Emre Birol]

Attachment: Proposed patch 2

Joey: Patch 385502 that we have landed on last Thursday has set threadPaneUIVersion to 7. It is relatively short gap for nightly users.. I think it is OK if the tab doesn't show up to whom have downloaded TB before this patch. Many thanks for your detailed review.

Mark: Can you take a look at this patch, and let me know what do you think about my usage of startPageUrlPref().

Comment 11

[Simon Paquet [:sipaq]]

Emre, I saw your post in mozilla.dev.l10n and mozilla.dev.apps.thunderbird.

I would oppose giving this bug late-l10n status. As said elsewhere, late-l10n bugs should be an exception. The keyword is for bugs that come up *after* the string freeze, but are very important/critical for the release and need to be fixed ASAP.

None of this applies to this bug. This can (and should) go in after beta 1. There's no need for us to push 30+ localizers to fix this within a very short timeframe.

Comment 12

[Emre Birol]

Well, this bug is a last minute addition to b1. The impression I got that we want it in b1, since "What's new page" will be the first thing the user sees when upgrade. But, giving that it is not still a blocker, maybe I am wrong.

We have three options; i) we don't submit this patch, and wait for b2. ii) we submit but we don't translate the tab title (only string needs translation in this bug) iii) we consider this bug as an exception. 

byran, rebron, bienvenu thoughts?

Comment 13

[David :Bienvenu]

Sorry, this bug wasn't on my radar, nor was it on the blocking list, or even nominated for blocking status :-(

That being said, I do think we want a first run web page to tell users what's new. It won't be localized, but that's true of our various web pages in beta, normally, isn't it?

Comments from anyone else about whether we should take this bug fix?

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

I'd like to get it in just so we can test the framework of opening up the what's new tab.  I wouldn't bother with the translations as I think we'll be changing the actual page inside the tab before b2 lands.

Comment 15

[David :Bienvenu]

yes, so, is not localizing the tab name a reasonable option for b1?

Comment 16

[Magnus Melin [:mkmelin]]

I'd have to agree with Simon, this isn't something absolutely "needed" for a first beta. I'd even go so far as to say late-l10n should never be used for alphas and betas before there is really a final string freeze.

Comment 17

[Simon Paquet [:sipaq]]

Not localizing it at all is bad as well, because it gives users a bad impression of the localization of the app.

I would propose that we reuse an existing string in our portfolio and change this to the new string as soon as beta1 is out. The following strings look they might be suitable candidates:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/migration/migration.properties#51
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/mozapps/update/updates.properties#18

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 348622 [details] [diff] [review]
Proposed patch 2

>@@ -936,6 +1012,9 @@ function UpgradeThreadPaneUI()
> 
>     if (threadPaneUIVersion < 7)
>     {
>+      // show the what's new tab when done with the pane
>+      gShowWhatsNewTab = true;

Hmm, I didn't make comment 5 clear enough. Hooking into threadPaneUIVersion is just wrong.

Take another look at startPageUrlPref() in mailWindow.js, this bit of code does all the version checking we need:

  var savedVersion = null;
  try {
    savedVersion = pref.getCharPref("mailnews.start_page_override.mstone");
  } catch (ex) {}

  if (savedVersion != "ignore")
  {
    var currentPlatformVersion = Components.classes["@mozilla.org/xre/app-info;1"].
                                            getService(Components.interfaces.nsIXULAppInfo).platformVersion;
    pref.setCharPref("mailnews.start_page_override.mstone", currentPlatformVersion);
    // Use the welcome URL the first time we run
    if (!savedVersion)
      prefForStartPageUrl = "mailnews.start_page.welcome_url";
    else if (currentPlatformVersion != savedVersion)
      prefForStartPageUrl = "mailnews.start_page.override_url";
  }

Note that "mailnews.start_page.welcome_url" is a first run page (for users with new profiles), and "mailnews.start_page.override_url" is the what's new page.

So, startPageUrlPref in mailnews.js essentially needs to change to return just "mailnews.start_page.url", this function can then be reduced to nothing, and we can have a new function to work out if we should show the what's new tab (and maybe the first run page, although at the moment we could make what's new the same as first run, or just not have the first run functionality).

>diff --git a/mail/locales/en-US/chrome/messenger/messenger.properties b/mail/locales/en-US/chrome/messenger/messenger.properties
>+
>+# tab title
>+whatsNew=What's new

Can't we just get the title of the document loaded into the tab? Its a browser element after all. It might look a bit strange whilst its loading, but its worth a try.

Comment 19

[David :Bienvenu]

OK, I'm going to mark this as a blocker for now, and re-assign to Emre since he's doing the code work. Emre, can you update the whiteboard status today with an ETA of when you'll have the code ready to be reviewed? Thx!

Comment 20

[Emre Birol]

It turns out that platformVersion is the version of Gecko, and we cannot use it.
Regarding to our discussion with Mark on IRC, this is the solution we think it might work:

 o Instead of storing platform version in "mailnews.start_page_override.mstone" pref, we gonna store nsIXULAppInfo.version instead.
 o What's New tab will be shown if this pref doesn't reflect the latest version.
 o The content of the What's New page will be "mailnews.start_page.override_url" 
 o Original startPageUrlPref() function will be modified to return either "mailnews.start_page.welcome_url", if new profile, or "mailnews.start_page.url"

This wouldn't cause any regression assuming that "mailnews.start_page_override.mstone" is not used somewhere else in the code to test platform version.

Comment 21

[Emre Birol]

The content of What's New patch is going to evolve over time. As a quick and dirty solution I like Simon's idea at comment #17. Particularly: 

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/mozapps/update/updates.properties#18

When we have all the pages, we can switch to Mark's idea, or localized tab title string approach.

If there is no objection, I will wrap this up today with the string at updates.properties#18

Comment 22

[Emre Birol]

Attachment: Patch rev 3

I used "updateType_major" string for tab title from chrome://mozapps/locale/update/updates.properties bundle as discussed.

Comment 23

[Mark Banner (:standard8)]

So the one problem I've seen with this so far is that if your mail startup page is set to (any?) about: url, then there appears to be no css on the what's new tab. I'm guessing we're doing something strange. I think its most likely to affect nightly users, as they are the ones most likely to be doing other things.

Interestingly it doesn't seem to matter if I set my mail startup page to a website, e.g. my blog.

Also I think we should knock up a quick page to use as default for nightly builds, so that when this lands, our nightly users don't get confused.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 348878 [details] [diff] [review]
Patch rev 3

No sr required for this, its all in mail/.

>diff --git a/mail/base/content/mailWindow.js b/mail/base/content/mailWindow.js
>--- a/mail/base/content/mailWindow.js
>+++ b/mail/base/content/mailWindow.js
>@@ -698,17 +698,8 @@ function startPageUrlPref()
>     savedVersion = pref.getCharPref("mailnews.start_page_override.mstone");
>   } catch (ex) {}
> 
>-  if (savedVersion != "ignore")
>-  {
>-    var currentPlatformVersion = Components.classes["@mozilla.org/xre/app-info;1"].
>-                                            getService(Components.interfaces.nsIXULAppInfo).platformVersion;
>-    pref.setCharPref("mailnews.start_page_override.mstone", currentPlatformVersion);
>-    // Use the welcome URL the first time we run
>-    if (!savedVersion)
>+  if (savedVersion != "ignore" && !savedVersion)
>       prefForStartPageUrl = "mailnews.start_page.welcome_url";

I think you should do the !savedVersion check first (i.e. just swap them round).

Also, the indented line should be 2-space indented from the if.

>+/**
>+ * Tests whether the application has been upgraded
>+ * or not. Updates the pref with the latest version,
>+ * returns true if upgraded, false otherwise.
>+ * 
>+ */ 
>+function IsApplicationUpgraded()
>+{
>+  let savedAppVersion = null;
>+  try {
>+    savedAppVersion = pref.getCharPref("mailnews.start_page_override.mstone");
>+  } catch (ex) {}
>+
>+  if (savedAppVersion != "ignore")
>+  {
>+    let currentApplicationVersion = Components.classes["@mozilla.org/xre/app-info;1"].
>+                                            getService(Components.interfaces.nsIXULAppInfo).version;

Please put the dot on the start of the line. Looking at this, its probably best to format it like this:

let currentApplicationVersion =
  Components.classes["@mozilla.org/xre/app-info;1"]
            .getService(Components.interfaces.nsIXULAppInfo).version;

>+    pref.setCharPref("mailnews.start_page_override.mstone", currentApplicationVersion);
>+           

nit: unnecessary whitespace on blank line.

>+//a tab to show the "what's new" page to the user
>+// at the very first start of upgraded TB 
>+var whatsnewTabType = {
>+  maxTabs: 1,

I think this should be under the whatsNew in the modes section (http://mxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#128).

>+  name: "whatsNew",
>+  perTabPanel: "iframe",
>+  modes: {
>+    whatsNew: {
>+      type: "whatsNew"    

nit: unnecessary whitespace at end of line

>+    }
>+  },
>+  openTab: function onTabOpened (aTab) {
>+    let startpage = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>+                              .getService(Components.interfaces.nsIURLFormatter)
>+                              .formatURLPref("mailnews.start_page.override_url");
>+    aTab.panel.setAttribute("src", startpage);
>+    

nit: whitespace on blank line

>+    try {
>+      let updateBundle = Components.classes["@mozilla.org/intl/stringbundle;1"].
>+                                getService(Components.interfaces.nsIStringBundleService).
>+                                  createBundle(URI_UPDATES_PROPERTIES);

nit: format this like the previous one for the formatURLPref call.
>+
>+      aTab.title = updateBundle.GetStringFromName("updateType_major");      

nit: whitespace at end of line

>+     }
>+    catch(e) {
>+      aTab.title = "What's New";
>+    }
>+  },
>+  closeTab: function onTabClosed (aTab) {
>+  },
>+  saveTabState: function onSaveTabState (aTab) {
>+  },
>+  showTab: function onShowTab (aTab) {
>+  }
>+};
>+
> 
> function SelectAndScrollToKey(aMsgKey)
> {
>@@ -669,7 +733,23 @@ function OnLoadMessenger()
>   // verifyAccounts returns true if the callback won't be called
>   if (verifyAccounts(LoadPostAccountWizard))
>     LoadPostAccountWizard();
>-       
>+  
>+  // initialize tabmail system
>+  let tabmail = document.getElementById('tabmail');
>+  if (tabmail)
>+  {
>+    tabmail.registerTabType(mailTabType);
>+    tabmail.openFirstTab();
>+  }
>+  

nit: whitespace on blank line.

Comment 25

[Mark Banner (:standard8)]

Attachment: Possible nightly update page

Rafael, due to the way this version process works, we'll want a page for nightly builds and as a general catch-all fallback. I've just knocked this together based on the current start page and added a few comments on it as to why its being shown (nightly users may not realise the version gets updated every now and again).

If you want to propose something different (including a complete restructuring) please do, I'm just trying to get us started as I think we want to be landing this at about the same time as the patch if we can.

Comment 26

[Emre Birol]

Attachment: Patch rev 4

nits are fixed.

Comment 27

[David :Bienvenu]

Comment on attachment 349017 [details] [diff] [review]
Patch rev 4

I'm guessing you want review - if not, sorry for butting in :-)

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 349017 [details] [diff] [review]
Patch rev 4

>+  if (savedAppVersion != "ignore")
>+  {
>+    let currentApplicationVersion = Components.classes["@mozilla.org/xre/app-info;1"]
>+                                            .getService(Components.interfaces.nsIXULAppInfo).version;

nit: . should align with the . above it. Please put .version on the next line aligning the same as well

>+var whatsnewTabType = {
>+  name: "whatsNew",
>+  perTabPanel: "iframe",
>+  modes: {
>+    maxTabs: 1,
>+    whatsNew: {
>+      type: "whatsNew"    
>+    }

nit: not quite right, maxTabs should be inside the whatsNew section, alongside type.

>+      let updateBundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                                .getService(Components.interfaces.nsIStringBundleService)
>+                                  .createBundle(URI_UPDATES_PROPERTIES);

nit: dots should all be underneath each other.

>+      aTab.title = updateBundle.GetStringFromName("updateType_major");
>+    }
>+    catch(e) {
>+      aTab.title = "What's New";
>+    }
>+  },
>+  closeTab: function onTabClosed (aTab) {
>+  },
>+  saveTabState: function onSaveTabState (aTab) {
>+  },
>+  showTab: function onShowTab (aTab) {
>+  }
>+};
> 
> function SelectAndScrollToKey(aMsgKey)
> {
>@@ -669,7 +737,22 @@ function OnLoadMessenger()
>   // verifyAccounts returns true if the callback won't be called
>   if (verifyAccounts(LoadPostAccountWizard))
>     LoadPostAccountWizard();
>-       
>+  
>+  // initialize tabmail system

nit: blank line before comment that has whitespace in it

>+  let tabmail = document.getElementById('tabmail');
>+  if (tabmail)
>+  {
>+    tabmail.registerTabType(mailTabType);
>+    tabmail.openFirstTab();
>+  }
>+  

nit: blank line that has whitespace in it.


r=me with that fixed, I'll check this in once we get the "developer" what's new page onto the website.

Comment 29

[Mark Banner (:standard8)]

The nightly/developer what's new page is now on the staging site at http://stage.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/

Comment 30

[Mark Banner (:standard8)]

(In reply to comment #29)
> The nightly/developer what's new page is now on the staging site at
> http://stage.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/

Slightly revised text now on the staging site. I'll shift it to production in the morning.

Comment 31

[Ronald Killmer]

(In reply to comment #30)
> (In reply to comment #29)
> > The nightly/developer what's new page is now on the staging site at
> > http://stage.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/
> 
> Slightly revised text now on the staging site. I'll shift it to production in
> the morning.

Page looks good Mark.

Comment 32

[Mark Banner (:standard8)]

The what's new page is now live on the website, currently waiting for redirects to be set up (bug 466291).

Comment 33

[Mark Banner (:standard8)]

UI patch has now been pushed: http://hg.mozilla.org/comm-central/rev/b87340f2236d

I'm going to say this is fixed and we'll deal with a beta 1 specific page in the release notes bug or a new bug.

Comment 34

[Magnus Melin [:mkmelin]]

FWIW, I think the "What's New page" is quite in your face as it is and I wonder what benefit it brings...

Seems to me there is no interested target audience for this
 - those who are interested will (have) read about what's new elsewhere
 - those who couldn't care less (the absolute majority) are likely to just be annoyed by it

And it doesn't exactly give a good impression that it pops up for first use, before any account is even set up. I would call for a much more subtle way of conveying this info.

Comment 35

[Mark Banner (:standard8)]

(In reply to comment #34)
> FWIW, I think the "What's New page" is quite in your face as it is and I wonder
> what benefit it brings...

I think we really should wait for this discussion until we get the real beta 1 (and beta 2/final) what's new pages. For people that have been served automatic updates, (especially on major version updates), this is our opportunity to point out what's changed to those folks that haven't bothered to read the release notes or have just received an update.

> And it doesn't exactly give a good impression that it pops up for first use,
> before any account is even set up. I would call for a much more subtle way of
> conveying this info.

IMO That's a bug. We should fix it, so I've raised bug 468481, will attach a patch in a while.