Closed Bug 955452 Opened 10 years ago Closed 10 years ago

Display buddy list in a tab.

Categories

(Instantbird Graveyard :: Conversation, enhancement)

x86
Other
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

Attachments

(1 file, 17 obsolete files)

50.76 KB, patch
florian
: review+
aleth
: review+
aleth
: review+
Details | Diff | Splinter Review
*** Original post on bio 2015 at 2013-06-25 17:45:00 UTC ***

Now that arbitrary tabs can be added to the conversation window, it would be useful to have a tab that displays the buddy list and lets you start conversations from it.
Depends on: 955449
Assignee: nobody → nhnt11
Attached patch A bit rough, but works. (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2517 at 2013-06-25 18:23:00 UTC ***

This patch has a few issues which I will address in the next, so not requesting review. Some of them are:
- Hangs when accounts go offline/online and all the associated contacts change status.
- finishImport is slower than I'd like.
- Doesn't use Mic's icon. Also need to include the tag icon as discussed on IRC.
- I've named everything "awesometab" but this may not be desirable for this bug.
- The command to open a new blist tab is in menus.xul. It works globally in Mac but I do not know about other OSes and need to investigate.
- A lot of other things that I need to clean up.

I've been working on some improvements to this patch that I haven't included due to incompleteness. Next patch should be much better :)
*** Original post on bio 2015 at 2013-06-25 23:40:01 UTC ***

