955553 - Provide a way to obtain the normalizedName of a nick

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[aleth [:aleth]]

*** 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.

Comment 1

[aleth [:aleth]]

*** 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.

Comment 2

[aleth [:aleth]]

Attachment: Patch

*** 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 ;)

Comment 3

[aleth [:aleth]]

Attachment: Patch v2

*** 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.

Comment 4

[aleth [:aleth]]

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 ***

Comment 5

[aleth [:aleth]]

Attachment: Patch v3

*** Original post on bio 2115 as attmnt 3073 at 2013-11-23 14:12:00 UTC ***

TwitterAccount did not exist.

Comment 6

[aleth [:aleth]]

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 ***

Comment 7

[Patrick Cloke [: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 ***

Comment 8

[Patrick Cloke [:clokep]]

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 ***

Comment 9

[aleth [:aleth]]

Attachment: Patch v4

*** Original post on bio 2115 as attmnt 3074 at 2013-11-23 14:31:00 UTC ***

aString -> aName

Comment 10

[aleth [:aleth]]

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 ***

Comment 11

[aleth [:aleth]]

Attachment: Patch v5

*** Original post on bio 2115 as attmnt 3075 at 2013-11-23 16:48:00 UTC ***

Trivial improvements.

Comment 12

[aleth [:aleth]]

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 ***

Comment 13

[aleth [:aleth]]

Attachment: Patch

*** 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.

Comment 14

[aleth [:aleth]]

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 ***

Comment 15

[aleth [:aleth]]

*** 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 16

[Patrick Cloke [:clokep]]

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.

Comment 17

[aleth [:aleth]]

Attachment: Patch v6

*** 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.

Comment 18

[aleth [:aleth]]

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 ***

Comment 19

[aleth [:aleth]]

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 ***

Comment 20

[aleth [:aleth]]

Attachment: Patch v7

*** 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 :)

Comment 21

[aleth [:aleth]]

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 ***

Comment 22

[Patrick Cloke [: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.

Comment 23

[Florian Quèze [: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.

Comment 24

[aleth [:aleth]]

*** 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.

Comment 25

[Patrick Cloke [:clokep]]

Attachment: Patch v7 to xpcom changes

*** 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.

Comment 26

[Patrick Cloke [:clokep]]

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 ***

Comment 27

[Florian Quèze [:florian]]

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! ;)

Comment 28

[aleth [: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: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."

Comment 29

[Florian Quèze [:florian]]

*** 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.

Comment 30

[Patrick Cloke [:clokep]]

Attachment: Patch v8

*** Original post on bio 2115 as attmnt 3084 at 2013-11-27 17:59:00 UTC ***

Meets review comments.

Comment 31

[Patrick Cloke [:clokep]]

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 ***

Comment 32

[Patrick Cloke [:clokep]]

Attachment: Patch v9

*** Original post on bio 2115 as attmnt 3085 at 2013-11-27 18:08:00 UTC ***

Updated comment.

Comment 33

[Patrick Cloke [:clokep]]

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 ***

Comment 34

[aleth [: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...

Comment 35

[Florian Quèze [:florian]]

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!

Comment 36

[Patrick Cloke [:clokep]]

*** Original post on bio 2115 at 2013-11-27 22:55:21 UTC ***

http://hg.instantbird.org/instantbird/rev/88a22847c66a

Comment 37

[Patrick Cloke [:clokep]]

Attachment: Add imIAccount implementation

*** Original post on bio 2115 as attmnt 3091 at 2013-11-28 14:12:00 UTC ***

We didn't do the imAccount implementation.

Comment 38

[aleth [:aleth]]

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 ***

Comment 39

[aleth [:aleth]]

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...

Comment 40

[Patrick Cloke [:clokep]]

Attachment: Add imIAccount implementation v2

*** 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 ***

Comment 41

[Patrick Cloke [:clokep]]

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 ***

Comment 42

[aleth [:aleth]]

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 ***

Comment 43

[Patrick Cloke [:clokep]]

*** Original post on bio 2115 at 2013-11-28 14:21:05 UTC ***

http://hg.instantbird.org/instantbird/rev/c8ff45f03f66