*** Original post on bio 613 at 2010-12-07 23:40:00 UTC *** Currently we only have presence information for IRC buddies who have been added to the buddy list. If this is only a shortcoming of the libpurple IRC protocol plugin then it would be great if we could fix this during the IRC plugin rewrite. (If we have this information in general for some reason then we could also show it on the participants list of MUCs by the way)

*** Original post on bio 613 at 2010-12-07 23:42:09 UTC *** Sorry for bugspam: I set some fields wrongly. No more late-night bug editing, I promise.

*** Original post on bio 613 at 2010-12-08 01:17:20 UTC *** I guess this is somewhat directed at me? IRC doesn't really have a way to keep track of who is online, you'd have to ping using the NAMES command [1] occasionally to know. Although if a user is in a room with you, you KNOW whether they are online (as long as you have not received a QUIT message, if you receive PART message then their status goes to unknown). Ideally we should take the information we know from the chats we're in and occasionally ping the server for users we care about and have no info on, which would be: (users on our buddy list + users with an open chat) but (not in an open chat). Combine this with known data from chats you should know everything you care about. [1] http://tools.ietf.org/html/rfc2812#section-3.2.5

*** Original post on bio 613 at 2010-12-08 17:40:29 UTC *** Changing this depending on IRC in JavaScript instead of blocking it. (It might not actually depend on it, but it certainly doesn't block it.)

*** Original post on bio 613 at 2011-02-24 18:53:15 UTC *** (In reply to comment #2) > IRC doesn't really have a way to keep > track of who is online, you'd have to ping using the NAMES command [1] > occasionally to know. It would be better to use the ISON command. (Which is a command according to http://developer.pidgin.im/ticket/9692.) (In reply to comment #2) > (users on our buddy list + users with an open chat) but (not in an > open chat). Should be: (users on our buddy list + users with an open IM) but (not in an open MUC).

*** Original post on bio 613 at 2012-04-06 20:52:24 UTC *** It seems the easiest way to fix this is to add every DM conversation partner as a buddy, at least temporarily. Then ISON information etc will propagate to the conversation via the buddy, where most of the relevant parts will be displayed automatically. With bug 954796 (bio 1362) (away handling), this should also resolve bug 954792 (bio 1358) (display away message).

*** Original post on bio 613 at 2012-04-06 21:00:19 UTC *** (In reply to comment #6) > It seems the easiest way to fix this is to add every DM conversation partner as > a buddy, at least temporarily. Isn't this almost equivalent to saying this is WONTFIX? I would definitely not want to have in my buddy list all the spammers that open private conversations with me. Or with "add as a buddy", did you mean inside a structure that's internal to the IRC code?

*** Original post on bio 613 at 2012-04-06 21:30:02 UTC *** (In reply to comment #7) > Isn't this almost equivalent to saying this is WONTFIX? I would definitely not > want to have in my buddy list all the spammers that open private conversations > with me. > > Or with "add as a buddy", did you mean inside a structure that's internal to > the IRC code? That's the question. You would *either* have an internal, temporary buddy type which is never visible in the contacts list, *or* A dynamic "Recently talked to" tag, which would work as follows: - The conversation partner of a new conversation gets added as a buddy (if they are not already listed) and given the "Recently" tag - The "Recently" tag is hidden by default. - Buddies automatically get removed from the "Recently" tag. - when there are more than N entries (10? more?) - after a certain time (days? weeks?) - A buddy whose *only* tag is the "Recently" tag gets deleted when he loses the tag. It has the nice side-effect of acting as a kind of automatic "Favourites" tag. Comments?

*** Original post on bio 613 at 2012-04-06 21:45:17 UTC *** (In reply to comment #8) > (In reply to comment #7) > > Isn't this almost equivalent to saying this is WONTFIX? I would definitely not > > want to have in my buddy list all the spammers that open private conversations > > with me. > > > > Or with "add as a buddy", did you mean inside a structure that's internal to > > the IRC code? > > That's the question. You would > > *either* have an internal, temporary buddy type which is never visible in the > contacts list, Yes. Unless you can update a conversation's information without a buddy attached to it. > *or* A dynamic "Recently talked to" tag, which would work as follows: > > Comments? This is totally outside the scope of this bug and should be handled somewhere else and is not an IRC issue.

*** Original post on bio 613 at 2012-04-06 21:55:52 UTC *** (In reply to comment #9) > Yes. Unless you can update a conversation's information without a buddy > attached to it. It seems wrong not to use the perfectly good mechanism which is already in place. > > *or* A dynamic "Recently talked to" tag, which would work as follows: > This is totally outside the scope of this bug and should be handled somewhere > else and is not an IRC issue. I agree, I just wanted to mention the possibility as it was suggested on IRC.

*** Original post on bio 613 at 2012-06-26 21:34:24 UTC *** EionRobb thinks this should possibly be fixed for all applicable prpls.

So I was thinking about this bug a bit more and the interfaces around prplIConvIM are confusing me. prplIConvIM [1] has a parameter which is an imIAccountBuddy (prpl reference im? This rings warning bells!) imIAccountBuddy [2] requires some stuff that requires the Contacts service. I would really love to fix this bug, but I'm unsure whether the backend services surrounding this are working as designed / in a way we want and we need to do something like a "temporary" tag as discussed in comment 8 OR if there's something bigger to fix in our assumptions about when contacts exist? It might be as simple as adding a few more checks on whether the imIAccountBuddy's buddy is null or not. [1] https://mxr.mozilla.org/comm-central/source/chat/components/public/prplIConversation.idl#80 [2] https://mxr.mozilla.org/comm-central/source/chat/components/public/imIContactsService.idl#223

(In reply to Patrick Cloke [:clokep] from comment #12) > So I was thinking about this bug a bit more and the interfaces around > prplIConvIM are confusing me. prplIConvIM [1] has a parameter which is an > imIAccountBuddy (prpl reference im? This rings warning bells!) > imIAccountBuddy [2] requires some stuff that requires the Contacts service. ... > It might be as simple as adding a few more checks on whether the imIAccountBuddy's buddy is null or not. Yes, there is no prplIAccountBuddy and maybe there should be, for this? Something that provides the data associated with an account buddy but is not registered with the contacts service.

(In reply to Patrick Cloke [:clokep] from comment #12) > So I was thinking about this bug a bit more and the interfaces around > prplIConvIM are confusing me. prplIConvIM [1] has a parameter which is an > imIAccountBuddy imIAccountBuddy is and has always been prplIAccountBuddy + a typo. Only prpls can implement that interface. The reason I made that mistake at the time I rewrote the buddy list backend code in JS is that this interface is defined in the same idl file a imIBuddy and imIContact. If you can do a mass s/imIAccountBuddy/prplIAccountBuddy/g without breaking anything (I thought we had concerns about breaking existing add-ons last time we discussed this?), rs=me.

I'm going to put up a patch which I think clear up a lot of the confusion around prplIAccountBuddy vs imIBuddy vs. imIContact so this bug can more easily be worked on.

Comment on attachment 8522962 [details] [diff] [review] Rename imIAccountBuddy to prplIAccountBuddy and add comments Review of attachment 8522962 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this and adding comments! ::: chat/components/public/imIContactsService.idl @@ +55,5 @@ > }; > > +/* > + * An imIContact represents a person, e.g. Alice or Bob. This person might have > + * multiple means of contacting them or interacting with them via the UI. "via the UI" is confusing here. @@ +58,5 @@ > + * An imIContact represents a person, e.g. Alice or Bob. This person might have > + * multiple means of contacting them or interacting with them via the UI. > + * > + * Remember that an imIContact can have multiple imIBuddys, each imIBuddy can > + * have multiple prplIAccountBuddys referencing it. Each of these implement Add an example explaining the purpose of the buddy/accountbuddy distinction either here or in the comment below. @@ +154,5 @@ > [optional] in wstring aData); > }; > > +/* > + * An imIBuddy represents a person's account on a particular network. E.g. alice s/network/prpl I think, to avoid confusion with IRC networks. @@ +215,5 @@ > [optional] in wstring aData); > }; > > +/* > + * A prplIAccountBuddy represents the connection between a network the current see above @@ +239,5 @@ > * All notifications (even unsupported ones) will be forwarded to the contact, > * its tags and nsObserverService. > */ > [scriptable, uuid(114d24ff-56a1-4fd6-9822-4992efb6e036)] > +interface prplIAccountBuddy: imIStatusInfo { Should this be moved into a different file now?

Comment on attachment 8522962 [details] [diff] [review] Rename imIAccountBuddy to prplIAccountBuddy and add comments Review of attachment 8522962 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/imIContactsService.idl @@ +55,5 @@ > }; > > +/* > + * An imIContact represents a person, e.g. Alice or Bob. This person might have > + * multiple means of contacting them or interacting with them via the UI. Indeed. Drop " or interacting with them via the UI". @@ +57,5 @@ > +/* > + * An imIContact represents a person, e.g. Alice or Bob. This person might have > + * multiple means of contacting them or interacting with them via the UI. > + * > + * Remember that an imIContact can have multiple imIBuddys, each imIBuddy can 'imIBuddys' seems awkward. In other comments I wrote "buddies (imIBuddy instances)" that's longer but doesn't have the risk that someone would attempt to mxr 'imIBuddys'. @@ +58,5 @@ > + * An imIContact represents a person, e.g. Alice or Bob. This person might have > + * multiple means of contacting them or interacting with them via the UI. > + * > + * Remember that an imIContact can have multiple imIBuddys, each imIBuddy can > + * have multiple prplIAccountBuddys referencing it. Each of these implement Same for 'prplIAccountBuddys' here. @@ +154,5 @@ > [optional] in wstring aData); > }; > > +/* > + * An imIBuddy represents a person's account on a particular network. E.g. alice No, prpl wouldn't be more correct, as gtalk and xmpp should be or are the same network. @@ +157,5 @@ > +/* > + * An imIBuddy represents a person's account on a particular network. E.g. alice > + * has two accounts on the Foo network: @lic4 and alice88, Bob has a single > + * account: BobbyBoy91. They both have a single account on the Bar network: > + * _alice_ and _bob_, respectively. This example doesn't really help me understand :-/. "E.g. alice has two accounts on the Foo network: @lic4 and alice88, each are representer by a different imIBuddy instance." would seem better to me. I don't see why Bob needs to be involved in that example. @@ +216,5 @@ > }; > > +/* > + * A prplIAccountBuddy represents the connection between a network the current > + * user belongs to and contact's accounts on those networks. E.g. @lic4 as seen I think you've got "user" and "contact" mixed up in that sentence. The account we care about is the user's, not the contact's. ::: im/components/ibConvStatsService.js @@ +471,5 @@ > this._statsByPrplConvId.set(conv.id, gStatsByConvId[id]); > > let possibleConv = null; > if (conv.buddy) { > + // First .buddy is an prplIAccountBuddy, second one is an imIBuddy. Should this be 'a prplI...' rather than 'an'?

To make this more confusing I've moved these changes to bug 955353 to keep this one reserved for the work that should actually happen in it. Sorry for this.

This gets it to work, although there are some errors thrown about _preferredBuddy not being set. Mostly I want some feedback on whether this is insane or if I should go through and fix the final issues.

Comment on attachment 8525038 [details] [diff] [review] WIP v1 Review of attachment 8525038 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/irc/irc.js @@ +14,5 @@ > > +// Load the contacts service so fake contacts can be created. > +let contactsService = {}; > +Services.scriptloader.loadSubScript( > + "resource:///components/imContacts.js", contactsService); I dislike this. Some method added to Services.contacts would be better. @@ +617,5 @@ > + // If the account has a buddy of this name, return it. > + if (this._account.buddies.has(this.name)) > + return this._account.buddies.get(this.name); > + > + // Otherwise, return an account buddy has a fake contact. Things to think about: What if a buddy already exists? (but no account buddy for this account) Does this fake buddy stay fake when the account buddy is added? If it does, does it become a real buddy when the user decides to add that buddy as a contact from the context menu? @@ +622,5 @@ > + let imBuddy = new contactsService.Buddy(null, > + this._account.normalize(this.name), > + this.name); > + imBuddy._preferredAccount = this._account; > + let buddy = new ircAccountBuddy(this._account, imBuddy, null, this.name); If you didn't create the fake buddy first, new ircAccountBuddy would create a buddy. But either way, the account buddy gets added to the database and never removed.

(In reply to aleth [:aleth] from comment #21) > If you didn't create the fake buddy first, new ircAccountBuddy would create > a buddy. But either way, the account buddy gets added to the database and > never removed. Correction: this is wrong, as accountBuddyAdded has to be called explicitly for that to happen and notifications to be sent out.

A newer iteration of this patch that I'm seeking feedback on. It not works when you add a buddy after the conversation is open, BUT the UI to do so from the tab/participant list still doesn't work.

Comment on attachment 8580335 [details] [diff] [review] WIP v2 Review of attachment 8580335 [details] [diff] [review]: ----------------------------------------------------------------- Looks very promising indeed! ::: chat/components/src/imContacts.js @@ +380,5 @@ > > +// A tag used with dummy imIBuddy instances, i.e. for prplIAccountBuddy > +// instances that won't show up in the contact list. > +var dummyTag = { > + _initTag: function() { How much of this code is duplicated? Can that be avoided? @@ +991,2 @@ > function Buddy(aId, aKey, aName, aSrvAlias, aContactId) { > + // Assign a negative id to dummy buddies that have a single account buddy. a single account buddy or a single dummy account buddy? @@ +1045,5 @@ > + this._id = DBConn.lastInsertRowID; > + BuddiesById[this._id] = this; > + > + // TODO Do we care? > + //this._notifyObservers("no-longer-dummy", oldId.toString()); Not if we can help it :-) I guess the notification which gets the buddy list to show the now real buddy is account-buddy-moved...? @@ +1461,5 @@ > getBuddyByNameAndProtocol: function(aNormalizedName, aPrpl) { > + if (!BuddiesByPrpl.hasOwnProperty(aPrpl.id)) > + return null; > + > + return BuddiesByPrpl[aPrpl.id][aNormalizedName] || null; Why this change? ie. do we want this to return dummies? @@ +1485,5 @@ > + buddy = new Buddy(null, normalizedName, aAccountBuddy.userName, > + aAccountBuddy.serverAlias, 0); > + > + // Initialize the 'buddy' field of the prplIAccountBuddy instance. > + aAccountBuddy.buddy = buddy; This should be outside the if clause I think ::: chat/modules/jsProtoHelper.jsm @@ +281,5 @@ > let oldTag = this._tag; > this._tag = aNewTag; > + // If an old tag doesn't exist, then the tag is initially being set. > + if (oldTag) > + Services.contacts.accountBuddyMoved(this, oldTag, aNewTag); Why this change?

UI places to fix: https://dxr.mozilla.org/comm-central/source/im/content/conversation.xml#1582 https://dxr.mozilla.org/comm-central/source/im/content/nsContextMenu.js#178

Comment on attachment 8580335 [details] [diff] [review] WIP v2 I think I didn't give feedback because aleth's feedback seemed enough. I hope you weren't waiting for me (if you were, sorry for the extreme review lag :-(). This still seems like a very desirable feature!