Closed
Bug 814414
Opened 12 years ago
Closed 11 years ago
Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false
Categories
(Thunderbird :: Toolbars and Tabs, defect)
Thunderbird
Toolbars and Tabs
Tracking
(thunderbird18 wontfix, thunderbird19 fixed, thunderbird20 fixed, thunderbird-esr1717+ fixed)
RESOLVED
FIXED
Thunderbird 20.0
People
(Reporter: sergei.ivn+bugzilla, Assigned: Paenglab)
References
Details
Attachments
(1 file, 1 obsolete file)
2.20 KB,
patch
|
florian
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-esr17+
|
Details | Diff | Splinter Review |
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.
See Also: → TB-AppMenu
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
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•12 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•12 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
Updated•12 years ago
|
tracking-thunderbird-esr17:
--- → 17+
Assignee | ||
Comment 2•12 years ago
|
||
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?
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
Florian, please could you review this? Maybe this would be something for ESR17 (especially for enterprises).
Comment 6•12 years ago
|
||
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•12 years ago
|
Keywords: checkin-needed
Comment 7•12 years ago
|
||
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+
Comment 8•11 years ago
|
||
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
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
Updated•11 years ago
|
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.
Description
•