(In reply to comment #1)

> - I've named everything "awesometab" but this may not be desirable for this
> bug.

"newtab" seems a more meaninful (and shorted!) name for most cases I've seen while looking very quickly through the patch.

> - The command to open a new blist tab is in menus.xul. It works globally in Mac
> but I do not know about other OSes and need to investigate.

On non-Mac, menus.xul is included only in the blist window.
*** Original post on bio 2015 at 2013-06-26 09:08:56 UTC ***

The big problem with this WIP is that (as discussed on IRC) the approach of storing everything in the DOM is wrong (unless you don't intend that tab to develop into the awesometab later). That's the bit you should fix first (getting the underlying data structure more or less right, so you can add other data types than contacts later), scrolling etc is a detail one can add later. It doesn't really make sense to track down bugs like "hangs when contacts go offline" until that is done imho.
Depends on: 955458
Attached patch Version 2 (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2537 at 2013-06-28 23:32:00 UTC ***

Here's a new version taking into consideration all the feedback and suggestions I got.
- Much better handling of DOM elements.
- "Infinite scrolling"
- Comments and code cleanup
- Proper handling of observer notifications
- Use Mic's icon on the tab, and Firefox's tag icon in contact elements
- Bug fixes
Attachment #8354305 - Flags: review?(florian)
Attachment #8354305 - Flags: review?(benediktp)
Attachment #8354305 - Flags: review?(aleth)
Comment on attachment 8354285 [details] [diff] [review]
A bit rough, but works.

*** Original change on bio 2015 attmnt 2517 at 2013-06-28 23:32:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354285 - Attachment is obsolete: true
*** Original post on bio 2015 at 2013-07-01 10:15:42 UTC ***

Comment on attachment 8354305 [details] [diff] [review] (bio-attmnt 2537)
Version 2

Some comments already before the new patch arrives ;)

> diff --git a/instantbird/content/awesometab.css b/instantbird/content/awesometab.css
> +richlistitem {
> +  -moz-binding: url("chrome://instantbird/content/awesometab.xml#awesometab-contact");
> +}

Please use a less generic name for the contact items.

> diff --git a/instantbird/content/awesometab.xml b/instantbird/content/awesometab.xml
> +      <method name="onResize">
> +        <body>
> +        <![CDATA[
> +          this.onScroll();

Maybe put a comment here that says that "onScroll" ensures that there are enough items loaded already or rename onScroll to make it clear by its name itself.

> +      <method name="onScroll">
> +        <body>
> +        <![CDATA[
> +          if (this.allElementsDisplayed)
> +            return;
> +          let lastVisibleIndex =
> +            this.listbox.getIndexOfFirstVisibleRow() + this.listbox.getNumberOfVisibleRows() - 1;
> +          if (lastVisibleIndex >= (this.contactElts.length - 3)) {

Please put a comment here what the 3 is for.

> +      <!-- Remove observers when closing tab. -->
> +      <method name="destroy">
> +        <body>
> +        <![CDATA[
> +          // destroy is called from both tabbrowser and the destructor.
> +          // We need to ensure we don't remove our observer twice if both call it.
> +          if (this._destroyed)

Is that really necessary?

> +      <method name="finishImport">
> +        <parameter name="aOtherAwesometab"/>
> +        <body>
> +        <![CDATA[
> +          this.awesomebar.value = aOtherAwesometab.awesomebar.value;
> +          this.awesomebar.selectionStart = aOtherAwesometab.awesomebar.selectionStart;
> +          this.awesomebar.selectionEnd = aOtherAwesometab.awesomebar.selectionEnd;
> +          this.contacts = aOtherAwesometab.contacts;
> +          this.contactsById = aOtherAwesometab.contactsById;
> +          this.imContactsById = aOtherAwesometab.imContactsById;

> +          for (elt of aOtherAwesometab.contactElts)
> +            this.pushElt();

"for (let i = 0; i < aOtherAwesometab.contactElts; ++i) this.pushElt();" ?

> +      <method name="_delayedFilter">
> +        <body>
> +        <![CDATA[
> +          this._pendingFilterCall = false;
> +          let filterTxt = this.awesomebar.value.toLowerCase();
> +          let filteredContacts = this.contacts.filter(function(aContact)
> +            aContact.name.split(/\s+/).some(function (s) s.startsWith(filterTxt)));

> MIC: This should also look at the the full name of the contact. Typing "John Doe" into the search field (e.g. because I have several Johns in my list) doesn't match contact "John Doe" because the user input is only matched against "John" and "Doe" separately at the moment.

> diff --git a/instantbird/themes/awesometab.css b/instantbird/themes/awesometab.css
> +.awesometab-toolbar {
> +  margin: 0 0;

"margin: 0;" will do.

> +  padding: 2px 2px;

"padding: 2px;"

> +.awesomebar {
> +  padding: 2px 2px;

Same here.

> +.awesometab-listbox {
> +  margin: 0 0;

And here.

> +.statusIcon[status="unknown"] {
> +  list-style-image: url('chrome://chat/skin/unknown-16.png');
> +}
> +
> +.statusIcon[status="chat"] {
> +  list-style-image: url('chrome://chat/skin/chat-16.png');
> +}
> +
> +.statusIcon[status="available"] {
> +  list-style-image: url('chrome://chat/skin/available-16.png');
> +}
> +
> +.statusIcon[status="unavailable"],
> +.statusIcon[status="away"] {
> +  list-style-image: url('chrome://chat/skin/away-16.png');
> +}
> +
> +.statusIcon[status="offline"],
> +.statusIcon[status="invisible"] {
> +  list-style-image: url('chrome://chat/skin/offline-16.png');
> +}
> +
> +.statusIcon[status="idle"] {
> +  list-style-image: url('chrome://chat/skin/idle-16.png');
> +}

What about including "status.css" into the awesometab instead of creating very similar rules here? It's working with background images, though!

> diff --git a/instantbird/themes/tag-win.png b/instantbird/themes/tag-win.png

There's both an Aero and non-Aero version of the image on Windows. You'll need to add the other an add a rule in awesometab.css to differentiate.
Attached patch Version 3 (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2545 at 2013-07-01 20:14:00 UTC ***

Here's a new patch.
- UI tweaks, looks cleaner imo.
- Better handling of navigation keys.
- finishImport does its replication job more completely.
- Address Mic's comments, except the following:

> Please use a less generic name for the contact items.

I'm using richlistbox.css to style the listbox, and this requires me to make the elements richlistitems.

>> +          // destroy is called from both tabbrowser and the destructor.
>> +          // We need to ensure we don't remove our observer twice if both call it.
>> +          if (this._destroyed)

> Is that really necessary?

Yes, as discussed on IRC: http://log.bezut.info/instantbird/130701/#m142

> What about including "status.css" into the awesometab instead of creating very
> similar rules here? It's working with background images, though!

I tried this, but it uses *.png instead of *-16.png, which looks bad.
Attachment #8354313 - Flags: review?(florian)
Attachment #8354313 - Flags: review?(benediktp)
Comment on attachment 8354305 [details] [diff] [review]
Version 2

*** Original change on bio 2015 attmnt 2537 at 2013-07-01 20:14:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354305 - Attachment is obsolete: true
Attachment #8354305 - Flags: review?(florian)
Attachment #8354305 - Flags: review?(benediktp)
Attachment #8354305 - Flags: review?(aleth)
Attached patch Fix a mistake (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2547 at 2013-07-01 21:00:00 UTC ***

Sorry, a couple lines got omitted in the previous patch, this fixes it.
Attachment #8354315 - Flags: review?(florian)
Attachment #8354315 - Flags: review?(benediktp)
Comment on attachment 8354313 [details] [diff] [review]
Version 3

*** Original change on bio 2015 attmnt 2545 at 2013-07-01 21:00:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354313 - Attachment is obsolete: true
Attachment #8354313 - Flags: review?(florian)
Attachment #8354313 - Flags: review?(benediktp)
*** Original post on bio 2015 at 2013-07-03 13:33:25 UTC ***

This looks pretty good to me! I have mainly nits, some of which may already have been addressed in the process of moving things to a service. Possibly we should r?/land the with-service version of this patch instead if that is the case?

When using 'magic constants' such as 10 and 3, please use const kWhatever both for legibility and to ensure you don't miss any instances should you replace these later.

It would help with readability if you moved the constructor/destructor to the top of the implementation, as comments explaining the variables are to be found there.

// List of contacts by ID, used to check for dupes.
this.contactsById = {};
// List of imIContacts by ID.
this.imContactsById = {};

Mic has already suggested these should be maps (lookup is faster and the API is more legible), but I guess that's up to you.
How about a single list, each entry being an object with a contact and an imContact?

Why do you need imContactsById at all?

>  this.imContactsById[aId].removeObserver(this);
Seems to remove an observer you never add?

There are two places you set the data of a contact from an imContact: addContact and updateContact. Please deduplicate, possibly even by making updateContact simply remove and then add, if that doesn't seem much more expensive.

>// If nothing has changed, we're being called only to display more elements.
>// In this case, we don't need to do the filtering again.
>if (this._prevFilterTxt != filterTxt || this._contactUpdated) {

Probably better to split this function in two and only call one of them when only calling to display more elements? Then you probably don't need the this._contactUpdated data-is-dirty flag either.

>              return aContact.name.startsWith(filterTxt) ||
>                aContact.name.split(/\s+/).some(function(s) s.startsWith(filterTxt));

Maybe this can be left for a followup, but when the user types two words followed by a space, we should interpret this as word1 AND word2, not as "word1 word2".

+          let items = this.listbox.selectedItems;
+          let convStarted = false;
+          for (item of items) {
+            if (!item.filtered) {
+              item.contact.createConversation();
+              convStarted = true;
+            }
+          }
+          if (!convStarted) {
+            let item = this.listbox.firstChild;
+            while (item && item.filtered)
+              item = item.nextSibling;
+            if (item) {
+              item.contact.createConversation();
+              convStarted = true;
+            }
+          }
+          // If a conversation was opened, close the tab. If the list was empty
+          // (no matches for search string), do nothing.
+          if (convStarted)
+            window.getTabBrowser().removeTab(this.tab);

This is hard to read. Can't we instead filter items to those entries we should start a conv with and return early if there are none or the list is empty?

The second if clause really seems to be about "what if nothing is selected". In this case I think we should only start a conversation if there is exactly one item in the list.
*** Original post on bio 2015 at 2013-07-03 13:36:17 UTC ***

"two words followed by a space": I meant "separated by whitespace" of course ;)
*** Original post on bio 2015 at 2013-07-03 14:29:09 UTC ***

"these should be maps (lookup is faster and the API is more legible)": The legibility win comes mainly from being able to drop hasOwnProperty(), which you can do here anyway as the keys are known to be just integers, and don't need a Map() for. (Whether Map lookup is actually faster compared to standard objects is actually not obvious as it depends on how objects are implemented internally, as flo pointed out.)
*** Original post on bio 2015 at 2013-07-03 16:04:36 UTC ***

Comment on attachment 8354315 [details] [diff] [review] (bio-attmnt 2547)
Fix a mistake

Quick comments (I haven't actually read the patch yet):
- We don't need the instantbird/content/awesometab.css file. Please use an element name less generic than richlistitem, and include the -moz-binding rule for it in instantbird/content/instantbird.css
- I agree with Mic's comment about using status.css. If it doesn't do what you need (yet), you should just fix it. Also, please check what I wrote about this on bug 954653 (bio 1221) comment 5 so that you don't bitrot with wnayes :-).
- let's change 'awesometab' in the code before landing.
- Coding style comment: when a field can fit on a single line, please keep it on one line, eg <field name="contact">null</field> rather than
      <field name="contact">
        null
      </field>
Attached patch Address review comments (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2550 at 2013-07-03 19:57:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354318 - Flags: review?(florian)
Attachment #8354318 - Flags: review?(benediktp)
Attachment #8354318 - Flags: review?(aleth)
Comment on attachment 8354315 [details] [diff] [review]
Fix a mistake

*** Original change on bio 2015 attmnt 2547 at 2013-07-03 19:57:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354315 - Attachment is obsolete: true
Attachment #8354315 - Flags: review?(florian)
Attachment #8354315 - Flags: review?(benediktp)
Attached patch Remove status.css modifications (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2551 at 2013-07-03 20:40:00 UTC ***

As discussed, I have removed the status.css stuff so that it can be done in a separate bug.
Attachment #8354319 - Flags: review?(florian)
Attachment #8354319 - Flags: review?(benediktp)
Attachment #8354319 - Flags: review?(aleth)
Comment on attachment 8354318 [details] [diff] [review]
Address review comments

*** Original change on bio 2015 attmnt 2550 at 2013-07-03 20:40:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354318 - Attachment is obsolete: true
Attachment #8354318 - Flags: review?(florian)
Attachment #8354318 - Flags: review?(benediktp)
Attachment #8354318 - Flags: review?(aleth)
*** Original post on bio 2015 as attmnt 2552 at 2013-07-03 20:41:00 UTC ***

I hope this helps while reviewing.
Depends on: 955468
*** Original post on bio 2015 at 2013-07-04 09:58:59 UTC ***

Comment on attachment 8354319 [details] [diff] [review] (bio-attmnt 2551)
Remove status.css modifications

OK, here are the comments on everything but newtab.xml as a first part of the review.

> diff --git a/instantbird/content/menus.js b/instantbird/content/menus.js
>    joinChat: function menu_joinChat() {
>      this.openDialog("Messenger:JoinChat", joinChatWindow);
>    },
> -

No unrelated whitespace changes, please.

> new file mode 100644
> --- /dev/null
> +++ b/instantbird/content/newtab.xml
> @@ -0,0 +1,518 @@
> +<?xml version="1.0"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!DOCTYPE bindings [
> +  <!ENTITY % newtabDTD SYSTEM "chrome://instantbird/locale/newtab.dtd" >
> +  %newtabDTD;
> +]>
> +
> +<bindings id="newtabBindings"
> +          xmlns="http://www.mozilla.org/xbl"
> +          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +          xmlns:xbl="http://www.mozilla.org/xbl">
> +  <binding id="newtab">
> +    <resources>
> +      <stylesheet src="chrome://instantbird/skin/newtab.css"/>
> +      <stylesheet src="chrome://instantbird/skin/richlistbox.css"/>
> +    </resources>
> +
> +    <content>
> +      <xul:stringbundle anonid="newtabbundle"
> +                        src="chrome://instantbird/locale/newtab.properties"/>
> +      <xul:vbox flex="1">
> +        <xul:toolbar class="newtab-toolbar">
> +          <xul:textbox class="filterbox" anonid="filterbox" type="search"
> +                       placeholder="&filterbox.placeholder;" flex="1"/>
> +        </xul:toolbar>
> +        <xul:richlistbox seltype="multiple" anonid="newtab-listbox"
> +                         class="newtab-listbox" flex="1"/>
> +      </xul:vbox>
> +    </content>
> +
> +    <implementation implements="nsIObserver">
> +      <property name="filterbox" readonly="true">
> +        <getter>
> +          return document.getAnonymousElementByAttribute(this, "anonid", "filterbox");
> +        </getter>
> +      </property>
> +
> +      <property name="listbox" readonly="true">
> +        <getter>
> +          return document.getAnonymousElementByAttribute(this, "anonid", "newtab-listbox");
> +        </getter>
> +      </property>
> +
> +      <property name="tab">
> +        <getter>
> +        <![CDATA[
> +          return this._tab;
> +        ]]>
> +        </getter>
> +        <setter>
> +        <![CDATA[
> +          this._tab = val;
> +          this._tab.setAttribute("label",
> +            document.getAnonymousElementByAttribute(this, "anonid", "newtabbundle")
> +                    .getString("newtab.label"));
> +          this._tab.setAttribute("image", "chrome://instantbird/skin/newConversation.png")
> +        ]]>
> +        </setter>
> +      </property>
> +
> +      <constructor>
> +      <![CDATA[
> +        if (!("MessageFormat" in window))
> +          Components.utils.import("resource:///modules/imTextboxUtils.jsm");
> +        MessageFormat.registerTextbox(this.filterbox);
> +        // Sorted list of contacts.
> +        this.contacts = [];
> +        // List of contacts by ID, used to check for dupes.
> +        this.contactsById = {};
> +        // List of currently added contact elements - array version of this.listbox.childNodes.
> +        this.contactElts = [];
> +      ]]>
> +      </constructor>
> +
> +      <destructor>
> +      <![CDATA[
> +        this.destroy();
> +      ]]>
> +      </destructor>
> +
> +      <method name="focus">
> +        <body>
> +        <![CDATA[
> +          this.filterbox.focus();
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="onResize">
> +        <body>
> +        <![CDATA[
> +          this.addMoreElements();
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Maintains computed number of elements to display. Incremented by 10 when
> +           the user scrolls near the bottom of the list, unless allElementsDisplayed is true. -->
> +      <field name="elementsToDisplay">0</field>
> +      <field name="allElementsDisplayed">false</field>
> +
> +      <!-- Increments elementsToDisplay and calls filter(). Used on scrolling/resizing. -->
> +      <method name="addMoreElements">
> +        <body>
> +        <![CDATA[
> +          if (this.allElementsDisplayed)
> +            return;
> +          // Add more elements when scrolling when the last visible element's
> +          // last index is less than or equal to this value.
> +          const kAddMoreElementsTreshold = 3;
> +          // Try to add this many elements when scrolling past the treshold.
> +          const kNumElementsIncrement = 10;
> +          let lastVisibleIndex =
> +            this.listbox.getIndexOfFirstVisibleRow() + this.listbox.getNumberOfVisibleRows() - 1;
> +          // Increment elementsToDisplay if lastVisibleIndex exceeds the treshold
> +          if (lastVisibleIndex >= (this.contactElts.length - kAddMoreElementsTreshold)) {
> +            this.elementsToDisplay += kNumElementsIncrement;
> +            this.filter();
> +          }
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Remove observers when closing tab. -->
> +      <method name="destroy">
> +        <body>
> +        <![CDATA[
> +          // destroy is called from both tabbrowser and the destructor.
> +          // We need to ensure we don't remove our observer twice if both call it.
> +          if (this._destroyed)
> +            return;
> +          Services.obs.removeObserver(this, "*");
> +          this._destroyed = true;
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="finishImport">
> +        <parameter name="aOtherNewtab"/>
> +        <body>
> +        <![CDATA[
> +          this.filterbox.value = aOtherNewtab.filterbox.value;
> +          this.filterbox.selectionStart = aOtherNewtab.filterbox.selectionStart;
> +          this.filterbox.selectionEnd = aOtherNewtab.filterbox.selectionEnd;
> +          this.contacts = aOtherNewtab.contacts;
> +          this.filteredContacts = aOtherNewtab.filteredContacts;
> +          this.contactsById = aOtherNewtab.contactsById;
> +          for (let i = 0; i < aOtherNewtab.contactElts.length; ++i)
> +            this.pushElt();
> +          this.elementsToDisplay = aOtherNewtab.elementsToDisplay;
> +          this._prevFilterTxt = aOtherNewtab._prevFilterTxt;
> +          this._contactUpdated = aOtherNewtab._contactUpdated;
> +          this.allElementsDisplayed = aOtherNewtab.allElementsDisplayed;
> +          this._delayedFilter();
> +          this.listbox.scrollToIndex(aOtherNewtab.listbox.getIndexOfFirstVisibleRow());
> +          for (let i = 0; i < aOtherNewtab.contactElts.length; ++i) {
> +            if (!aOtherNewtab.contactElts[i].selected)
> +              continue;
> +            this.listbox.addItemToSelection(this.contactElts[i]);
> +          }
> +          this.filterbox.addEventListener("keyup", this.filter.bind(this));
> +          this.listbox.addEventListener("scroll", this.addMoreElements.bind(this));
> +          Services.obs.addObserver(this, "*", false);
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="init">
> +        <body>
> +        <![CDATA[
> +          let contacts = Services.contacts.getContacts();
> +          for (let contact of contacts)
> +            this.addContact(contact);
> +          this.filteredContacts = this.contacts;
> +          this.resetElementsToDisplay();
> +          this.filter();
> +          this.filterbox.addEventListener("keyup", this.filter.bind(this));
> +          this.listbox.addEventListener("scroll", this.addMoreElements.bind(this));
> +          this.listbox.addEventListener("dblclick", this.startConversations.bind(this));
> +          Services.obs.addObserver(this, "*", false);
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Add contact to this.contacts, in its correct position.
> +           Does not add to the listbox. -->
> +      <method name="addContact">
> +        <parameter name="aImContact"/>
> +        <body>
> +        <![CDATA[
> +          if (aImContact.id in this.contactsById) // Already added.
> +            return;
> +          /* When multiple contacts change at once, we receive notifications for
> +           * each of them one by one and so resort them one by one. If we maintain
> +           * a list of imIContacts, it would be impossible to resort them properly
> +           * because of this, since they would all have changed. To work around
> +           * this problem, we maintian a list of custom contact objects, storing
> +           * only the lowercase display name and status type for sorting purposes.
> +           */
> +          let contact = {
> +            id: aImContact.id,
> +            name: aImContact.displayName.toLowerCase(),
> +            status: aImContact.statusType,
> +            imContact: aImContact
> +          };
> +          let pos = this.getPositionToInsert(contact);
> +          this.contacts.splice(pos, 0, contact);
> +          this.contactsById[aImContact.id] = contact;
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Remove contact from this.contacts. Does not update listbox. -->
> +      <method name="removeContact">
> +        <parameter name="aId"/>
> +        <body>
> +        <![CDATA[
> +          this.contacts.splice(this.contacts.indexOf(this.contactsById[aId]), 1);
> +          delete this.contactsById[aId];
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Binary search to find the correct location for the given contact -->
> +      <method name="getPositionToInsert">
> +        <parameter name="aContact"/>
> +        <body>
> +        <![CDATA[
> +          let end = this.contacts.length;
> +          // Avoid the binary search loop if the contacts were already sorted.
> +          if (end != 0 &&
> +             this.sortComparator(aContact, this.contacts[end - 1]) < 0) {
> +            let start = 0;
> +            while (start < end) {
> +              let middle = Math.floor((start + end) / 2);
> +              if (this.sortComparator(aContact, this.contacts[middle]) < 0)
> +                end = middle;
> +              else
> +                start = middle + 1;
> +            }
> +          }
> +          return end;
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Takes two items from contacts list (not imIContacts) and compares first
> +           their statusTypes, then their display names. -->
> +      <method name="sortComparator">
> +        <parameter name="aContactA"/>
> +        <parameter name="aContactB"/>
> +        <body>
> +        <![CDATA[
> +          return (aContactB.status - aContactA.status) ||
> +            aContactA.name.localeCompare(aContactB.name);
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Dispatch a call to _delayedFilter, to avoid consecutive multiple calls. -->
> +      <method name="filter">
> +        <body>
> +        <![CDATA[
> +          if (this._pendingFilterCall)
> +            return;
> +          this._pendingFilterCall = true;
> +          Services.tm.mainThread.dispatch(this._delayedFilter.bind(this),
> +            Components.interfaces.nsIEventTarget.DISPATCH_NORMAL);
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <field name="_prevFilterTxt">
> +        ""
> +      </field>
> +      <method name="_delayedFilter">
> +        <body>
> +        <![CDATA[
> +          this._pendingFilterCall = false;
> +          // Don't do anything if the tab has been removed.
> +          if (this._destroyed)
> +            return;
> +          let filterTxt = this.filterbox.value.toLowerCase();
> +          // If nothing has changed, we're being called only to display more elements.
> +          // In this case, we don't need to do the filtering again.
> +          if (this._prevFilterTxt != filterTxt || this._contactUpdated) {
> +            this.filteredContacts = this.contacts.filter(function(aContact) {
> +              return aContact.name.startsWith(filterTxt) ||
> +                aContact.name.split(/\s+/).some(function(s) s.startsWith(filterTxt));
> +            });
> +            // If the filter string has changed, reset number of elements to display.
> +            if (this._prevFilterTxt != filterTxt) {
> +              this.resetElementsToDisplay();
> +              this._prevFilterTxt = filterTxt;
> +            }
> +          }
> +          this._contactUpdated = false;
> +          let filteredContacts = this.filteredContacts;
> +          if (filteredContacts.length < this.elementsToDisplay) {
> +            this.elementsToDisplay = filteredContacts.length;
> +            this.allElementsDisplayed = true;
> +          }
> +          /* Add/remove list elements until we have the number required.
> +           * Then rebuild them all from filteredContacts.
> +           */
> +          while (this.contactElts.length > this.elementsToDisplay)
> +            this.popElt();
> +          while (this.contactElts.length < this.elementsToDisplay)
> +            this.pushElt();
> +          for (let i = 0; i < this.contactElts.length; ++i)
> +            this.contactElts[i].build(filteredContacts[i].imContact);
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="resetElementsToDisplay">
> +        <body>
> +        <![CDATA[
> +          let listbox = this.listbox;
> +          // We need to have a list item so that we can use its height to compute
> +          // the number of elements to display. If we don't have one, we add it,
> +          // but only if it will be used (i.e. this.filteredContacts.length > 0).
> +          if (this.filteredContacts.length > 0) {
> +            if (!listbox.firstChild)
> +              this.pushElt();
> +            // We add 10 more elements than will be visible.
> +            this.elementsToDisplay =
> +              (listbox.clientHeight / listbox.firstChild.clientHeight) + 10;
> +          }
> +          else
> +            this.elementsToDisplay = 0;
> +          this.allElementsDisplayed = false;
> +          listbox.scrollToIndex(0);
> +          listbox.clearSelection();
> +          // Ensure the first element will be selected if the user presses
> +          // down (for example) from the filterbox.
> +          listbox.selectedIndex = -1;
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="pushElt">
> +        <body>
> +        <![CDATA[
> +          // We use richlistitem as the nodeName so that CSS rules from richlistbox.css
> +          // get applied. We use the className to apply the -moz-binding.
> +          let elt = document.createElementNS(
> +            "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +            "richlistitem");
> +          elt.className = "newtab-contact";
> +          this.listbox.appendChild(elt);
> +          this.contactElts.push(elt);
> +          return elt;
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="popElt">
> +        <body>
> +        <![CDATA[
> +          this.listbox.removeChild(this.listbox.lastChild);
> +          this.contactElts.pop();
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- Start conversations with all selected items, or the first visible
> +           item if none are selected. -->
> +      <method name="startConversations">
> +        <body>
> +        <![CDATA[
> +          if (!this.listbox.firstChild)
> +            return;
> +          let items = this.listbox.selectedItems;
> +          if (items.length == 0) {
> +            this.listbox.firstChild.contact.createConversation();
> +            return;
> +          }
> +          for (let item of items)
> +            item.contact.createConversation();
> +          window.getTabBrowser().removeTab(this.tab);
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <!-- nsIObserver implementation -->
> +      <method name="observe">
> +        <parameter name="aSubject"/>
> +        <parameter name="aTopic"/>
> +        <parameter name="aData"/>
> +        <body>
> +        <![CDATA[
> +          if (!aTopic.startsWith("contact-"))
> +            return;
> +          if (aTopic == "contact-no-longer-dummy") {
> +            // Contact ID changed. aData is the old ID.
> +            this.contactsById[aSubject.id] = this.contactsById[aData];
> +            delete this.contactsById[aData];
> +            return;
> +          }
> +          else if (aTopic == "contact-added")
> +            this.addContact(aSubject);
> +          else if (aTopic == "contact-removed")
> +            this.removeContact(aSubject.id);
> +          else {
> +            this.removeContact(aSubject.id);
> +            this.addContact(aSubject);
> +          }
> +          this._contactUpdated = true;
> +          this.filter();
> +        ]]>
> +        </body>
> +      </method>
> +    </implementation>
> +
> +    <handlers>
> +      <handler event="keydown">
> +      <![CDATA[
> +        const modifierKeyCodes = [KeyEvent.DOM_VK_ALT, KeyEvent.DOM_VK_META,
> +                                  KeyEvent.DOM_VK_SHIFT, KeyEvent.DOM_VK_CONTROL];
> +
> +        // If the key is a modifier, don't do anything.
> +        if (modifierKeyCodes.indexOf(event.keyCode) != -1)
> +          return;
> +
> +        const navKeyCodes = [KeyEvent.DOM_VK_PAGE_UP, KeyEvent.DOM_VK_PAGE_DOWN,
> +                             KeyEvent.DOM_VK_HOME, KeyEvent.DOM_VK_END,
> +                             KeyEvent.DOM_VK_UP, KeyEvent.DOM_VK_DOWN];
> +
> +        let listbox = this.listbox;
> +        let modifiersPressed = event.altKey || event.metaKey || event.shiftKey || event.ctrlKey;
> +        // If it's not a navigation key, make sure filterbox has focus and return.
> +        if (navKeyCodes.indexOf(event.keyCode) == -1) {
> +          this.filterbox.focus();
> +          return;
> +        }
> +        // If modifiers are pressed, let whatever is focused handle the event.
> +        if (modifiersPressed)
> +          return;
> +        // Otherwise, give listbox focus so that it can process the navigation key.
> +        listbox.focus();
> +        // The selectedIndex is -1 after a new filter. In this case, pressing a
> +        // navkey from the filterbox should select the first element on the list.
> +        if (listbox.selectedIndex < 0 && listbox.firstChild) {
> +          listbox.selectItem(listbox.firstChild);
> +          listbox.ensureIndexIsVisible(0);
> +          event.preventDefault();
> +          event.stopPropagation();
> +        }
> +      ]]>
> +      </handler>
> +
> +      <handler event="keyup">
> +      <![CDATA[
> +        if (event.keyCode != 13) // Not the enter key!
> +          return;
> +        this.startConversations();
> +      ]]>
> +      </handler>
> +    </handlers>
> +  </binding>
> +
> +  <binding id="newtab-contact"
> +    extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> +    <content>
> +      <xul:hbox flex="1">
> +        <xul:stack class="buddyStatusIconStack" mousethrough="always">
> +          <xul:image anonid="buddyIcon" class="buddyIcon" xbl:inherits="src=buddyIcon,status"/>
> +          <xul:image anonid="statusIcon" class="statusTypeIcon" xbl:inherits="status"/>
> +        </xul:stack>
> +        <xul:vbox flex="1">
> +          <xul:spacer flex="1"/>
> +          <xul:hbox>
> +            <xul:label class="displayName" flex="1" crop="end" xbl:inherits="value=displayName,status"/>
> +            <xul:image class="tagIcon" src="chrome://instantbird/skin/tag.png" xbl:inherits="status"/>
> +            <xul:label class="tags" crop="end" xbl:inherits="value=tags,status"/>
> +          </xul:hbox>
> +          <xul:spacer flex="1"/>
> +          <xul:hbox class="statusAndProtoIconBox">
> +            <xul:label class="statusText" flex="1" crop="end" xbl:inherits="value=statusText,status"/>
> +            <xul:image class="protoIcon" xbl:inherits="src=protoIcon,status"/>
> +          </xul:hbox>
> +        </xul:vbox>
> +      </xul:hbox>
> +    </content>
> +
> +    <implementation>
> +      <field name="contact">
> +        null
> +      </field>
> +
> +      <method name="build">
> +        <parameter name="aContact"/>
> +        <body>
> +        <![CDATA[
> +          this.contact = aContact;
> +          this.setAttribute("displayName", this.contact.displayName);
> +          if (!("Status" in window))
> +            Components.utils.import("resource:///modules/imStatusUtils.jsm");
> +          let status = Status.toLabel(this.contact.statusType);
> +          if (this.contact.statusText) status += " - " + this.contact.statusText;
> +          this.setAttribute("statusText", status);
> +          let tagNames = this.contact.getTags().map(function(aTag) aTag.name);
> +          tagNames.sort(function(a, b) a.toLowerCase().localeCompare(b.toLowerCase()));
> +          this.setAttribute("tags", tagNames.join(", "));
> +          let buddy = this.contact.preferredBuddy;
> +          this.setAttribute("protoIcon", buddy.protocol.iconBaseURI + "icon.png");
> +          this.setAttribute("buddyIcon", buddy.buddyIconFilename);
> +          this.setAttribute("status", Status.toAttribute(buddy.statusType));
> +        ]]>
> +        </body>
> +      </method>
> +    </implementation>
> +  </binding>
> +</bindings>
> diff --git a/instantbird/locales/en-US/chrome/instantbird/instantbird.dtd b/instantbird/locales/en-US/chrome/instantbird/instantbird.dtd
>  <!ENTITY joinChat              "Join Chat...">
>  <!ENTITY joinChat.commandkey   "j">
>  <!ENTITY joinChat.accesskey    "J">
> +<!ENTITY newtab              "New Conversation...">
> +<!ENTITY newtab.commandkey   "t">
> +<!ENTITY newtab.accesskey    "n">

The indentation is wrong here.

> diff --git a/instantbird/locales/en-US/chrome/instantbird/newtab.properties b/instantbird/locales/en-US/chrome/instantbird/newtab.properties
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +newtab.label=New Conversation

We already have a string of this name in tabbrowser.dtd (they don't clash but maybe it's confusing to have the same name in a dtd and a properties file).

> diff --git a/instantbird/modules/imWindows.jsm b/instantbird/modules/imWindows.jsm
> +    let addNewTab = function(aWindow) {
> +      if (!aWindow)
> +        return false;
> +      let newtab = aWindow.document.createElementNS(
> +        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +        "newtab");
> +      if (!aWindow.getTabBrowser().addPanel(newtab))
> +        return false;
> +      aWindow.getTabBrowser().selectPanel(newtab);
> +      newtab.ownerDocument.defaultView.focus();
> +      newtab.init();
> +      return true;
> +    }
> +    if (!addNewTab(win)) {
> +      win = Services.ww.openWindow(null, "chrome://instantbird/content/instantbird.xul",

The URI is available in a constant defined at top of this file.

> diff --git a/instantbird/themes/newtab.css b/instantbird/themes/newtab.css
> new file mode 100644
> --- /dev/null
> +++ b/instantbird/themes/newtab.css
> @@ -0,0 +1,103 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +.newtab-toolbar {
> +  margin: 0;
> +  padding: 2px;
> +  border-style: none;
> +  -moz-appearance: none;
> +%ifdef XP_MACOSX
> +  background-color: -moz-mac-chrome-active;
> +  background-image: -moz-linear-gradient(rgba(255,255,255,.43), rgba(255,255,255,0));
> +  border-bottom: 1px solid rgba(0, 0, 0, 0.57);
> +}
> +
> +.newtab-toolbar:-moz-window-inactive {
> +  background-color: -moz-mac-chrome-inactive;
> +  border-bottom-color: rgba(0, 0, 0, 0.32);
> +%else
> +  background-color: -moz-Dialog;
> +%ifdef XP_WIN
> +  background-image: -moz-linear-gradient(rgba(255,255,255,.5), rgba(255,255,255,0));
> +%else
> +  background-image: -moz-linear-gradient(rgba(255,255,255,.3), rgba(255,255,255,0));
> +%endif
> +%ifndef XP_WIN
> +  border-bottom: 1px solid ThreeDShadow;
> +%else
> +  border-bottom: none;
> +%endif
> +%endif
> +}

This is copied from conversation.css, right? Maybe we could deduplicate that in a reasonable way? If not, just leave it for now.

> +.newtab-listbox {
> +  margin: 0;
> +  -moz-appearance: none;
> +%ifndef XP_MACOSX
> +  border-bottom: 2px solid;
> +  -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
> +%endif
> +}

I can't say anything about the styles on Mac OS. Florian maybe?

> +.newtab-contact {
> +  padding-top: 4px !important;
> +  padding-bottom: 4px !important;
> +}

Is !important really needed here? Maybe "richlistitem.newtab-contact" would suffice to have a high enough specificity?

> +.displayName {
> +  font-size: large;
> +}
> +
> +.tags {
> +  margin-left: 4px !important;
> +  margin-right: 0px !important;

Why is !important needed here? I couldn't find an obvious reason.

> +.protoIcon {
> +  width: 16px;
> +  height: 16px;
> +  max-width: 16px;
> +  max-height: 16px;
> +  margin-right: 0px !important;

What's the reason for this again?
*** Original post on bio 2015 at 2013-07-04 10:01:13 UTC ***

I'm sorry, I thought I had removed the section about newtab.xml already :(
Comment on attachment 8354319 [details] [diff] [review]
Remove status.css modifications

*** Original change on bio 2015 attmnt 2551 at 2013-07-05 11:15:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354319 - Flags: review?(benediktp) → review-
*** Original post on bio 2015 at 2013-07-05 11:15:22 UTC ***

Comment on attachment 8354319 [details] [diff] [review] (bio-attmnt 2551)
Remove status.css modifications

> diff --git a/instantbird/content/newtab.xml b/instantbird/content/newtab.xml
> +      <!-- Maintains computed number of elements to display. Incremented by 10 when
> +           the user scrolls near the bottom of the list, unless allElementsDisplayed is true. -->
> +      <field name="elementsToDisplay">0</field>

I'd prefer something with "count" in it for this field to make clear that this doesn't contain any elements but only the number of them.

> +      <field name="allElementsDisplayed">false</field>

Maybe a name like "hasMoreElementsToDisplay" (Obs: inverted logic!) would better indicate that this is a boolean flag for something?

> +      <!-- Increments elementsToDisplay and calls filter(). Used on scrolling/resizing. -->
> +      <method name="addMoreElements">
> +        <body>
> +        <![CDATA[
> +          if (this.allElementsDisplayed)
> +            return;
> +          // Add more elements when scrolling when the last visible element's
> +          // last index is less than or equal to this value.
> +          const kAddMoreElementsTreshold = 3;
> +          // Try to add this many elements when scrolling past the treshold.
> +          const kNumElementsIncrement = 10;
> +          let lastVisibleIndex =
> +            this.listbox.getIndexOfFirstVisibleRow() + this.listbox.getNumberOfVisibleRows() - 1;

What about wrapping this line like this?
let lastVisibleIndex = this.listbox.getIndexOfFirstVisibleRow() +
                         this.listbox.getNumberOfVisibleRows() - 1;

> +          // Increment elementsToDisplay if lastVisibleIndex exceeds the treshold

Nit: There is a dot missing at the end of the comment.

> +
> +      <method name="finishImport">
> +        <parameter name="aOtherNewtab"/>
> +        <body>
> +        <![CDATA[
> +          this.filterbox.value = aOtherNewtab.filterbox.value;
> +          this.filterbox.selectionStart = aOtherNewtab.filterbox.selectionStart;
> +          this.filterbox.selectionEnd = aOtherNewtab.filterbox.selectionEnd;
> +          this.contacts = aOtherNewtab.contacts;
> +          this.filteredContacts = aOtherNewtab.filteredContacts;
> +          this.contactsById = aOtherNewtab.contactsById;
> +          for (let i = 0; i < aOtherNewtab.contactElts.length; ++i)
> +            this.pushElt();
> +          this.elementsToDisplay = aOtherNewtab.elementsToDisplay;
> +          this._prevFilterTxt = aOtherNewtab._prevFilterTxt;
> +          this._contactUpdated = aOtherNewtab._contactUpdated;
> +          this.allElementsDisplayed = aOtherNewtab.allElementsDisplayed;
> +          this._delayedFilter();
> +          this.listbox.scrollToIndex(aOtherNewtab.listbox.getIndexOfFirstVisibleRow());
> +          for (let i = 0; i < aOtherNewtab.contactElts.length; ++i) {
> +            if (!aOtherNewtab.contactElts[i].selected)
> +              continue;
> +            this.listbox.addItemToSelection(this.contactElts[i]);
> +          }

I haven't checked yet that this actually contains every piece of information that is needed to restore the state of the tab from the other window. I'll let you know on IRC if I find something missing.

> +          this.filterbox.addEventListener("keyup", this.filter.bind(this));
> +          this.listbox.addEventListener("scroll", this.addMoreElements.bind(this));

After reading the "init" method, I'd expect that a double click event listener needs to be added to the listbox here?

> +      <method name="init">

> +          this.filterbox.addEventListener("keyup", this.filter.bind(this));
> +          this.listbox.addEventListener("scroll", this.addMoreElements.bind(this));
> +          this.listbox.addEventListener("dblclick", this.startConversations.bind(this));
> +          Services.obs.addObserver(this, "*", false);

If the double click event listener was really missing above, it might be nice to deduplicate this to avoid adding something in one place and forgetting it in the other. Otherwise I don't know if there's much to gain movign this to an own method.

> +      <!-- Add contact to this.contacts, in its correct position.
> +           Does not add to the listbox. -->
> +      <method name="addContact">
> +        <parameter name="aImContact"/>
> +        <body>
> +        <![CDATA[
> +          if (aImContact.id in this.contactsById) // Already added.
> +            return;
> +          /* When multiple contacts change at once, we receive notifications for
> +           * each of them one by one and so resort them one by one. If we maintain
> +           * a list of imIContacts, it would be impossible to resort them properly
> +           * because of this, since they would all have changed. To work around
> +           * this problem, we maintian a list of custom contact objects, storing

There's a spelling mistake in "maintain".

> +      <method name="removeContact">
> +        <parameter name="aId"/>
> +        <body>
> +        <![CDATA[
> +          this.contacts.splice(this.contacts.indexOf(this.contactsById[aId]), 1);

If there's no contact with this id, you'd currently remove the last item of the array as far as I can tell. Can we be sure that we never try to remove a contact with a non-existant id from here?

> +      <field name="_prevFilterTxt">
> +        ""
> +      </field>

Put this on a single line, please.

> +      <method name="_delayedFilter">
> +        <body>
> +        <![CDATA[

> +          let filterTxt = this.filterbox.value.toLowerCase();
> +          // If nothing has changed, we're being called only to display more elements.
> +          // In this case, we don't need to do the filtering again.

Don't you want it to say "If a contact was updated or the filter string was changed, the list of filtered contacts has to be updated."?

> +          if (this._prevFilterTxt != filterTxt || this._contactUpdated) {

Is there a better name than _contactUpdated for it? Something that clearly tells that the list of results needs to be updated?

> +      <method name="resetElementsToDisplay">
> +        <body>
> +        <![CDATA[
> +          let listbox = this.listbox;
> +          // We need to have a list item so that we can use its height to compute
> +          // the number of elements to display. If we don't have one, we add it,
> +          // but only if it will be used (i.e. this.filteredContacts.length > 0).
> +          if (this.filteredContacts.length > 0) {
> +            if (!listbox.firstChild)
> +              this.pushElt();
> +            // We add 10 more elements than will be visible.
> +            this.elementsToDisplay =
> +              (listbox.clientHeight / listbox.firstChild.clientHeight) + 10;

Please define a constant on the binding that you can use from addMoreElements and here.

> +      <method name="pushElt">
> +      <method name="popElt">

These names are terrible to be honest ;) What about "append/removeContactElement"?

> +    <handlers>
> +      <handler event="keydown">

I'll need to look at this more closely again but it seemed OK at a glance.

> +
> +      <handler event="keyup">
> +      <![CDATA[
> +        if (event.keyCode != 13) // Not the enter key!
> +          return;

There's key constants like you used them above, so we don't need to check for numeric values. (I hope you haven't found this elsewhere in our code?)


> +  <binding id="newtab-contact"

> +          <xul:hbox>
> +            <xul:label class="displayName" flex="1" crop="end" xbl:inherits="value=displayName,status"/>
> +            <xul:image class="tagIcon" src="chrome://instantbird/skin/tag.png" xbl:inherits="status"/>

Shouldn't the image be rather set from CSS?

> +    <implementation>
> +      <field name="contact">
> +        null
> +      </field>

We have fields with only little content defined on a single line as far as I know.

> +      <method name="build">
> +        <parameter name="aContact"/>
> +        <body>
> +        <![CDATA[
> +          this.contact = aContact;
> +          this.setAttribute("displayName", this.contact.displayName);
> +          if (!("Status" in window))
> +            Components.utils.import("resource:///modules/imStatusUtils.jsm");
> +          let status = Status.toLabel(this.contact.statusType);
> +          if (this.contact.statusText) status += " - " + this.contact.statusText;

The if statement and its 'block' should be on separate lines.
*** Original post on bio 2015 as attmnt 2555 at 2013-07-05 17:54:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354323 - Flags: review?(florian)
Attachment #8354323 - Flags: review?(benediktp)
Attachment #8354323 - Flags: review?(aleth)
Comment on attachment 8354319 [details] [diff] [review]
Remove status.css modifications

*** Original change on bio 2015 attmnt 2551 at 2013-07-05 17:54:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354319 - Attachment is obsolete: true
Attachment #8354319 - Flags: review?(florian)
Attachment #8354319 - Flags: review?(aleth)
Comment on attachment 8354323 [details] [diff] [review]
Address review comments and fix derps

*** Original change on bio 2015 attmnt 2555 at 2013-07-06 19:06:07 UTC ***

I only looked at the changes in response to my own review comments, to save time.

+      <method name="_delayedFilter">
+        <body>
+        <![CDATA[
+          this._pendingFilterCall = false;
+          // Don't do anything if the tab has been removed.
+          if (this._destroyed)
+            return;
+          let filterTxt = this.filterbox.value.toLowerCase();
+          // Filtering needs to be done only if either the filter string has
+          // changed, or a contact was updated.
+          if (this._prevFilterTxt != filterTxt || this._contactUpdated) {
+            this.filteredContacts = this.contacts.filter(function(aContact) {
+              return aContact.name.startsWith(filterTxt) ||
+                aContact.name.split(/\s+/).some(function(s) s.startsWith(filterTxt));
+            });
+            // If the filter string has changed, reset number of elements to display.
+            if (this._prevFilterTxt != filterTxt) {
+              this.resetElementsToDisplay();
+              this._prevFilterTxt = filterTxt;
+            }
+          }
+          this._contactUpdated = false;
+          let filteredContacts = this.filteredContacts;
+          if (filteredContacts.length < this.numberOfElementsToDisplay) {
+            this.numberOfElementsToDisplay = filteredContacts.length;
+            this.hasMoreElementsToDisplay = false;
+          }
+          /* Add/remove list elements until we have the number required.
+           * Then rebuild them all from filteredContacts.
+           */
+          while (this.contactElts.length > this.numberOfElementsToDisplay)
+            this.popContactElement();
+          while (this.contactElts.length < this.numberOfElementsToDisplay)
+            this.pushContactElement();
+          for (let i = 0; i < this.contactElts.length; ++i)
+            this.contactElts[i].build(filteredContacts[i].imContact);
+        ]]>
+        </body>
+      </method>

This is a method that does two things: 
1) Filter the contacts (as the name suggests) 
2) Update the display, adding more contacts if needed.
If you turn this into two methods, I think it will be clearer (also when moving parts of this to the service later), and you should be able to do without the flag this._contactUpdated.

>+         if (items.length == 0)
Nit: We tend to use if (!items.length)
Attachment #8354323 - Flags: review?(aleth) → review-
Comment on attachment 8354323 [details] [diff] [review]
Address review comments and fix derps

*** Original change on bio 2015 attmnt 2555 at 2013-07-08 10:13:00 UTC ***

There's no tag icon on Windows XP. Loading the URI in DOMi says that it can't be found. Maybe a packaging problem?

> diff --git a/instantbird/content/menus.js b/instantbird/content/menus.js
>    isCommandEnabled: function(aCmd) {
>      let enumerator = Services.accounts.getAccounts();
>      while (enumerator.hasMoreElements()) {
>        let acc = enumerator.getNext();
>        if (acc.connected && (aCmd == "cmd_addbuddy" || acc.canJoinChat))

You need to add your command here too, otherwise it will be disabled if there's no account connected that supports MUCs (that's how I found it today).

> diff -u b/instantbird/content/newtab.xml b/instantbird/content/newtab.xml

> +      <!-- Maintains computed number of elements to display. Incremented when the user
> +           scrolls near the bottom of the list, unless hasMoreElementsToDisplay is true. -->

"[...], unless hasMoreElementsToDisplay is false."

> +      <field name="numberOfElementsToDisplay">0</field>
> +      <field name="hasMoreElementsToDisplay">false</field>

This field needs to be set to true by default now.
Both fields could have a readonly="true" attribute if we really want them to be constants.

> -              this.pushElt();
> -            // We add 10 more elements than will be visible.
> -            this.elementsToDisplay =
> -              (listbox.clientHeight / listbox.firstChild.clientHeight) + 10;
> +              this.pushContactElement();
> +            // Add enough elements to fill the visible area, plus kNumElementsIncrement

Nit: missing dot at end of sentence.

> diff -u b/instantbird/modules/imWindows.jsm b/instantbird/modules/imWindows.jsm
> --- b/instantbird/modules/imWindows.jsm
> +++ b/instantbird/modules/imWindows.jsm
> @@ -170,8 +170,8 @@
>        return true;
>      }
>      if (!addNewTab(win)) {
> -      win = Services.ww.openWindow(null, "chrome://instantbird/content/instantbird.xul",
> -                                   "_blank", "chrome,toolbar,resizable", null);
> +      win = Services.ww.openWindow(null, CONVERSATION_WINDOW_URI, "_blank",
> +                             "chrome,toolbar,resizable", null);

Thanks and please adapt the indentation too!

> diff -u b/instantbird/themes/newtab.css b/instantbird/themes/newtab.css

> -.newtab-contact [status="offline"]:not(.statusIcon16),
> -.newtab-contact [status="unknown"]:not(.statusIcon16) {
> +.newtab-contact [status="offline"]:not(.statusTypeIcon),
> +.newtab-contact [status="unknown"]:not(.statusTypeIcon) {
>    opacity: 0.5;
>  }

Nice, this is even something that I had missed! :)
Attachment #8354323 - Flags: review?(benediktp) → review-
Attached patch Address review comments (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2556 at 2013-07-09 13:21:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354324 - Flags: review?(florian)
Attachment #8354324 - Flags: review?(benediktp)
Attachment #8354324 - Flags: review?(aleth)
Comment on attachment 8354320 [details]
Diff between old awesometab.xml and new newtab.xml

*** Original change on bio 2015 attmnt 2552 at 2013-07-09 13:21:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354320 - Attachment is obsolete: true
Comment on attachment 8354323 [details] [diff] [review]
Address review comments and fix derps

*** Original change on bio 2015 attmnt 2555 at 2013-07-09 13:21:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354323 - Attachment is obsolete: true
Attachment #8354323 - Flags: review?(florian)
Comment on attachment 8354324 [details] [diff] [review]
Address review comments

*** Original change on bio 2015 attmnt 2556 at 2013-07-09 13:33:42 UTC ***

>        if (!("MessageFormat" in window))
>          Components.utils.import("resource:///modules/imTextboxUtils.jsm");
>        MessageFormat.registerTextbox(this.filterbox);

As flo pointed out, you don't really want to do this, right?
Attachment #8354324 - Flags: review?(aleth) → review-
Attached patch Fix a derp (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2557 at 2013-07-09 13:59:00 UTC ***

Filter function is renamed to refresh.
Remove the ellipsis at the end of "Start typing a contact name"
Make tag text crop first, then display name.
Attachment #8354325 - Flags: review?(florian)
Attachment #8354325 - Flags: review?(benediktp)
Attachment #8354325 - Flags: review?(aleth)
Comment on attachment 8354324 [details] [diff] [review]
Address review comments

*** Original change on bio 2015 attmnt 2556 at 2013-07-09 13:59:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354324 - Attachment is obsolete: true
Attachment #8354324 - Flags: review?(florian)
Attachment #8354324 - Flags: review?(benediktp)
Attached patch Set font size from CSS. (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2558 at 2013-07-09 14:03:00 UTC ***

Sorry for forgetting this in the previous patch.
Attachment #8354326 - Flags: review?(florian)
Attachment #8354326 - Flags: review?(benediktp)
Attachment #8354326 - Flags: review?(aleth)
Comment on attachment 8354325 [details] [diff] [review]
Fix a derp

*** Original change on bio 2015 attmnt 2557 at 2013-07-09 14:03:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354325 - Attachment is obsolete: true
Attachment #8354325 - Flags: review?(florian)
Attachment #8354325 - Flags: review?(benediktp)
Attachment #8354325 - Flags: review?(aleth)
Attached patch Fix more mistakes. (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2559 at 2013-07-09 14:39:00 UTC ***

Sorry. I hope this one is good.
Attachment #8354327 - Flags: review?(florian)
Attachment #8354327 - Flags: review?(benediktp)
Attachment #8354327 - Flags: review?(aleth)
Comment on attachment 8354326 [details] [diff] [review]
Set font size from CSS.

*** Original change on bio 2015 attmnt 2558 at 2013-07-09 14:39:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354326 - Attachment is obsolete: true
Attachment #8354326 - Flags: review?(florian)
Attachment #8354326 - Flags: review?(benediktp)
Attachment #8354326 - Flags: review?(aleth)
Comment on attachment 8354327 [details] [diff] [review]
Fix more mistakes.

*** Original change on bio 2015 attmnt 2559 at 2013-07-09 14:47:06 UTC ***

I didn't give this another full review, just looked at the various places I mentioned in previous comments.
Attachment #8354327 - Flags: review?(aleth) → review+
Comment on attachment 8354327 [details] [diff] [review]
Fix more mistakes.

*** Original change on bio 2015 attmnt 2559 at 2013-07-09 22:44:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354327 - Flags: review?(florian) → review-
*** Original post on bio 2015 at 2013-07-09 22:44:06 UTC ***

Comment on attachment 8354327 [details] [diff] [review] (bio-attmnt 2559)
Fix more mistakes.

r-, but most of the comments are easy to address. Looking forward to actually trying the next iteration of the patch :-).

>       let acc = enumerator.getNext();
>-      if (acc.connected && (aCmd == "cmd_addbuddy" || acc.canJoinChat))
>+      if (acc.connected &&
>+        (aCmd == "cmd_addbuddy" || aCmd == "cmd_newtab" || acc.canJoinChat))

Nit: indent is wrong here, should be:
if (blah &&
    (foo || bar))
  doStuff;



>+  <binding id="newtab">

>+        <xul:richlistbox seltype="multiple" anonid="newtab-listbox"

I don't think handling multiple selection brings much value in here, but the code handling it seems alright, so I don't mind.


>+          this._tab.setAttribute("image", "chrome://instantbird/skin/newConversation.png")

I would like if you could find a way to set this image by CSS, so that we can easily set a different icon for Mac retina later.
Maybe use a class?

And if you don't, nit: missing semicolon at the end of the line :-P.


>+      <!-- Maintains computed number of elements to display. Incremented when the user
>+           scrolls near the bottom of the list, unless hasMoreElementsToDisplay is false. -->
>+      <field name="numberOfElementsToDisplay">0</field>
>+      <field name="hasMoreElementsToDisplay">true</field>
>+      <!-- Try to add this many elements when scrolling past the threshold. -->
>+      <field name="kNumElementsIncrement" readonly="true">10</field>
>+      <!-- Add more elements when scrolling when the last visible element's
>+           last index is less than or equal to this value. -->

I couldn't understand this comment and had to read the code to understand what that constant does. Please rephrase.

>+      <field name="kAddMoreElementsThreshold" readonly="true">3</field>
>+
>+      <!-- Increments numberOfElementsToDisplay and calls refresh(). Used on scrolling/resizing. -->
>+      <method name="addMoreElements">
>+        <body>
>+        <![CDATA[
>+          if (!this.hasMoreElementsToDisplay)
>+            return;
>+          let lastVisibleIndex = this.listbox.getIndexOfFirstVisibleRow() +
>+                                   this.listbox.getNumberOfVisibleRows() - 1;
>+          // Increment numberOfElementsToDisplay if lastVisibleIndex exceeds the threshold.
>+          if (lastVisibleIndex >= (this.contactElts.length - this.kAddMoreElementsThreshold)) {

Nit: you can drop the extra () here.

>+      <!-- Remove observers when closing tab. -->
>+      <method name="destroy">
>+        <body>
>+        <![CDATA[
>+          // destroy is called from both tabbrowser and the destructor.
>+          // We need to ensure we don't remove our observer twice if both call it.
>+          if (this._destroyed)
>+            return;
>+          Services.obs.removeObserver(this, "*");

You really don't want to observe ALL notifications. 

>+          this._destroyed = true;
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="finishImport">
>+        <parameter name="aOtherNewtab"/>

Maybe aOtherNew*T*ab?

>+        <body>
>+        <![CDATA[
>+          this.filterbox.value = aOtherNewtab.filterbox.value;
>+          this.filterbox.selectionStart = aOtherNewtab.filterbox.selectionStart;
>+          this.filterbox.selectionEnd = aOtherNewtab.filterbox.selectionEnd;

Calling 6 times the filterbox getter? :(

>+          this.contacts = aOtherNewtab.contacts;
>+          this.filteredContacts = aOtherNewtab.filteredContacts;
>+          this.contactsById = aOtherNewtab.contactsById;
>+          for (let i = 0; i < aOtherNewtab.contactElts.length; ++i)
>+            this.pushContactElement();
>+          this.numberOfElementsToDisplay = aOtherNewtab.numberOfElementsToDisplay;
>+          this._prevFilterTxt = aOtherNewtab._prevFilterTxt;
>+          this._contactUpdated = aOtherNewtab._contactUpdated;
>+          this.hasMoreElementsToDisplay = aOtherNewtab.hasMoreElementsToDisplay;

Code duplication here. I think you can simplify with a list of fields you want to import.
Eg.
let fieldsToImport = ["foo", bar"];
for (let field of fieldsToImport)
  this[field] = aOtherNewtab[field];


>+          this._delayedRefresh();
>+          this.listbox.scrollToIndex(aOtherNewtab.listbox.getIndexOfFirstVisibleRow());
>+          for (let i = 0; i < aOtherNewtab.contactElts.length; ++i) {
>+            if (!aOtherNewtab.contactElts[i].selected)
>+              continue;
>+            this.listbox.addItemToSelection(this.contactElts[i]);
>+          }

Can the 2 for loops of this method be combined into one?


>+      <method name="init">

Do we really need this method? Any reason why this can't be inlined in the constructor?

>+      <method name="addListeners">

>+          Services.obs.addObserver(this, "*", false);

No :).


>+      <method name="addContact">

>+          this.contactsById[aImContact.id] = contact;

Nit: contact.id is likely faster than aImContact.id here (it just accesses a property of a regular JS object, instead of calling a getter of an XPCOM object through XPConnect).


>+      <!-- Binary search to find the correct location for the given contact -->
>+      <method name="getPositionToInsert">
>+        <parameter name="aContact"/>
>+        <body>
>+        <![CDATA[
>+          let end = this.contacts.length;
>+          // Avoid the binary search loop if the contacts were already sorted.
>+          if (end != 0 &&
>+             this.sortComparator(aContact, this.contacts[end - 1]) < 0) {

Nit: indent is wrong here.


>+      <method name="startConversations">
>+        <body>
>+        <![CDATA[
>+          if (!this.listbox.firstChild)
>+            return;
>+          let items = this.listbox.selectedItems;
>+          if (!items.length)
>+            this.listbox.firstChild.contact.createConversation();
>+          for (let item of items)
>+            item.contact.createConversation();
>+          window.getTabBrowser().removeTab(this.tab);

In the future, we will likely want the real conversation tab to replace the 'new tab' tab. Especially if we start animating the creating of new tabs, like Firefox does. Doesn't matter now though.


>+      <!-- nsIObserver implementation -->
>+      <method name="observe">
>+        <parameter name="aSubject"/>
>+        <parameter name="aTopic"/>
>+        <parameter name="aData"/>
>+        <body>
>+        <![CDATA[
>+          if (!aTopic.startsWith("contact-"))
>+            return;
>+          if (aTopic == "contact-no-longer-dummy") {
>+            // Contact ID changed. aData is the old ID.

aData is a string containing the old id. You need something like:
let oldId = parseInt(aData);

You probably also need:

So this...
>+            this.contactsById[aSubject.id] = this.contactsById[aData];
>+            delete this.contactsById[aData];
becomes:
      let id = aSubject.id;
      this.contactsById[id] = this.contactsById[oldId];
      delete this.contactsById[oldId];

And I think you also need:
      this.contactsById[id].id = id;


>+          else {
>+            this.removeContact(aSubject.id);
>+            this.addContact(aSubject);

I think this needs a comment saying that if the status or display name changes, the sorting order may change, so it's easier to remove and re-add the contact than to update it.


>+      <method name="build">

>+          this.setAttribute("tags", tagNames.join(", "));

Usually I would say that strings displayed in the UI need to be localizable, but ", " is likely OK.
Attached patch Address flo's comments (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2562 at 2013-07-10 01:35:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354330 - Flags: review?(florian)
Attachment #8354330 - Flags: review?(benediktp)
Attachment #8354330 - Flags: review?(aleth)
Comment on attachment 8354327 [details] [diff] [review]
Fix more mistakes.

*** Original change on bio 2015 attmnt 2559 at 2013-07-10 01:35:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354327 - Attachment is obsolete: true
Attachment #8354327 - Flags: review?(benediktp)
Depends on: 955472
*** Original post on bio 2015 as attmnt 2564 at 2013-07-10 09:40:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354332 - Flags: review?(florian)
Attachment #8354332 - Flags: review?(benediktp)
Attachment #8354332 - Flags: review?(aleth)
Comment on attachment 8354330 [details] [diff] [review]
Address flo's comments

*** Original change on bio 2015 attmnt 2562 at 2013-07-10 09:40:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354330 - Attachment is obsolete: true
Attachment #8354330 - Flags: review?(florian)
Attachment #8354330 - Flags: review?(benediktp)
Attachment #8354330 - Flags: review?(aleth)
*** Original post on bio 2015 as attmnt 2566 at 2013-07-10 13:01:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354334 - Flags: review?(florian)
Attachment #8354334 - Flags: review?(benediktp)
Attachment #8354334 - Flags: review?(aleth)
Comment on attachment 8354332 [details] [diff] [review]
Use tabtype attribute from bug 2035 to set icon.

*** Original change on bio 2015 attmnt 2564 at 2013-07-10 13:01:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354332 - Attachment is obsolete: true
Attachment #8354332 - Flags: review?(florian)
Attachment #8354332 - Flags: review?(benediktp)
Attachment #8354332 - Flags: review?(aleth)
Comment on attachment 8354334 [details] [diff] [review]
Change name of "tabtype" to "type"

*** Original change on bio 2015 attmnt 2566 at 2013-07-11 00:41:18 UTC ***

- Nit: "when being imported to another window." You import from or export to, but you can't "import to".

- The patch doesn't apply cleanly. instantbird/themes/instantbird.css fails. Likely some bitrot from bug 955472 (bio 2035).


Now I've actually tried the patch, here's some feedback:

- Pressing Command+T on the Contacts window flashes the "File" menu but does nothing else (not even an error in the console) if I don't have any connected account. You likely want to disable the command when it won't work.

- My AIM contacts that don't have a buddy icon don't show the placeholder icon; instead the icon is just blank. In my terminal (this is a debug build), I see lots of lines like this:
Chrome file doesn't exist: [...]/InstantbirdDebug.app/Contents/MacOS/chrome/instantbird/content/instantbird/null
I looked at a richlistitem with a blank icon with DOM Inspector, and there is a "buddyIcon" attribute set to the string "null". My guess is that when there's no buddy icon for libpurple accounts the getter returns null and for js-prpls, the getter returns "". I think you just need to add a null check somewhere in your code.

- The mouse interactions are wrong. I think it should behave like Firefox's awesomebar, where hovering selects (or at least highlights) and clicking once validates. Nobody will think of double clicking here.

+ The blue outline (Mac) around the filter box is ugly; compare with the blue outline of the findbar's searchbox in a conversation.

+ The "+" button is ugly (at least on Mac) compared to the same button on Firefox. I suspect we miss a few CSS rules. Not a real surprise, given that we've had this button for 4 years but never displayed it; yet!

+ There seem to be performance issues. When I press Command+T the UI seems to freeze for half or a third of second. Stuff is slower than usual because I'm running a debug build, but some users may also have slow machines. I don't think this is worth investigating immediately as I suspect the performance aspect will significantly change once you move to a service.

+ The focus ring around elements of the richlistbox when using the up/down arrow is ugly. Not sure where it's coming from, as we don't have it on the account manager or the buddy list.

NB: in the above comments, "-" means 'needs to be addressed before landing' and '+' means 'can either be addressed here before landing or in a follow-up'.
Attachment #8354334 - Flags: review?(florian) → review-
Attached file Address comments (obsolete) —
*** Original post on bio 2015 as attmnt 2573 at 2013-07-11 22:10:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attached patch Address comments (obsolete) — Splinter Review
*** Original post on bio 2015 as attmnt 2574 at 2013-07-11 22:12:00 UTC ***

Previous attachment broken, sorry
Attachment #8354342 - Flags: review?(florian)
Attachment #8354342 - Flags: review?(benediktp)
Attachment #8354342 - Flags: review?(aleth)
Comment on attachment 8354334 [details] [diff] [review]
Change name of "tabtype" to "type"

*** Original change on bio 2015 attmnt 2566 at 2013-07-11 22:12:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354334 - Attachment is obsolete: true
Attachment #8354334 - Flags: review?(benediktp)
Attachment #8354334 - Flags: review?(aleth)
Comment on attachment 8354341 [details]
Address comments

*** Original change on bio 2015 attmnt 2573 at 2013-07-11 22:12:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354341 - Attachment is obsolete: true
*** Original post on bio 2015 as attmnt 2576 at 2013-07-11 23:19:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354344 - Flags: review?(florian)
Attachment #8354344 - Flags: review?(benediktp)
Attachment #8354344 - Flags: review?(aleth)
Comment on attachment 8354342 [details] [diff] [review]
Address comments

*** Original change on bio 2015 attmnt 2574 at 2013-07-11 23:19:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354342 - Attachment is obsolete: true
Attachment #8354342 - Flags: review?(florian)
Attachment #8354342 - Flags: review?(benediktp)
Attachment #8354342 - Flags: review?(aleth)
Comment on attachment 8354344 [details] [diff] [review]
Some bugfixes and tweaks

*** Original change on bio 2015 attmnt 2576 at 2013-07-11 23:48:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354344 - Flags: review?(florian) → review+
*** Original post on bio 2015 at 2013-07-11 23:53:52 UTC ***

Can't wait to try this in the nightlies!!!

http://hg.instantbird.org/instantbird/rev/e0a2d039654e

Please file the follow ups tomorrow.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Depends on: 955477
Depends on: 955478
Depends on: 955479
Depends on: 955480
Depends on: 955481
Depends on: 955482
Depends on: 955483
Depends on: 955484
Depends on: 955485
Depends on: 955488
Comment on attachment 8354344 [details] [diff] [review]
Some bugfixes and tweaks

*** Original change on bio 2015 attmnt 2576 at 2013-07-19 19:55:28 UTC ***

Just noticed there was still a review request open on this ;)
Attachment #8354344 - Flags: review?(benediktp)
Attachment #8354344 - Flags: review?(aleth)
Attachment #8354344 - Flags: review+
Attachment #8354344 - Flags: review+
Depends on: 955502
Depends on: 955505
Depends on: 955506
Depends on: 955515
Depends on: 955518
Depends on: 955545
Depends on: 955554
Blocks: 955013
No longer depends on: 955483, 955506, 955518
You need to log in before you can comment on or make changes to this bug.