Closed Bug 955492 Opened 10 years ago Closed 10 years ago

New conversation tab needs to display open conversations and those on hold.

Categories

(Instantbird Graveyard :: Conversation, enhancement)

x86
Other
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

Attachments

(2 files, 8 obsolete files)

49.39 KB, patch
benediktp
: review+
Details | Diff | Splinter Review
1.23 KB, patch
clokep
: review+
Details | Diff | Splinter Review
*** Original post on bio 2055 at 2013-07-16 18:15:00 UTC ***

There needs to be an indication that a conversation is already open (e.g. a label saying "Switch to conversation". Also a way to resume conversations on hold including MUCs, and conversations with people not on the contact list.
*** Original post on bio 2055 as attmnt 2605 at 2013-07-16 18:17:00 UTC ***

This doesn't show open conversations (not on hold) yet, and needs way more comments, so I'm requesting feedback for this initial version.
Attachment #8354374 - Flags: feedback?(aleth)
Attachment #8354374 - Flags: feedback?(florian)
Attachment #8354374 - Flags: feedback?(benediktp)
Attachment #8354374 - Flags: feedback?(clokep)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Comment on attachment 8354374 [details] [diff] [review]
Move data management to a new service, add support for convs on hold including MUCs

*** Original change on bio 2055 attmnt 2605 at 2013-07-16 18:54:34 UTC ***

I've only given this a brief and partial look.

The indentation in the makefile changes seems wrong.

All those vars at the top of the prototype need comments.

I really dislike the fact that we are adding yet another set of *conv objects, this._conv's, and let conv = ... . We have too many already. It would be better if when conv occurs it referred to the existing conversation objects and the awesometab used something else for its listitems for differentiation.

I think we can take it as understood that the listitems are "possible conversations", so even something generic like "items" or "entries" would do.
There is no possibility of confusion as the awesometab only has one such list.
We can then improve readability by renaming _addContactConv -> _addContact etc.

Why do we need this._inited and the stats-service-inited notification? Can't we simply tell existing awesometabs to refresh (stats-service-convs-updated?) when each chunk of entries has been added?

>+ let conv = new PossibleConversation();
>+ conv.buildFromImContact(aImContact);

This pattern occurs a lot. Why not give the PossibleConversation constructor a parameter? "buildFrom" sounds like something a constructor should do. Maybe by having a generic PossibleConversation and then PossibleContactConversations etc that inherit from that and have the corresponding "buildFrom" in their constructor. This would also avoid having methods like "get imContact()" and the corresponding attributes for PossibleConversations where this does not apply.

(Of course with naming changes along the lines I suggested above it would be something like GenericNewtabItem, ContactNewtabItem, etc.)

Currently, open conversations in tabs don't seem to be handled. You should also check autojoined MUCs are added correctly (and even if the Hide AutoJoins add-on is installed ;) ).
Attachment #8354374 - Flags: feedback?(aleth) → feedback+
Comment on attachment 8354374 [details] [diff] [review]
Move data management to a new service, add support for convs on hold including MUCs

*** Original change on bio 2055 attmnt 2605 at 2013-07-16 19:17:40 UTC ***

>diff --git a/instantbird/components/Makefile.in b/instantbird/components/Makefile.in
>+MODULE = instantbird
What does this mean?

>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
>+  _kNotificationsToObserve: ["contact-added", "contact-removed", "contact-status-changed",
>+                             "contact-display-name-changed", "contact-no-longer-dummy",
>+                             "contact-preferred-buddy-changed", "contact-moved",
>+                             "ui-conversation-hidden", "showing-ui-conversation"],
This is kind of weird seeing a constant as an object property.

>+  _getPositionToInsert: function(aConv) {
>+    let end = this._convs.length;
>+    // Avoid the binary search loop if the _convs were already sorted.
>+    if (end != 0 &&
>+       this._sortComparator(aConv, this._convs[end - 1]) < 0) {
Reverse this statement and return early in this case.

>+  _sortComparator: function(aConvA, aConvB) {
>+    return (aConvB.statusType - aConvA.statusType) ||
>+      aConvA.lowerCaseName.localeCompare(aConvB.lowerCaseName);
Does || in this situation do what you want?

>+function PossibleConversation() {
>+  this._lowerCaseName = null;
>+  this._displayName = null;
>+  this._statusType = null;
>+  this._statusText = null;
>+  this._imContact = null;
>+  this._buddy = null;
>+  this._isChat = null;
>+  this._account = null;
Can't all this just be in the prototype?

>+}
>+
Nit: No space here.

>diff --git a/instantbird/components/ibIConvStatsService.idl b/instantbird/components/ibIConvStatsService.idl
>+interface ibIPossibleConversation: imIStatusInfo {
This seems pretty specific to conversations w/ contacts...

>diff --git a/instantbird/content/newtab.xml b/instantbird/content/newtab.xml
I didn't look at this.

Besides that, I agree with aleth's comments.
Attachment #8354374 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 2055 at 2013-07-16 21:37:27 UTC ***

(In reply to comment #2)

> I really dislike the fact that we are adding yet another set of *conv objects

I also dislike it.

> It would be better
> if when conv occurs it referred to the existing conversation objects and the
> awesometab used something else for its listitems for differentiation.

> I think we can take it as understood that the listitems are "possible
> conversations", so even something generic like "items" or "entries" would do.

Depending on what you exactly meant, I may or may not agree.
If the variable name is in a place where a short name like "conv" is explicit enough, then I see nothing wrong with using something generic like "item".

On the other hand, I think "PossibleConversation" is a better name for an interface/a service than "NewtabItem", as it refers to what the object represents, rather than how it can be used.

> Why do we need this._inited

Nit: If we need it (not sure given aleth's comment), 'inited' feels a bit wrong to me, as I think we used 'initialized' in other places for the same thing.
*** Original post on bio 2055 at 2013-07-16 21:42:23 UTC ***

(In reply to comment #3)

> >diff --git a/instantbird/components/Makefile.in b/instantbird/components/Makefile.in
> >+MODULE = instantbird
> What does this mean?

It's the name of the .xpt file that will be created once the .idl files in this folder are compiled.

The variable that actually decides this name is XPIDL_MODULE, but if it isn't set rules.mk fallbacks to MODULE.


> >diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
> >+  _kNotificationsToObserve: ["contact-added", "contact-removed", "contact-status-changed",
> >+                             "contact-display-name-changed", "contact-no-longer-dummy",
> >+                             "contact-preferred-buddy-changed", "contact-moved",
> >+                             "ui-conversation-hidden", "showing-ui-conversation"],
> This is kind of weird seeing a constant as an object property.

Having both a '_' and a 'k' prefix seems strange too. I would just move this out of the object and define it as
const kObservedNotifications = [...] at the top level.

> >+  _sortComparator: function(aConvA, aConvB) {
> >+    return (aConvB.statusType - aConvA.statusType) ||
> >+      aConvA.lowerCaseName.localeCompare(aConvB.lowerCaseName);
> Does || in this situation do what you want?

I think it does.


I agree with all the above comments I haven't quoted.
Comment on attachment 8354374 [details] [diff] [review]
Move data management to a new service, add support for convs on hold including MUCs

*** Original change on bio 2055 attmnt 2605 at 2013-07-16 22:33:46 UTC ***

Some feedback (not a full review):
- Observing "ui-conversation-hidden", "showing-ui-conversation" doesn't seem a great idea. I think you'll keep references to closed conversations if conversations on hold are closed withing being shown first.
Listening to "new-ui-conversation" and "ui-conversation-closed" instead looks like it would have less unintended consequences.

- I'm not fully happy about the approach here that means keeping references to everything (all contacts on all conversations) from a service, and doing work in the background, even when the user doesn't use the new tab.

The reason for having a service and keeping references from it to stuff is that computing all the values we need about contacts to do a decent ranking is way too expensive to be done on the fly when opening the new tab.

This reason doesn't apply to conversations. We only have a handfull of them (rarely more than 20) at a time, and the stuff we need about them is cheap to compute. So the approach I had in mind was to just call Services.conversations.getUIConversations() when we need to display data, sort the resulting list of existing conversations, and merge the resulted sorted list with the already sorted list of contacts, before returning the data.

Also, in the future I think we will want to keep references only to the N top ranked contacts (where N may be 20 or so). We want the awesometab to open quickly and populate immediately the visible items. It doesn't matter much if it takes half a second to display results the first time you type something in a filter box.

- You can merge the _addConv and the _getPositionToInsert (called only once) methods.

- ibIPossibleConversation shouldn't have both an imIContact and an imIAccountBuddy. After looking at the code, it looks like the imIContact one is the one you will most likely want to keep.
Attachment #8354374 - Flags: feedback?(florian) → feedback-
*** Original post on bio 2055 at 2013-07-16 22:36:05 UTC ***

(In reply to comment #6)

> if conversations on hold are closed withing being shown first.

I meant 'without' of course.
Comment on attachment 8354374 [details] [diff] [review]
Move data management to a new service, add support for convs on hold including MUCs

*** Original change on bio 2055 attmnt 2605 at 2013-07-17 08:48:12 UTC ***

As you've got lots of other feedback already, I concentrated a bit more (review like) on the code.

> diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js

> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Cc is unused.

> +  _contactConvsById: {},
> +  _hiddenContactConvsById: {},
> +  _uiConvsById: {},

I don't understand the reluctance to use maps when the desired behavior is that of a map :(

> +  _firstInitConditionDone: false,

This is never used, is it?

> +  getFilteredConvs: function(aFilterStr, aFilteredConvsCount) {

getFilteredConvs is called with "" later. What about setting a default value of "" for aFilterStr?

> +  addObserver: function(aObserver) {
> +    if (this._observers.indexOf(aObserver) == -1)
> +      this._observers.push(aObserver);
> +    if (this._inited)
> +      aObserver.observe(this, "stats-service-inited");

It seems strange to send a notification for something that might have happened much earlier. What about an initialized flag on the service that the consumers could check to see if they need to observe the stats-service-inited notification or if they could start using the service right away?

> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic == "profile-after-change")
> +      Services.obs.addObserver(this, "prpl-init", false);

Can't we use something like this in the constructor of the service?
if (!Services.core.initialized)
  Services.obs.addObserver(this, "prpl-init", false);
It seems to make more obvious what's going on: "When the service is initialized, check if the Core service is available and wait for the appropriate notification if not".

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

> -              <xul:image class="tagIcon" xbl:inherits="status"/>
> +              <xul:image anonid="tagIcon" class="tagIcon" xbl:inherits="status"/>

Are you going to use the status on the tag icon somehow?

>        <method name="build">

> +            document.getAnonymousElementByAttribute(this, "anonid", "tagIcon")
> +                    .collapsed = true;
> +            this.setAttribute("tags", aTarget.account.name);

The attribute name "tags" (and the anonid "tagIcon") doesn't seem right here anymore as the icon and label are used for either tags, the account name or "Switch to tab" (later in this function) now.

> +            this.setAttribute("protoIcon", aTarget.account.protocol.iconBaseURI + "icon.png");
> +            this.setAttribute("buddyIcon", "");

An appropriately sized MUC icon as flo suggested on IRC could be OK here. I also like your idea of a placeholder icon showing multiple users instead of only one though.
Attachment #8354374 - Flags: feedback?(benediktp) → feedback+
*** Original post on bio 2055 at 2013-07-17 09:41:32 UTC ***

(In reply to comment #4)
> > It would be better
> > if when conv occurs it referred to the existing conversation objects and the
> > awesometab used something else for its listitems for differentiation.
> 
> > I think we can take it as understood that the listitems are "possible
> > conversations", so even something generic like "items" or "entries" would do.
> 
> Depending on what you exactly meant, I may or may not agree.
> If the variable name is in a place where a short name like "conv" is explicit
> enough, then I see nothing wrong with using something generic like "item".
> 
> On the other hand, I think "PossibleConversation" is a better name for an
> interface/a service than "NewtabItem", as it refers to what the object
> represents, rather than how it can be used.

I'm not concerned about the interface/service names, but I'm concerned about variable and method names, which I think should not include "conv" or "conversation" when referring to listitems/'possible conversations'. Otherwise one would have to use "PossibleConv" as opposed to "ActuallyExistingConv" etc to be explicit and make things readable ;) (In some places in the code this is actually done (aPossibleConversation) which is OK I think, but a this._possibleConversations property seems a bit long compared to this.items or similar.)
*** Original post on bio 2055 at 2013-07-17 09:47:18 UTC ***

(In reply to comment #6)
> Also, in the future I think we will want to keep references only to the N top
> ranked contacts (where N may be 20 or so). We want the awesometab to open
> quickly and populate immediately the visible items. It doesn't matter much if
> it takes half a second to display results the first time you type something in
> a filter box.

I don't understand this comment :-S but maybe nhnt11 does...

> - ibIPossibleConversation shouldn't have both an imIContact and an
> imIAccountBuddy. After looking at the code, it looks like the imIContact one is
> the one you will most likely want to keep.

This issue might disappear when instead of just ibIPossibleConversation there are distinct classes/interfaces inheriting from that for the different types of listitems, as I suggested above.
Attached patch Address feedback (obsolete) — Splinter Review
*** Original post on bio 2055 as attmnt 2622 at 2013-07-21 16:00:00 UTC ***

I've spent quite some time trying to address all your feedback in a sensible way, while considering stuff that needs to be done in the future. I'm sorry for the delay due to personal problems.
Attachment #8354391 - Flags: review?(florian)
Attachment #8354391 - Flags: feedback?(clokep)
Attachment #8354391 - Flags: review?(benediktp)
Attachment #8354391 - Flags: review?(aleth)
Comment on attachment 8354374 [details] [diff] [review]
Move data management to a new service, add support for convs on hold including MUCs

*** Original change on bio 2055 attmnt 2605 at 2013-07-21 16:00:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354374 - Attachment is obsolete: true
Comment on attachment 8354391 [details] [diff] [review]
Address feedback

*** Original change on bio 2055 attmnt 2622 at 2013-07-22 13:16:29 UTC ***

My only real feedback is I'm not convinced that ibIPossibleConversation should inherit from imIStatusInfo, but if everyone else is OK with this, then continue on.
Attachment #8354391 - Flags: feedback?(clokep) → feedback+
Comment on attachment 8354391 [details] [diff] [review]
Address feedback

*** Original change on bio 2055 attmnt 2622 at 2013-07-22 16:15:37 UTC ***

Thanks for the many improvements in this patch :)

Regarding the naming bikeshedding: In prplIConversation.idl we have prplIConversation differentiated to prplIConvIM and prplIConvChat. So how about (in parallel with this) keeping PossibleConversation for the interface and using PossibleConvIM and PossibleConvChat (or, if you prefer, the shorter PossibleIM and PossibleChat) for contacts and MUCs respectively? ExistingConversation would cover existing conversations and chats.

>+function ConvStatsService() {}
>+ConvStatsService.prototype = {
>+  // Sorted list of contacts, stored as PossibleContacts.
>+  _possibleContacts: [],
>+  // PossibleContacts stored by id.
>+  _possibleContactsById: new Map(),

How about calling this simply _contacts and _contactsById ? _contacts would then be a list of PossibleConvIMs. Calling this _convIMs is not future-proof as you will later have to handle possible conversations with people who are not imContacts, and they won't be in this list, right? Or will this be a list of all currently cached entries - in which case they won't all be contacts? 

>+  _initContacts: function() {
>+    let contacts = Services.contacts.getContacts();

I think we can take this for now (once the ranking data is added, we can hopefully do without adding all the contacts on startup).

>+  _addPossibleContact: function(aContact) {

_addContact is enough.

>+  _removePossibleContact: function(aId) {

Move this below addContact.

>+  addObserver: function(aObserver) {
>+    if (this._observers.indexOf(aObserver) == -1)
>+      this._observers.push(aObserver);
>+  },
>+
>+  removeObserver: function(aObserver) {
>+    this._observers = this._observers.filter(function(o) o !== aObserver);
>+  },

I realize this is just standard copypasted code, so you don't _have_ to change it, but I can't resist pointing out that this._observers really wants to be a Set ;) 

>+    if (!this.hasOwnProperty("_observers"))
>+      return;

You could avoid this by initializing it in the constructor.

When an account connects or disconnects while an awesometab is open, do we send a data-stale notification to the awesometab for each contact going offline (possibly causing it to repaint much more often than needed) or...? 

>+  createConversation: function() this.contact.createConversation(),

createConversation: this.contact.createConversation,
should work and is shorter ;)

>+  get statusType() this._statusType,
>+  get statusText() this._statusText,

Is there enough duplication like this that it would be better to inherit the shared stuff from a JS PossibleConversation object?

>+++ b/instantbird/components/ibIConvStatsService.idl

Needs more comments ;)

>+interface ibIPossibleConversation: imIStatusInfo {

If you do stick with inheriting from StatusInfo, explain the nonstandard way in which you are using some of those entries for MUCs.
Attachment #8354391 - Flags: review?(aleth) → feedback+
Attached patch Address comments (obsolete) — Splinter Review
*** Original post on bio 2055 as attmnt 2625 at 2013-07-22 20:01:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354394 - Flags: review?(florian)
Attachment #8354394 - Flags: review?(benediktp)
Attachment #8354394 - Flags: review?(aleth)
Comment on attachment 8354391 [details] [diff] [review]
Address feedback

*** Original change on bio 2055 attmnt 2622 at 2013-07-22 20:01:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354391 - Attachment is obsolete: true
Attachment #8354391 - Flags: review?(florian)
Attachment #8354391 - Flags: review?(benediktp)
Comment on attachment 8354394 [details] [diff] [review]
Address comments

*** Original change on bio 2055 attmnt 2625 at 2013-07-23 22:48:58 UTC ***

Some coding style nits:

function Blah() {
  ...
}
Blah.prototype = {
  ...
}; <-- you missed that semicolon in a few places.


"} else {" should be:
}
else {



Unless you intend to filter on the info text, I don't think PossibleConvFromContact should compute the list of tags in the constructor. If it's only for display, isn't this something the UI could do from the imIContact, when actually displaying the item?


this._isChat = false; -> Initizalization to consts in the constructore are things that should be in the prototype.


In the ExistingConversation constructor, I see aUIConv.buddy on 2 consecutive lines. Time for a let buddy = ... variable ;).


this._initContacts() does a bunch of stuff that looks potentially expensive. I don't think it should be done immediately upon receiving the prpl-init notification. For now, could you please add an executeSoon there? I hope we can have something lazy once we have ranking in place.


It wasn't obvious to me why _forceRefresh was a field rather than an optional parameter (you explained to me on IRC), so this is likely worth adding a code comment somewhere.

Overall, this looks pretty good. Not r+'ing yet as I'm really tired and may be missing things that will be obvious after some sleep.
Attachment #8354394 - Flags: feedback+
Attached patch Address flo's feedback comments (obsolete) — Splinter Review
*** Original post on bio 2055 as attmnt 2628 at 2013-07-24 11:06:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354397 - Flags: review?(florian)
Attachment #8354397 - Flags: review?(benediktp)
Attachment #8354397 - Flags: review?(aleth)
Comment on attachment 8354394 [details] [diff] [review]
Address comments

*** Original change on bio 2055 attmnt 2625 at 2013-07-24 11:06:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354394 - Attachment is obsolete: true
Attachment #8354394 - Flags: review?(florian)
Attachment #8354394 - Flags: review?(benediktp)
Attachment #8354394 - Flags: review?(aleth)
Comment on attachment 8354397 [details] [diff] [review]
Address flo's feedback comments

*** Original change on bio 2055 attmnt 2628 at 2013-07-24 11:23:12 UTC ***

Why didn't you do

>+let PossibleConversation = {
_isChat: false,
>+  get isChat() this._isChat,

and then you don't need 

>+ PossibleConvFromContact.prototype = {
>+  get isChat() false,

>+++ b/instantbird/components/ibIConvStatsService.idl
>+  void addObserver(in nsIObserver aObserver);
>+  void removeObserver(in nsIObserver aObserver);
>+  // A stats-service-contact-updated notification is fired when a contact has
>+  // been updated and observers need to refresh.

Nit: Move the comment above the observer methods.

r+ with these changes :)
Attachment #8354397 - Flags: review?(aleth) → review-
Attached patch Address aleth's comments (obsolete) — Splinter Review
*** Original post on bio 2055 as attmnt 2629 at 2013-07-24 11:34:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354398 - Flags: review?(florian)
Attachment #8354398 - Flags: review?(benediktp)
Attachment #8354398 - Flags: review?(aleth)
Comment on attachment 8354397 [details] [diff] [review]
Address flo's feedback comments

*** Original change on bio 2055 attmnt 2628 at 2013-07-24 11:34:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354397 - Attachment is obsolete: true
Attachment #8354397 - Flags: review?(florian)
Attachment #8354397 - Flags: review?(benediktp)
Comment on attachment 8354398 [details] [diff] [review]
Address aleth's comments

*** Original change on bio 2055 attmnt 2629 at 2013-07-24 11:40:10 UTC ***

>+PossibleConvFromContact.prototype = {
>+  _isChat: false,

Is there a reason you didn't put this in the prototype instead? I don't like that the prototype has an isChat getter that tries to return an _isChat that may be undefined.
Attachment #8354398 - Flags: review?(aleth) → review-
*** Original post on bio 2055 at 2013-07-24 11:49:38 UTC ***

Ah, I misunderstood you. Do you think the other fields (_statusType, _statusText, etc) need defaults too?
*** Original post on bio 2055 at 2013-07-24 11:54:50 UTC ***

(In reply to comment #20)
> Ah, I misunderstood you. Do you think the other fields (_statusType,
> _statusText, etc) need defaults too?

We sometimes do this but I don't mind in this case because these fields, not being set to constants, will always be set by the constructors anyway. If you think that assumption is wrong then add defaults.
*** Original post on bio 2055 at 2013-07-24 11:56:59 UTC ***

Okay! I don't really have much of an opinion so I'll leave it how it is except for isChat.
*** Original post on bio 2055 as attmnt 2630 at 2013-07-24 12:30:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354399 - Flags: review?(florian)
Attachment #8354399 - Flags: review?(benediktp)
Attachment #8354399 - Flags: review?(aleth)
Comment on attachment 8354398 [details] [diff] [review]
Address aleth's comments

*** Original change on bio 2055 attmnt 2629 at 2013-07-24 12:30:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354398 - Attachment is obsolete: true
Attachment #8354398 - Flags: review?(florian)
Attachment #8354398 - Flags: review?(benediktp)
Comment on attachment 8354399 [details] [diff] [review]
Add default _isChat value, remove trailing hyphen from statuses without statusText.

*** Original change on bio 2055 attmnt 2630 at 2013-07-24 12:34:13 UTC ***

I think I've looked at this too often to spot anything else. Let's see what Mic says ;)
Attachment #8354399 - Flags: review?(aleth) → review+
*** Original post on bio 2055 as attmnt 2631 at 2013-07-24 17:17:00 UTC ***

Not r?'ing aleth and flo this time since aleth already r+'d and flo is on vacation.
Attachment #8354400 - Flags: review?(benediktp)
Comment on attachment 8354399 [details] [diff] [review]
Add default _isChat value, remove trailing hyphen from statuses without statusText.

*** Original change on bio 2055 attmnt 2630 at 2013-07-24 17:17:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354399 - Attachment is obsolete: true
Attachment #8354399 - Flags: review?(florian)
Attachment #8354399 - Flags: review?(benediktp)
Blocks: 955503
Comment on attachment 8354400 [details] [diff] [review]
Fix location of %endif in newtab.css

*** Original change on bio 2055 attmnt 2631 at 2013-07-25 22:15:23 UTC ***

Very nice in general :)

> diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js
> 
> +  getFilteredConvs: function(aFilterStr, aFilteredConvsCount) {
> +    // Clone this._contacts to ensure it doesn't get modified
> +    // when we insert ui conversations.
> +    let filteredConvs = this._contacts.slice(0);
> +    let existingConvs = Services.conversations.getUIConversations().map(
> +                          function(uiConv) new ExistingConversation(uiConv));
> +    for (let existingConv of existingConvs) {
> +      let pos = this._getPositionToInsert(existingConv, filteredConvs);
> +      filteredConvs.splice(pos, 0, existingConv);
> +      // Remove the contact to prevent a duplicate.
> +      let contact = existingConv.uiConv.contact;
> +      if (contact && this._contactsById.has(contact.id)) {
> +        filteredConvs.splice(
> +          filteredConvs.indexOf(this._contactsById.get(contact.id)), 1);
> +      }
> +    }
> +    if (aFilterStr) {
> +      aFilterStr = aFilterStr.toLowerCase();
> +      filteredConvs = filteredConvs.filter(function(c) {
> +        return c.lowerCaseName.startsWith(aFilterStr) ||
> +          c.lowerCaseName.split(/\s+/).some(function(s) s.startsWith(aFilterStr));
> +      });
> +    }

Just a comment here: you could filter the arrays first and merge only the results then to avoid having to insert more elements than you will need eventually. This might not be relevant at the moment but could be once you've got the suggestions for MUCs working and need to insert them here?

> +let PossibleConversation = {
> 
> +  get lowerCaseName() {
> +    if (!this._lowerCaseName)
> +      this._lowerCaseName = this._displayName.toLowerCase();
> +    return this._lowerCaseName;
> +  },

Wouldn't a lazy getter be simpler here? It would be nice if you'd address this before this gets checked in. Please set [checkin-needed] on the whiteboard and add a comment that the new patch is also approved already. Thanks! :)

get lowerCaseName() {
  delete this.lowerCaseName;
  return this.lowerCaseName = this._displayName.toLowerCase();
},
Attachment #8354400 - Flags: review?(benediktp) → review+
Attached patch Mic's lazy getter suggestion (obsolete) — Splinter Review
*** Original post on bio 2055 as attmnt 2644 at 2013-07-25 22:33:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354413 - Flags: review?(benediktp)
Comment on attachment 8354400 [details] [diff] [review]
Fix location of %endif in newtab.css

*** Original change on bio 2055 attmnt 2631 at 2013-07-25 22:33:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354400 - Attachment is obsolete: true
*** Original post on bio 2055 as attmnt 2645 at 2013-07-25 23:04:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354414 - Flags: review?(benediktp)
Comment on attachment 8354413 [details] [diff] [review]
Mic's lazy getter suggestion

*** Original change on bio 2055 attmnt 2644 at 2013-07-25 23:04:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354413 - Attachment is obsolete: true
Attachment #8354413 - Flags: review?(benediktp)
*** Original post on bio 2055 at 2013-07-26 10:42:45 UTC ***

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

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Comment on attachment 8354414 [details] [diff] [review]
Ensure this builds on Windows (jar.mn filename mistake)

*** Original change on bio 2055 attmnt 2645 at 2013-07-26 14:43:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354414 - Flags: review?(benediktp) → review+
*** Original post on bio 2055 as attmnt 2646 at 2013-07-27 10:00:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354415 - Flags: review?(benediktp)
Comment on attachment 8354415 [details] [diff] [review]
Fix breakage, add instantbird.xpt to package-manifest.in

*** Original change on bio 2055 attmnt 2646 at 2013-07-27 10:24:39 UTC ***

Background is that the new interfaces aren't defined in the latest nightly [1] while it worked for nhnt11 and clokep when they've built themselves.
I'm not familiar with building and packaging and would like if either clokep or flo could have a look at this.


[1] Trying to access the entries in Components.interfaces:

Timestamp: 27.07.2013 12:20:23
Warning: ReferenceError: reference to undefined property Components.interfaces.ibIConvStatsService
Source File: javascript:%20Components.interfaces.ibIConvStatsService
Line: 1

Timestamp: 27.07.2013 12:20:58
Warning: ReferenceError: reference to undefined property Components.interfaces.ibIPossibleConversation
Source File: javascript:%20Components.interfaces.ibIPossibleConversation
Line: 1
Attachment #8354415 - Flags: review?(benediktp) → review?(clokep)
Attachment #8354415 - Flags: review?(florian)
*** Original post on bio 2055 at 2013-07-27 12:20:00 UTC ***

I asked about this on #developers, and got a positive reply about the solution in my patch (http://logbot.glob.com.au/?c=mozilla%23developers#c707345)
Comment on attachment 8354415 [details] [diff] [review]
Fix breakage, add instantbird.xpt to package-manifest.in

*** Original change on bio 2055 attmnt 2646 at 2013-07-27 16:09:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354415 - Flags: review?(florian)
Attachment #8354415 - Flags: review?(clokep)
Attachment #8354415 - Flags: review+
*** Original post on bio 2055 at 2013-07-27 16:24:44 UTC ***

Packaging fix: http://hg.instantbird.org/instantbird/rev/d069c5374c3f
Depends on: 955512
Depends on: 955513
Depends on: 955514
Depends on: 955519
Depends on: 955563
You need to log in before you can comment on or make changes to this bug.