Closed Bug 954887 Opened 10 years ago Closed 10 years ago

Add explanatory comments to document normalizedNames

Categories

(Chat Core :: IRC, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: FeuerFliege, Assigned: aleth)

References

Details

Attachments

(1 file, 6 obsolete files)

*** Original post on bio 1454 at 2012-05-21 15:30:00 UTC ***

normalizedName() used in logger.js isn't stripping special characters.

aleth: clokep_work: I think normalizedName is overridden
flo: so it's http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#532
flo: what did libpurple do?
clokep_work: Oh, bleh... http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIConversation.idl#64 yeah I probably overwrite that incorrectly then.

<http://log.bezut.info/instantbird/120521/#m386>
*** Original post on bio 1454 at 2012-05-21 15:32:47 UTC ***

This is an IRC bug, I think I redefine normalizedName at some point to be the "server normalized" name, the comment in the IDL clearly says it is used for log files. This should be as easy as just removing the definitions, but we need to check if they're used anywhere first in irc.js.
Component: General → IRC
OS: Windows 7 → All
Hardware: x86 → All
Summary: logger.js uses wrong normalizedName() → normalizedName for JS-IRC is wrong
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1454 as attmnt 1508 at 2012-05-22 01:37:00 UTC ***

Note that there's no comment for what the normalizedName of a imIAccountBuddy is [1], so I assumed it should follow the rest. I tested this for a bit and it seems to work OK.

[1] http://lxr.instantbird.org/instantbird/source/chat/components/public/imIContactsService.idl#259
Attachment #8353260 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353260 [details] [diff] [review]
Patch v1

*** Original change on bio 1454 attmnt 1508 at 2012-05-22 10:12:44 UTC ***

Looks good to me. But I would suggest you add a comment explaining the distinction between the various "normalize*" that can occur and what they are used for, to avoid future confusion.
Attachment #8353260 - Flags: review?(bugzilla) → review+
*** Original post on bio 1454 at 2012-05-22 10:17:31 UTC ***

Wait, have you checked this doesn't cause problems with 
http://lxr.instantbird.org/instantbird/source/instantbird/content/buddytooltip.xml#289 ?

I think JS-IRC sends the account normalized name here in response.

See also bug 954804 (bio 1370).(In reply to comment #2)

> Note that there's no comment for what the normalizedName of a imIAccountBuddy
> is [1], so I assumed it should follow the rest. I tested this for a bit and it
> seems to work OK.
> 
> [1]
> http://lxr.instantbird.org/instantbird/source/chat/components/public/imIContactsService.idl#259

Maybe there should be a comment?
Comment on attachment 8353260 [details] [diff] [review]
Patch v1

*** Original change on bio 1454 attmnt 1508 at 2012-05-22 10:21:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353260 - Flags: review+ → review-
*** Original post on bio 1454 at 2012-05-22 12:26:59 UTC ***

The following interfaces have this:
imIBuddy [1]: has a comment that says "normalized userName", but not why it is normalized.
imIAccountBuddy [2]
prplIAccount [3]
prplIConversation [4] (this one already has an adequate comment that I apparently ignored): used to generate safe folder names.
prplIProtocol [5]

Any help figuring out what these other ones are used for would be helpful. :)

[1] http://lxr.instantbird.org/instantbird/source/chat/components/public/imIContactsService.idl#181
[2] http://lxr.instantbird.org/instantbird/source/chat/components/public/imIContactsService.idl#259
[3] http://lxr.instantbird.org/instantbird/source/chat/components/public/imIAccount.idl#129
[4] http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIConversation.idl#64
[5] http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIProtocol.idl#51
*** Original post on bio 1454 at 2012-05-22 12:53:32 UTC ***

Also
JS-IRC ircAccount.normalize [6]
prplIConvChat.getNormalizedChatBuddyName [7] (has a comment: for starting a private conversation and observers requesting/waiting for a reply from requestBuddyInfo)

