792800 - IRC nickname addition in the address book fields for chat

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[George Fiotakis]

Thunderbird now supports irc channels, it would be handy to add a field for at least registered nicknames on irc channels at the Address Book contact's "chat" panel.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I'd be willing to mentor this if this is something we want.

Comment 2

[Florian Quèze [:florian]]

FWIW, I haven't included an IRC field in the address book cards because an IRC nick without also specifying the IRC network is useless for the purpose of detecting presence or matching an IM conversation with an address book contact.

That said, if people want to have such a field to just store some information, I have nothing against it.

Comment 3

[George Fiotakis]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)

>if people want to have such a field to just store some
> information, I have nothing against it.

On second thought, if some nickname is not registered it would be useless/dangerous to associate it with an AB contact. Just storing the info would be nice and enough.

Comment 4

[Avinash [:hardfire]]

I would be interested to work on this. I am new to the platform but can do clean HTML and JS. I would appreciate if someone could point me on how to start and yes, is this something that we want ?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Avinash from comment #4)
> I would be interested to work on this. I am new to the platform but can do
> clean HTML and JS. I would appreciate if someone could point me on how to
> start and yes, is this something that we want ?

Hey Avinash!

Great to hear that you're interested! Your first step is to create a Thunderbird build for your OS.

https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build

Let me know when you've got that set up, and I'll let you know the next step.

Thanks!

-Mike

Comment 6

[Avinash [:hardfire]]

Hello Mike, I have a Thunderbird build ready. what next ? where should i be looking at?

Comment 7

[Florian Quèze [:florian]]

I think you will want to look at the patch from bug 759328 to see which files need to be edited to add an additional chat field.

Comment 8

[Avinash [:hardfire]]

Hello Mike and Florian, That patch link did help and i have been able to add a IRC field to the address book. It saves and shows up in the address book. Is that all we wanted or have i missed something. 
Do i send the screenshots and the patch?

Comment 9

[Janit Anjaria]

Hey Mike and Florian!
I am a beginner to the source code,and would like to work on this hack.It would be great if you would help me out with the same.
And my area of expertise is C++.So is this task achievable...??

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to Avinash from comment #8)
> Hello Mike and Florian, That patch link did help and i have been able to add
> a IRC field to the address book. It saves and shows up in the address book.
> Is that all we wanted or have i missed something. 
> Do i send the screenshots and the patch?

Avinash:

