Closed Bug 954826 Opened 10 years ago Closed 10 years ago

Suggest popular protocols during account creation

Categories

(Instantbird Graveyard :: Account manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: wnayes)

References

()

Details

Attachments

(3 files, 5 obsolete files)

*** Original post on bio 1391 at 2012-04-24 01:46:00 UTC ***

An idea that's been kicked around a lot on IRC but seems to have no bug filed: we'd like to simplify the account creation wizard to only initially show the few accounts we feel the user is interested in adding. Since this will most likely vary by language / location, these need to be localizable.

This would show the user a short list of accounts (for en-US, probably: GTalk, Facebook, AIM, MSN and Twitter) + a choice for "other" which would show all installed protocols, like we have now.

It's possible we'd want to show all user installed protocols in the "short list" as well. (As these would most likely be ones that the user is interested in adding)...

For the record my choices for the necessary protocols for en-US are purely subjective and based on experience. :)
Attached image Mic's mockup
*** Original post on bio 1391 as attmnt 1384 at 2012-04-24 01:50:00 UTC ***

See http://log.bezut.info/instantbird/111105/#m46

09:44:39 <Mic> That's the context for this: http://img543.imageshack.us/img543/9219/ibaccountwizard.png
09:45:07 <Mic> Netsoul and Sametime were only examples for user-installed protocols here of course ;)
09:46:09 <Mic> The section headers shouldn't be selectable here... maybe I need to find a different solution.
10:11:38 <flo> Mic: I don't understand that complicated design :(
10:11:51 <flo> what's the point of the "Custom Protocols" list?
10:15:40 <Mic> To show protocols that the user has added himself.
10:16:33 <flo> ok
10:16:46 <flo> let me do a quick mockup of what I had in mind for a "simplified protocol list"
10:17:45 <Mic> I was also thinking about just putting the user-installed + popular + currently protocols into one list and not differentiate them anyhow
10:19:30 <Mic> The way I did it seemed clearer in showing that this is only a selection of protocols and not all the user can chose from.
10:21:32 <Mic> Other ideas were to get rid of the listbox (at least visually) and put a list of larger icons with larger network names there + a more visible "Your network is not in the list? Look here..." somewhere.
10:22:04 <Mic> Well, just show me your mockup and we'll see ;)
10:22:50 <flo> Mic: "Other ideas were to get rid of the listbox (at least visually) and put a list of larger icons with larger network names there + a more visible "Your network is not in the list? Look here..." somewhere." what's way closer to what I had in mind
10:32:35 <flo> http://queze.net/goinfre/ibwizard.png Selecting "other" and clicking "next" would just show the current list.
10:33:20 <flo> the list of the 3 or 4 popular protocols listed is localizable, and the descriptive strings are localizable too.
10:33:52 <flo> showing larger icons is part of the goal, as I would like people to click familiar icons, rather than reading labels to search through a long list
10:48:53 <Mic> hmm, what's your idea concerning user-installed protocols?
10:49:25 <flo> how are they different from other unpopular protocols?
10:50:02 <flo> if the user took the time to install them, it's unlikely that he won't think to click "show more"
10:51:06 <Mic> If the user took the time to install them, he's likely to use them?
10:52:29 <flo> which situation are you trying to optimize?
10:52:53 <flo> the case I was trying to improve is the noob user scared by a huge list of protocols he has never heard about
10:53:15 <Mic> I was thinking about "user wanting to set up an account"
10:53:30 <flo> setting up an account is a rare event
10:53:33 <Mic> i.e. we show popular and the ones he installed first and hide all others in the "More" list
10:53:51 <flo> it doesn't need to be extremely fast, an additional click isn't a horrible burden.
10:54:09 <flo> scaring someone off during the first start with a messy list seemed worst to me
10:55:14 <flo> Mic: showing the most recently installed prpl inside the list of 4 popular networks (replacing the least popular of the 4) would be nice. But is it worth it?
10:55:41 <Mic> What would you do with the descriptive text in this case, btw?
10:55:46 <flo> I don't like the idea of displaying several separate lists on the same listbox, because then you can't rely on the list being sorted to quickly find what you want, you have to look at each sublist
10:56:06 <flo> "you've recently installed a plugin for this network" ? :-D
10:56:18 <Mic> I ditched the sublist idea already, you won't need to argue against it;)
10:56:27 <flo> :)
10:56:47 <Mic> That would be nice but it would be nicer to have a descriptive text for each protocol plugin.
10:57:34 <Mic> Would it be hard to add a localized descriptive text for each protocol plugin that can be fetched somehow?
10:58:19 <flo> I wouldn't assume a protocol installed as an add-on contain any string in the current locale of the application.
10:58:46 <flo> localization of add-ons is painful
10:59:10 <flo> (because add-on author have to duplicate all the work of coordinating the work of translators)
10:59:39 <Mic> Yes, I know.
10:59:49 <Mic> First-hand ;)
13:35:47 <clokep> Mic, flo: Perhaps we should show the account wizard with the just installed protocol selected after they install a protocol?
Attached image flo's mockup
*** Original post on bio 1391 as attmnt 1385 at 2012-04-24 01:51:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 1391 as attmnt 1423 at 2012-05-01 17:20:00 UTC ***

This patch adds a "Top Protocols" wizardpage to the Account Wizard, which reads a list of localized protocols from an added "accountWizard.properties" file. The appearance of the new page can be seen here:

http://i.imgur.com/7h5td.png

The usage of the new richlistbox is the same as that of the regular protocol listbox, with the exception of the "Other..." item that leads to the standard protocol list when it is double clicked or the page is advanced. If the user chooses the Other item, the wizard navigation prevents returning to the top protocols page again.
Attachment #8353175 - Flags: review?(clokep)
Comment on attachment 8353175 [details] [diff] [review]
Working example of Florian's mockup

*** Original change on bio 1391 attmnt 1423 at 2012-05-01 18:13:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353175 - Flags: review?(clokep) → review-
Attachment #8353175 - Flags: review?(florian)
*** Original post on bio 1391 at 2012-05-01 18:13:29 UTC ***

Comment on attachment 8353175 [details] [diff] [review] (bio-attmnt 1423)
Working example of Florian's mockup