[6]
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#532
[7]
http://lxr.instantbird.org/instantbird/source/chat/components/public/prplIConversation.idl#125
*** Original post on bio 1454 at 2013-04-10 13:38:56 UTC ***

(In reply to comment #5)
> The following interfaces have this:
> imIBuddy [1]: has a comment that says "normalized userName", but not why it is
> normalized.
> imIAccountBuddy [2]
> prplIAccount [3]
> prplIConversation [4] (this one already has an adequate comment that I
> apparently ignored): used to generate safe folder names.
> prplIProtocol [5]
> 
> Any help figuring out what these other ones are used for would be helpful. :)

From #instantbird (slightly edited):
9:21:38 AM - flo-retina: they are all mostly the same thing
9:22:19 AM - flo-retina: ie "generate a string that can be used to check for duplicates, and that is suitable to be used a as folder name for log storage purpose (no special chars...)"
9:22:41 AM - flo-retina: the only crazy exception is prplIConvChat.getNormalizedChatBuddyName
9:31:37 AM - flo-retina: [which is used] to start a private conversation with a participant of a MUC

I would think that normalizing for log storage should be done in the logger though, not in the prpl...
*** Original post on bio 1454 at 2013-04-10 14:03:35 UTC ***

(In reply to comment #7)

> I would think that normalizing for log storage should be done in the logger
> though, not in the prpl...

The logger has no way to know that for some protocols foobar@example.org is the same as foo.bar@example.org.
Attached patch Unbitrotted patch v1 (obsolete) — Splinter Review
*** Original post on bio 1454 as attmnt 2452 at 2013-05-23 03:45:00 UTC ***

This is an unbitrotted patch.

The comment for getNormalizedChatBuddyName was already expanded in bug 954804 (bio 1370), so I think this is just getting rid of the usage of normalizedName.

As Florian said, the other normalize* functions are really what they say they are: normalizing for the logger. We can add comments if you still want.
Attachment #8354219 - Flags: review?(aleth)
Comment on attachment 8353260 [details] [diff] [review]
Patch v1

*** Original change on bio 1454 attmnt 1508 at 2013-05-23 03:45:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353260 - Attachment is obsolete: true
Comment on attachment 8354219 [details] [diff] [review]
Unbitrotted patch v1

*** Original change on bio 1454 attmnt 2452 at 2013-05-24 11:02:26 UTC ***

This patch looks good, but we should definitely add explanatory comments (if absent) to the various cases in comment 5 and comment 6. A bit of extra verbosity seems OK considering the confusion in this bug ;)

Also, you missed 
http://lxr.instantbird.org/instantbird/source/instantbird/content/buddytooltip.xml#256
which maybe was wrong to start with and should use getNormalizedChatBuddyName, despite it not being a chat buddy?
Attachment #8354219 - Flags: review?(aleth) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1454 as attmnt 2456 at 2013-05-25 00:46:00 UTC ***

