Closed Bug 783491 Opened 12 years ago Closed 12 years ago

tweak outdated code in mail/base/content/FilterListDialog.js

Categories

(MailNews Core :: Filters, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(2 files)

I think this lines should be changed:

*In function setFolder(msgFolder):
- This code should be removed. The comment does not belong here
and the functions calls are already part of rebuildFilterList() call above:
   // Get the first folder for this server. INBOX for
   // imap and pop accts and 1st news group for news.
   updateButtons();
   updateCountBox();

*In function updateButtons():
- This variable could be inlined with the next code line:
var filter = currentFilter();
- This comment does not match the code (the "unparsable" part has inverted logic):
// "edit" only enabled when one filter selected or if we couldn't parse the filter
var disabled = !oneFilterSelected || filter.unparseable
- The variable name could also be improved, as the other ones in the function.
Actually we could probably use one variable for all the disabling decisions as they are sequential.
- Add spaces to:
    var downDisabled = !(oneFilterSelected && list.currentIndex < list.getRowCount()-1);

- These can probably be inlined (variables removed) as the other buttons are:
    // special buttons
    var buttonTop = document.getElementById("reorderTopButton");
    var buttonBottom = document.getElementById("reorderBottomButton");

    buttonTop.disabled = upDisabled;
    buttonBottom.disabled = downDisabled;

*In function updateCountBox():
- The second argument to Replace does not need to be an array, also add a space.
.replace("#1",[len]);

Mconley, are these proposals sane?
I also think this comment is not true (in current versions these elements are exposed in News too, probably CanRunFiltersAfterTheFact is true now):
   // run filters after the fact not supported by news
   if (CanRunFiltersAfterTheFact(msgFolder.server)) {
     document.getElementById("runFiltersFolder").removeAttribute("hidden");
     document.getElementById("runFiltersButton").removeAttribute("hidden");
     document.getElementById("folderPickerPrefix").removeAttribute("hidden");

     // for POP3 and IMAP, select the first folder, which is the INBOX
     document.getElementById("runFiltersFolder").selectedIndex = 0;
     runMenu.selectFolder(getFirstFolder(msgFolder));
   }
   else {
     document.getElementById("runFiltersFolder").setAttribute("hidden", "true");
     document.getElementById("runFiltersButton").setAttribute("hidden", "true");
     document.getElementById("folderPickerPrefix").setAttribute("hidden", "true");
   }

This code could also be optimized by setting hidden on the parent hbox, that contains these 3 elements.
And use something like hbox.hidden = !CanRunFiltersAfterTheFact(msgFolder.server);
IMHO "- This variable could be inlined with the next code line:" is not a reason that we should be touching working code, unless there is a concurrent fix that should be done. Code churn can cause its own problems, as well as take effort.
The variable
var nsMsgSearchScope = Components.interfaces.nsMsgSearchScope;

is not used at all places where it could be.

In function:
function initializeDialog(filter)
 var gSearchScope = getFilterScope(getScope(filter), filter.filterType, filter.filterList);

I don't see a reason for this variable to have 'g' prefix, it is not global.
Comment 3 is invalid, that line is from another file.
Attached patch patchSplinter Review
Attachment #668605 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 668605 [details] [diff] [review]
patch

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

Ok i suppose, but i'm not super exited about using global variables, though in this case it may make things easier...

::: mail/base/content/FilterListDialog.js
@@ +792,5 @@
>  {
>    if (event.charCode == KeyEvent.DOM_VK_SPACE)
>    {
> +    for each (let item in gFilterListbox.selectedItems)
> +      toggleFilter(item);

add braces for the for-statement
Attachment #668605 - Flags: review?(mkmelin+mozilla) → review+
This using of global variable for elements that are often references seems to be standard practice (see also Seamonkey files and the filter list dialog). Only when all of the code is inside an object then also these variables are inside the object.

How would you solve it better?
That's just because the code is old, and practices used to be bad :) In general they make coding hard as the code gets so much less self contained. 

Since this is just for inside the dialog I think it's ok, but in general just using document.getElementById is almost zero cost and should be preferred.
I like the global vars for the elements better as you can see you use the same element in all functions and can change the ID easily if needed.

I'd like to know how costly document.getElementById is and will benchmark it somehow.
Keywords: checkin-needed
Last i checked it's hardly measurable, you need to many many calls and to get it up to 1ms.
https://hg.mozilla.org/comm-central/rev/fbead5325860
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 19.0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached patch patch fixSplinter Review
Sorry, forgot to address the nit from review.
Attachment #671593 - Flags: review+
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
https://hg.mozilla.org/comm-central/rev/6a8917ac00c9
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.