First of all, thanks for attaching this!  Overall it looks very good. Some of my comments are just coding style nits.

>diff --git a/instantbird/content/accountWizard.js b/instantbird/content/accountWizard.js
>@@ -41,30 +41,71 @@ Cu.import("resource:///modules/imService
>+    var topProtocolStrings = document.getElementById("topProtocolsBundle");
>+    var protocolDelimiter = topProtocolStrings.getString("topProtocol.list.delimiter");
Any particular reason we made the separator localizable? The IDs cannot contain a "," AFAIK so I think we can safely force it to be a comma separated list.

>+    var topProtocols = topProtocolStrings.getString("topProtocol.list").split(protocolDelimiter);
>+    
>     protos.forEach(function(proto) {
>       var id = proto.id;
>       var item = protoList.appendItem(proto.name, id, id);
>       item.setAttribute("image", proto.iconBaseURI + "icon.png");
>       item.setAttribute("class", "listitem-iconic");
>+      
>+      for (var i = 0; i < topProtocols.length; i++) {
Nit: ++i

>+        if (topProtocols[i] == id) {
Actually looking on this more...I think instead of the second for loop you can do:
if (topProtocols.indexOf(id) != -1)
to check if the protocol exists.


>+          var tItem = topProtoList.insertItemAt(topProtoList.itemCount - 1, "", id);
>+          tItem.removeChild(tItem.childNodes[0]);
Why are we removing this child node? It looks to me like you actually want to do this at the very end of this if-statement, i.e. you create the element first and then insert it.

I do wonder if the entire element should be made as a binding instead of build dynamically. I'm not sure if there any benefits to that besides core organization. If not we should probably add some comments to that section about what each group is doing.

>+    if (topProtoList.itemCount < 2) {
>+      wizard.getPageById("accountwelcome").next = "accountotherprotocol";
>+    }
I think this could use a comment saying that if the only option is "Other" skip to that page automatically.
Nit: Don't include { } if there's only one line inside the block.

>@@ -95,25 +136,23 @@ var accountWizard = {
>+  selectProtocol: function aw_selectProtocol(listId) {
>+    var protoList = document.getElementById(listId);
>     var id = protoList.selectedItem.value;
>     this.proto = Services.core.getProtocolById(id);
>-
>     return true;
>   },
> 
>-
Generally we try to avoid arbitrary space changes in our patches, but I don't actually have a problem with these two.

>   insertUsernameField: function aw_insertUsernameField(aName, aLabel, aParent,
>                                                        aDefaultValue) {
>     var hbox = document.createElement("hbox");
>     hbox.setAttribute("id", aName + "-hbox");
>     hbox.setAttribute("align", "baseline");
>     hbox.setAttribute("equalsize", "always");
> 
>     var label = document.createElement("label");
>@@ -566,10 +605,31 @@ var accountWizard = {
>+  otherItemSelected: function (){
>+    var topProtoList = document.getElementById("topprotolist");
>+    if (topProtoList.selectedIndex == topProtoList.itemCount - 1) {
>+      return true;
>+    }
>+    return false;
The if-statement isn't necessary, just return the boolean.

>diff --git a/instantbird/content/accountWizard.xul b/instantbird/content/accountWizard.xul
Skipping this file for now. I'll get back to it.

>diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd b/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd
>--- a/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd
>+++ b/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd
>@@ -2,16 +2,18 @@
> <!ENTITY accountProtocolGetMore.label "Get more...">
>+<!ENTITY accountProtocolOther.label   "Other...">
Uhh...weird character here, what's up with that? (Wrong file encoding or something?)

>diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
>@@ -0,0 +1,11 @@
>+# LOCALIZATION NOTE
>+# The selected protocols below should be the most relevant
>+# to the target language / location and comma delimited. Avoid
>+# exceeding 4 protocols to prevent scrolling.
>+topProtocol.list=prpl-gtalk,prpl-facebook,prpl-twitter,prpl-msn
>+topProtocol.list.delimiter=,
Where can translators get this magic list of IDs? :)

>+topProtocol.prpl-gtalk.description=Talk to friends through GMail or any XMPP server
>+topProtocol.prpl-facebook.description=Communicate with Facebook Chat friends
>+topProtocol.prpl-twitter.description=Keep up to date through Twitter feeds
>+topProtocol.prpl-msn.description=Microsoft Windows Live Messenger (formerly MSN)
This probably needs a note above this too! (What's the format of this? How is it used?)

So I didn't try this code, but I'll hopefully have time to do it tonight and give you some more feedback. :)
*** Original post on bio 1391 at 2012-05-01 18:19:09 UTC ***

Assigning to Will.
Assignee: nobody → wnayes
Status: NEW → ASSIGNED
*** Original post on bio 1391 at 2012-05-01 18:32:43 UTC ***

I wonder if one could unify what are now two pages, accounttopprotocol and accountotherprotocol (if I understand correctly - I haven't tested it), by having the "Other..." option expand the list of protocols when double-clicked. 

I am not sure what the best way of styling this would be though. "Expand" is usually indicated in IB by a downward arrow (cf contacts), that might be enough?

Nitpicking suggestion: s/longer list of networks/complete list of networks
Status: ASSIGNED → NEW
*** Original post on bio 1391 at 2012-05-01 18:43:54 UTC ***

(In reply to comment #6)
> I wonder if one could unify what are now two pages, accounttopprotocol and
> accountotherprotocol (if I understand correctly - I haven't tested it), by
> having the "Other..." option expand the list of protocols when double-clicked. 

I don't think so, as the list of top protocols has a longer description for each protocol, that we don't have for others.
*** Original post on bio 1391 at 2012-05-01 19:33:08 UTC ***

(In reply to comment #7)
> (In reply to comment #6)
> > I wonder if one could unify what are now two pages, accounttopprotocol and
> > accountotherprotocol (if I understand correctly - I haven't tested it), by
> > having the "Other..." option expand the list of protocols when double-clicked. 
> 
> I don't think so, as the list of top protocols has a longer description for
> each protocol, that we don't have for others.

You could replace the whole list with single-line items when "expand" is clicked. A CSS transition could make this smooth. 
My suggestion was more about avoiding a separate page (and the associated "flicker") for what is a list of protocols in both cases.
*** Original post on bio 1391 at 2012-05-02 10:22:45 UTC ***

Something else to ponder: What happens if you end up with no "top protocols" (either because the property is left blank or none of the prpls are installed)? Seems like you should just skip that page, but I'm not sure if we care for this situation.
Status: NEW → ASSIGNED
*** Original post on bio 1391 at 2012-05-02 10:43:54 UTC ***

(In reply to comment #9)
> Something else to ponder: What happens if you end up with no "top protocols"
> (either because the property is left blank or none of the prpls are installed)?
> Seems like you should just skip that page, but I'm not sure if we care for this
> situation.

Seems you already covered this. :)
>    if (topProtoList.itemCount < 2) {
>      wizard.getPageById("accountwelcome").next = "accountotherprotocol";
>    }
Comment on attachment 8353175 [details] [diff] [review]
Working example of Florian's mockup

*** Original change on bio 1391 attmnt 1423 at 2012-05-02 11:29:41 UTC ***

Some quick feedback:
- First, this looks very promising! :-)
- I agree with all the review comments given by Patrick in comment 4.
- I think I would have only populated the list of popular protocols when loading the dialog, and populated the full list only if it's going to be displayed, but I don't think this really matters; your approach works too.
- The "welcome" page may not be needed anymore.
- We will probably want to change the wording of some strings a bit.

(In reply to comment #3)
> If the user
> chooses the Other item, the wizard navigation prevents returning to the top
> protocols page again.

I don't think this behavior is wanted.

I'm not going to do a detailed review of the code now, as I think you already have enough feedback to move forward.

We had a discussion on this topic on IRC a few minutes ago, you may want to read it: http://log.bezut.info/instantbird/120502/#m102
Attachment #8353175 - Flags: review?(florian)
*** Original post on bio 1391 as attmnt 1428 at 2012-05-03 01:44:00 UTC ***

I've rewritten the patch to contain both lists within a single richlistbox. The two different types of entries (top/normal protocols) now use XBL bindings instead of being pieced together. The "Other..." entry was removed in favor of a separate checkbox which toggles showing top/normal entries.

This should reduce any scrollbar confusion, and simplifies the flow of the wizard. When the topProtocols.list localized entry is left empty, the wizard functions as it did prior.

I did not change the description wordings or top protocols selected, as they are more or less fillers until a final decision is made.

I did my best to write in the coding style of the other files I've worked with, as well as the Coding Style MDN page. I still left one of the empty line removals, as the rest of the file only used single line spaces between functions.
Attachment #8353180 - Flags: review?(clokep)
*** Original post on bio 1391 at 2012-05-03 13:38:24 UTC ***

(In reply to comment #12)
> Created attachment 8353180 [details] [diff] [review] (bio-attmnt 1428) [details]
> Protocol toplist with XBL bindings
> 
> I've rewritten the patch to contain both lists within a single richlistbox. The
> two different types of entries (top/normal protocols) now use XBL bindings
> instead of being pieced together. The "Other..." entry was removed in favor of
> a separate checkbox which toggles showing top/normal entries.
So after discussing this a bit more on IRC between Florian, aleth and I. We think the first iteration of the UI is a simpler user experience (UX). Sorry for the confusion we might have caused -- we didn't really give good feedback.

Was there any reason you choose this implementation or was the IRC conversation between Florian, aleth and I just too confusing to follow? If there's reasons you think this way is much cleaner (I see you say it "simplifies the flow of the wizard"), why is that? Maybe you can convince us we're wrong. :)

The XBL binding implementation looks pretty good, although one downside of this is all the paddings are now wrong on the icons of the "normalProtocol" binding.  Instead of setting the class to enforce a particular binding, it's nicer to just create the binding, see [1] and [2] for a similar example.

> This should reduce any scrollbar confusion, and simplifies the flow of the
> wizard. When the topProtocols.list localized entry is left empty, the wizard
> functions as it did prior.
I don't think worrying about scrolling is /too/ important, as the number of "top protocols" might vary by locale.

> I did not change the description wordings or top protocols selected, as they
> are more or less fillers until a final decision is made.
I agree, this can wait until the code is closer to finalized!

> I did my best to write in the coding style of the other files I've worked with,
> as well as the Coding Style MDN page. I still left one of the empty line
> removals, as the rest of the file only used single line spaces between
> functions.
I didn't see any glaring issues, but it's not worth giving any coding style nits right now, as the behavior is changing a bit still.

[1] http://lxr.instantbird.org/instantbird/source/instantbird/content/blist.js#737
[2] http://lxr.instantbird.org/instantbird/source/instantbird/content/blist.css#161
*** Original post on bio 1391 as attmnt 1448 at 2012-05-05 20:56:00 UTC ***

I gave another go at a design closer to the first patch submitted. XBL bindings are used with the richlistbox that shows the top protocols, while a regular listbox uses the listbox-iconic style to display the full protocol list.

I did not modify the wizard back/next flow, which was an issue in the first patch (my reasoning back then was that I was designing it to feel like a single page). Removing the welcome page in another bug would likely make the back/next work with the nonlinear path of this design better. I am also experiencing a focus issue on the welcome page; the Cancel button is focused instead of the Next button.

The top protocols page has no selection by default, which allows the Next button to move to the full (keyboard searchable) list right away. I also changed the design of the "Other..." entry, which I am interested in feedback about.
Attachment #8353201 - Flags: review?(clokep)
Comment on attachment 8353180 [details] [diff] [review]
Protocol toplist with XBL bindings

*** Original change on bio 1391 attmnt 1428 at 2012-05-06 13:53:38 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353180 - Flags: review?(clokep) → review-
Comment on attachment 8353201 [details] [diff] [review]
Fixed issues with the first patch

*** Original change on bio 1391 attmnt 1448 at 2012-05-06 14:45:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353201 - Flags: review-
Attachment #8353201 - Flags: feedback+
*** Original post on bio 1391 at 2012-05-06 14:45:42 UTC ***

Comment on attachment 8353201 [details] [diff] [review] (bio-attmnt 1448)
Fixed issues with the first patch

I looked at the code and tried the patch. It looks good overall (feedback+) but there are still lots of details to address before it's ready to be checked-in (review-).

>diff --git a/instantbird/content/accountWizard.js b/instantbird/content/accountWizard.js

>+    let topProtoList = document.getElementById("topprotolist");
>+    let topProtocols = accountWizard.getTopProtocols();

The getTopProtocols method seems to be called only once, it seems it could be inlined instead.

>+    for (var i = 0; i < topProtocols.length; ++i) {
>+      let proto = Services.core.getProtocolById(topProtocols[i]);

It seems you don't use i anywhere inside this loop, so you could use this style instead:
 for each (let topProtocol in topProtocols)
   // do something with topProtocol


>+      if (proto == null)
>+        continue;
>+
>+      let tItem = document.createElement("richlistitem");

What does the t prefix mean in tItem? The only prefixes we really use in JS code are k for constants, and _ for member variables/methods that are intended to be used only internally.

You can use a custom name instead of "richlistitem" for your element. It helps for the CSS setting the -moz-binding value :).

>+      let otherItem = document.getElementById("otherListItem");

Should this line be outside of the loop?

>+      topProtoList.insertBefore(tItem, otherItem);

Or it you don't put it outside of the loop, you don't really need the variable and can just inline it here as it's used only once.

>+      let desc = document.getElementById("topProtocolsBundle")
>+                         .getString("topProtocol." + proto.id + ".description");
>+      tItem.build(proto, desc);
>+    }

I think a previous version of your patch used to handle the case where there's no topProtocol (or non of them is installed). This patch doesn't seem to handle this case any more.


>     // there is a strange selection bug without this timeout
>     setTimeout(function() {
>-      protoList.selectedIndex = 0;
>+      topProtoList.selectedIndex = -1;
>     }, 0);

What's the default behavior if you remove this code? Would it select the first item, or not select anything?

>@@ -95,25 +100,24 @@ var accountWizard = {
>       return;
>     }
> 
>     var exists = accountWizard.proto.accountExists(name);
>     wizard.canAdvance = !exists;
>     duplicateWarning.hidden = !exists;
>   },
> 
>-  selectProtocol: function aw_selectProtocol() {
>-    var protoList = document.getElementById("protolist");
>+  selectProtocol: function aw_selectProtocol(aListId) {
>+    var protoList = document.getElementById(aListId);
>     var id = protoList.selectedItem.value;
>     this.proto = Services.core.getProtocolById(id);

Shouldn't this method save the information of which list was used to select the protocol, so that an event handler in the onpagerewound of the username page can make the "back" button work correctly?

By the way, is the parameter really needed? Isn't there an way to get the currently displayed page?

>+  advanceTopProtocolPage: function(){

Coding style: function() {
(no space before "(", a space between ")" and "{").

>+    let topProtoList = document.getElementById("topprotolist");
>+    let wizard = document.getElementById("accountWizard");
>+
>+    if (topProtoList.selectedItem != null &&
>+       (topProtoList.selectedIndex != (topProtoList.itemCount - 1))) {

This seems complicated. What about testing topProtoList.selectedItem.id == "otherListItem"?
(If you used topProtoList.selectedItem twice, put it in a temporary variable.)

>+  showProtocolPage: function(){
>+    let protoList = document.getElementById("protolist");
>+
>+    if (protoList.itemCount > 0)
>+      return;
>+

It seems the "accountWizard.setGetMoreProtocols();" line of onload should be moved here.

>+    let protos = [];

>diff --git a/instantbird/content/accountWizard.xml b/instantbird/content/accountWizard.xml

>+<?xml version="1.0"?>
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1

>+   -
>+   - The Original Code is the this file.

This line of the license header seems wrong. You probably meant "Instantbird" instead of "the this file".

These days new files tend to have MPL2 license headers (http://www.mozilla.org/MPL/headers/). Copying and pasting them is less error prone as nothing needs to be edited in the process. (I don't mind if you want to keep the MPL 1.1/GPL 2.0/LGPL 2.1 header.)

>+  <binding id="topProtocol" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content>
>+      <xul:hbox align="baseline">

Is this hbox really needed? Have you tried putting the align attribute (and maybe an orient attribute) directly on the <content> tag? Attributes placed on <content> go to the DOM node that has the binding attached to it.

>+    <implementation>
>+     <method name="build">
>+      <parameter name="aProtocol"/>
>+      <parameter name="protoDescription"/>
>+      <body>
>+      <![CDATA[
>+        this.setAttribute("name", aProtocol.name);
>+        this.setAttribute("description", protoDescription);
>+        this.setAttribute("prplicon", aProtocol.iconBaseURI + "icon32.png");

I think the build method should also set the value property (your patch currently does it in onload).

>+  <binding id="otherProtocol" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

I don't think this binding is useful. It could be inlined in the xul file.

>+    <content>
>+      <xul:hbox align="baseline">
>+        <xul:spacer width="40" height="0"/>

Spacer are usually useful to add a flexible space. For a static space, you can just add a margin to the next element.

>+        <xul:description class="top-proto-name" value="&accountProtocolShowMore.label;"/>
>+      </xul:hbox>

I think I preferred when the last item had the same height as the other items of the list. (Others seemed to agree on IRC).

>diff --git a/instantbird/content/accountWizard.xul b/instantbird/content/accountWizard.xul

>+  <wizardpage id="accounttoplist" pageid="accounttoplist" next="accountprotocol"
>+              label="&accountProtocolTitle.label;"
>+              onpageadvanced="return accountWizard.advanceTopProtocolPage();">
>+    <description>&accountTopProtocolInfo.label;</description>
>+    <separator/>
>+    <label value="&accountProtocolField.label;" control="protolist"
>+           id="protoLabel" hidden="true"/>
>+    <richlistbox flex="1" id="topprotolist"
>+             ondblclick="document.getElementById('accountWizard').advance();">

Nit: The indentation doesn't seem good here.

I will comment on the strings used when the code will look like it's (almost) ready. String changes are trivial anyway :-).


>diff --git a/instantbird/themes/accountWizard.css b/instantbird/themes/accountWizard.css
>--- a/instantbird/themes/accountWizard.css
>+++ b/instantbird/themes/accountWizard.css
>@@ -39,16 +39,37 @@
> #accountsummary {
>   overflow: visible;
> }
> 
> #summarygrid {
>   overflow: auto;
> }
> 
>+#topprotolist richlistitem {

The descendant selector is expensive. Using a custom name of the elements would solve this.
If you don't know what I mean by "expensive" when talking about some CSS code, https://developer.mozilla.org/en/Writing_Efficient_CSS is a very informative recommended reading.

The -moz-binding CSS rules (and more generally any rule that defines what content be displayed (example: "display: none"), as opposed to how it will look) usually go in CSS files in instantbird/content/.

>+.top-proto-description {
>+  font-size: 90%;
>+  color: #3D3D3D;

This rule causes the color of the description to be ugly for selected items (the default color changes when the item is selected).
Maybe you could use opacity instead?
*** Original post on bio 1391 at 2012-05-06 21:52:08 UTC ***

Thank you for the detailed feedback! :) Most of those points are straightforward, however I have a couple of questions.

>You can use a custom name instead of "richlistitem" for your element.
>It helps for the CSS setting the -moz-binding value :).

Do you mean something something simple like this?

   let item = document.createElement("toplistitem");

I seem to lose the skinning from richlistbox.css, as selecting no longer is visibly noticeable, etc., when making that change. Since the only richlistitems present in this form are the ones being targeted (besides otherListItem), #topprotolist can be removed and the same effect is achieved.

>Shouldn't this method save the information of which list was used to select the
>protocol, so that an event handler in the onpagerewound of the username page
>can make the "back" button work correctly?

I have this working with setUserData() creating a "previous" attribute, and the event handler directing the back button from getUserData(). Is this an acceptable solution, or is there a better way?
*** Original post on bio 1391 at 2012-05-06 22:26:33 UTC ***

(In reply to comment #16)
> Thank you for the detailed feedback! :)

You are welcome. I'm pleased to see this is making good progress!

> >You can use a custom name instead of "richlistitem" for your element.
> >It helps for the CSS setting the -moz-binding value :).
> 
> Do you mean something something simple like this?
> 
>    let item = document.createElement("toplistitem");

Yes. Although I would maybe have used something a little more descriptive like "protocol" or "topprotocol" or something similar.

> I seem to lose the skinning from richlistbox.css, as selecting no longer is
> visibly noticeable, etc., when making that change.

Ah, I didn't think of that :-/. So I think you will want to keep "richlistitem" as the tag name; maybe you can add a class? (element.className = some name)

> Since the only richlistitems
> present in this form are the ones being targeted (besides otherListItem),
> #topprotolist can be removed and the same effect is achieved.

But you would still need a specific rule with !important for the otherListItem then?
 
> >Shouldn't this method save the information of which list was used to select the
> >protocol, so that an event handler in the onpagerewound of the username page
> >can make the "back" button work correctly?
> 
> I have this working with setUserData() creating a "previous" attribute, and the
> event handler directing the back button from getUserData(). Is this an
> acceptable solution, or is there a better way?

I thought you would just set a JS property on the DOM node (elt.foo = something to set it, and just elt.foo to get it later). I don't see what the set/getUserData method do in addition to that (honestly, I didn't even know these methods before looking them up in Google to understand your comment :-)).
Attached patch Corrections on previous patch (obsolete) — Splinter Review
*** Original post on bio 1391 as attmnt 1453 at 2012-05-07 22:30:00 UTC ***

I have made the changes from the last feedback in this patch. 

-The function of the back/next buttons was fixed, though it took another helper function to keep the wizard page stack from causing issues.

-A class was used instead of the CSS descendant selector, which was specified in a new CSS file in instantbird/content/.

-The height of the "Show all" entry was increased as before, and opacity was used on the descriptions.
Attachment #8353206 - Flags: review?(florian)
Comment on attachment 8353201 [details] [diff] [review]
Fixed issues with the first patch

*** Original change on bio 1391 attmnt 1448 at 2012-05-07 22:30:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353201 - Attachment is obsolete: true
Attachment #8353201 - Flags: review?(clokep)
Comment on attachment 8353206 [details] [diff] [review]
Corrections on previous patch

*** Original change on bio 1391 attmnt 1453 at 2012-05-07 23:16:56 UTC ***

Thanks for the updated patch. This looks good overall, but I've still found a few nits:

>diff --git a/instantbird/content/accountWizard.js b/instantbird/content/accountWizard.js

>   onload: function aw_onload() {
>-    accountWizard.setGetMoreProtocols();
>+    let topProtoList = document.getElementById("topprotolist");
>+    let topProtocolStrings = document.getElementById("topProtocolsBundle");

It seems respecting the 80 columns limit would be easier if you named that variable "bundle".

>+    if (topProtoList.itemCount < 2)
>+      document.getElementById("accountWizard")
>+              .getPageById("accountwelcome").next = "accountprotocol";

Coding style: even if there's a single instruction after an if statement, if it's on several lines, we require the { } around it.

>+    topProtoList.selectedIndex = -1;

I assume you have verified that this is different from the default behavior :).


>   selectProtocol: function aw_selectProtocol() {
>-    var protoList = document.getElementById("protolist");
>+    var currentPage = document.getElementById("accountWizard").currentPage;

It seems what you wanted to have in a variable was pageId rather than currentPage.

>+    var protoList;
>+    if (currentPage.pageid == "accounttoplist")
>+      protoList = document.getElementById("topprotolist");
>+    else
>+      protoList = document.getElementById("protolist");

This feels like duplicated code.
What about using a variable like this:
    var listId = pageId == "accounttoplist" ? "topprotolist" : "protolist";


>+  advanceTopProtocolPage: function() {
>+    let wizard = document.getElementById("accountWizard");

wizard is used only once. Inline it? (same comment for the next method)

>+    let selectedProtocol = document.getElementById("topprotolist").selectedItem;
>+
>+    if (selectedProtocol != null && selectedProtocol.id != "otherListItem") {

This is equivalent to:
    if (selectedProtocol && selectedProtocol.id != "otherListItem") {


>+      accountWizard.selectProtocol();
>+      wizard.goTo("accountusername");
>+      return false;
>+    }
>+    return true;
>+  },
>+  
>+  rewindFromUsernamePage: function () {

Coding style: no space between "function" and "()".

>+
>+  showProtocolPage: function() {
>+    accountWizard.setGetMoreProtocols();

I thought in my previous comment I suggested putting this after the check for whether the list is already initialized. Is there a reason you want to call this each time the full protocol list is going to be displayed?


>diff --git a/instantbird/content/accountWizard.xml b/instantbird/content/accountWizard.xml

>+     <method name="build">
>+      <parameter name="aProtocol"/>
>+      <parameter name="aDescription"/>
>+      <body>
>+      <![CDATA[
>+        this.setAttribute("name", aProtocol.name);
>+        this.setAttribute("description", aDescription);
>+        this.setAttribute("prplicon", aProtocol.iconBaseURI + "icon32.png");
>+        this.setAttribute("value", aProtocol.id);

Any reason for not just doing this.value = aProtocol.id; ?

>diff --git a/instantbird/content/accountWizard.xul b/instantbird/content/accountWizard.xul

> <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> <?xml-stylesheet href="chrome://instantbird/skin/accountWizard.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://instantbird/content/accountWizard.css" type="text/css"?>

I wouldn't say this is following a fixed rule, but I would have included the content css file before the skin one.

I just looked at the other xul files we have, and it seems what we have done is:
- include first the css files where the name of the file was omitted (chrome://<package>/skin/ is a shortcut for chrome://<package>/skin/<package>.css)
- then include all content css files
- and finally all the skin css files.

I still haven't reviewed the localizable strings.
Attachment #8353206 - Flags: review?(florian) → review-
Comment on attachment 8353175 [details] [diff] [review]
Working example of Florian's mockup

*** Original change on bio 1391 attmnt 1423 at 2012-05-08 00:40:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353175 - Attachment is obsolete: true
Comment on attachment 8353180 [details] [diff] [review]
Protocol toplist with XBL bindings

*** Original change on bio 1391 attmnt 1428 at 2012-05-08 00:40:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353180 - Attachment is obsolete: true
*** Original post on bio 1391 at 2012-05-08 01:53:10 UTC ***

Comment on attachment 8353206 [details] [diff] [review] (bio-attmnt 1453)
Corrections on previous patch

I think flo covered all the code changes I would also make, so I'll take a look at the strings.

>diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd b/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd
>@@ -1,17 +1,21 @@
>+<!ENTITY accountTopProtocolInfo.label    "Here are some of the popular protocols from your region.">
See below about "protocols", I think "for your region" might make more sense than "from your region".

> <!ENTITY accountProtocolTitle.label   "Protocol">
> <!ENTITY accountProtocolInfo.label    "Please choose the protocol of your IM account in the following list.">
> <!ENTITY accountProtocolField.label   "Protocol:">
> <!ENTITY accountProtocolGetMore.label "Get more...">
>+<!ENTITY accountProtocolShowMore.label  "Show all available protocols">
>+<!ENTITY accountProtocolShowMore.description  "List all of the currently installed account protocols">
I think we prefer to use "networks" (instead of "protocols") when displaying things in the UI, but it seems all these strings refer to "protocols". Probably makes sense to leave it and clean it all up at once then.

>diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
>@@ -0,0 +1,16 @@
>+# LOCALIZATION NOTE
>+# The selected protocols below should be the most relevant
>+# to the target language / location and comma delimited. Avoid
>+# exceeding 4 protocols to prevent scrolling. A list of the
>+# available protocols can be found at
>+#     https://wiki.instantbird.org/Protocol_Identifiers
Glad you made a wiki page for this! :) Looks good!

>+topProtocol.list=prpl-gtalk,prpl-facebook,prpl-twitter,prpl-msn
Having no scientific evidence whatsoever...I think we should include prpl-aim and prpl-yahoo for en-US. I know this would create scrolling though... It'd be pretty excellent to know which ones are actually popular. :( (For evidence though, I use almost exclusively AIM and GTalk. I don't talk to anyone on Yahoo or MSN, but I'm assuming you think MSN is popular for some reason...I know it tends to be more popular on the west coast).  I don't want to bikeshed too much on this until we have actual evidence though (and this is certainly easy enough to change at another point :)).

Is there a specific order these or in or just what you felt is "most popular" first?

>+# LOCALIZATION NOTE
>+# These are the protocol descriptions of the top protocols
>+# specified above. A description should be provided for
>+# each protocol ID listed above.
This makes sense to me, but I'd like it if one of our localizers could read over this comment and ensure it makes sense (or at least flo, I have a distinct lack of l10n skills...knowing only English).

>+topProtocol.prpl-gtalk.description=Talk to friends through GMail or any XMPP server
Maybe "Talk to your friends on Google Talk", there's a separate XMPP protocol that is used for arbitrary XMPP accounts. I think GTalk is federated, but I think this implies the wrong thing.

>+topProtocol.prpl-facebook.description=Communicate with Facebook Chat friends
I think "Facebook friends" without the "Chat" would make more sense (they're people you added through the Facebook website, afterall, and "Facebook friends" is fairly standard terminology).

>+topProtocol.prpl-twitter.description=Keep up to date through Twitter feeds
"Stay up to date with your Twitter timeline!"?

>+topProtocol.prpl-msn.description=Microsoft Windows Live Messenger (formerly MSN)
Straight and to the point. :)
*** Original post on bio 1391 at 2012-05-08 13:19:05 UTC ***

I've updated the bug summary to reflect the actual work.
Summary: Simplify account creation wizard → Suggest popular protocols during account creation
*** Original post on bio 1391 as attmnt 1455 at 2012-05-08 19:34:00 UTC ***

I've fixed up the nits from the last review; there's definitely a lot to consider when merging new code with an established set of coding guidelines :).

> >+    topProtoList.selectedIndex = -1;
>I assume you have verified that this is different from the default behavior :).

The default selection without this code is the "Show more protocols" entry, which looks a bit odd. I haven't had any issues without using setTimeout(), although I don't know the background behind that previous bug and whether it should be left in.

>Coding style: no space between "function" and "()".

I've been tripped up by the surrounding code a couple of times now :) 
Is it beyond the scope of this bug to bother changing these?
>  openURL: function (aURL) { ... }
>  setGetMoreProtocols: function (){ ... }

I've been looking at the expression or two like this...
> if (previousPage != "accountprotocol") {
>   wizard.goTo(previousPage);
>   return false;
> }
> return true;

Would it be considered readable to use some form of short-circuit evaluation like this (may not be 100% correct):
return !(previousPage != "accountprotocol" && wizard.goTo(previousPage));


I've also added in most of the localization suggestions; leaving the term "protocol" for now.

>Is there a specific order these or in or just what you felt is "most popular"
first?

The protocols I've had in the patch were from the mockups posted earlier. I definitely think AIM should be found in the list (It's in this patch now). Yahoo IM is not as popular from where I am, though it should probably be added as well depending on how many entries will be included.
Attachment #8353208 - Flags: review?(florian)
Comment on attachment 8353206 [details] [diff] [review]
Corrections on previous patch

*** Original change on bio 1391 attmnt 1453 at 2012-05-08 19:34:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353206 - Attachment is obsolete: true
*** Original post on bio 1391 at 2012-05-08 21:56:06 UTC ***

(In reply to comment #22)
> Created attachment 8353208 [details] [diff] [review] (bio-attmnt 1455) [details]
> >Coding style: no space between "function" and "()".
> 
> I've been tripped up by the surrounding code a couple of times now :) 
> Is it beyond the scope of this bug to bother changing these?
> >  openURL: function (aURL) { ... }
> >  setGetMoreProtocols: function (){ ... }
It's outside the scope of the bug, but depending on who your reviewer is, they might let it slide. ;) Usually it's preferred to do "clean up" type changes separately since they're meant to have no change on the functionality of the code. If you include lots of whitespace changes, etc. mixed with functional changes it becomes difficult to find regressions.

> I've been looking at the expression or two like this...
> > if (previousPage != "accountprotocol") {
> >   wizard.goTo(previousPage);
> >   return false;
> > }
> > return true;
> 
> Would it be considered readable to use some form of short-circuit evaluation
> like this (may not be 100% correct):
> return !(previousPage != "accountprotocol" && wizard.goTo(previousPage));
I don't find this more readable, but you could do (if you think it's more readable...):
if (previousPage == "accountprotocol")
  return true;
wizard.goTo(previousPage)
return false;

> I've also added in most of the localization suggestions; leaving the term
> "protocol" for now.
Yeah, makes sense. :) I'll file a bug at some point to switch it throughout the UI.

Note that I've only skimmed your patch, but I didn't notice anything out of place.
Comment on attachment 8353208 [details] [diff] [review]
Continued work on code fixes and string localizations

*** Original change on bio 1391 attmnt 1455 at 2012-05-11 13:05:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353208 - Flags: review?(florian) → review-
*** Original post on bio 1391 at 2012-05-11 13:05:50 UTC ***

Comment on attachment 8353208 [details] [diff] [review] (bio-attmnt 1455)
Continued work on code fixes and string localizations

>diff --git a/instantbird/content/accountWizard.xul b/instantbird/content/accountWizard.xul

>+  <wizardpage id="accounttoplist" pageid="accounttoplist" next="accountprotocol"
>+              label="&accountProtocolTitle.label;"
>+              onpageadvanced="return accountWizard.advanceTopProtocolPage();">
>+    <description>&accountTopProtocolInfo.label;</description>
>+    <separator/>

I think we could add class="thin" on this separator and the one on the full protocol list.

>+    <label value="&accountProtocolField.label;" control="protolist"
>+           id="protoLabel" hidden="true"/>

- The value of the id attribute needs to be unique across the document.
- The control attribute of a label indicates which element should be focused if the label is clicked. I'm almost sure "protolist" isn't the value you wanted here.
- This label is hidden and I don't see any way it could become visible (both in the page you added and the page with the full list of protocols), so I would suggest just removing it in both places (don't forget to also remove the string in the .dtd file).


>diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd b/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd

>+<!ENTITY accountTopProtocolInfo.label    "Here are some of the popular protocols for your region.">

I think clokep (can you confirm/deny? :-)) wanted us to drop "for your region" from this string, with the rationale that users don't care that the list has been customized for them and don't need to know. I don't feel strongly either way.

> <!ENTITY accountProtocolTitle.label   "Protocol">
> <!ENTITY accountProtocolInfo.label    "Please choose the protocol of your IM account in the following list.">
> <!ENTITY accountProtocolField.label   "Protocol:">

If you follow my suggestion of removing the hidden label, you can remove that string.

> <!ENTITY accountProtocolGetMore.label "Get more...">
>+<!ENTITY accountProtocolShowMore.label  "Show all available protocols">
>+<!ENTITY accountProtocolShowMore.description  "List all of the currently installed account protocols">

I would suggest:
"Show all protocols"
"Choose from the full list of protocols"

Rationale:
- We discussed on IRC what of "available", "installed" and "supported" is the least confusing without really agreeing. (A protocol supported by an add-on that isn't installed is technically still "available" (as an add-on), and the protocols in the list haven't necessarily been installed by the user... most of the time they are built into the application by default.) So for clarity it may be better to avoid using these words here.
- Your other descriptions start with a verb of an action the user can do (Chat, Talk, Stay up to date), so I think it would be an improvement to replace "List" (and action for the application) by "Choose" an action for the user).

>diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
>@@ -0,0 +1,16 @@
>+# LOCALIZATION NOTE
>+# The selected protocols below should be the most relevant
>+# to the target language / location and comma delimited.
>+# Exceeding 4 protocols may cause scrolling. A list of the
>+# available protocols can be found at
>+#     https://wiki.instantbird.org/Protocol_Identifiers
>+topProtocol.list=prpl-gtalk,prpl-facebook,prpl-twitter,prpl-aim,prpl-msn

Should yahoo be offered in that list for en-US? I don't know how popular it still is (I've never used it myself, but I'm not in the US).

>+
>+# LOCALIZATION NOTE
>+# These are the descriptions of the top protocols specified above.
>+# A description should be provided for each protocol ID listed above.
>+topProtocol.prpl-gtalk.description=Talk to your friends on Google Talk

Maybe "Talk to your Gmail contacts"? (I'm trying to avoid repeating the protocol title, and I think gmail refers to people as "contacts" rather than "friends")

>+topProtocol.prpl-facebook.description=Communicate with Facebook friends
>+topProtocol.prpl-twitter.description=Stay up to date with your Twitter timeline
>+topProtocol.prpl-aim.description=Chat with your buddies on AOL Instant Messenger
>+topProtocol.prpl-msn.description=Microsoft Windows Live Messenger (formerly MSN)

These 4 descriptions are fine with me :-).


>diff --git a/instantbird/themes/accountWizard.css b/instantbird/themes/accountWizard.css

>+#otherListItem {
>+  padding-left: 40px;

To have this last item have the same size as the others, maybe add here:
  min-height: 40px;
and to center vertically:
  -moz-box-pack: center;

>+}
>+
>+.top-proto-name {
>+  font-size: 110%;
>+}
>+
>+.top-proto-description {
>+  font-size: 90%;

This results in font-size values that don't render very well on my screen (12.1px and 9.9px respectively).
I would suggest using "larger" and "smaller" instead.


Please try to avoid adding trailing whitespace. To spot them quickly, you can add this line to the [extensions] section of your .hg/hgrc file:
color =

Trailing whitespace will then appear in red in the output of hg diff in your terminal.


This looks very good overall, and I'm very likely to not have any additional comment at the next iteration :-).
*** Original post on bio 1391 at 2012-05-11 13:25:47 UTC ***

(In reply to comment #24)
> Comment on attachment 8353208 [details] [diff] [review] (bio-attmnt 1455) [details]
> Continued work on code fixes and string localizations
> >diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd b/instantbird/locales/en-US/chrome/instantbird/accountWizard.dtd
> 
> >+<!ENTITY accountTopProtocolInfo.label    "Here are some of the popular protocols for your region.">
> 
> I think clokep (can you confirm/deny? :-)) wanted us to drop "for your region"
> from this string, with the rationale that users don't care that the list has
> been customized for them and don't need to know. I don't feel strongly either
> way.
I did, but if you really want to keep it I'm fine with that.


> >diff --git a/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
> >new file mode 100644
> >--- /dev/null
> >+++ b/instantbird/locales/en-US/chrome/instantbird/accountWizard.properties
> >@@ -0,0 +1,16 @@
> >+# LOCALIZATION NOTE
> >+# The selected protocols below should be the most relevant
> >+# to the target language / location and comma delimited.
> >+# Exceeding 4 protocols may cause scrolling. A list of the
> >+# available protocols can be found at
> >+#     https://wiki.instantbird.org/Protocol_Identifiers
> >+topProtocol.list=prpl-gtalk,prpl-facebook,prpl-twitter,prpl-aim,prpl-msn
> 
> Should yahoo be offered in that list for en-US? I don't know how popular it
> still is (I've never used it myself, but I'm not in the US).
I think we can not include it for now, it's already a long list. We should attempt to get some user statistics on this at some point (somebody file a bug?).

> >+
> >+# LOCALIZATION NOTE
> >+# These are the descriptions of the top protocols specified above.
> >+# A description should be provided for each protocol ID listed above.
> >+topProtocol.prpl-gtalk.description=Talk to your friends on Google Talk
> 
> Maybe "Talk to your Gmail contacts"? (I'm trying to avoid repeating the
> protocol title, and I think gmail refers to people as "contacts" rather than
> "friends")
How about "Google contacts" (since that includes things like Google+ too). I also dislike mentioning "Gmail" specifically.
*** Original post on bio 1391 at 2012-05-11 15:37:21 UTC ***

We discussed this a bit more on IRC, please include yahoo (we can always remove it later) and use "Gmail contacts" (as apparently you can't IM without a Gmail account, it's not just a Google account).

Feel free to ask on IRC about a description for Yahoo (so hopefully the next patch will be r+!)
*** Original post on bio 1391 as attmnt 1470 at 2012-05-15 00:21:00 UTC ***

I have made the changes suggested on IRC and in the previous reviews. The alignment of the top protocol entries was also changed to center from baseline, which seems to have the more desired effect. Yahoo was added as an en-US top protocol along with a description.
Attachment #8353223 - Flags: review?(florian)
Comment on attachment 8353208 [details] [diff] [review]
Continued work on code fixes and string localizations

*** Original change on bio 1391 attmnt 1455 at 2012-05-15 00:21:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353208 - Attachment is obsolete: true
Comment on attachment 8353223 [details] [diff] [review]
Additional string changes and top protocol tweaks

*** Original change on bio 1391 attmnt 1470 at 2012-05-16 16:26:13 UTC ***


>-<!ENTITY accountProtocolInfo.label    "Please choose the protocol of your IM account in the following list.">
>-<!ENTITY accountProtocolField.label   "Protocol:">
>+<!ENTITY accountProtocolInfo.label    "Please choose the protocol of your IM account.">

When changing the content of a localizable string, we need to change its id too to ensure localizers will notice the change when updating their translations.

I'm adding a 2 at the end of the accountProtocolInfo.label id before commiting the patch.

This looks great, thanks, and congratulation for your first patch!
Attachment #8353223 - Flags: review?(florian) → review+
*** Original post on bio 1391 at 2012-05-16 16:26:51 UTC ***

Patch pushed as https://hg.instantbird.org/instantbird/rev/30564a208a4e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Depends on: 954870
Depends on: 954872
*** Original post on bio 1391 at 2012-05-21 01:06:25 UTC ***

Some links for localizers (salvaged from the duped bug, and looking for an update):

Global IM market share 2008 by country
http://billionsconnected.com/blog/wp-content/uploads/2008/08/global_im_market_share_stats_july_08.pdf

Worldwide IM market share 2011 (by client)
http://www.opswat.com/sites/default/files/OPSWAT-Market-Share-Report-June-2011.pdf
*** Original post on bio 1391 at 2012-05-21 15:53:30 UTC ***

I added these links and a couple others to the wiki page. If we find more we should put them there.
Depends on: 954953
You need to log in before you can comment on or make changes to this bug.