(In reply to comment #10)
> Also, you missed 
> http://lxr.instantbird.org/instantbird/source/instantbird/content/buddytooltip.xml#256
> which maybe was wrong to start with and should use getNormalizedChatBuddyName,
> despite it not being a chat buddy?

I did not do this aspect...I was not confident in being able to keep straight what the different conversation objects were, since getNormalizedChatBuddyName needs a prplIConversation.
Attachment #8354223 - Flags: review?(aleth)
Comment on attachment 8354219 [details] [diff] [review]
Unbitrotted patch v1

*** Original change on bio 1454 attmnt 2452 at 2013-05-25 00:46:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354219 - Attachment is obsolete: true
*** Original post on bio 1454 at 2013-05-25 12:31:58 UTC ***

(In reply to comment #11)
> I did not do this aspect...I was not confident in being able to keep straight
> what the different conversation objects were, since getNormalizedChatBuddyName
> needs a prplIConversation.
Right, in fact it needs a MUC.

So I guess we need to do something hackish with wrappedJSObject() to get the IRC-normalized version of aBuddy.username :( Does this work:
account.prplAccount.wrappedJSObject.normalizeNick(aBuddy.username)
Comment on attachment 8354223 [details] [diff] [review]
Patch v2

*** Original change on bio 1454 attmnt 2456 at 2013-05-30 19:09:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354223 - Flags: review?(aleth)
Depends on: 955420
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1454 as attmnt 3014 at 2013-11-04 14:49:00 UTC ***

I don't think the current normalizedNames provided by JS-IRC are wrong - they are supposed to be unique identifiers and using the server normalization seems fine for that purpose.

So this patch is the previous patch without the IRC changes of the previous patch, i.e. it just adds helpful comments. I changed the comment in light of the fact that protocols no longer need worry about escaping normalizedNames to make them suitable log folder names.

We should probably retitle this bug or move the patch to a different bug and mark this one INVALID if this patch is r+.
Attachment #8354795 - Flags: review?(clokep)
Comment on attachment 8354223 [details] [diff] [review]
Patch v2

*** Original change on bio 1454 attmnt 2456 at 2013-11-04 14:49:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354223 - Attachment is obsolete: true
Comment on attachment 8354795 [details] [diff] [review]
Patch

*** Original change on bio 1454 attmnt 3014 at 2013-11-04 14:58:17 UTC ***

I'd rather just morph this bug, personally. Let's not make the history more confusing.
Attachment #8354795 - Flags: review?(clokep) → review+
Severity: normal → trivial
Summary: normalizedName for JS-IRC is wrong → Add explanatory comments to document normalizedNames
Comment on attachment 8354795 [details] [diff] [review]
Patch

*** Original change on bio 1454 attmnt 3014 at 2013-11-04 15:00:41 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354795 - Flags: review?(florian)
Whiteboard: [checkin-needed]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1454 as attmnt 3015 at 2013-11-04 15:47:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354796 - Flags: review?(florian)
Comment on attachment 8354795 [details] [diff] [review]
Patch

*** Original change on bio 1454 attmnt 3014 at 2013-11-04 15:47:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354795 - Attachment is obsolete: true
Attachment #8354795 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1454 as attmnt 3017 at 2013-11-04 16:24:00 UTC ***

Hand-editing diffs Considered Harmful.
Attachment #8354798 - Flags: review?(florian)
Comment on attachment 8354796 [details] [diff] [review]
Patch

*** Original change on bio 1454 attmnt 3015 at 2013-11-04 16:24:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354796 - Attachment is obsolete: true
Attachment #8354796 - Flags: review?(florian)
Whiteboard: [checkin-needed]
Assignee: clokep → aleth
Comment on attachment 8354798 [details] [diff] [review]
Patch

*** Original change on bio 1454 attmnt 3017 at 2013-11-11 16:17:11 UTC ***

>diff --git a/chat/components/public/prplIConversation.idl b/chat/components/public/prplIConversation.idl
>--- a/chat/components/public/prplIConversation.idl
>+++ b/chat/components/public/prplIConversation.idl

>-  /* The normalized name of the conversation (suitable for a log file name) */
>+  /* A name that can be used to check for duplicates and is the basis */
>+  /* for the directory name for log storage. */
>   readonly attribute AUTF8String normalizedName;

Nit: the comment style doesn't match what we usually do.

We usually do:
/* Blah blah blah
   blah blah. */
Attachment #8354798 - Flags: review?(florian) → review+
Attached patch PatchSplinter Review
*** Original post on bio 1454 as attmnt 3037 at 2013-11-11 16:18:00 UTC ***

Nit fix, r+ carried forward.
Attachment #8354818 - Flags: review+
Comment on attachment 8354798 [details] [diff] [review]
Patch

*** Original change on bio 1454 attmnt 3017 at 2013-11-11 16:18:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354798 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
*** Original post on bio 1454 at 2013-11-11 18:17:28 UTC ***

This didn't actually apply, but I fixed it on checkin.

http://hg.instantbird.org/instantbird/rev/29a88d2a25ed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.