msgHdrViewOverlay needs cleaning up

RESOLVED FIXED in Thunderbird 14.0

Status

Thunderbird
Message Reader UI
--
trivial
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: squib, Assigned: aceman)

Tracking

Trunk
Thunderbird 14.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
The code in msgHdrViewOverlay.js is pretty messy, and really needs someone to go over it and clean it up. Since I've been doing a bunch of stuff in there, I'm volunteering myself to do this. Notable issues include:

* A wide assortment of quote/brace/indent styles
* Various differently-formatted comments
* Comments (and code) which take >80 characters of space for no good reason
* Strangely named objects (why is an object named "createNewAttachmentInfo"? It
  should just be "AttachmentInfo").

Once the changes to the attachment bar settle down, I'll start uploading patches for various parts of this.
(Reporter)

Comment 1

6 years ago
For anyone else looking to work on this (hint hint :)), we should be using double quotes and K&R indent style: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Naming_and_formatting_code

For commenting functions/globals, we should probably use the Javadoc-style comments:

/**
 * Function description
 *
 * @param aFoo the foo object
 * @return the bar object
 */
(Assignee)

Updated

6 years ago
Assignee: squibblyflabbetydoo → acelists
(Assignee)

Updated

6 years ago
Blocks: 738991
(Assignee)

Comment 2

6 years ago
Created attachment 609043 [details] [diff] [review]
patch 1 - indents/braces/quotes
Attachment #609043 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 3

6 years ago
Comment on attachment 609043 [details] [diff] [review]
patch 1 - indents/braces/quotes

Looking at the patch, I see a bunch of unchanged places that still use Allman braces (opening brace on a new line). We should probably just fix all of them.
Attachment #609043 - Flags: review?(squibblyflabbetydoo) → review-
(Assignee)

Comment 4

6 years ago
Well you said I didn't need to change everything. So I touched only those that need it (could be ambiguous or were ugly). But no problem :)
(Reporter)

Comment 5

6 years ago
(In reply to :aceman from comment #4)
> Well you said I didn't need to change everything. So I touched only those
> that need it (could be ambiguous or were ugly). But no problem :)

Sorry, I meant that you didn't necessarily need to do all of the bullet points I listed in comment 0. I think if we're going to fix the brace style though, we should do it for the whole file. If you prefer, you could do that in multiple parts to reduce the potential for bitrot, but in that case, I think we should at least make sure that each function uses an internally-consistent style.
(Assignee)

Comment 6

6 years ago
Created attachment 609106 [details] [diff] [review]
patch 1 - indents/braces/quotes v2
Attachment #609043 - Attachment is obsolete: true
Attachment #609106 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 7

6 years ago
Comment on attachment 609106 [details] [diff] [review]
patch 1 - indents/braces/quotes v2

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

Ok, I did a quick pass on this. r- for now, but it mostly looks good. Unfortunately, I'll be busy for about a week, so I won't be able to review followup patches until then.

Standard8, what's our policy for braces around single-line if blocks? Mozilla's style is to *always* do it, but I don't think Thunderbird obeys that.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1132,4 @@
>      address.emailAddress = addresses.value[index];
>      address.fullAddress = fullNames.value[index];
>      address.displayName = names.value[index];
> +    if (headerEntry.useToggle) {

I think Thunderbird's prevailing style is *not* to put braces around single-line if blocks (despite Mozilla's rules in general). We should probably see what Standard8 thinks about that.

My *personal* preference is to omit the braces for single-line blocks (note: single-line, not single-statement), but that's just me.

@@ +1496,3 @@
>      let server = gFolderDisplay.selectedMessage.folder.server;
>      if (server)
> +      return server.QueryInterface(Components.interfaces.nsISubscribableServer);

If we do go with always using braces for if blocks, we need braces here.

