Closed Bug 955543 Opened 6 years ago Closed 6 years ago

Tags Service Should Provide a Default Contact Group

Categories

(Chat Core :: General, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: qheaden, Assigned: qheaden)

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 2105 at 2013-08-13 03:40:00 UTC ***

Throughout the Instantbird code we find a default contact group defined as "Contacts". Since this is used by a few JS prpls, we should define this on tags service so that it can be accessed using Services.tags.
Attached patch Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2105 as attmnt 2717 at 2013-08-14 18:41:00 UTC ***

This patch adds default tag functionality to the tags service, and changes JS-XMPP and JS-Yahoo to make use if it.
Attachment #8354486 - Flags: review?(clokep)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
Comment on attachment 8354486 [details] [diff] [review]
Patch 1

*** Original change on bio 2105 attmnt 2717 at 2013-08-14 19:20:21 UTC ***

>diff --git a/chat/components/src/imContacts.js b/chat/components/src/imContacts.js
>@@ -135,16 +139,20 @@ TagsService.prototype = {
>+  // Get the default tag.
>+  getDefaultTag: function() {
>+    return this.createTag(_("defaultGroup"));
>+  },
getDefaultTag: function() this.createTag(_("defaultGroup"))

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>     else {
>-      let tagName = _("defaultGroup");
>+      let tagName;
>       for each (let group in aItem.getChildren("group")) {
>         let name = group.innerText;
>         if (name) {
>           tagName = name;
>           break; // TODO we should create an accountBuddy per group,
>                  // but this._buddies would probably not like that...
>         }
>       }
>-      let tag = Services.tags.createTag(tagName);
>+      // If the buddy is associated with a group, use that group name. Otherwise
>+      // use the default group.
>+      let tag = tagName ? Services.tags.createTag(tagName) :
>+                          Services.tags.getDefaultTag();
>       buddy = new this._accountBuddyConstructor(this, null, tag, jid);
>     }
I think this can be done in a better way, do let tag = defaultTag(), then if we find name set the tag to be this based off the tagname and break. That should clean this up a bit.

>         if (!this._account.hasBuddy(this.userName)) {
>-          let tag = Services.tags.createTag(_("buddy.auth.defaultGroup"));
>+          let tag = Services.tags.getDefaultTag();
>           this._account.addBuddy(tag, this.userName);
Do we need to save tag as a variable?
Attachment #8354486 - Flags: review?(clokep) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2105 as attmnt 2718 at 2013-08-14 19:30:00 UTC ***

This addresses all of the feedback given in the last review.
Attachment #8354487 - Flags: review?(clokep)
Comment on attachment 8354486 [details] [diff] [review]
Patch 1

*** Original change on bio 2105 attmnt 2717 at 2013-08-14 19:30:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354486 - Attachment is obsolete: true
Comment on attachment 8354487 [details] [diff] [review]
Patch 2

*** Original change on bio 2105 attmnt 2718 at 2013-08-14 20:06:53 UTC ***

Looks good to me, I'd like Florian to look over this briefly.
Attachment #8354487 - Flags: review?(clokep) → review+
Attachment #8354487 - Flags: review?(florian)
Attached patch Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2105 as attmnt 2719 at 2013-08-15 08:13:00 UTC ***

This patch addresses the issues Florian pointed out in v2 during an IRC conversation. First, I substituted the getDefaultTag() method with the more compact defaultTag attribute. Second, I stopped using the instantbird localization file in chat/ code, but I created a localization file local to the chat/ folder. Finally, I simplified some code in addbuddy.js.

Since Florian spotted these needed changes, I've set him as the reviewer on this version.
Attachment #8354488 - Flags: review?(florian)
Comment on attachment 8354487 [details] [diff] [review]
Patch 2

*** Original change on bio 2105 attmnt 2718 at 2013-08-15 08:13:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354487 - Attachment is obsolete: true
Attachment #8354487 - Flags: review?(florian)
Comment on attachment 8354488 [details] [diff] [review]
Patch 3

*** Original change on bio 2105 attmnt 2719 at 2013-08-16 10:46:30 UTC ***

> interface imITagsService: nsISupports {
>+  readonly attribute imITag defaultTag;

I think after saying I disliked the comment you had in the previous patch, I suggested a better comment to go with this. Did you not like it?

>diff --git a/chat/components/src/imContacts.js b/chat/components/src/imContacts.js
>--- a/chat/components/src/imContacts.js
>+++ b/chat/components/src/imContacts.js

>+XPCOMUtils.defineLazyGetter(this, "_", function()
>+  l10nHelper("chrome://chat/locale/tags.properties")
>+);

Shouldn't the file rather be named contacts.properties, as it's used for the imContacts.js file?


>diff --git a/chat/locales/en-US/tags.properties b/chat/locales/en-US/tags.properties
>new file mode 100644
>--- /dev/null
>+++ b/chat/locales/en-US/tags.properties
>@@ -0,0 +1,5 @@
>+# 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/.
>+
>+defaultGroup=Contacts

This needs a localization note explaining how the string will be used.

I suggest "This is the name of the group that will automatically be created when adding a buddy without specifying a group."


>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm

>-      let tagName = _("defaultGroup");
>+      // Use the default tag if no group name was found. Otherwise, use the
>+      // group name.
>+      let tag = Services.tags.defaultTag;

It's slightly annoying that this will create the default tag even if we don't need it.

I would suggest:
        let tag;

>       for each (let group in aItem.getChildren("group")) {
>         let name = group.innerText;
>         if (name) {
>-          tagName = name;
>+          tag = Services.tags.createTag(name);
>           break; // TODO we should create an accountBuddy per group,
>                  // but this._buddies would probably not like that...
>         }
>       }
>-      let tag = Services.tags.createTag(tagName);
>       buddy = new this._accountBuddyConstructor(this, null, tag, jid);

and here:
        buddy = new this._accountBuddyConstructor(this, null,
                                                  tag || Services.tags.defaultTag, jid);


>diff --git a/instantbird/content/addbuddy.js b/instantbird/content/addbuddy.js
>--- a/instantbird/content/addbuddy.js
>+++ b/instantbird/content/addbuddy.js
>@@ -26,17 +26,17 @@ var addBuddy = {
>     accountList.selectedIndex = 0;
>   },
> 
>   buildTagList: function ab_buildTagList() {
>     var tagList = document.getElementById("taglist");
>     let tags = Services.tags.getTags();
>     if (!tags.length) {
>       let bundle = document.getElementById("instantbirdBundle");
>-      tags.push(Services.tags.createTag(bundle.getString("defaultGroup")));
>+      tags.push(Services.tags.defaultTag);

Is http://lxr.instantbird.org/instantbird/source/instantbird/locales/en-US/chrome/instantbird/instantbird.properties#14 still used anywhere?




Note: now that we are special casing the default group, I think we could also special case the way that group is stored, so that name of that group is correctly changed in the UI if the user starts a localized Instantbird after using a en-US one. Currently this would create another 'default' group with the localized version "Contacts" string. This doesn't sound very difficult, but the migration path for existing users isn't trivial, so I don't think we should bother with this at this point. The changes in this patch are already a good improvement, and I think it can land once the few details I mentioned above are fixed.
Attachment #8354488 - Flags: review?(florian) → review-
Attached patch Patch 4 (obsolete) — Splinter Review
*** Original post on bio 2105 as attmnt 2721 at 2013-08-16 15:39:00 UTC ***

This patch addresses the feedback given in the previous review.

> Is
> http://lxr.instantbird.org/instantbird/source/instantbird/locales/en-US/chrome/instantbird/instantbird.properties#14
> still used anywhere?

A look at LXR shows no other use of it in the code. http://lxr.instantbird.org/instantbird/search?string=defaultGroup
Attachment #8354490 - Flags: review?(florian)
Comment on attachment 8354488 [details] [diff] [review]
Patch 3

*** Original change on bio 2105 attmnt 2719 at 2013-08-16 15:39:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354488 - Attachment is obsolete: true
*** Original post on bio 2105 at 2013-08-16 15:40:28 UTC ***

Oops. I forgot to change the r= in the commit message. Don't forget to change that if you land it.
Comment on attachment 8354490 [details] [diff] [review]
Patch 4

*** Original change on bio 2105 attmnt 2721 at 2013-08-16 18:24:10 UTC ***

>+XPCOMUtils.defineLazyGetter(this, "_", function()
>+  l10nHelper("chrome://chat/locale/tags.properties")

You forgot to change this.

>+      buddy = new this._accountBuddyConstructor(this, null, tag ||
>+                                                Services.tags.defaultTag, jid);

The indent is wrong here, it looks like "tag" and "Services..." are 2 different parameters.
Attachment #8354490 - Flags: review?(florian) → review-
Attached patch Patch 5 (obsolete) — Splinter Review
*** Original post on bio 2105 as attmnt 2723 at 2013-08-16 18:35:00 UTC ***

Made fixes to the two issues from last review.
Attachment #8354492 - Flags: review?(florian)
Comment on attachment 8354490 [details] [diff] [review]
Patch 4

*** Original change on bio 2105 attmnt 2721 at 2013-08-16 18:35:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354490 - Attachment is obsolete: true
Comment on attachment 8354492 [details] [diff] [review]
Patch 5

*** Original change on bio 2105 attmnt 2723 at 2013-08-16 20:55:33 UTC ***

Looks good. I just have 2 trivial nits, sorry for not spotting them earlier.

>diff --git a/chat/locales/jar.mn b/chat/locales/jar.mn
>--- a/chat/locales/jar.mn
>+++ b/chat/locales/jar.mn
>@@ -11,8 +11,9 @@
> 	locale/@AB_CD@/chat/conversations.properties (%conversations.properties)
> 	locale/@AB_CD@/chat/facebook.properties	(%facebook.properties)
> 	locale/@AB_CD@/chat/irc.properties	(%irc.properties)
> 	locale/@AB_CD@/chat/logger.properties (%logger.properties)
> 	locale/@AB_CD@/chat/status.properties	(%status.properties)
> 	locale/@AB_CD@/chat/twitter.properties	(%twitter.properties)
> 	locale/@AB_CD@/chat/xmpp.properties	(%xmpp.properties)
> 	locale/@AB_CD@/chat/yahoo.properties	(%yahoo.properties)
>+	locale/@AB_CD@/chat/contacts.properties	(%contacts.properties)

It looks like this list of files was sorted alphabetically.

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm

>+      // Use the default tag if no group name was found. Otherwise, use the
>+      // group name.

This comment doesn't add any value. Reading it takes longer than reading "tag || Services.tags.defaultTag", and when there's a comment above a block of code, I typically assume that reading the comment gives enough of an understanding to skip reading the block of code after it until an empty line or the next comment. This isn't true here.

I think I would just remove the comment, the code seems pretty self explanatory here.

>+      buddy = new this._accountBuddyConstructor(this, null,
>+                                                tag || Services.tags.defaultTag,
>+                                                jid);

If you agree with these 2 changes, I can do them when doing the checkin (but if you can quickly attach an updated patch, it will be appreciated :-)).
Attachment #8354492 - Flags: review?(florian) → review+
Attached patch Patch 6Splinter Review
*** Original post on bio 2105 as attmnt 2724 at 2013-08-16 21:01:00 UTC ***

I agree with the changes. :)
Attachment #8354493 - Flags: review?(florian)
Comment on attachment 8354492 [details] [diff] [review]
Patch 5

*** Original change on bio 2105 attmnt 2723 at 2013-08-16 21:01:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354492 - Attachment is obsolete: true
Comment on attachment 8354493 [details] [diff] [review]
Patch 6

*** Original change on bio 2105 attmnt 2724 at 2013-08-16 21:03:03 UTC ***

(In reply to comment #12)
> Created attachment 8354493 [details] [diff] [review] (bio-attmnt 2724) [details]
> Patch 6
> 
> I agree with the changes. :)

Thanks for the new patch. You could have just carried the r+ forward, and marked the bug checkin-needed ;).
Attachment #8354493 - Flags: review?(florian) → review+
*** Original post on bio 2105 at 2013-08-17 00:05:05 UTC ***

http://hg.instantbird.org/instantbird/rev/900531a13dc4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
*** Original post on bio 2105 at 2013-08-19 13:03:54 UTC ***

(In reply to comment #6)
> Note: now that we are special casing the default group, I think we could also
> special case the way that group is stored, so that name of that group is
> correctly changed in the UI if the user starts a localized Instantbird after
> using a en-US one. Currently this would create another 'default' group with the
> localized version "Contacts" string. This doesn't sound very difficult, but the
> migration path for existing users isn't trivial, so I don't think we should
> bother with this at this point. The changes in this patch are already a good
> improvement, and I think it can land once the few details I mentioned above are
> fixed.

Quentin, please file a follow up for this.
You need to log in before you can comment on or make changes to this bug.