Holy smokes, I have no idea how your comment slipped past my radar. :( If you have a patch, yes, please post it!  Screenshots are good too.

-Mike

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Janit Anjaria from comment #9)
> Hey Mike and Florian!
> I am a beginner to the source code,and would like to work on this hack.It
> would be great if you would help me out with the same.
> And my area of expertise is C++.So is this task achievable...??

Hey Janit,

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Whoops - pressed submit too early. Let's try that again.

Hey Janit,

I think this bug is already being worked on. We kind of dropped the ball on assigning it though. Let's see if Avinash posts a patch, and if not, we can let you pick up the ball and drive this one.

Thanks,

-Mike

Comment 13

[Avinash [:hardfire]]

(In reply to Mike Conley (:mconley) from comment #10)
> (In reply to Avinash from comment #8)
> > Hello Mike and Florian, That patch link did help and i have been able to add
> > a IRC field to the address book. It saves and shows up in the address book.
> > Is that all we wanted or have i missed something. 
> > Do i send the screenshots and the patch?
> 
> Avinash:
> 
> Holy smokes, I have no idea how your comment slipped past my radar. :( If
> you have a patch, yes, please post it!  Screenshots are good too.
> 
> -Mike

Hello Mike,

Yes I think i have the patch and i'll get a screenshot. Let me just search where i have it . I had dropped this after i got no replies, thought it was not required. I will get back with the patch and screenshot asap. 

Thanks and Regards,
Avinash

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Avinash from comment #13)
> (In reply to Mike Conley (:mconley) from comment #10)
> > (In reply to Avinash from comment #8)
> > > Hello Mike and Florian, That patch link did help and i have been able to add
> > > a IRC field to the address book. It saves and shows up in the address book.
> > > Is that all we wanted or have i missed something. 
> > > Do i send the screenshots and the patch?
> > 
> > Avinash:
> > 
> > Holy smokes, I have no idea how your comment slipped past my radar. :( If
> > you have a patch, yes, please post it!  Screenshots are good too.
> > 
> > -Mike
> 
> Hello Mike,
> 
> Yes I think i have the patch and i'll get a screenshot. Let me just search
> where i have it . I had dropped this after i got no replies

That was our fault. My apologies for dropping the ball!

Comment 15

[Avinash [:hardfire]]

Attachment: Screenshot

Comment 16

[Avinash [:hardfire]]

Attachment: patch

Comment 17

[Avinash [:hardfire]]

(In reply to Mike Conley (:mconley) from comment #14)
> (In reply to Avinash from comment #13)
> > (In reply to Mike Conley (:mconley) from comment #10)
> > > (In reply to Avinash from comment #8)
> > > > Hello Mike and Florian, That patch link did help and i have been able to add
> > > > a IRC field to the address book. It saves and shows up in the address book.
> > > > Is that all we wanted or have i missed something. 
> > > > Do i send the screenshots and the patch?
> > > 
> > > Avinash:
> > > 
> > > Holy smokes, I have no idea how your comment slipped past my radar. :( If
> > > you have a patch, yes, please post it!  Screenshots are good too.
> > > 
> > > -Mike
> > 
> > Hello Mike,
> > 
> > Yes I think i have the patch and i'll get a screenshot. Let me just search
> > where i have it . I had dropped this after i got no replies
> 
> That was our fault. My apologies for dropping the ball!

Hello Mike,
I have added a patch for it and a screenshot. 
I'll have a look and see if i have missed anything. I am new to both mercurial and the thunderbird platform so do be nice :D
Do notify me if there is a mistake or improvements that can be made.

Regards,
Avinash

Comment 18

[Ludovic Hirlimann [:Usul]]

Comment on attachment 713810 [details] [diff] [review]
patch

Patches needs to request reviews - doing it for you now.

Comment 19

[Avinash [:hardfire]]

(In reply to Ludovic Hirlimann [:Usul] from comment #18)
> Comment on attachment 713810 [details] [diff] [review]
> patch
> 
> Patches needs to request reviews - doing it for you now.

oh! thank you so much .. I am new to the workflow. will get a hang of it soon.

Comment 20

[Patrick Cloke [:clokep]]

I'm going to drive by review here from a high level...
"name#channel" doesn't really make sense, someone's nick is network dependent, not channel dependent. This is a bit unique compared to all the other networks that are listed (except maybe "Jabber ID", not all networks are affiliated...). Just something to think about!

Also, is this supposed to be machine parsable? If so, more effort will need to be put into thinking about the separator character.

Comment 21

[Avinash [:hardfire]]

(In reply to Patrick Cloke [:clokep] from comment #20)
> I'm going to drive by review here from a high level...
> "name#channel" doesn't really make sense, someone's nick is network
> dependent, not channel dependent. This is a bit unique compared to all the
> other networks that are listed (except maybe "Jabber ID", not all networks
> are affiliated...). Just something to think about!
> 
> Also, is this supposed to be machine parsable? If so, more effort will need
> to be put into thinking about the separator character.

O yes! "name#channel" actually doesn't make sense at all, we can have something like "name@network" maybe.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 713810 [details] [diff] [review]
patch

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

Thanks Avinash!

Overall, this looks really good. Just a few style nits to fix, and some suggestions on strings.

Great job,

-Mike

::: mail/components/addrbook/content/abCardOverlay.js
@@ +64,5 @@
>           ["QQ", "_QQ"],
>           ["MSN", "_MSN"],
>           ["ICQ", "_ICQ"],
> +         ["XMPP", "_JabberId"],
> +		 ["IRC", "_IRC"]

Please fix the indentation here.

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +156,4 @@
>    cvData.cvMSN        = doc.getElementById("cvMSN");
>    cvData.cvICQ        = doc.getElementById("cvICQ");
>    cvData.cvXMPP       = doc.getElementById("cvXMPP");
> +  cvData.cvIRC       = doc.getElementById("cvIRC");

I don't normally like forcing the equals signs to line up like this, but we might as well be consistent. Please add another space to line up the space.

::: mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
@@ +144,4 @@
>  <!ENTITY ICQ.accesskey                  "I">
>  <!ENTITY XMPP.label                     "Jabber ID:">
>  <!ENTITY XMPP.accesskey                 "J">
> +<!ENTITY IRC.label                     "IRC name#channel:">

I agree that IRC name#channel doesn't make a whole lot of sense.

But I'm not sure that IRC name@server is much better. As clokep notes, it seems to imply more functionality (that Thunderbird will parse it).

I think we should stick to "IRC Nick" for now to match the one in addressBook.properties - let's keep this patch simple.

::: mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
@@ +65,4 @@
>  propertyMSN=MSN
>  propertyICQ=ICQ
>  propertyXMPP=Jabber ID
> +propertyIRC =IRC Nick

No space before =

::: mailnews/addrbook/public/nsIAbCard.idl
@@ +351,4 @@
>  #define kMSNProperty                "_MSN"
>  #define kICQProperty                "_ICQ"
>  #define kXMPPProperty               "_JabberId"
> +#define kIRCProperty               "_IRC"

Space to line up with "_JabberId"

Comment 23

[Avinash [:hardfire]]

Attachment: patch #2

New Patch with indentation fix and changed "IRC#channel" to "IRC Nick"

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 713944 [details] [diff] [review]
patch #2

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

Looks great! Sorry - I missed something in my last review. I think that'll do it once it's fixed.

::: mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
@@ +144,5 @@
>  <!ENTITY ICQ.accesskey                  "I">
>  <!ENTITY XMPP.label                     "Jabber ID:">
>  <!ENTITY XMPP.accesskey                 "J">
> +<!ENTITY IRC.label                      "IRC Nick:">
> +<!ENTITY IRC.accesskey                  "IRC">

Whoops - I missed this in my last review. An accesskey should only be a single key.

I is being used here. Let's use "R" instead.

Comment 25

[Avinash [:hardfire]]

Attachment: Patch #3

changed accessorkey to "R"

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 714264 [details] [diff] [review]
Patch #3

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

Looks great! Thanks Avinash!

Comment 27

[Avinash [:hardfire]]

awesome and thanks! need to read more on xul and xulrunner!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f3562c434314

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I just realized that this landed with my name as the committer. Avinash, for future patches, please make sure that your have hg configured to generate patches with all of the needed commit information. Sorry :(
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 30

[Avinash [:hardfire]]

oh!sad .. no issues, i made those changes now ... had no idea of that last time :'(

Comment 31

[Philip Chee]

> I just realized that this landed with my name as the committer
You could back it out and reland it with the correct patch author :P

Comment 32

[Avinash [:hardfire]]

is it possibl(In reply to Philip Chee from comment #31)
> > I just realized that this landed with my name as the committer
> You could back it out and reland it with the correct patch author :P

yay! if it is possible, i can send another patch .. with me as the author :D