Last Comment Bug 780200 - FillAttachmentListPopup() is fixed to ID attachmentMenuList
: FillAttachmentListPopup() is fixed to ID attachmentMenuList
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Mike Conley (:mconley)
Depends on:
Blocks: TB-AppMenu
  Show dependency treegraph
Reported: 2012-08-03 10:02 PDT by Richard Marti (:Paenglab)
Modified: 2012-08-21 12:53 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 (12.16 KB, patch)
2012-08-03 12:06 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v2 (14.72 KB, patch)
2012-08-04 11:42 PDT, Mike Conley (:mconley)
squibblyflabbetydoo: review-
Details | Diff | Splinter Review
Patch v3 (14.77 KB, patch)
2012-08-13 10:08 PDT, Mike Conley (:mconley)
squibblyflabbetydoo: review+
Details | Diff | Splinter Review

Description Richard Marti (:Paenglab) 2012-08-03 10:02:27 PDT
FillAttachmentListPopup() looks as it accepts every popup ID to fill the needed content but only works when the ID is attachmentMenuList.

I need this function in bug 650170 to fill my popup appmenu_attachmentMenuList.
Comment 1 Mike Conley (:mconley) 2012-08-03 12:06:47 PDT
Created attachment 648797 [details] [diff] [review]
Patch v1

This patch updates the oncommand stuff in the attachment menupopup to use the traditional XUL command pattern.

Also adds overlay nodes so that the attachment menu in the appmenu from bug 650170 will work.

How's my driving, squib? Did I miss any corner cases here, and am I OK to attach the AttachmentMenuController to top.controllers?

Comment 2 Richard Marti (:Paenglab) 2012-08-03 13:41:06 PDT
Mike, thanks for the patch. It works on both menus but not together. This means when a message with attachments is selected and I open the main attachment menu, the attachments are shown. Now I go to the AppMenu's attachment menu, no attachment but only the four lower items are shown. I change now to a other message and open the AppMenu's attachment menu and the new attachment is shown. On main attachment menu there is still the old message's attachment shown.

It is very unlikely someone uses both menus for the same message but if he change his usage he can be surprised by this.
Comment 3 Jim Porter (:squib) 2012-08-04 01:29:42 PDT
Comment on attachment 648797 [details] [diff] [review]
Patch v1

Review of attachment 648797 [details] [diff] [review]:

Clearing review based on comment 2, but I don't see anything obviously wrong with the code aside from that.
Comment 4 Mike Conley (:mconley) 2012-08-04 11:42:54 PDT
Created attachment 649021 [details] [diff] [review]
Patch v2

Ok, found the problem there - there was a global preventing re-entry into the function that populates the menupopup with the files.

That global looked like a kludge to deal with the behaviour of how the onpopupshowing event works (it bubbles up, and gets fired on parents as well). I removed the global, and just stop the onpopupshowing event from propagating when it's fired from the sub menupopup (the one that lists the options for individual files).
Comment 5 Richard Marti (:Paenglab) 2012-08-04 13:47:10 PDT
Yeah, now it works on both menupopups.
Comment 6 Jim Porter (:squib) 2012-08-04 15:06:01 PDT
I haven't run with this yet, but we may want to look at fixing bug 514785 in this bug as well. We'll face similar issues with enabling/disabling the appmenu item as in that bug.
Comment 7 Jim Porter (:squib) 2012-08-10 21:27:31 PDT
Comment on attachment 649021 [details] [diff] [review]
Patch v2

Review of attachment 649021 [details] [diff] [review]:

The context menu in the attachment list (attachmentListContext) doesn't properly update its menuitem states when you open it. I think the rest is ok, though.
Comment 8 Mike Conley (:mconley) 2012-08-13 09:54:13 PDT
(In reply to Jim Porter (:squib) from comment #6)
> I haven't run with this yet, but we may want to look at fixing bug 514785 in
> this bug as well. We'll face similar issues with enabling/disabling the
> appmenu item as in that bug.

So I looked into that, and I'm concerned that it might be more trouble than it's worth to fix this one bug (since Richard wants this for 17). The problem, is that the attachment menu stuff is loaded in and functions in msgHdrViewOverlay.xul/.js which, of course, is not loaded if message previewing is off.

We might want to move that stuff outside of msgHdrViewOverlay.xul/.js somewhere down the line, but at this point, I think it's out of scope.

Thanks for the review and for finding that bug! New patch coming soon...
Comment 9 Mike Conley (:mconley) 2012-08-13 10:08:54 PDT
Created attachment 651444 [details] [diff] [review]
Patch v3

This patch makes the attachmentListContext menuitems refresh themselves again.
Comment 10 Mike Conley (:mconley) 2012-08-20 13:51:05 PDT
Hey Jim, just a heads up - this patch is required to land before bug 650170 can land, and we want 650170 for TB 17 (1 week).

If you don't have cycles for this patch, let me know, and I'll redirect it.


Comment 11 Jim Porter (:squib) 2012-08-20 13:55:06 PDT
I'll take a look at it this evening...
Comment 12 Jim Porter (:squib) 2012-08-21 02:52:57 PDT
Comment on attachment 651444 [details] [diff] [review]
Patch v3

Review of attachment 651444 [details] [diff] [review]:

Looks good to me!
Comment 13 Mike Conley (:mconley) 2012-08-21 06:23:49 PDT
Thanks, Jim!
Comment 14 Mike Conley (:mconley) 2012-08-21 12:53:32 PDT

Note You need to log in before you can comment on or make changes to this bug.