Closed Bug 657856 Opened 9 years ago Closed 9 years ago

Improve interface for attachment objects in the message reader

Categories

(Thunderbird :: Message Reader UI, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 7.0

People

(Reporter: squib, Assigned: squib)

References

Details

Attachments

(1 file, 2 obsolete files)

For some bugs I'm working on, it would be very nice if the interface for attachment objects in Javascript was improved. Currently, they are "just" dictionaries, which is a bit clumsy, especially when checking for deleted attachments (attachment.contentType == "text/x-moz-deleted").

Attached is a patch to improve the interface by giving the objects some useful methods. I also eliminated the cloneAttachment function, since as far as I can tell, it served no useful purpose.

Existing tests should cover most everything here already.
Attachment #533157 - Flags: review?(bwinton)
Attached patch Tiny change (obsolete) — Splinter Review
This is just a tiny change over the last version, to keep some code from exceeding 80 characters in a subsequent patch. :)
Attachment #533157 - Attachment is obsolete: true
Attachment #533157 - Flags: review?(bwinton)
Attachment #533165 - Flags: review?(bwinton)
Comment on attachment 533165 [details] [diff] [review]
Tiny change

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

I've listed a couple of nits below, but this patch seems like a decent improvement, so r=me with those fixed.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1682,5 @@
> +   */
> +  get isDeleted()
> +  {
> +    return this.contentType == "text/x-moz-deleted";
> +  },

Do you want to change mail/base/content/mailCore.js line 591 to use this attribute, too?

@@ +1692,5 @@
> +   */
> +  get isEmpty()
> +  {
> +    let io = Components.classes["@mozilla.org/network/io-service;1"]
> +                       .getService(Components.interfaces.nsIIOService);

Please use Services.io instead, if you can.

@@ +1977,3 @@
>        item.setAttribute("context", "attachmentListContext");
>  
> +      item.attachment = attachment;

Why aren't we cloning the attachment anymore?

@@ +2300,5 @@
> +       case 'saveAs':
> +         actionFunction = function(attachment) { attachment.save(); };
> +       case 'detach':
> +         actionFunction = function(attachment) { attachment.detach(true); };
> +     }

Do we want a default case that logs an error, in case we accidentally type "saveas" sometime?
Attachment #533165 - Flags: review?(bwinton) → review+
(In reply to comment #2)
> Comment on attachment 533165 [details] [diff] [review] [review]
> Tiny change
> 
> Review of attachment 533165 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> I've listed a couple of nits below, but this patch seems like a decent
> improvement, so r=me with those fixed.
> 
> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +1682,5 @@
> > +   */
> > +  get isDeleted()
> > +  {
> > +    return this.contentType == "text/x-moz-deleted";
> > +  },
> 
> Do you want to change mail/base/content/mailCore.js line 591 to use this
> attribute, too?
> 
> @@ +1692,5 @@
> > +   */
> > +  get isEmpty()
> > +  {
> > +    let io = Components.classes["@mozilla.org/network/io-service;1"]
> > +                       .getService(Components.interfaces.nsIIOService);
> 
> Please use Services.io instead, if you can.

Done.

> @@ +1977,3 @@
> >        item.setAttribute("context", "attachmentListContext");
> >  
> > +      item.attachment = attachment;
> 
> Why aren't we cloning the attachment anymore?

As far as I can tell, it's totally unnecessary, and just leads to extra objects being created. I could only see it being useful if we were modifying any of the values in the AttachmentInfo objects, but we aren't.

> @@ +2300,5 @@
> > +       case 'saveAs':
> > +         actionFunction = function(attachment) { attachment.save(); };
> > +       case 'detach':
> > +         actionFunction = function(attachment) { attachment.detach(true); };
> > +     }
> 
> Do we want a default case that logs an error, in case we accidentally type
> "saveas" sometime?

That switch statement only gets hit when action is 'open' or 'saveAs'. The default case is after that (the 'detach' case shouldn't be there, and I've since removed it).
Pulling forward r+. I think this should go into Miramar, since it blocks bug 656045, which should also be in Miramar if at all possible.
Attachment #533165 - Attachment is obsolete: true
Attachment #534978 - Flags: review+
Attachment #534978 - Flags: approval-thunderbird3.3?
Comment on attachment 534978 [details] [diff] [review]
Address review comments

Sorry, but I don't want the added risk for this on beta. There's no dataloss afaict, just strange files if you do hit it. I also doubt it will be hit that frequently. I think we can take it for the next version, but I've not yet set up the approval flag for aurora.
Attachment #534978 - Flags: approval-thunderbird3.3? → approval-thunderbird3.3-
Checked in (with a tiny change to use Services.io): http://hg.mozilla.org/comm-central/rev/ad6e9c9ea574
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
Depends on: 676754
You need to log in before you can comment on or make changes to this bug.