Closed Bug 1057873 Opened 10 years ago Closed 7 years ago

Add UI to import accounts from other clients

Categories

(Instantbird Graveyard :: Account wizard, enhancement)

Product:
Instantbird Graveyard
Component:
Account wizard
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: wnayes, Assigned: wnayes)

References

Details

Attachments

(1 file)

Current state of the import wizard
56.19 KB, patch
Details | Diff | Splinter Review 
Bug 955133 implements backend functionality in chat/ that can be used to import existing accounts from other chat clients. This bug is for tracking progress on providing UI to access these importing features.

The initially attached patch is mostly the import wizard code from my GSoC project, which is functional but may need some significant design work.
Attachment #8478011 - Flags: review?(florian)
Blocks: 954928
Blocks: 1057874
Comment on attachment 8478011 [details] [diff] [review]
Current state of the import wizard

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

I haven't reviewed the accountWizard.js file yet.

::: im/content/accountWizard.xml
@@ +44,5 @@
> +
> +  <binding id="existingAccount" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> +    <content>
> +      <xul:vbox flex="1">
> +        <xul:hbox flex="1" align="center">

Do you really need this vbox and this hbox? They both seem to have a single child, so I wonder if you couldn't replace them with adding some attributes (maybe align and/or orient).

@@ +50,5 @@
> +            <xul:checkbox anonid="accountSelected"
> +                          xbl:inherits="checked" class="summaryCheckbox"
> +                          oncommand="document.getBindingParent(this).updateChecked();"/>
> +          </xul:vbox>
> +          <xul:vbox>

These 2 vbox also have a single child. They may also be removable. Maybe with the help of a 'pack' attribute in addition to the 'align' one.

@@ +65,5 @@
> +            <xul:hbox align="center" class="textGroup">
> +              <xul:label class="showChecked" xbl:inherits="checked"
> +                         value="&accountExisting.doImport.label;"/>
> +              <xul:label class="hideChecked" xbl:inherits="checked"
> +                         value="&accountExisting.noImport.label;"/>

I have doubts on the accessibility of this. I wouldn't be surprised if both labels were read by screenreaders. See http://hg.mozilla.org/comm-central/rev/e08a567d8b02 for what we did in a similar case before.

@@ +78,5 @@
> +    </content>
> +    <implementation>
> +      <property name="autologin"
> +                onget="return document.getAnonymousElementByAttribute(this,
> +                                                  'anonid',

Strange indent here.

@@ +99,5 @@
> +          let protocol = Services.core.getProtocolById(aExistingAccount.protocolId);
> +          if (protocol)
> +            this.setAttribute("prplicon", protocol.iconBaseURI + "icon32.png");
> +          else
> +            Cu.reportError("Attempted to build existingAccount binding with null protocol!");

Is this a case we really expect to happen?

@@ +108,5 @@
> +        <parameter name="aExistingAccount"/>
> +        <body>
> +        <![CDATA[
> +          let account = this.account[this.getSelectedImporterId()];
> +          return (account.name == aExistingAccount.name &&

Is this good enough as a way to compare account names? Related concerns in bug 954943.

@@ +131,5 @@
> +
> +          // Add a menulist entry (regardless if it is shown yet)
> +          let importers = document.getAnonymousElementByAttribute(this, "anonid",
> +                                                                  "availableImporters");
> +          let newitem = importers.appendItem(importer.name);

nit: newItem

@@ +140,5 @@
> +            this.account = {};
> +          this.account[importerId] = aExistingAccount;
> +
> +          if (importers.itemCount > 1) {
> +            label.className += " hiddenSource";

label.classList.add("hiddenSource");

@@ +174,5 @@
> +  <binding id="newAccount" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> +    <content align="top">
> +      <xul:vbox flex="1">
> +        <xul:hbox flex="1" align="center">
> +          <xul:vbox>

You probably have more boxes than strictly needed here too.

@@ +180,5 @@
> +                          xbl:inherits="checked" class="summaryCheckbox"
> +                          oncommand="document.getBindingParent(this).updateChecked();"/>
> +          </xul:vbox>
> +          <xul:vbox>
> +            <xul:image xbl:inherits="src=prplicon,checked" class="summaryIcon"/>

nit: prplIcon

@@ +187,5 @@
> +            <xul:hbox align="baseline">
> +              <xul:label xbl:inherits="value=username,checked" class="summaryName"/>
> +              <xul:separator flex="1"/>
> +              <xul:checkbox label="&accountSummary.connectNow.label;"
> +                            dir="reverse" class="summaryAutologinCheckbox"

have we spelled this Autologin or AutoLogin in other places?

@@ +194,5 @@
> +            <xul:hbox align="center" class="textGroup">
> +              <xul:label class="showChecked" xbl:inherits="checked"
> +                         value="&accountNew.doCreate.label;"/>
> +              <xul:label class="hideChecked" xbl:inherits="checked"
> +                         value="&accountNew.noCreate.label;"/>

There may be an accessibility issue here too.

@@ +203,5 @@
> +    </content>
> +    <implementation>
> +      <property name="checked"
> +                onget="return document.getAnonymousElementByAttribute(this,
> +                                                  'anonid',

Strange indent here.

@@ +216,5 @@
> +                onget="return this.getAttribute('alias');"
> +                onset="return this.setAttribute('alias', val);"/>
> +      <property name="autologin"
> +                onget="return document.getAnonymousElementByAttribute(this,
> +                                                  'anonid',

Strange indent here.

@@ +223,5 @@
> +        <parameter name="aAccountObj"/>
> +        <body>
> +        <![CDATA[
> +          let protoIcon = aAccountObj.proto.iconBaseURI + "icon32.png";
> +          this.setAttribute("prplicon", protoIcon);

prplIcon

@@ +248,5 @@
> +        </body>
> +      </method>
> +    </implementation>
> +  </binding>
> +

nit: we don't need the empty line here.

::: im/locales/en-US/chrome/instantbird/accountWizard.properties
@@ +24,5 @@
> +skipButton.accessKey=S
> +addButton.label=Add another account
> +addButton.accessKey=A
> +
> +searchStatus.searching.label=Looking for existing accounts...

Use the unicode ellipsis character: …

@@ +25,5 @@
> +addButton.label=Add another account
> +addButton.accessKey=A
> +
> +searchStatus.searching.label=Looking for existing accounts...
> +searchStatus.success.label=Instantbird was able to find %S existing accounts on your computer. To view what was found, press Next.

This wants to be a localized plural: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#Developing_with_PluralForm

@@ +28,5 @@
> +searchStatus.searching.label=Looking for existing accounts...
> +searchStatus.success.label=Instantbird was able to find %S existing accounts on your computer. To view what was found, press Next.
> +searchStatus.failure.label=Instantbird could not find any existing accounts to import. To configure a new account, press Next.
> +
> +summaryItem.clienttext.label=from %S

Please add a localization note indicating how this string will be used, and what %S will be replaced with.

::: im/themes/accountWizard.css
@@ +37,5 @@
> +
> +.accountSearchFailure {
> +  list-style-image: url("chrome://global/skin/icons/information-16.png");
> +}
> +

We will probably want to have an ifdef XP_MACOSX section here with CSS rules using @2x.png files for retina screens.
Attachment #8478011 - Flags: review?(florian)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: