Closed Bug 955553 Opened 10 years ago Closed 10 years ago

Provide a way to obtain the normalizedName of a nick

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

()

Details

Attachments

(2 files, 11 obsolete files)

*** Original post on bio 2115 at 2013-08-19 15:58:00 UTC ***

There is currently no way to obtain the normalizedName corresponding to a participant name. Therefore we are not able to reliably e.g. find existing logs or existing buddies, because the API calls for this expect normalizedNames.
*** Original post on bio 2115 at 2013-10-14 17:09:55 UTC ***

Basically this involves adding a method to the API to run whatever function the normalizedName getter applies, only on an arbitrary string.
Blocks: 953891, 955655
Blocks: 954979
Blocks: 955698
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3071 at 2013-11-23 12:08:00 UTC ***

This adds (for the JS protos) a getNormalizedNameForBuddy and getNormalizedNameForConversation to the accounts and a getNormalizedNameForAccount to the protocols.

I'm not sure the normalizedNames for XMPP buddies and conversations are correct the way they are, but that would be a different bug.

What's missing are the IDL changes and the implementation for prplxpcom (which should be easier). It's also obviously untested.

Over to you ;)
Attachment #8354852 - Flags: feedback?(clokep)
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3072 at 2013-11-23 14:01:00 UTC ***

Keeps the normalize function implementations on the account and refers to them statically from the protocol.
Attachment #8354853 - Flags: feedback?(clokep)
Comment on attachment 8354852 [details] [diff] [review]
Patch

*** Original change on bio 2115 attmnt 3071 at 2013-11-23 14:01:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354852 - Attachment is obsolete: true
Attachment #8354852 - Flags: feedback?(clokep)
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3073 at 2013-11-23 14:12:00 UTC ***

TwitterAccount did not exist.
Attachment #8354854 - Flags: feedback?(clokep)
Comment on attachment 8354853 [details] [diff] [review]
Patch v2

*** Original change on bio 2115 attmnt 3072 at 2013-11-23 14:12:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354853 - Attachment is obsolete: true
Attachment #8354853 - Flags: feedback?(clokep)
Comment on attachment 8354853 [details] [diff] [review]
Patch v2

*** Original change on bio 2115 attmnt 3072 at 2013-11-23 14:13:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354853 - Attachment description: Patch → Patch v2
Comment on attachment 8354854 [details] [diff] [review]
Patch v3

*** Original change on bio 2115 attmnt 3073 at 2013-11-23 14:13:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354854 - Attachment description: Patch → Patch v3
Attached patch Patch v4 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3074 at 2013-11-23 14:31:00 UTC ***

aString -> aName
Attachment #8354855 - Flags: feedback?(clokep)
Comment on attachment 8354854 [details] [diff] [review]
Patch v3

*** Original change on bio 2115 attmnt 3073 at 2013-11-23 14:31:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354854 - Attachment is obsolete: true
Attachment #8354854 - Flags: feedback?(clokep)
Attached patch Patch v5 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3075 at 2013-11-23 16:48:00 UTC ***

Trivial improvements.
Attachment #8354856 - Flags: feedback?(clokep)
Comment on attachment 8354855 [details] [diff] [review]
Patch v4

*** Original change on bio 2115 attmnt 3074 at 2013-11-23 16:48:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354855 - Attachment is obsolete: true
Attachment #8354855 - Flags: feedback?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3076 at 2013-11-23 18:24:00 UTC ***

So after discussing this on IRC, we decided to drop getNormalizedNameForAccount as there is currently no use case for it.

getNormalizedNameForBuddy and getNormalizedNameForConversation are now just getNormalizedName(), which is possible as no current protocol normalizes MUCs, conversations, and usernames differently.
Attachment #8354857 - Flags: feedback?(clokep)
Comment on attachment 8354856 [details] [diff] [review]
Patch v5

*** Original change on bio 2115 attmnt 3075 at 2013-11-23 18:24:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354856 - Attachment is obsolete: true
Attachment #8354856 - Flags: feedback?(clokep)
*** Original post on bio 2115 at 2013-11-23 18:27:25 UTC ***

I wonder if getNormalizedName is a good name (since it won't work for account normalizednames), maybe whoever adds the IDL/prplxpcom side of this will have a better idea...
Comment on attachment 8354857 [details] [diff] [review]
Patch

*** Original change on bio 2115 attmnt 3076 at 2013-11-23 18:29:52 UTC ***

Tentatively this looks reasonable.
Attachment #8354857 - Flags: feedback?(clokep) → feedback+
Attached patch Patch v6 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3079 at 2013-11-24 19:42:00 UTC ***

I believe I misunderstood the XMPP code. A getNormalizedName for XMPP should always use normalizeJID. The reason the existing code always just returns the unmodified name is because for XMPP, that's always already the normalized JID.
Attachment #8354860 - Flags: feedback?(clokep)
Comment on attachment 8354857 [details] [diff] [review]
Patch

*** Original change on bio 2115 attmnt 3076 at 2013-11-24 19:42:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354857 - Attachment is obsolete: true
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354860 [details] [diff] [review]
Patch v6

*** Original change on bio 2115 attmnt 3079 at 2013-11-24 19:44:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354860 - Attachment description: Patch → Patch v6
Attached patch Patch v7 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3080 at 2013-11-24 20:00:00 UTC ***

flo suggested we just call getNormalizedName normalize(), which matches what we already use in various prpls.

Together with the fact that for all prpls, we now use the same normalization function for all normalizedNames (buddies, conversations, accounts) this makes for a nice simplification :)
Attachment #8354861 - Flags: feedback?(clokep)
Comment on attachment 8354860 [details] [diff] [review]
Patch v6

*** Original change on bio 2115 attmnt 3079 at 2013-11-24 20:00:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354860 - Attachment is obsolete: true
Attachment #8354860 - Flags: feedback?(clokep)
Comment on attachment 8354861 [details] [diff] [review]
Patch v7

*** Original change on bio 2115 attmnt 3080 at 2013-11-25 11:49:57 UTC ***

This looks fine to me (for IRC + Twitter + jsProtoHelper), I'd like Florian to look over the XMPP code. I'll take a whack at adding this stuff to the interfaces / purplexpcom soon.
Attachment #8354861 - Flags: feedback?(clokep) → feedback+
Attachment #8354861 - Flags: feedback?(florian)
Comment on attachment 8354861 [details] [diff] [review]
Patch v7

*** Original change on bio 2115 attmnt 3080 at 2013-11-25 11:56:22 UTC ***

I looked yesterday; it seemed fine, although:
- the patch would be less confusing to look at if it included the idl changes
- private conversations with JS-XMPP MUC participants may now require even more hacks than before; but I think we agreed to not block on this edge case until it's actually supported.
Attachment #8354861 - Flags: feedback?(florian) → feedback+
*** Original post on bio 2115 at 2013-11-25 16:53:17 UTC ***

(In reply to comment #13)
> - private conversations with JS-XMPP MUC participants may now require even more
> hacks than before; but I think we agreed to not block on this edge case until
> it's actually supported.

I don't think this patch makes anything worse, it only clarifies the status quo. So if/when XMMP MUC PMs are supported, it should be clearer what needs to be changed and what it will affect.
Attached patch Patch v7 to xpcom changes (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3083 at 2013-11-27 17:41:00 UTC ***

Note that I made the patch author aleth so really I'm a reviewer and he's an author...but he needs to look over this again.
Attachment #8354864 - Flags: review?(florian)
Attachment #8354864 - Flags: review?(aleth)
Comment on attachment 8354861 [details] [diff] [review]
Patch v7

*** Original change on bio 2115 attmnt 3080 at 2013-11-27 17:41:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354861 - Attachment is obsolete: true
Comment on attachment 8354864 [details] [diff] [review]
Patch v7 to xpcom changes

*** Original change on bio 2115 attmnt 3083 at 2013-11-27 17:46:52 UTC ***


>+  // Request the account normalize a name for use in comparisions when an object
>+  // may not yet exist.
>+  AUTF8String normalize(in AUTF8String aBuddyName);

"buddy" is confusing here. Just go with aName.

>+/* AUTF8String normalize(in AUTF8String aBuddyName); */

Change it here too.

>+NS_IMETHODIMP purpleAccount::Normalize(const nsACString & aName, nsACString & aNormalizedName)

Note: you already named it aName here ;).

Seems reasonable otherwise. Will make aleth happy! ;)
Attachment #8354864 - Flags: review?(florian) → review+
Comment on attachment 8354864 [details] [diff] [review]
Patch v7 to xpcom changes

*** Original change on bio 2115 attmnt 3083 at 2013-11-27 17:50:47 UTC ***

