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

RESOLVED FIXED in Thunderbird 20.0

Status

defect
--
minor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sergei.ivn+bugzilla, Assigned: Paenglab)

Tracking

Trunk
Thunderbird 20.0
Dependency tree / graph

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

7 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

7 years ago
See Also: → TB-AppMenu
Reporter

Updated

7 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

7 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

Comment 1

7 years ago
confirming per BWinton's Bug 650170 Comment 99.
Blocks: TB-AppMenu
Status: UNCONFIRMED → NEW
Component: Untriaged → Toolbars and Tabs
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: 17 → Trunk

Updated

7 years ago
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
Assignee

Comment 2

7 years ago
Posted patch patch (obsolete) — Splinter Review
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

7 years ago
Posted patch patch v2Splinter Review
(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

7 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

7 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+
You need to log in before you can comment on or make changes to this bug.