Icons on toolbar is disabled if you focus is on one Inbox email without switch folders

NEW
Assigned to

Status

Thunderbird
Toolbars and Tabs
8 years ago
8 years ago

People

(Reporter: emily chen, Assigned: Boying Lu)

Tracking

x86
OpenSolaris

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
Steps: 
1. Delete .thunderbird profile
2. Create a account like gmail account
3. Focus on Inbox, it takes time to download and list the emails in your inbox 
4. Click and Focus on one email 
5. Right click on the toolbar-> Customize..., drag and drop one item icon to toolbar, for example, drag "delete" and "reply "button 
6. The  "delete" and "reply "button is disabled even you focus on one email. 
7. If you switch to other folder and switch back to "Inbox" folder, then focus on one email, the  "delete" and "reply "buttons works now. They are enabled after switch folder.
(Assignee)

Comment 1

8 years ago
Created attachment 419651 [details] [diff] [review]
patch
Attachment #419651 - Flags: review?(bienvenu)

Updated

8 years ago
Attachment #419651 - Flags: review?(bienvenu) → review?(bugmail)

Comment 2

8 years ago
Comment on attachment 419651 [details] [diff] [review]
patch

Thx for the patch. I'm not the best person to review this - asuth or someone he defers to would be better, but a few comments:

Mozilla style is not to have the space after the '('. We also try to use let instead of var where we can.

+  for ( var i = 0; i < children.length; i++)

space after the ',' here:

+  var children,disabled;

(and use let)

this code:

+  if (observed_item_id)
+  {
+    observed_item = document.getElementById(observed_item_id);
+    disabled = observed_item.getAttribute("disabled");
+    if (disabled)
+      root.disabled = true;
+    else 
+      root.disabled = false;
+  }

can be something like:

if (observed_item_id)
  root.disabled = document.getElementById(observed_item_id).getAttribute("disabled");
(Assignee)

Comment 3

8 years ago
Created attachment 419867 [details] [diff] [review]
revised patch based on David's comments
Assignee: nobody → brian.lu
Attachment #419651 - Attachment is obsolete: true
Attachment #419867 - Flags: review?(bugmail)
Attachment #419651 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Attachment #419867 - Attachment is obsolete: true
Attachment #419867 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Attachment #419867 - Flags: review?(bugmail)
(Assignee)

Comment 4

8 years ago
Created attachment 420489 [details] [diff] [review]
remove unused variables and revised based on David's comments
Attachment #420489 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Attachment #419867 - Flags: review?(bugmail)
Attachment #420489 - Attachment is patch: true
Attachment #420489 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 420489 [details] [diff] [review]
remove unused variables and revised based on David's comments

This seems overkill.  It looks like this bug is only happening because someone long ago misunderstood how 'this' works or we change things up or something.  Attaching a patch with my proposed counter-patch next.
Attachment #420489 - Flags: review?(bugmail) → review-
Created attachment 421151 [details] [diff] [review]
call UpdateMailToolbar after customization to trigger re-evaluation of all toolbar items

Thank you for the previous patch.  Can you verify that this handles the cases you intended your patch to handle?  If this does not handle all the cases, something along the lines of this is preferred; we don't need to be particularly efficient given that the conclusion of the customization dialog is a very rare event... re-evaluating everything should be fine.
Attachment #420489 - Attachment is obsolete: true
Attachment #421151 - Flags: review?(brian.lu)
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> Created an attachment (id=421151) [details]
> call UpdateMailToolbar after customization to trigger re-evaluation of all
> toolbar items
> 
> Thank you for the previous patch.  Can you verify that this handles the cases
> you intended your patch to handle?  If this does not handle all the cases,
> something along the lines of this is preferred; we don't need to be
> particularly efficient given that the conclusion of the customization dialog is
> a very rare event... re-evaluating everything should be fine.

I tried your patch on the latest trunk code. But it doesn't solve the problem. :(

I'll continue to work on your patch.
(Assignee)

Comment 8

8 years ago
(In reply to comment #6)
> Created an attachment (id=421151) [details]
> call UpdateMailToolbar after customization to trigger re-evaluation of all
> toolbar items
> 
> Thank you for the previous patch.  Can you verify that this handles the cases
> you intended your patch to handle?  If this does not handle all the cases,
> something along the lines of this is preferred; we don't need to be
> particularly efficient given that the conclusion of the customization dialog is
> a very rare event... re-evaluating everything should be fine.
Sorry, I'm not a native English speaker. Do you mean we don't need to figure out what should be updated and what not. We can just update every toolbar item?
(Assignee)

Updated

8 years ago
Attachment #421151 - Flags: review?(brian.lu) → review-
(In reply to comment #8)
> Sorry, I'm not a native English speaker. Do you mean we don't need to figure
> out what should be updated and what not. We can just update every toolbar item?

Exactly.

It's possible UpdateMailToolbar is actually only updating a single commandset which may not contain all the commands (including the one you are testing)... what icon were you adding/removing for testing purposes?
You need to log in before you can comment on or make changes to this bug.