Thanks for looking at this!

>   // 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;
>+  // Request the account normalize a name for use in comparisions when an object
>+  // may not yet exist.
>+  AUTF8String normalize(in AUTF8String aBuddyName);

aBuddyName -> aName. This is not just for buddies.

And since this confusion seems to have crept in, I would suggest a more explicit comment like
"Normalize a name for use in comparisons or as an identifier when an object may not exist. This method is used to generate the normalizedNames for accounts, accountbuddies, and conversations."
Attachment #8354864 - Flags: review?(aleth) → review-
*** Original post on bio 2115 at 2013-11-27 17:57:27 UTC ***

(In reply to comment #17)

> "Normalize a name for use in comparisons or as an identifier when an object may
> not exist."

I almost wrote a review comment here too. I think "when an object may not exist" could be expended to "when an object providing a normalizedName doesn't exist yet or isn't accessible."


> "This method is used to generate the normalizedNames for accounts,
> accountbuddies, and conversations."

This second sentence isn't necessarily true. Don't include it, or say something like "some prpl..." in it.
Attached patch Patch v8 (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3084 at 2013-11-27 17:59:00 UTC ***

Meets review comments.
Attachment #8354865 - Flags: review?(florian)
Attachment #8354865 - Flags: review?(aleth)
Comment on attachment 8354864 [details] [diff] [review]
Patch v7 to xpcom changes

*** Original change on bio 2115 attmnt 3083 at 2013-11-27 17:59:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354864 - Attachment is obsolete: true
Attached patch Patch v9Splinter Review
*** Original post on bio 2115 as attmnt 3085 at 2013-11-27 18:08:00 UTC ***

Updated comment.
Attachment #8354866 - Flags: review?(florian)
Attachment #8354866 - Flags: review?(aleth)
Comment on attachment 8354865 [details] [diff] [review]
Patch v8

*** Original change on bio 2115 attmnt 3084 at 2013-11-27 18:08:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354865 - Attachment is obsolete: true
Attachment #8354865 - Flags: review?(florian)
Attachment #8354865 - Flags: review?(aleth)
Comment on attachment 8354866 [details] [diff] [review]
Patch v9

*** Original change on bio 2115 attmnt 3085 at 2013-11-27 18:10:04 UTC ***

Thanks! Let's hope it's the last normalize bug for a while...
Attachment #8354866 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
Comment on attachment 8354866 [details] [diff] [review]
Patch v9

*** Original change on bio 2115 attmnt 3085 at 2013-11-27 18:33:01 UTC ***

Thanks!
Attachment #8354866 - Flags: review?(florian) → review+
*** Original post on bio 2115 at 2013-11-27 22:55:21 UTC ***

http://hg.instantbird.org/instantbird/rev/88a22847c66a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
Attached patch Add imIAccount implementation (obsolete) — Splinter Review
*** Original post on bio 2115 as attmnt 3091 at 2013-11-28 14:12:00 UTC ***

We didn't do the imAccount implementation.
Attachment #8354874 - Flags: review?(florian)
Comment on attachment 8354874 [details] [diff] [review]
Add imIAccount implementation

*** Original change on bio 2115 attmnt 3091 at 2013-11-28 14:13:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354874 - Flags: review?(florian) → review+
Comment on attachment 8354874 [details] [diff] [review]
Add imIAccount implementation

*** Original change on bio 2115 attmnt 3091 at 2013-11-28 14:16:18 UTC ***

Actually I don't think normalize() should be in the UnknownProtocol. There does not seem to be an UnknownAccount...
Attachment #8354874 - Flags: review+ → review-
*** Original post on bio 2115 as attmnt 3092 at 2013-11-28 14:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354875 - Flags: review?(aleth)
Comment on attachment 8354874 [details] [diff] [review]
Add imIAccount implementation

*** Original change on bio 2115 attmnt 3091 at 2013-11-28 14:17:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354874 - Attachment is obsolete: true
Comment on attachment 8354875 [details] [diff] [review]
Add imIAccount implementation v2

*** Original change on bio 2115 attmnt 3092 at 2013-11-28 14:18:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354875 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2115 at 2013-11-28 14:21:05 UTC ***

http://hg.instantbird.org/instantbird/rev/c8ff45f03f66
Whiteboard: [checkin-needed]
Blocks: 955709
You need to log in before you can comment on or make changes to this bug.