Fix the getNormalizedName stub in nsContextMenu

RESOLVED FIXED in 1.5

Status

Instantbird
Conversation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

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

Updated

4 years ago
Blocks: 953891
Depends on: 955553
(Assignee)

Comment 1

4 years ago
Created attachment 8354877 [details] [diff] [review]
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.
Attachment #8354877 - Flags: review?(florian)
(Assignee)

Comment 2

4 years ago
Created attachment 8354878 [details] [diff] [review]
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 ***
Attachment #8354878 - Flags: review?(florian)
(Assignee)

Comment 3

4 years ago
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 ***
Attachment #8354877 - Attachment is obsolete: true
Attachment #8354877 - Flags: review?(florian)
(Assignee)

Comment 4

4 years ago
Created attachment 8354879 [details] [diff] [review]
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.
Attachment #8354879 - Flags: review?(florian)
(Assignee)

Comment 5

4 years ago
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 ***
Attachment #8354878 - Attachment is obsolete: true
Attachment #8354878 - Flags: review?(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?
(Assignee)

Comment 7

4 years ago
Created attachment 8354881 [details] [diff] [review]
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.
Attachment #8354881 - Flags: review?(florian)
(Assignee)

Comment 8

4 years ago
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 ***
Attachment #8354879 - Attachment is obsolete: true
Attachment #8354879 - Flags: review?(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 foo.bar@server.com (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 "room@conference.server.com/foo" so your code would work.
(Assignee)

Comment 10

4 years ago
*** 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 foo.bar@server.com (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.
*** 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.
(Assignee)

Comment 12

4 years ago
Created attachment 8354882 [details] [diff] [review]
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 ***
Attachment #8354882 - Flags: review?(florian)
(Assignee)

Comment 13

4 years ago
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 ***
Attachment #8354881 - Attachment is obsolete: true
Attachment #8354881 - Flags: review?(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 :-).
Attachment #8354882 - Flags: review?(florian) → review+
(Assignee)

Updated

4 years ago
Whiteboard: [checkin-needed]
*** Original post on bio 2260 at 2013-12-01 23:33:37 UTC ***

http://hg.instantbird.org/instantbird/rev/011df1026413
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
(Assignee)

Updated

4 years ago
Depends on: 955731
You need to log in before you can comment on or make changes to this bug.