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

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Filters
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 19.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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?
(Assignee)

Comment 1

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

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Comment 3 is invalid, that line is from another file.
(Assignee)

Comment 5

5 years ago
Created attachment 668605 [details] [diff] [review]
patch
Attachment #668605 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 6

5 years ago
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+
(Assignee)

Comment 7

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

Comment 8

5 years ago
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.
(Assignee)

Comment 9

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

Comment 10

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

5 years ago
Created attachment 671593 [details] [diff] [review]
patch fix

Sorry, forgot to address the nit from review.
Attachment #671593 - Flags: review+
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
https://hg.mozilla.org/comm-central/rev/6a8917ac00c9
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.