Closed Bug 954064 Opened 10 years ago Closed 10 years ago

Unable to accept IRC invite

Categories

(Chat Core :: General, defect)

Product:
Chat Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: clokep, Assigned: florian)

References

Details

Attachments

(2 files, 8 obsolete files)

address most of comment 22
12.61 KB, patch
florian
: review-
Details | Diff | Splinter Review
js-irc patch to match attachment 514
5.52 KB, patch
clokep
: review+
Details | Diff | Splinter Review 
*** Original post on bio 628 at 2010-12-14 05:01:00 UTC ***

Currently we're unable to accept an invite of IRC and the following shows up in the error console:
> Error: Requesting action: (null) (Accept chat invitation?)
> Source File: http://hg.instantbird.org/instantbird/raw-file/d69795a2a97a/purple/libpurple/request.c
> Line: 1379
> Source Code:
> request: purple_request_action_varg

Perhaps a popup should occur (or a tab should open as if you received a message, i.e. it's red) and when you click it you have a prompt asking if you want to join the channel or not. (Perhaps similar to Mic's idea for bug 954050 (bio 614))
Blocks: 954011
*** Original post on bio 628 at 2010-12-14 11:06:23 UTC ***

(In reply to comment #0)
> Perhaps a popup should occur (or a tab should open as if you received a
> message, i.e. it's red) and when you click it you have a prompt asking if you
> want to join the channel or not. (Perhaps similar to Mic's idea for bug 954050 (bio 614))

I think we should make sure that the user can't be bothered with the same request frequently by the way.

Another thing: no matter how we display this invitation, we should make sure it goes well with cases like protected (+k) channels. That means we shouldn't prompt twice (once for the invitation, second to enter a password) but show only a single prompt which also asks to enter the key.
*** Original post on bio 628 at 2010-12-16 07:22:56 UTC ***

Please, no popup :).
I think we should either accept the invitation automatically if it comes from a trusted contact (hard to define notion, admittedly ...), or stack it somewhere in the buddy list like we will/should do for the requests when someone adds you do his buddy list.
*** Original post on bio 628 at 2010-12-16 09:25:47 UTC ***

(In reply to comment #2)
> Please, no popup :).
> I think we should either accept the invitation automatically if it comes from a
> trusted contact (hard to define notion, admittedly ...), or stack it somewhere
> in the buddy list like we will/should do for the requests when someone adds you
> do his buddy list.

Isn't defining someone as trusted especially difficult on IRC?

It's not clear to me if you saw this, so does "opening a tab with prompt in background" count as type of unwanted popup or would it be a valid solution (c.f. bug 954050 (bio 614) comment 1) ?
*** Original post on bio 628 at 2010-12-16 09:42:38 UTC ***

(In reply to comment #3)

> Isn't defining someone as trusted especially difficult on IRC?
Yes :(. We could try to define some rules, but they would be very hard to label explicitly for the users. Things like "I've exchanged messages with that person already", "This person is operator in a channel I auto-join", "This person has been talking a lot for a lot of time in channels where I am and it hasn't bothered me (= I haven't stopped frequenting that channel)" ... Nothing really clear :(.
 
> It's not clear to me if you saw this, so does "opening a tab with prompt in
> background" count as type of unwanted popup or would it be a valid solution
> (c.f. bug 954050 (bio 614) comment 1) ?

If you've already opened the conversation tab, why not just displaying the conversation in it?
*** Original post on bio 628 at 2010-12-16 09:59:19 UTC ***

(In reply to comment #4)
> (In reply to comment #3)
> 
> > Isn't defining someone as trusted especially difficult on IRC?
> Yes :(. We could try to define some rules, but they would be very hard to label
> explicitly for the users. Things like "I've exchanged messages with that person
> already", "This person is operator in a channel I auto-join", "This person has
> been talking a lot for a lot of time in channels where I am and it hasn't
> bothered me (= I haven't stopped frequenting that channel)" ... Nothing really
> clear :(.

I could imagine that the rules would be so specific that this case becomes a special case where suddently a channel is joined automatically and the user wonders what's going on (the rules are some invisible thing that the user doesn't know and even less sees that they really apply)?

> > It's not clear to me if you saw this, so does "opening a tab with prompt in
> > background" count as type of unwanted popup or would it be a valid solution
> > (c.f. bug 954050 (bio 614) comment 1) ?
> 
> If you've already opened the conversation tab, why not just displaying the
> conversation in it?

hmm, yes. Even though you can't know if the user really wants to appear in this channel. If the channel is automatically joined then there should be a way to disable that for the future (like a notification bar saying: "You have been invited to this channel. [It's OK.] [Ask everytime] [Ignore invitations to this channel]").

If they opt out of this behaviour we could fall back to
*** Original post on bio 628 at 2010-12-16 10:00:35 UTC ***

If they opt out of this automatic behaviour we could fall back to some sort of prompt.


(sorry for the bugspam)
Depends on: 954075
*** Original post on bio 628 as attmnt 458 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-30 01:23:00 UTC ***

This gets enough to send an observer notification on purpleICoreService when a request action happens.  Haven't done the UI part yet, since I haven't got a good design for that, but this at least makes things possible...

Not convinced the API is quite right, though, since nothing prevents multiple things from listening to the same topic...
*** Original post on bio 628 at 2010-12-30 11:35:41 UTC ***

I'm afraid the approach you are taking may be a lot of work for a somewhat disappointing result (I really don't see how the UI would know it's an invitation for a chat, so I don't see how the UI could be usable).

If you could forget for a few seconds the current libpurple request API, do you know which API you would design for this case?

If I understood correctly, you are trying to make the purple_request_accept_cancel call from serv_got_chat_invite work. Is there anything better we could do for example with the "chat-invited" signal? (we could change it a bit, or add a new signal if needed, or whatever...)
*** Original post on bio 628 at 2010-12-30 11:51:51 UTC ***

Yeah, I think we should not stick to the purple APIs. It might be a starting point but if the way they are conceived look disappointing, you can put a completely different API above it and do a proxy to clean up things.
*** Original post on bio 628 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2010-12-31 00:15:06 UTC ***

Ah, I had somehow missed the fact that there was a "chat-invited" signal since the console spam was so easy to spot :D

All we really need here for the specific case of IRC invites is the connection and the name of the channel (and ideally the name of the person doing the invite), so the signal should be enough.  It looks like there's possibly a message too - presumably for non-IRC chat invites?

So yeah, I don't think we need to touch the purple side.  Probably make purpleIAccount have observers and fire it on that, or something?  Could just send the various params (all text) as a property bag, since we no longer need to deal with callback functions...
*** Original post on bio 628 as attmnt 481 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-14 19:13:00 UTC ***

(This is completely unrelated to the previous WIP)

Okay, second approach: this allows message to have action buttons (in this case, "Invite" and "Ignore"), looking like normal messages.  Hopefully that means things can be ignored too, by ignoring that user.  (This doesn't deal with the protocol side, because that's coming next)
Attachment #8352224 - Flags: review?(florian)
*** Original post on bio 628 as attmnt 482 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-14 19:15:00 UTC ***

And this interacts with attachment 8352224 [details] [diff] [review] (bio-attmnt 481) to display those buttons, and join the channel when the appropriate button is clicked.
Attachment #8352225 - Flags: review?(clokep)
*** Original post on bio 628 as attmnt 483 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-14 19:22:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 628 at 2011-01-14 19:37:06 UTC ***

(In reply to comment #13)
> Created an attachment (id=483) [details]
> screenshot, attachment 8352224 [details] [diff] [review] (bio-attmnt 481) [details] + attachment 8352225 [details] [diff] [review] (bio-attmnt 482) [details]

Just a quick comment that I'm not sure how well this fits into the overall conversation view (although this appears on the server conversation so it might not matter as much).  Also, I think this is ok how it is, but we need to ensure it's theme-able by message styles.
Comment on attachment 8352225 [details] [diff] [review]
make the IRC-JavaScript protocol emit those invites

*** Original change on bio 628 attmnt 482 at 2011-01-14 21:05:14 UTC ***

>+const Ci = Components.interfaces;
>+const Cc = Components.classes;

These shouldn't be necessary they're defined in ircProtocol.js (and this gets added into that scope).

> serverMessage = function(message) {
>+  this._serverNick = message.source;
What is _serverNick used for? It's possible to get messages from different servers and by setting this. something you're setting this on the full account object.

>+      if (message.params.length < 2 || message.params[0] != this._nickname) {
This probably needs to be normalized. In IRC Mook == mook == mOOk, etc. This can be done with:
normalize(message.params[0]) != normalize(this._nickname)

>+      this._getConversation(this._serverNick || message.source).writeMessage(
>+        message.source || this._serverNick,
Why can't we just use message.source here? That's what the rest of my code uses.

>+         actions: imXPCOMUtils.nsIArray([
>+          imXPCOMUtils.XPCOMWrapper({
>+            label: "Join " + message.params[1],
>+            action: function(){ self._sendMessage("JOIN", [message.params[1]])},
>+            QueryInterface: XPCOMUtils.generateQI([Ci.purpleIMessageAction])}),
>+          imXPCOMUtils.XPCOMWrapper({
>+            label: "Ignore",
>+            action: function(){ /* nothing */},
>+            QueryInterface: XPCOMUtils.generateQI([Ci.purpleIMessageAction])})
>+         ])
I think this can be simplified by providing an "action" object in jsProtoHelper perhaps? I'd prefer something like:
actions: [
new Action(<label>, <action function>)...
]

Where the array is made enumerable in the UI code instead of having to add it to every place we want a set of actions passed.

>+      // XXX Mook: actions should be a nsIArray or something here, I think.
>+      // (make sure it's re-enumerable)
Is this comment still valid with your imXPCOMUtils.jsm?


>diff --git a/modules/jsProtoHelper.jsm b/modules/jsProtoHelper.jsm

flo wrote this part of that file so he'll need to review it, it looks good as far as I can tell though. (Although this will no longer apply. I'll post an unbitrotted version after this.)
Attachment #8352225 - Flags: review?(clokep) → review-
*** Original post on bio 628 as attmnt 484 at 2011-01-14 21:09:00 UTC ***

Unbitrotted version from all the work that flo and I did on jsProtoHelper.jsm today.
*** Original post on bio 628 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-01-15 01:12:35 UTC ***

(In reply to comment #15)
> (From update of attachment 8352225 [details] [diff] [review] (bio-attmnt 482) [details])
> >+const Ci = Components.interfaces;
> >+const Cc = Components.classes;
> 
> These shouldn't be necessary they're defined in ircProtocol.js (and this gets
> added into that scope).
Actually, C.u.import creates a new scope and evaluates in that, then grabs the things listed in EXPORTED_SYMBOLS and passes them to the importer.  This is why you can't access window.* even from jsms included in page script.

> 
> > serverMessage = function(message) {
> >+  this._serverNick = message.source;
> What is _serverNick used for? It's possible to get messages from different
> servers and by setting this. something you're setting this on the full account
> object.
Hmm. this is meant to be a way to bump things back to the server tab, since it's the |who| we use for server messages (for moznet, for example, usually sand.mozilla.org).    When you switch servers, you'd get a new 001 (connection established).
> 
> >+      if (message.params.length < 2 || message.params[0] != this._nickname) {
> This probably needs to be normalized. In IRC Mook == mook == mOOk, etc. This
> can be done with:
> normalize(message.params[0]) != normalize(this._nickname)
Will do, thanks!
> 
> >+      this._getConversation(this._serverNick || message.source).writeMessage(
> >+        message.source || this._serverNick,
> Why can't we just use message.source here? That's what the rest of my code
> uses.
for the second one, probably yes (since there's always somebody doing the invite, even if it's ChanServ); for the first one, using message.source means it will appear in the tab for that person, like a PM... I guess that can work, too, actually.  Hmm, let me do that, then, and drop _serverNick.
> 
> >+         actions: imXPCOMUtils.nsIArray([
> >+          imXPCOMUtils.XPCOMWrapper({
> >+            label: "Join " + message.params[1],
> >+            action: function(){ self._sendMessage("JOIN", [message.params[1]])},
> >+            QueryInterface: XPCOMUtils.generateQI([Ci.purpleIMessageAction])}),
> >+          imXPCOMUtils.XPCOMWrapper({
> >+            label: "Ignore",
> >+            action: function(){ /* nothing */},
> >+            QueryInterface: XPCOMUtils.generateQI([Ci.purpleIMessageAction])})
> >+         ])
> I think this can be simplified by providing an "action" object in jsProtoHelper
> perhaps? I'd prefer something like:
> actions: [
> new Action(<label>, <action function>)...
> ]
Oooh, I like this.
> 
> Where the array is made enumerable in the UI code instead of having to add it
> to every place we want a set of actions passed.
Sadly, that array must implement nsIArray, so it can't be done in the UI side (on the other side of the XPCOM boundary); however, I can probably do it in the Message constructor instead.
> 
> >+      // XXX Mook: actions should be a nsIArray or something here, I think.
> >+      // (make sure it's re-enumerable)
> Is this comment still valid with your imXPCOMUtils.jsm?
nope; thanks for finding it.
> 
> 
> >diff --git a/modules/jsProtoHelper.jsm b/modules/jsProtoHelper.jsm
> 
> flo wrote this part of that file so he'll need to review it, it looks good as
> far as I can tell though. (Although this will no longer apply. I'll post an
> unbitrotted version after this.)
Thanks! Very much appreciated.
Attached patch address comment 15 (obsolete) — — Splinter Review 
*** Original post on bio 628 as attmnt 485 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-15 19:18:00 UTC ***

this addresses comment 15, among other things moving the messages to the PM tab.
Attachment #8352228 - Flags: review?(clokep)
Comment on attachment 8352225 [details] [diff] [review]
make the IRC-JavaScript protocol emit those invites

*** Original change on bio 628 attmnt 482 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-15 19:18:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352225 - Attachment is obsolete: true
Comment on attachment 8352227 [details] [diff] [review]
make the IRC-JavaScript protocol emit those invites (unbitrotted)

*** Original change on bio 628 attmnt 484 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-15 19:18:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352227 - Attachment is obsolete: true
*** Original post on bio 628 as attmnt 486 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-15 19:21:00 UTC ***

this mostly gets around a bug where every button used the last callback... oops :)
Attachment #8352229 - Flags: review?(florian)
Comment on attachment 8352224 [details] [diff] [review]
try 2, WIP: allow conversation messages to have actions

*** Original change on bio 628 attmnt 481 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-15 19:21:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352224 - Attachment is obsolete: true
Attachment #8352224 - Flags: review?(florian)
Comment on attachment 8352228 [details] [diff] [review]
address comment 15

*** Original change on bio 628 attmnt 485 at 2011-01-15 23:31:39 UTC ***

>diff --git a/modules/ircParser.jsm b/modules/ircParser.jsm
>-var EXPORTED_SYMBOLS = ["ircParser"];
>+var EXPORTED_SYMBOLS = ["ircParser", "normalize"];
Good idea, I can change this file to be ircUtils or something.

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+//Components.utils.import("resource:///modules/jsProtoHelper.jsm");
>+Components.utils.import("resource://irc-js/jsProtoHelper.jsm"); // XXX Custom jsProtoHelper
I see where we need jsProtoHelper, but where are we using anything from XPCOMUtils?

>@@ -84,7 +100,28 @@ ircParser = {
>     },
>     "INVITE": function(message) {
>       // INVITE  <nickname> <channel>
>-      // XXX prompt user to join channel
>+      Components.utils.import("resource://app/modules/imXPCOMUtils.jsm");
>+      if (message.params.length < 2 ||
>+          normalize(message.params[0]) != normalize(this._nickname))
>+      {
>+        // somebody else is being invited, ignore for now
>+        return;
>+      }
>+      let self = this;
>+      this._getConversation(message.nickname || message.source).writeMessage(
>+        message.nickname || message.source,
>+        [message.nickname, "invited",
>+         message.params[0], "to join",
>+         message.params[1]].join(" "),
>+        {incoming: true,
>+         actions: [
>+          new Action(
>+            "Join " + message.params[1],
>+            function(){ self._sendMessage("JOIN", [message.params[1]]); }),
>+          new Action("Ignore")
>+         ]
>+        }
>+      );
>     },
>     "JOIN": function(message) {
>       // JOIN ( <channel> *( "," <channel> ) [ <key> *( "," <key> ) ] ) / "0"
This is much more reasonable for someone to write who might not understand the under workings of the protocols (i.e. if someone random wants to add a new account), I like this a lot better.

>diff --git a/modules/jsProtoHelper.jsm b/modules/jsProtoHelper.jsm
>+function Action(aLabel, aAction)
>+{
>+  this.label = aLabel;
>+  this.action = aAction || function(){ /* no action */ };
>+}
>+Action.prototype = {
>+  label: "<unknown>",
>+  action: null,
Since the constructor forces an action it doesn't need to be defined in the prototype, and we could do something similar with the label, i.e.
this.label = aLabel || "<unknown>";
although this isn't localized, etc.

>+  QueryInterface: XPCOMUtils.generateQI([Ci.purpleIMessageAction])
>+}
I think here you want something more like:
__proto__: ClassInfo("purpleIMessageAction", "generic message action object")

I'm gonna have flo look at the rest of this part, but I assume it's OK.
> const GenericConversationPrototype = {
>   __proto__: ClassInfo("purpleIConversation", "generic conversation object"),
>   flags: Ci.nsIClassInfo.DOM_OBJECT,
>@@ -463,8 +484,9 @@ const GenericConversationPrototype = {
>       this._observers.splice(index, 1);
>   },
>   notifyObservers: function(aSubject, aTopic, aData) {
>+    var subject = imXPCOMUtils.XPCOMWrapper(aSubject);
>     for each (let observer in this._observers)
>-      observer.observe(aSubject, aTopic, aData);
>+      observer.observe(subject, aTopic, aData);
>   },
> 
>   doCommand: function(aMsg) false,
Attachment #8352228 - Flags: review?(clokep) → review-
Attachment #8352228 - Flags: review?(florian)
Attached patch address comment 20 (obsolete) — — Splinter Review 
*** Original post on bio 628 as attmnt 487 by mook.moz+bugs.instantbird AT gmail.com at 2011-01-16 04:40:00 UTC ***

(In reply to comment #20)
This goes on top of attachment 8352228 [details] [diff] [review] (bio-attmnt 485), I expect to get at least one r- from flo and can stack this on top at when I try to address that.

> (From update of attachment 8352228 [details] [diff] [review] (bio-attmnt 485) [details])
> >diff --git a/modules/ircParser.jsm b/modules/ircParser.jsm
> I see where we need jsProtoHelper, but where are we using anything from
> XPCOMUtils?
Oops; there used to be a call to generateQI, but no more.  Removed.

> >diff --git a/modules/jsProtoHelper.jsm b/modules/jsProtoHelper.jsm
> Since the constructor forces an action it doesn't need to be defined in the
> prototype, and we could do something similar with the label, i.e.
> this.label = aLabel || "<unknown>";
> although this isn't localized, etc.
But... having an Action with no useful label makes no sense, because it can't tell the user what it's supposed to be doing.  (Nothing done here.)

> 
> >+  QueryInterface: XPCOMUtils.generateQI([Ci.purpleIMessageAction])
> >+}
> I think here you want something more like:
> __proto__: ClassInfo("purpleIMessageAction", "generic message action object")
yep; just happened to write this before ClassInfo got used so much.  Changed.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Comment on attachment 8352229 [details] [diff] [review]
update app patch to match attachment 485 [details]

*** Original change on bio 628 attmnt 486 at 2011-01-25 09:03:27 UTC ***

># HG changeset patch
># User Mook <mook.moz+hg@gmail.com>
># Date 1295074741 28800
>
>extend messages to allow actions in the form of buttons
>
>diff --git a/instantbird/content/convbrowser.xml b/instantbird/content/convbrowser.xml
>--- a/instantbird/content/convbrowser.xml
>+++ b/instantbird/content/convbrowser.xml
>@@ -301,8 +301,31 @@
>                                            null, this._textModifiers);
>             let next = isNextMessage(this.theme, aMsg, this._lastMessage);
>             let html = getHTMLForMessage(aMsg, this.theme, next);
>-            let body = doc.getElementsByTagName("body")[0];
>             let newElt = insertHTMLForMessage(aMsg, html, doc, next);
>+            if (aMsg instanceof Ci.purpleIActionableMessage) {

Messing with the HTML of a message should probably be done inside getHTMLForMessage, except if there's an excellent reason to do otherwise.

>diff --git a/instantbird/modules/imXPCOMUtils.jsm b/instantbird/modules/imXPCOMUtils.jsm
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/modules/imXPCOMUtils.jsm

Do we really need all that wrapper code? If we do, then there are probably other places where it should be used, to keep code consistency.

>diff --git a/instantbird/themes/messages/bubbles/main.css b/instantbird/themes/messages/bubbles/main.css
>--- a/instantbird/themes/messages/bubbles/main.css
>+++ b/instantbird/themes/messages/bubbles/main.css
>@@ -89,6 +89,16 @@ div.bubble hr {
>   border-bottom: 1px solid rgba(255, 255, 255, 0.5);
> }
> 
>+div.actionable-message {
>+  text-align: end;
>+}
>+div.actionable-message[collapsed] {
>+  -moz-transition-property: height;
>+  -moz-transition-duration: 1s;
>+  height: 0;
>+  visibility: collapse;
>+}
>+

If you need changes in every message theme, you are up for trouble. Can your changes fit in the CSS file shared by all conversations? (conv.css)


>diff --git a/purple/purplexpcom/public/purpleIMessage.idl b/purple/purplexpcom/public/purpleIMessage.idl
>--- a/purple/purplexpcom/public/purpleIMessage.idl
>+++ b/purple/purplexpcom/public/purpleIMessage.idl
>@@ -94,3 +94,34 @@ interface purpleIMessage: nsISupports {
>   /*  PURPLE_MESSAGE_NO_LINKIFY  = 0x4000  /**< Message should not be auto-linkified */
>   readonly attribute boolean noLinkification;
> };
>+
>+interface nsIArray;
>+interface nsIDOMEventListener;
>+
>+/*
>+ * This interface represents an extended purpleIMessage which requests that the
>+ * user perform some action
>+ */
>+[scriptable, uuid(f89ee3ec-66a2-40e1-ae32-4aa8625ccd11)]
>+interface purpleIActionableMessage: purpleIMessage
>+{
>+  /* An array of actions the user may perform; they are expected to implement
>+   * purpleIMessageAction.
>+   */
>+  readonly attribute nsIArray actions;

Any reason for using an nsIArray rather than a plain JS array?
Like this:
  void getActions([optional] out unsigned long actionCount,
                  [retval, array, size_is(actionCount)] out purpleIMessageAction actions);


To be compatible with all message themes, what about displaying the possible actions in a floating toolbar that would appear when hovering the message, rather than displaying buttons in the conversation?
Attachment #8352229 - Flags: review?(florian) → review-
*** Original post on bio 628 at 2011-01-25 09:04:16 UTC ***

Comment on attachment 8352228 [details] [diff] [review] (bio-attmnt 485)
address comment 15

>@@ -463,8 +484,9 @@ const GenericConversationPrototype = {
>       this._observers.splice(index, 1);
>   },
>   notifyObservers: function(aSubject, aTopic, aData) {
>+    var subject = imXPCOMUtils.XPCOMWrapper(aSubject);
>     for each (let observer in this._observers)
>-      observer.observe(aSubject, aTopic, aData);
>+      observer.observe(subject, aTopic, aData);
>   },
> 
>   doCommand: function(aMsg) false,

What is this change for?
*** Original post on bio 628 at 2011-01-25 09:58:29 UTC ***

Should the Join/Ignore buttons honor the OK/Cancel button order of the OS (Windows: OK left of Cancel, Mac OS: reversed, 'Linux': ummm..) by the way?
Comment on attachment 8352228 [details] [diff] [review]
address comment 15

*** Original change on bio 628 attmnt 485 at 2011-01-28 18:03:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352228 - Flags: review?(florian)
*** Original post on bio 628 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-01-29 22:19:17 UTC ***

(In reply to comment #23)
> (From update of attachment 8352228 [details] [diff] [review] (bio-attmnt 485) [details])
> >@@ -463,8 +484,9 @@ const GenericConversationPrototype = {
> >       this._observers.splice(index, 1);
> >   },
> >   notifyObservers: function(aSubject, aTopic, aData) {
> >+    var subject = imXPCOMUtils.XPCOMWrapper(aSubject);
> >     for each (let observer in this._observers)
> >-      observer.observe(aSubject, aTopic, aData);
> >+      observer.observe(subject, aTopic, aData);
> >   },
> > 
> >   doCommand: function(aMsg) false,
> 
> What is this change for?

(Answering this first, because this is short and easy)
It wrapps aSubject in a XPConnect wrapper (XPCWrappedJSClass + XPCNativeWrapper), because notifyObserver (and addObserver) is called from JS land without going through XPCOM wrapping.  This makes aSubject instanceof Ci.Foo work (i.e. call QueryInterface).  Without it, you can get to places where the subject is never wrapped (because XPCOM was never involved), which breaks the interface contract.
*** Original post on bio 628 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-01-29 23:14:51 UTC ***

(In reply to comment #22)

> Messing with the HTML of a message should probably be done inside
> getHTMLForMessage, except if there's an excellent reason to do otherwise.
Agreed; will go think of something that works.

> Do we really need all that wrapper code? If we do, then there are probably
> other places where it should be used, to keep code consistency.
Quite a bit of it, yes; I just wanted to be complete since it's a very useful API (from experience working with Songbird, at least).  Lets me say "here's some enumerator of some sort, hand me something I can do a for each loop with, thanks".

> If you need changes in every message theme, you are up for trouble. Can your
> changes fit in the CSS file shared by all conversations? (conv.css)
I think so; will go figure out something in conjunction with the thing above.

> Any reason for using an nsIArray rather than a plain JS array?
> Like this:
>   void getActions([optional] out unsigned long actionCount,
>                   [retval, array, size_is(actionCount)] out
> purpleIMessageAction actions);
Ah; I had wanted to use a getter, but that does indeed make things nicer.  Yeah, I think I can do that and possibly drop some of the jsm code usage...

> To be compatible with all message themes, what about displaying the possible
> actions in a floating toolbar that would appear when hovering the message,
> rather than displaying buttons in the conversation?
Hmm. I'm having trouble understanding what you mean; can you do a quick mockup (in MS Paint or whatever, it doesn't have to look nice)?
*** Original post on bio 628 as attmnt 514 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:35:00 UTC ***

This... mostly addresses comment 22; minus the floating toolbar bit, because I can't think of a way to do that that would look okay on {simple,bubbles,dark}.  Not that it looks good now...

And, yes: at this point, it's possible to remove imXPCOMUtils usage.  It's just used in one place to wrap observers to make instanceof work; it can be done with a straight SIP instead.

This patch _shouldn't_ get a r+, however: now that I'm using imThemes, the name "action" is very bad, because it conflicts with actions in the /me sense.  I can't think of a good name right now, though...
Attachment #8352255 - Flags: review?(florian)
Comment on attachment 8352201 [details] [diff] [review]
WIP, part 1: observer notification

*** Original change on bio 628 attmnt 458 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:35:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352201 - Attachment is obsolete: true
Comment on attachment 8352226 [details]
screenshot, attachment 481 [details] [diff] [review] + attachment 482 [details] [diff] [review]

*** Original change on bio 628 attmnt 483 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:35:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352226 - Attachment is obsolete: true
Comment on attachment 8352229 [details] [diff] [review]
update app patch to match attachment 485 [details]

*** Original change on bio 628 attmnt 486 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:35:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352229 - Attachment is obsolete: true
*** Original post on bio 628 as attmnt 515 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:52:00 UTC ***

hmm, actually, attachment 8352255 [details] [diff] [review] (bio-attmnt 514) doesn't seem to work right with Dark... Not sure why yet, but it's inserting the actionable-message inside <div id="insert">.
Attachment #8352256 - Flags: review?(clokep)
Comment on attachment 8352228 [details] [diff] [review]
address comment 15

*** Original change on bio 628 attmnt 485 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:52:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352228 - Attachment is obsolete: true
Comment on attachment 8352230 [details] [diff] [review]
address comment 20

*** Original change on bio 628 attmnt 487 by mook.moz+bugs.instantbird AT gmail.com at 2011-02-06 00:52:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352230 - Attachment is obsolete: true
*** Original post on bio 628 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-02-09 08:42:10 UTC ***

Comment on attachment 8352255 [details] [diff] [review] (bio-attmnt 514)
address most of comment 22

Bah, I guess I should do that instead...

Please ignore the changes in instantbird/modules/Makefile.in (and pretend that imXPCOMUtils.jsm doesn't exist); also, that in attachment 8352256 [details] [diff] [review] (bio-attmnt 515), notifyObservers just uses a nsISupportsInterfacePointer directly (with the comment left in).

While I still think the jsm is a good idea, it's not good enough of an idea to ride along on this bug :p

(If I get an r+, I'll attach new patches with the above fixed - those are trivial changes - in the mean time, sleep :p )
Comment on attachment 8352256 [details] [diff] [review]
js-irc patch to match attachment 514 [details]

*** Original change on bio 628 attmnt 515 at 2011-02-10 03:06:07 UTC ***

Per the changes in Comment 19.

Also I made some changes to ircParser.jsm, but I'll manually apply the patch if necessary.
Attachment #8352256 - Flags: review?(clokep) → review+
Comment on attachment 8352255 [details] [diff] [review]
address most of comment 22

*** Original change on bio 628 attmnt 514 at 2011-02-18 18:05:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352255 - Flags: review?(florian) → review-
*** Original post on bio 628 at 2011-02-18 18:05:14 UTC ***

Comment on attachment 8352255 [details] [diff] [review] (bio-attmnt 514)
address most of comment 22

>diff --git a/instantbird/modules/Makefile.in b/instantbird/modules/Makefile.in
>--- a/instantbird/modules/Makefile.in
>+++ b/instantbird/modules/Makefile.in
>@@ -45,6 +45,7 @@ EXTRA_JS_MODULES = \
> 	imContentSink.jsm \
> 	imServices.jsm \
> 	imSmileys.jsm \
>+	imXPCOMUtils.jsm \
IIRC, you finally decided to not include this file as part of this patch?


>diff --git a/instantbird/modules/imThemes.jsm b/instantbird/modules/imThemes.jsm
>@@ -523,6 +522,40 @@ function getHTMLForMessage(aMsg, aTheme,
>         actionMessageTemplate = getMetadata(aTheme, "ActionMessageTemplate");
>       html = html.replace(/%message%/g, actionMessageTemplate);
>     }
>+    if (aMsg instanceof Ci.purpleIActionableMessage) {
Would you prefer adding a hasActions flag to the purpleIMessage interface, so that you don't need to bother creating a wrapper?

>+      let actionHtml = "";
>+      let actionTemplate =
>+        '<button class="" onclick="%actionCode%">%actionLabel%</button>';
What's the empty class attribute for?

>+      if (hasMetadataKey(aTheme, "ActionLabel"))
>+        actionMessageTemplate = getMetadata(aTheme, "ActionLabel");
Did you mean actionTemplate here instead of actionMessageTemplate?

What about "ActionButton" for the name of the entry in the Info.plist file?
(it doesn't seem to have anything to do with the label).

>+      let codeTemplate = "" + <![CDATA[
>+          var box = event.target;
>+          while (box && !box.classList.contains('actionable-message'))
>+            box = box.parentNode;
>+          if (box) {
>+            box.setAttribute('collapsed', true);
>+            var action = box._originalMsg.getActions()[%idx%];
>+            if (action && ('action' in action) && action.action)
>+              action.action.handleEvent(event);
>+          }
>+          return false;
>+        ]]>;
I thought I already wrote something about this before, but can't find it so I guess I only thought it: would it be possible to put the code actually doing the actions inside the convbrowser or conversation binding in a single click event handler, rather than duplicating it for each button? That code could then also be shared if we ever decide to put the possible actions related to a message in the context menu of the message.

If you go this way, I think you will need to put an attribute on the buttons indicating which action the button is for.
These attributes would also be useful to add button icons via CSS.

>+      for (let [idx, action] in Iterator(aMsg.getActions())) {
>+        if (!(action instanceof Ci.purpleIMessageAction)) {
>+          continue;
>+        }
I'm not sure I understand. aMsg.getActions returns an array, right? What's idx then?
If as I think getActions returns an array of purpleIMessageAction, then the instanceof check is guaranteed (by XPConnect) to always return true, isn't it?

>+        let code = codeTemplate.replace(/%idx%/g, idx);
>+        actionHtml += actionTemplate.replace(/%actionLabel%/g, action.label)
>+                                    .replace(/%actionCode%/g, code);
>+      }
This whole loop looks like you could have used .map / .join to get the string of the HTML code for all the actions.

>+
>+      let actionableMessageTemplate =
>+        '%message% <div class="actionable-message">%actions%</div>';
Nit: Double quotes are used everywhere else in the file, so I would prefer \ escaping the double quotes inside the string and keep using double quotes for the string.

>diff --git a/instantbird/modules/imXPCOMUtils.jsm b/instantbird/modules/imXPCOMUtils.jsm
Not reviewing this file now.

>diff --git a/purple/purplexpcom/public/purpleIMessage.idl b/purple/purplexpcom/public/purpleIMessage.idl

>+[scriptable, uuid(364a9a74-4f82-4e34-8d98-d22c10f0cb70)]
>+interface purpleIActionableMessage: purpleIMessage
>+{
>+  /* An array of actions the user may perform */
>+  void getActions([optional] out unsigned long actionCount,
>+                  [retval, array, size_is(actionCount)] out purpleIMessageAction actions);

Would you prefer adding this in purpleIMessage (with an hasActions flag) and avoid the additional interface?
We want to redesign/cleanup this interface in bug 954127 (bio 692) anyway.

>+[scriptable, uuid(7e470f0e-d948-4d9a-b8dc-4beecf6554b9)]
>+interface purpleIMessageAction: nsISupports
>+{
>+  /* The display name for the action
>+   * TODO: figure out i18n - possibly via getters?
>+   */
>+  readonly attribute ACString label;
The label will be localized (inside the protocol plugin code).
I think you also need a non-localized id string.

>+  /* Attempt to get an icon for the action
>+   * @param width desired width for the icon
>+   * @param height desired height for the icon
>+   * @return an URL to an icon, or null
>+   */
>+  ACString getIcon(in long width, in long height);
>+  /* The handler to invoke when the action is taken */
Are protocol plugins really going to give icons?

By the way, I'm glad to see you figured out how the message theme system works! :)
*** Original post on bio 628 at 2011-02-18 18:14:42 UTC ***

(In reply to comment #31)

> By the way, I'm glad to see you figured out how the message theme system works!
> :)

And I'm sorry it took me so long to give review comments!
*** Original post on bio 628 at 2011-02-19 07:01:26 UTC ***

When I reviewed attachment 8352255 [details] [diff] [review] (bio-attmnt 514), I concentrated mostly on the content of the patch and forgot to think about what wasn't included yet that I would like to see.
I think an "actionable" class should be added in the messageClasses method so that theme authors can display actionable messages differently if they want.
*** Original post on bio 628 at 2011-05-20 16:10:17 UTC ***

In terms of UI for this, I think using the notification bar (which is no longer used for topics, see bug 954178 (bio 744)) might be useful.
*** Original post on bio 628 at 2011-10-11 09:12:18 UTC ***

Chat invitations are now (since https://hg.instantbird.org/instantbird/rev/f5ee39dc2b13) accepted automatically, with a hidden preference to disable this behavior.

So this bug should either be closed, or morphed into a discussion of how we can add a good UI for that in a future version.
*** Original post on bio 628 at 2011-10-11 10:29:30 UTC ***

(In reply to comment #35)
> Chat invitations are now (since
> https://hg.instantbird.org/instantbird/rev/f5ee39dc2b13) accepted
> automatically, with a hidden preference to disable this behavior.
> 
> So this bug should either be closed, or morphed into a discussion of how we can
> add a good UI for that in a future version.

In light of this, I'd like to close this bug and we should file a new bug about a better UI (and optionally accepting chat invitations), but the original request is fixed now: you are able to accept chat invitations.
Assignee: bugzilla → florian
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Component: Conversation → General
Product: Instantbird → Chat Core
Resolution: --- → FIXED
Target Milestone: --- → 1.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: