955709 - Fix the getNormalizedName stub in nsContextMenu

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[aleth [:aleth]]

*** Original post on bio 2260 at 2013-11-28 14:52:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2260 as attmnt 3094 at 2013-11-28 14:52:00 UTC ***

For the logs case I am not sure what the right thing to do is. The question is
what the normalizedName will be *for a private conversation opened with a MUC
chatbuddy*. (For IRC it makes no difference whether we use
getNormalizedChatBuddyName or normalize in this case.)

> And the getLogsForAccountAndName implementation likely wants to call
> account.normalize; rather than the caller.

Since we have misgivings about MUC usernames, we should not rely on normalize()
giving the desired result here. So I'm keeping it outside logger.js.

Comment 2

[aleth [:aleth]]

Attachment: Patch2

*** Original post on bio 2260 as attmnt 3095 at 2013-11-28 15:20:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 3

[aleth [:aleth]]

Comment on attachment 8354877 [details] [diff] [review]
Patch

*** Original change on bio 2260 attmnt 3094 at 2013-11-28 15:20:14 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 4

[aleth [:aleth]]

Attachment: Patch3

*** Original post on bio 2260 as attmnt 3096 at 2013-11-28 15:35:00 UTC ***

This is a little more consistent, but it still feels a bit hackish.

Comment 5

[aleth [:aleth]]

Comment on attachment 8354878 [details] [diff] [review]
Patch2

*** Original change on bio 2260 attmnt 3095 at 2013-11-28 15:35:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Florian Quèze [:florian]]

*** Original post on bio 2260 at 2013-11-28 17:28:25 UTC ***

Comment on attachment 8354879 [details] [diff] [review] (bio-attmnt 3096)
Patch3

>diff --git a/instantbird/content/nsContextMenu.js b/instantbird/content/nsContextMenu.js

>   getLogsForNick: function(aNick) {

>+    let account = this.conv.account;
>+    // We need the normalizedName of private conversations opened
>+    // with a MUC chatbuddy.
>+    let normalizedName =
>+      account.normalize(this.conv.target.getNormalizedChatBuddyName(aNick));

Haven't we said the account.normalize call should be done inside logger.js?


>-    let isAddContact = this.onNick && !isTwitter && !this.buddy;
>+
>+    let isAddContact = this.onNick && !isTwitter;
>+    let account = this.conv.account;
>+    // We don't want to support adding chatBuddies as contacts if we have no
>+    // way of finding out if such a contact already exists. This is a problem
>+    // in particular for XMPP MUCs.
>+    let normalizedName =
>+      account.normalize(this.conv.target.getNormalizedChatBuddyName(this.nick));
>+    if (normalizedName == account.normalize(this.nick)) {

What I suggested on IRC is:
let normalizedNick = this.conv.target.getNormalizedChatBuddyName(this.nick);
if (account.normalize(normalizedNick) == normalizedNick)
  // allow adding the contact

Can you explain why what's in the patch does something else?

Comment 7

[aleth [:aleth]]

Attachment: Patch4

*** Original post on bio 2260 as attmnt 3098 at 2013-11-28 17:38:00 UTC ***

(In reply to comment #3)
> >+    let normalizedName =
> >+      account.normalize(this.conv.target.getNormalizedChatBuddyName(aNick));
> 
> Haven't we said the account.normalize call should be done inside logger.js?

It would not be consistent with yesterday's decision that not all normalizedNames necessarily have to come from applying normalize().

> >+    let normalizedName =
> >+      account.normalize(this.conv.target.getNormalizedChatBuddyName(this.nick));
> >+    if (normalizedName == account.normalize(this.nick)) {
> 
> What I suggested on IRC is:
> let normalizedNick = this.conv.target.getNormalizedChatBuddyName(this.nick);
> if (account.normalize(normalizedNick) == normalizedNick)
>   // allow adding the contact
> 
> Can you explain why what's in the patch does something else?

The idea is to check that the normalizedName of the nick would match that generated from the normalizedChatBuddyName.

Comment 8

[aleth [:aleth]]

Comment on attachment 8354879 [details] [diff] [review]
Patch3

*** Original change on bio 2260 attmnt 3096 at 2013-11-28 17:38:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Florian Quèze [:florian]]

*** Original post on bio 2260 at 2013-11-28 19:07:27 UTC ***

(In reply to comment #4)
> Created attachment 8354881 [details] [diff] [review] (bio-attmnt 3098) [details]
> Patch4
> 
> (In reply to comment #3)
> > >+    let normalizedName =
> > >+      account.normalize(this.conv.target.getNormalizedChatBuddyName(aNick));
> > 
> > Haven't we said the account.normalize call should be done inside logger.js?
> 
> It would not be consistent with yesterday's decision that not all
> normalizedNames necessarily have to come from applying normalize().

OK. I'm not enthusiastic about this, but it makes some reasonable amount of sense.

> > >+    let normalizedName =
> > >+      account.normalize(this.conv.target.getNormalizedChatBuddyName(this.nick));
> > >+    if (normalizedName == account.normalize(this.nick)) {
> > 
> > What I suggested on IRC is:
> > let normalizedNick = this.conv.target.getNormalizedChatBuddyName(this.nick);
> > if (account.normalize(normalizedNick) == normalizedNick)
> >   // allow adding the contact
> > 
> > Can you explain why what's in the patch does something else?
> 
> The idea is to check that the normalizedName of the nick would match that
> generated from the normalizedChatBuddyName.

The case I had in mind is:
- the nick is "foo"
- the normalized chat buddy name is [e-mail] (because this XMPP server doesn't hide the JID of the chat room participants, so the prpl can return something useful; which doesn't necessarily contain the nick).

In the case where the server hides the JIDs, the normalized chat buddy name would be "[e-mail]/foo" so your code would work.

Comment 10

[aleth [:aleth]]

*** Original post on bio 2260 at 2013-11-28 19:18:10 UTC ***

(In reply to comment #5)
> > It would not be consistent with yesterday's decision that not all
> > normalizedNames necessarily have to come from applying normalize().
> 
> OK. I'm not enthusiastic about this, but it makes some reasonable amount of
> sense.

I'm not super enthusiastic about it either. If you prefer we can move the normalize(), but we will have to remember to check where it is called when adding XMPP MUC PMs.

> > > What I suggested on IRC is:
> > > let normalizedNick = this.conv.target.getNormalizedChatBuddyName(this.nick);
> > > if (account.normalize(normalizedNick) == normalizedNick)
> > >   // allow adding the contact
> > > 
> > > Can you explain why what's in the patch does something else?
> > 
> > The idea is to check that the normalizedName of the nick would match that
> > generated from the normalizedChatBuddyName.
> 
> The case I had in mind is:
> - the nick is "foo"
> - the normalized chat buddy name is [e-mail] (because this XMPP
> server doesn't hide the JID of the chat room participants, so the prpl can
> return something useful; which doesn't necessarily contain the nick).

Thanks, I wasn't aware this might happen.

Comment 11

[Florian Quèze [:florian]]

*** Original post on bio 2260 at 2013-11-28 20:36:10 UTC ***

(In reply to comment #6)
> If you prefer we can move the
> normalize(), but we will have to remember to check where it is called when
> adding XMPP MUC PMs.

Don't bother, it's not a real improvement.

Comment 12

[aleth [:aleth]]

Attachment: Patch5

*** Original post on bio 2260 as attmnt 3099 at 2013-11-29 11:25:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[aleth [:aleth]]

Comment on attachment 8354881 [details] [diff] [review]
Patch4

*** Original change on bio 2260 attmnt 3098 at 2013-11-29 11:25:45 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8354882 [details] [diff] [review]
Patch5

*** Original change on bio 2260 attmnt 3099 at 2013-11-29 11:33:02 UTC ***

Thanks! I'm looking forward to forgetting this issue ever existed :-).

Comment 15

[Patrick Cloke [:clokep]]

*** Original post on bio 2260 at 2013-12-01 23:33:37 UTC ***

http://hg.instantbird.org/instantbird/rev/011df1026413