Closed Bug 955000 Opened 6 years ago Closed 5 years ago

Add someone as a buddy directly from an open conversation

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1569 at 2012-07-03 14:32:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1569 as attmnt 1729 at 2012-07-03 14:32:00 UTC ***

This WIP adds a context menu entry to the tab/conv-top from which one can add the current conversation partner as a buddy.

I think this is wanted, e.g. for IRC DMs and for protocols which don't automatically send authorisation requests when someone else initiates a conversation.

Would work well if bug 954979 (bio 1547) was fixed and adding a buddy was reported back to the various conversation objects.
Attachment #8353488 - Flags: feedback?(clokep)
Depends on: 954979
Whiteboard: [has-strings]
*** Original post on bio 1569 at 2012-07-04 11:37:59 UTC ***

(In reply to comment #0)
> I think this is wanted, e.g. for IRC DMs and for protocols which don't
> automatically send authorisation requests when someone else initiates a
> conversation.

Just to add, I didn't realize the issue here is rather bug 953504 (bio 53).
*** Original post on bio 1569 at 2012-07-04 12:08:20 UTC ***

(In reply to comment #0)
> Would work well if bug 954979 (bio 1547) was fixed and adding a buddy was reported back to
> the various conversation objects.

Bug 954979 (bio 1547) now has a patch waiting, so these can be tested together.
Comment on attachment 8353488 [details] [diff] [review]
Patch

*** Original change on bio 1569 attmnt 1729 at 2012-07-17 14:06:45 UTC ***

>diff --git a/chrome/en-US/locale/en-US/instantbird/tabbrowser.dtd b/chrome/en-US/locale/en-US/instantbird/tabbrowser.dtd
> <!ENTITY  closeOtherTabs.label     "Close Other Tabs">
>-<!ENTITY  openTabInNewWindow.label     "Open in a New Window">
>+<!ENTITY  openTabInNewWindow.label     "Move to New Window">
> <!ENTITY  openTabInNewWindow.accesskey "W">
>+<!ENTITY  addAsBuddyToTag.label     "Add as Buddy to Tag">
Why not just "Add Buddy"? (And shouldn't we use "Contact" not "Buddy"?)

>+<!ENTITY  addAsBuddyToTag.accesskey "B">
>+<!ENTITY  addNewTagCmd.label     "Add New Tag...">
>+<!ENTITY  addNewTagCmd.accesskey "N">

>diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml
>+     <method name="addAsBuddyToTag">
>+         if (this._conv.isChat || this._conv.buddy || !this._conv.account.connected)
>+           return;
>+         this._conv.account.addBuddy(aTag, this._conv.title);
I think we could use a comment.

>@@ -193,36 +206,101 @@
>       <method name="updatePopupMenu">

These Boolean statements need comments above them.
>+            document.getElementById("context_addAsBuddyToTag").hidden =
>+              !conv || conv.isChat || conv.buddy;
>+            document.getElementById("context_addAsBuddyToTag").disabled =
>+              !conv || !conv.account.connected;
>+            if (conv && conv.account.connected && !conv.isChat && !conv.buddy) {
>+              let popup = document.getElementById("context_tagsPopup");
>+              let item;
>+              while ((item = popup.firstChild) && item.localName != "menuseparator")
>+                popup.removeChild(item);
>+
>+              let tags = Services.tags.getTags();
>+              if (!tags.length) {
>+                let bundle =
>+                  document.getAnonymousElementByAttribute(this, "anonid", "instantbirdBundle");
>+                tags.push(Services.tags.createTag(bundle.getString("defaultGroup")));
>+              }
>+
>+              let sortFunction = function (a, b) {
>+                let [a, b] = [a.name.toLowerCase(), b.name.toLowerCase()];
>+                return a < b ? 1 : a > b ? -1 : 0;
>+              };
>+              tags.sort(sortFunction).forEach(function (aTag) {
>+                item = document.createElement("menuitem");
>+                item.setAttribute("label", aTag.name);
>+                item.groupId = aTag.id;
>+                popup.insertBefore(item, popup.firstChild);
>+              });
This looks awfully similar to http://lxr.instantbird.org/instantbird/source/instantbird/content/blist.js#152 is there any way we can not duplicate the code?

>+      <method name="addAsBuddyToTag">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            let id = aEvent.originalTarget.groupId;
>+            if (!id)
>+              return;
>+            let tag = Services.tags.getTagById(id);
>+            this.mContextTab.linkedConversation.addAsBuddyToTag(tag);
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="addAsBuddyToNewTag">
>+        <body>
>+          <![CDATA[
>+            let bundle =
>+              document.getAnonymousElementByAttribute(this, "anonid", "instantbirdBundle").stringBundle;
>+            let title = bundle.GetStringFromName("newTagPromptTitle");
>+            let message = bundle.GetStringFromName("newTagPromptMessage");
>+            let name = {};
>+            if (!Services.prompt.prompt(window, title, message, name, null,
>+                                        {value: false}) || !name.value)
>+              return; // the user canceled
This opens a dialog? That's pretty gross, but I guess that's how we do it in other places too. Is this also duplicated from somewhere?
Attachment #8353488 - Flags: feedback?(clokep) → feedback+
*** Original post on bio 1569 at 2012-07-17 14:41:46 UTC ***

(In reply to comment #3)
> >+<!ENTITY  addAsBuddyToTag.label     "Add as Buddy to Tag">
> Why not just "Add Buddy"? (And shouldn't we use "Contact" not "Buddy"?)

We currently use "Buddy" in the main menu etc. It should be consistent with that.

I've never been completely clear about what the distinction was supposed to be (apart from internally, where 'contacts' allow for merged 'buddies').
 
> This looks awfully similar to
> http://lxr.instantbird.org/instantbird/source/instantbird/content/blist.js#152
> is there any way we can not duplicate the code?

Yes, it's adapted from there. The way not to duplicate would be to separate out the tag context menu into a separate file I suppose and use some additional if clauses as the actions differ of course. Then it could be included in both cases. I'm not sure at the moment how amenable the blist code is to that. It's the main reason I asked for feedback (i.e. is this feature wanted in this form). 

Potential benefit would be allowing a Tags... context menu entry in the tab context menu for existing buddies as well.

> >+      <method name="addAsBuddyToNewTag">
> >+        <body>
> >+          <![CDATA[
> >+            let bundle =
> >+              document.getAnonymousElementByAttribute(this, "anonid", "instantbirdBundle").stringBundle;
> >+            let title = bundle.GetStringFromName("newTagPromptTitle");
> >+            let message = bundle.GetStringFromName("newTagPromptMessage");
> >+            let name = {};
> >+            if (!Services.prompt.prompt(window, title, message, name, null,
> >+                                        {value: false}) || !name.value)
> >+              return; // the user canceled
> This opens a dialog? That's pretty gross, but I guess that's how we do it in
> other places too. Is this also duplicated from somewhere?

Of course, it's what happens in the corresponding blist context menu as well.
In this case, the dialog is even acceptable (where else would you enter the tag name).
Attached patch 955000.patch (obsolete) — Splinter Review
There's been a lot of changes in the surrounding code, so the previous comments on this bug can be safely ignored.

The patch is longer than you might expect as I removed some remaining duplication around the tab submenu, which is now generated entirely from JS.
Assignee: nobody → aleth
Attachment #8353488 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8409735 - Flags: review?(florian)
Comment on attachment 8409735 [details] [diff] [review]
955000.patch

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

::: im/content/conversation.xml
@@ +1578,5 @@
> +              menu.setAttribute("label", bundle.GetStringFromName(id + ".label"));
> +              menu.setAttribute("accesskey",
> +                                bundle.GetStringFromName(id + ".accesskey"));
> +              let addContact =
> +                aTag => this._conv.account.addBuddy(aTag, this._conv.title);

Did you mean .name here rather than .title?

@@ +1579,5 @@
> +              menu.setAttribute("accesskey",
> +                                bundle.GetStringFromName(id + ".accesskey"));
> +              let addContact =
> +                aTag => this._conv.account.addBuddy(aTag, this._conv.title);
> +              menu.onDOMInsert = function() {

Can't this use a standard popupshowing event?

The "onDOMInsert" name here looks a lot like you are using a (deprecated) mutation event.

@@ +1591,3 @@
>              let showLogsItem = createMenuItem("contextShowLogs", function() conv.showLogs());
> +            if (!this.hasLogs())
> +              showLogsItem.setAttribute("disabled", "true");

Why is that change needed?

::: im/modules/ibTagMenu.jsm
@@ +20,2 @@
>    this.target = aTarget;
> +  this.onAddTag = aOnAddTag;

How would you feel about doing this.onAddTag = aOnAddTag || aOnTag; here and making that parameter optional too?

@@ +21,5 @@
> +  this.onAddTag = aOnAddTag;
> +  this.onTag = aOnTag;
> +
> +  // Set up the tag menu at the menu element specified by aMenuId.
> +  let menu = this.document.getElementById(aMenuId);

I would be tempted by |let document = this.document;| just to make the rest of the code look more familiar.

@@ +24,5 @@
> +  // Set up the tag menu at the menu element specified by aMenuId.
> +  let menu = this.document.getElementById(aMenuId);
> +  let popup = menu.firstChild;
> +  if (popup)
> +    popup.remove();

Is this constructor called each time a context menu is shown? If so, shouldn't this cleanup code be called when the menu is closed rather than when we open a menu again?

@@ +26,5 @@
> +  let popup = menu.firstChild;
> +  if (popup)
> +    popup.remove();
> +  popup = this.document.createElement("menupopup");
> +  menu.appendChild(popup);

If I was writing this code, I would probably move this line to the end of the function, so that all the elements are inserted to the document at once. I don't think it actually makes a real difference here though.

@@ +41,4 @@
>  }
>  TagMenu.prototype = {
> +  tagsPopupShowing: function(aEvent) {
> +    aEvent.stopPropagation();

Why is this needed? Probably could do with a comment.

@@ +79,5 @@
>      if (!id)
>        return false;
>  
>      try {
> +      return this.onTag(Services.tags.getTagById(id));

Could we use this.onTag.call here to avoid the bind calls you have in a few callers?

@@ +95,5 @@
>        return false; // the user canceled
>  
>      try {
>        // If the tag already exists, createTag will return it.
> +      return this.onAddTag(Services.tags.createTag(name.value));

Same here.
Attachment #8409735 - Flags: review?(florian) → review-
Comment on attachment 8409735 [details] [diff] [review]
955000.patch

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

::: im/content/conversation.xml
@@ +1578,5 @@
> +              menu.setAttribute("label", bundle.GetStringFromName(id + ".label"));
> +              menu.setAttribute("accesskey",
> +                                bundle.GetStringFromName(id + ".accesskey"));
> +              let addContact =
> +                aTag => this._conv.account.addBuddy(aTag, this._conv.title);

Yes, that's better, thanks.

@@ +1579,5 @@
> +              menu.setAttribute("accesskey",
> +                                bundle.GetStringFromName(id + ".accesskey"));
> +              let addContact =
> +                aTag => this._conv.account.addBuddy(aTag, this._conv.title);
> +              menu.onDOMInsert = function() {

It's not an event at all. I'll rename it so it doesn't sound like an event handler.

The function is called from the popupshowing handler (in tabbrowser.xml).

@@ +1591,3 @@
>              let showLogsItem = createMenuItem("contextShowLogs", function() conv.showLogs());
> +            if (!this.hasLogs())
> +              showLogsItem.setAttribute("disabled", "true");

This is a drive-by bugfix - the JS attribute doesn't get mapped to the DOM if the binding isn't attached yet.

::: im/modules/ibTagMenu.jsm
@@ +20,2 @@
>    this.target = aTarget;
> +  this.onAddTag = aOnAddTag;

I prefer keeping the actions explicit.

@@ +24,5 @@
> +  // Set up the tag menu at the menu element specified by aMenuId.
> +  let menu = this.document.getElementById(aMenuId);
> +  let popup = menu.firstChild;
> +  if (popup)
> +    popup.remove();

No, it's possible for the constructor to be called only once, but then to have multiple popupshowing/hidings.

@@ +79,5 @@
>      if (!id)
>        return false;
>  
>      try {
> +      return this.onTag(Services.tags.getTagById(id));

Good idea!
Attached patch 955000.patch 2 (obsolete) — Splinter Review
Attachment #8409735 - Attachment is obsolete: true
Attachment #8440293 - Flags: review?(florian)
Comment on attachment 8440293 [details] [diff] [review]
955000.patch 2

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

::: im/content/nsContextMenu.js
@@ +181,3 @@
>      addAction("AddContact", isAddContact);
> +    if (isAddContact) {
> +      let nickAddContact = this.nickAddContact.bind(this);

weren't you intending to remove this line?

::: im/modules/ibTagMenu.jsm
@@ +11,5 @@
>  XPCOMUtils.defineLazyGetter(this, "_", function()
>    l10nHelper("chrome://instantbird/locale/instantbird.properties")
>  );
>  
> +// When aOnTag and aOnAddTag are called, this will be set to aParent.

I think you can rephrase this for better readability. Either quote the word 'this', or something like:
aOnTag and aOnAddTag will be called with aParent as the this value.

@@ +29,5 @@
> +  if (popup)
> +    popup.remove();
> +  popup = document.createElement("menupopup");
> +  this.popup = popup;
> +  popup.addEventListener("command", this.tag.bind(this));

Could you replace this:
popup.addEventListener("command", this);
and implement:
 handleEvent(aEvent) {
   switch (aEvent.name) {
     ...
   }
 }
in the prototype?

This would save us all these .bind calls (each of them creates a new function that stays around).
Attached patch 955000.patch 3 (obsolete) — Splinter Review
Attachment #8440293 - Attachment is obsolete: true
Attachment #8440293 - Flags: review?(florian)
Attachment #8440302 - Flags: review?(florian)
Comment on attachment 8440302 [details] [diff] [review]
955000.patch 3

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

Thanks, and sorry the review took so long!
Attachment #8440302 - Flags: review?(florian) → review+
Attached patch 955000.patch 4Splinter Review
Some simplification at the cost of stopPropagating in one instance where it's not really needed.
Attachment #8440302 - Attachment is obsolete: true
Attachment #8440314 - Flags: review?(florian)
Attachment #8440314 - Flags: review?(florian) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0f438f2411e7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has-strings]
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.