Last Comment Bug 792800 - IRC nickname addition in the address book fields for chat
: IRC nickname addition in the address book fields for chat
Status: RESOLVED FIXED
[mentor=mconley][lang=js|xul]
:
Product: Thunderbird
Classification: Client Software
Component: Address Book (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Thunderbird 21.0
Assigned To: Avinash [:hardfire]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-20 04:49 PDT by George Fiotakis
Modified: 2013-02-15 20:35 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screenshot (63.07 KB, image/jpeg)
2013-02-13 22:49 PST, Avinash [:hardfire]
no flags Details
patch (5.82 KB, patch)
2013-02-13 22:51 PST, Avinash [:hardfire]
mconley: review-
Details | Diff | Review
patch #2 (5.82 KB, patch)
2013-02-14 09:37 PST, Avinash [:hardfire]
mconley: review-
Details | Diff | Review
Patch #3 (5.82 KB, patch)
2013-02-14 21:45 PST, Avinash [:hardfire]
mconley: review+
Details | Diff | Review

Description George Fiotakis 2012-09-20 04:49:02 PDT
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) - (Away until June 29th) 2012-09-21 07:17:36 PDT
I'd be willing to mentor this if this is something we want.
Comment 2 Florian Quèze [:florian] [:flo] 2012-09-21 07:33:58 PDT
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 2012-09-21 08:48:48 PDT
(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] 2012-11-14 09:05:51 PST
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) - (Away until June 29th) 2012-11-15 18:06:22 PST
(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] 2012-11-18 01:46:37 PST
Hello Mike, I have a Thunderbird build ready. what next ? where should i be looking at?
Comment 7 Florian Quèze [:florian] [:flo] 2012-11-18 02:18:57 PST
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] 2012-11-20 09:35:30 PST
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 2013-02-11 05:44:56 PST
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) - (Away until June 29th) 2013-02-12 08:02:52 PST
(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) - (Away until June 29th) 2013-02-12 08:03:22 PST
(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) - (Away until June 29th) 2013-02-12 08:04:20 PST
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] 2013-02-12 08:48:28 PST
(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) - (Away until June 29th) 2013-02-12 08:49:37 PST
(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] 2013-02-13 22:49:45 PST
Created attachment 713807 [details]
Screenshot
Comment 16 Avinash [:hardfire] 2013-02-13 22:51:36 PST
Created attachment 713810 [details] [diff] [review]
patch
Comment 17 Avinash [:hardfire] 2013-02-13 22:53:24 PST
(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] 2013-02-14 01:09:25 PST
Comment on attachment 713810 [details] [diff] [review]
patch

Patches needs to request reviews - doing it for you now.
Comment 19 Avinash [:hardfire] 2013-02-14 01:29:22 PST
(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] 2013-02-14 03:37:36 PST
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] 2013-02-14 05:46:19 PST
(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) - (Away until June 29th) 2013-02-14 09:14:15 PST
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] 2013-02-14 09:37:19 PST
Created attachment 713944 [details] [diff] [review]
patch #2

New Patch with indentation fix and changed "IRC#channel" to "IRC Nick"
Comment 24 Mike Conley (:mconley) - (Away until June 29th) 2013-02-14 09:46:57 PST
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] 2013-02-14 21:45:27 PST
Created attachment 714264 [details] [diff] [review]
Patch #3

changed accessorkey to "R"
Comment 26 Mike Conley (:mconley) - (Away until June 29th) 2013-02-15 06:43:14 PST
Comment on attachment 714264 [details] [diff] [review]
Patch #3

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

Looks great! Thanks Avinash!
Comment 27 Avinash [:hardfire] 2013-02-15 08:25:58 PST
awesome and thanks! need to read more on xul and xulrunner!
Comment 28 Ryan VanderMeulen [:RyanVM] 2013-02-15 08:41:17 PST
https://hg.mozilla.org/comm-central/rev/f3562c434314
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-02-15 18:03:30 PST
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] 2013-02-15 18:07:04 PST
oh!sad .. no issues, i made those changes now ... had no idea of that last time :'(
Comment 31 Philip Chee 2013-02-15 20:21:22 PST
> 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] 2013-02-15 20:35:56 PST
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

Note You need to log in before you can comment on or make changes to this bug.