@@ +1786,5 @@
>    var canDetachSelected = CanDetachAttachments() && !allSelectedDetached &&
>                            !allSelectedDeleted;
>  
> +  openMenu.disabled   = allSelectedDeleted;
> +  saveMenu.disabled   = allSelectedDeleted;

Our convention is not to align equals signs, since it just results in more work when adding a net assignment to the list (I prefer aligning equals signs, but when in Rome...)

To avoid making this patch completely unwieldy, let's just keep spacing in assignments the way it is for now. We can address that separately.

@@ +1844,3 @@
>  
> +  let detached  = currentAttachments[0].isExternalAttachment;
> +  let deleted   = !currentAttachments[0].hasFile;

As mentioned above, we should just leave the spacing as-is.

@@ +1847,4 @@
>    let canDetach = CanDetachAttachments() && !deleted && !detached;
>  
> +  openItem.disabled   = deleted;
> +  saveItem.disabled   = deleted;

Leave the spacing as-is here, too.

@@ +1871,5 @@
>    });
>    let canDetach = CanDetachAttachments() && !allDeleted && !allDetached;
>  
> +  openAllItem.disabled   = allDeleted;
> +  saveAllItem.disabled   = allDeleted;

Leave the spacing as-is here, too.

@@ +2075,5 @@
>    var attachmentSplitter = document.getElementById("attachment-splitter");
>    var bundle = document.getElementById("bundle_messenger");
>  
>    if (expanded === undefined)
>      expanded = !attachmentToggle.checked;

Again, if we're going with always using braces for if blocks, we should use braces here.
Attachment #609106 - Flags: review?(squibblyflabbetydoo) → review-
(Assignee)

Updated

6 years ago
Attachment #609106 - Flags: feedback?(mbanner)
(Assignee)

Comment 8

5 years ago
I bet this will bitrot nicely before we can move on here ;)
Standard8, can you answer comment 7?

Comment 9

5 years ago
(In reply to Jim Porter (:squib) from comment #7)
> Standard8, what's our policy for braces around single-line if blocks?
> Mozilla's style is to *always* do it, but I don't think Thunderbird obeys
> that.

The policy is no braces for single-line if blocks. BUT, if it's an else-if and one of the else-if blocks need braces, they should all have it.
(Assignee)

Comment 10

5 years ago
Created attachment 614469 [details] [diff] [review]
[checked in] patch 1 - indents/braces/quotes v3

Ok, let's try this.
Attachment #609106 - Attachment is obsolete: true
Attachment #609106 - Flags: feedback?(mbanner)
Attachment #614469 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 11

5 years ago
Comment on attachment 614469 [details] [diff] [review]
[checked in] patch 1 - indents/braces/quotes v3

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

Looks good to me.
Attachment #614469 - Flags: review?(squibblyflabbetydoo) → review+
(Assignee)

Comment 12

5 years ago
Can this part be checked in?
(Reporter)

Comment 13

5 years ago
Sure.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [checkin patch 614469, leave open for next steps]
http://hg.mozilla.org/comm-central/rev/9075147bd5eb
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [checkin patch 614469, leave open for next steps]
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Updated

5 years ago
Attachment #614469 - Attachment description: patch 1 - indents/braces/quotes v3 → [checked in] patch 1 - indents/braces/quotes v3
(Assignee)

Comment 15

5 years ago
Created attachment 619390 [details] [diff] [review]
patch 2 - comments/long lines

Fixed long lines and reformatted comments.

I have left some overlong lines that will go away as part of bug 738991.
Attachment #619390 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 16

5 years ago
The patch will need adapting to patch in bug 746450.
But the review can still proceed.
Depends on: 746450
(Reporter)

Comment 17

5 years ago
Comment on attachment 619390 [details] [diff] [review]
patch 2 - comments/long lines

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

This is just a quick pass; I'll probably find more nits to pick when I go over this in more detail. Most of it looks good, though. Clearing out review for now, since this has bit-rotted.

::: mail/base/content/msgHdrViewOverlay.js
@@ +119,5 @@
> +    {name:"newsgroups", outputFunction:OutputNewsgroups},
> +    {name:"references", outputFunction:OutputMessageIds},
> +    {name:"followup-to", outputFunction:OutputNewsgroups},
> +    {name:"content-base"},
> +    {name:"tags"} ];

