The default bug view has changed. See this FAQ.

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

4 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

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

Updated

4 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

4 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

4 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

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