FillAttachmentListPopup() is fixed to ID attachmentMenuList

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Paenglab, Assigned: mconley)

Tracking

unspecified
Thunderbird 17.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
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?

-Mike
Attachment #648797 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 2

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

5 years ago
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.
Attachment #648797 - Flags: review?(squibblyflabbetydoo)
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).
Attachment #648797 - Attachment is obsolete: true
Attachment #649021 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 5

5 years ago
Yeah, now it works on both menupopups.

Comment 6

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

5 years ago
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.
Attachment #649021 - Flags: review?(squibblyflabbetydoo) → review-
(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...
Created attachment 651444 [details] [diff] [review]
Patch v3

This patch makes the attachmentListContext menuitems refresh themselves again.
Attachment #649021 - Attachment is obsolete: true
Attachment #651444 - Flags: review?(squibblyflabbetydoo)
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.

Thanks!

-Mike

Comment 11

5 years ago
I'll take a look at it this evening...

Comment 12

5 years ago
Comment on attachment 651444 [details] [diff] [review]
Patch v3

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

Looks good to me!
Attachment #651444 - Flags: review?(squibblyflabbetydoo) → review+
Thanks, Jim!
comm-central: https://hg.mozilla.org/comm-central/rev/50c073240806
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.