I think we should indent by 2 spaces here, since that's the usual style.

@@ +141,5 @@
> + * currentHeaderData  this is an array of header name and value pairs for the
> + *                    currently displayed message. It's purely a data object
> + *                    and has no view information. View information is
> + *                    contained in the view objects.
> + *                    For a given entry in this array you can ask for:

Drop the "currentHeaderData", and unindent these lines. We don't use that notation for any of the other globals.

@@ +152,5 @@
> + * For the currently displayed message, we store all the attachment data. When
> + * displaying a particular view, it's up to the view layer to extract this
> + * attachment data and turn it into something useful.
> + * For a given entry in the attachments list, you can ask for the following
> + * properties:

We can drop this and all the properties listed afterwards. This is all documented in the AttachmentInfo code further down. But please do say that currentAttachments is an array of AttachmentInfo objects. :)

@@ +166,5 @@
>  const nsIAbListener = Components.interfaces.nsIAbListener;
>  const nsIAbCard = Components.interfaces.nsIAbCard;
>  
> +/**
> + * createHeaderEntry: Our constructor method which creates a header Entry

We can drop the "createHeaderEntry" here.

@@ +232,5 @@
>    }
>  
>    if (prefBranch.getBoolPref("mailnews.headers.showOrganization")) {
> +    var organizationEntry = { name:"organization",
> +                              outputFunction:updateHeaderValue };

Spaces after the colons, please. Likewise with the code following this.

@@ +307,5 @@
>    let openInNewWindow = document.getElementById("otherActionsOpenInNewWindow");
>    openInTab.hidden = openInNewWindow.hidden = opensAreHidden;
>  
> +  // dispatch an event letting any listeners know that we have loaded
> +  // the message pane

While we're here, could you give this sentence case?

@@ +840,5 @@
>    }
>  }
>  
> +/**
> + * Flush out any local state being held by a header entry for a given table.

Could you add some documentation about the headerTable argument? Likewise for the functions following this one.

@@ +1186,5 @@
> + * addresses, extracts them one by one, linkifying each email address into
> + * a mailto url.
> + *
> + * @param headerEntry  Then we add the link-ified email address to the parentDiv
> + *                     passed in.

This comment doesn't seem to make sense. I think the description belongs in the paragraph above.

@@ +1436,5 @@
> + * is found in the address books, then it book will contain an nsIAbDirectory,
> + * and card will contain an nsIAbCard.
> + *
> + * @param emailAddress  If the email address is not found, both
> + *                      items will contain null.

This comment also doesn't make sense, and should go in the preceding paragraph. Also, please add a @return line, too.

@@ +1521,5 @@
>  
>  /**
>   * Takes the email address title button, extracts the email address we stored
>   * in there and opens a compose window with that address.
> + * @param addressNode  a node which has a "fullAddress" or "newsgroup" attribute

Could you add an empty line before this one?

@@ +1789,5 @@
>      let stream = Services.io.newChannelFromURI(url).open();
>  
>      let inputStream = Components.classes["@mozilla.org/binaryinputstream;1"]
> +                                .createInstance(Components.interfaces
> +                                                          .nsIBinaryInputStream);

You can leave this on one line. The general guideline I use is "keep to <=80 characters on a line, unless it's involving Components.classes or Components.interfaces. Then I format it like this line was originally.
Attachment #619390 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 18

5 years ago
Created attachment 623294 [details] [diff] [review]
patch 2 - comments/long lines v2

Fixed nits. 

>> +/**
>> + * Flush out any local state being held by a header entry for a given table.
>Could you add some documentation about the headerTable argument? Likewise for >the functions following this one.
Sorry I can't do this, I do not know what the functions do. I just reformat existing info.
Attachment #619390 - Attachment is obsolete: true
Attachment #623294 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 19

5 years ago
Comment on attachment 623294 [details] [diff] [review]
patch 2 - comments/long lines v2

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

This looks pretty good, aside from a few places we should be using sentence case. I also think I bitrotted this in bug 696414, so r-'ing for now just to keep track of that. Once you fix the bitrot, I think we can check this in, and resolve this bug. There are probably other little things we can clean up, but I think we can address those gradually, as we patch this file for other reasons.

::: mail/base/content/msgHdrViewOverlay.js
@@ +120,5 @@
> +  {name:"newsgroups", outputFunction:OutputNewsgroups},
> +  {name:"references", outputFunction:OutputMessageIds},
> +  {name:"followup-to", outputFunction:OutputNewsgroups},
> +  {name:"content-base"},
> +  {name:"tags"} ];

Could you please put spaces after the colons in this list? Spaces between the curly braces are optional (do whichever you think is better).

@@ +614,3 @@
>      {
> +      // presentation level change....don't show vcards as external attachments
> +      // in the UI. Libmime already renders them inline.

I think I bitrotted this from bug 696414.

@@ +834,5 @@
>  }
>  
> +/**
> + * Flush out any local state being held by a header entry for a given table.
> + */

