Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false

RESOLVED FIXED in Thunderbird 20.0

Status

Thunderbird
Toolbars and Tabs
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Serg Iv, Assigned: Paenglab)

Tracking

Trunk
Thunderbird 20.0

Thunderbird Tracking Flags

(thunderbird18 wontfix, thunderbird19 fixed, thunderbird20 fixed, thunderbird-esr1717+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

mail.chat.enabled to false


Actual results:

Appmenu entries "Go -> Chat" and "Tools -> Chat status" still here


Expected results:

No menu entries for chats.
(Reporter)

Updated

5 years ago
See Also: → bug 650170
(Reporter)

Updated

5 years ago
Summary: Menu entries for chat is in AppMenu with mail.chat.enabled set to disable → Menu entries for chat is in AppMenu when mail.chat.enabled set to disable
(Reporter)

Updated

5 years ago
Summary: Menu entries for chat is in AppMenu when mail.chat.enabled set to disable → Menu entries for chat is in AppMenu when mail.chat.enabled set to false
confirming per BWinton's Bug 650170 Comment 99.
Blocks: 650170
Status: UNCONFIRMED → NEW
Component: Untriaged → Toolbars and Tabs
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: 17 → Trunk
Severity: normal → minor
Summary: Menu entries for chat is in AppMenu when mail.chat.enabled set to false → Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false
tracking-thunderbird-esr17: --- → 17+
(Assignee)

Comment 2

5 years ago
Created attachment 684734 [details] [diff] [review]
patch

This patch removes the menuitems.

I tried also to combine two menuitems like you suggested in bug 650170 comment 100, but this didn't work:

  setStatusMenupopupCommand: function(aEvent) {
    let target = aEvent.originalTarget;
    if (target.getAttribute("id") == "imStatusShowAccounts" || "appmenu_imStatusShowAccounts") {
      openIMAccountMgr();
      return;
    }

Instead of the status change only the IMAccountMgr opens.

Florian, what do you think, is this patch safe for 17ESR?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #684734 - Flags: review?(florian)
(In reply to Richard Marti [:Paenglab] from comment #2)

Shouldn't appmenu_afterChatSeparator also be hidden?


>   setStatusMenupopupCommand: function(aEvent) {
>     let target = aEvent.originalTarget;
>     if (target.getAttribute("id") == "imStatusShowAccounts" ||
> "appmenu_imStatusShowAccounts") {
>       openIMAccountMgr();
>       return;
>     }

I suggested:
     if (target.getAttribute("id") == "imStatusShowAccounts") ||
         target.getAttribute("id") == "appmenu_imStatusShowAccounts")) {
(Assignee)

Comment 4

5 years ago
Created attachment 684792 [details] [diff] [review]
patch v2

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Richard Marti [:Paenglab] from comment #2)
> 
> Shouldn't appmenu_afterChatSeparator also be hidden?

fixed

> I suggested:
>      if (target.getAttribute("id") == "imStatusShowAccounts") ||
>          target.getAttribute("id") == "appmenu_imStatusShowAccounts")) {

This works after removing of two braces.
Attachment #684734 - Attachment is obsolete: true
Attachment #684734 - Flags: review?(florian)
Attachment #684792 - Flags: review?(florian)
(Assignee)

Comment 5

4 years ago
Florian, please could you review this? Maybe this would be something for ESR17 (especially for enterprises).
Comment on attachment 684792 [details] [diff] [review]
patch v2

I have some doubts on appmenu_afterChatSeparator, but it shouldn't matter much, and I assume you have tested the patch and it seemed fine to you when looking at that menu with chat disabled.

Sorry for the delay here :-/.

This indeed seems safe for esr17.

[Approval Request Comment]
Regression caused by (bug #): bug 650170
User impact if declined: Some chat menu items will be visible when chat is pref'ed off, which may happen in enterprise contexts.
Testing completed (on c-c, etc.): none by me. I hope Richard has tested the patch.
Risk to taking this patch (and alternatives if risky): Extremely low (even if there was a typo in some ids, there's a null check in the code that would ensure the typo doesn't break anything...).
Attachment #684792 - Flags: review?(florian)
Attachment #684792 - Flags: review+
Attachment #684792 - Flags: approval-comm-esr17?
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Comment on attachment 684792 [details] [diff] [review]
patch v2

[Triage Comment]
Please land an aurora as well. We shouldn't need to land on beta as we're not releasing that.
Attachment #684792 - Flags: approval-comm-esr17?
Attachment #684792 - Flags: approval-comm-esr17+
Attachment #684792 - Flags: approval-comm-aurora+
https://hg.mozilla.org/comm-central/rev/248eb4d0013c
https://hg.mozilla.org/releases/comm-aurora/rev/798271c299a7
https://hg.mozilla.org/releases/comm-esr17/rev/66343c7d1323
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
status-thunderbird18: --- → wontfix
status-thunderbird19: --- → fixed
status-thunderbird20: --- → fixed
status-thunderbird-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.