Closed
Bug 954887
Opened 10 years ago
Closed 10 years ago
Add explanatory comments to document normalizedNames
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: FeuerFliege, Assigned: aleth)
References
Details
Attachments
(1 file, 6 obsolete files)
4.75 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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>
Comment 1•10 years ago
|
||
*** 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
Comment 2•10 years ago
|
||
*** 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)
Updated•10 years ago
|
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
*** 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?
Assignee | ||
Comment 5•10 years ago
|
||
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-
Comment 6•10 years ago
|
||
*** 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
Assignee | ||
Comment 7•10 years ago
|
||
*** 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
Comment 8•10 years ago
|
||
*** 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...
Comment 9•10 years ago
|
||
*** 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.
Comment 10•10 years ago
|
||
*** 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 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
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-
Comment 13•10 years ago
|
||
*** 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 14•10 years ago
|
||
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
Assignee | ||
Comment 15•10 years ago
|
||
*** 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)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
*** 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)
Assignee | ||
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Severity: normal → trivial
Summary: normalizedName for JS-IRC is wrong → Add explanatory comments to document normalizedNames
Assignee | ||
Comment 20•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 21•10 years ago
|
||
*** 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)
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
*** 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)
Assignee | ||
Comment 24•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Updated•10 years ago
|
Assignee: clokep → aleth
Comment 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
*** 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+
Assignee | ||
Comment 27•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 28•10 years ago
|
||
*** 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.
Description
•