Could you add a javadoc for the headerTable param here? The following should be sufficient (it's used elsewhere in the file):

 * @param aHeaderTable Table of header entries

@@ +846,5 @@
>    }
>  }
>  
> +/**
> + * make sure that any valid header entry in the table is collapsed

Sentence case, please.

@@ +855,5 @@
>      headerEntry.enclosingRow.collapsed = true;
>  }
>  
> +/**
> + * make sure that any valid header entry in the table specified is visible

Sentence case, please.

@@ +890,5 @@
>      // how many empty headers do we need to add?
>      var numEmptyHeaders = gMinNumberOfHeaders - numVisibleHeaders;
>  
> +    // we may have already dynamically created our empty rows and we just need
> +    // to make them visible

Sentence case, please.

@@ +899,5 @@
>        }
>      }
>  
> +    // ok, now if we have any extra dummy headers we need to add, create a new
> +    // header widget for them

Sentence case...

@@ +965,5 @@
>  }
>  
> +/**
> + * default method for updating a header value into a header entry
> + */

Sentence case, and please document the parameters:

 * @param headerEntry  A single header from currentHeaderData
 * @param headerValue  The new value for headerEntry

@@ +1039,3 @@
>  function UpdateExpandedMessageHeaders() {
> +  // iterate over each header we received and see if we have a matching entry
> +  // in each header view table...

Sentence case...
Attachment #623294 - Flags: review?(squibblyflabbetydoo) → review-
(Assignee)

Comment 20

5 years ago
Created attachment 631496 [details] [diff] [review]
[checked in] patch 2 - comments/long lines v3

Thanks, done.
Attachment #623294 - Attachment is obsolete: true
Attachment #631496 - Flags: review?(squibblyflabbetydoo)
(Reporter)

Comment 21

5 years ago
Comment on attachment 631496 [details] [diff] [review]
[checked in] patch 2 - comments/long lines v3

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

Looks good. Sorry about the delay!
Attachment #631496 - Flags: review?(squibblyflabbetydoo) → review+
(Assignee)

Comment 22

5 years ago
Thanks!
Keywords: checkin-needed
Comment on attachment 631496 [details] [diff] [review]
[checked in] patch 2 - comments/long lines v3

https://hg.mozilla.org/comm-central/rev/4878362e857d
Attachment #631496 - Attachment description: patch 2 - comments/long lines v3 → [checked in] patch 2 - comments/long lines v3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.