1429726 - Make showCommandInSpecialFolder() take an array of commandIDs as first argument

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1389062 +++

A bit of code cleanup for better performance: 

mailContextMenus.js
mailWindowOverlay.js

We're running showCommandInSpecialFolder() twice for the same folder, but different commands. We should tweak this to run once for a given folder, and take an array of commands.

(In reply to :aceman from comment #46)
> Comment on attachment 8933374 [details] [diff] [review]
> @@ +88,4 @@
> >    showCommandInSpecialFolder("cmd_newMsgFromTemplate",
> >                               Components.interfaces.nsMsgFolderFlags.Templates);
> > +  showCommandInSpecialFolder("cmd_editTemplateMsg", 
> > +                             Components.interfaces.nsMsgFolderFlags.Templates);
> 
> The determination if folder is a Templates folder inside 
> showCommandInSpecialFolder is quite heavy and we already know if the folder
> is a Templates folder (from the call above). So this looks inefficient, but
> the construct is the norm here.
> ShowMenuItem("cmd_newMsgFromTemplate",
> IsMenuItemShowing("cmd_editTemplateMsg")) would be a faster hack, but we are
> setting commands, not menuitems so it is semantically not correct.
> 
> Any interest in a followup to make showCommandInSpecialFolder() take an
> array of commandIDs as first argument? :)

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1429726_showCommandInSpecialFolder_polish.patch

Simple.
But untested.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1429726_showCommandInSpecialFolder_polish.patch

Nitfix error from automatic replacement

Comment 3

[Jorg K (CEST = GMT+2)]

Hmm, I thought the idea was to teach the function to take a string or array? Anyway, this is fine, I'll let Aceman be the judge.

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1429726_showCommandInSpecialFolder_polish.patch

(In reply to Jorg K (GMT+1) from comment #3)
> Hmm, I thought the idea was to teach the function to take a string or array?

Yes, we can do that for maximum convenience.

Comment 5

[:aceman]

Comment on attachment 8941816 [details] [diff] [review]
1429726_showCommandInSpecialFolder_polish.patch

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

Thanks, this is what I meant.

::: mail/base/content/mailWindowOverlay.js
@@ +607,5 @@
> +    aCommandIds = [aCommandIds];
> +
> +  aCommandIds.forEach(cmdId => {
> +      document.getElementById(cmdId).setAttribute("hidden", !inSpecialFolder);
> +    }

The curly braces aren't needed.

@@ +608,5 @@
> +
> +  aCommandIds.forEach(cmdId => {
> +      document.getElementById(cmdId).setAttribute("hidden", !inSpecialFolder);
> +    }
> +  )

Missing semicolon.

@@ +609,5 @@
> +  aCommandIds.forEach(cmdId => {
> +      document.getElementById(cmdId).setAttribute("hidden", !inSpecialFolder);
> +    }
> +  )
> +

The empty line isn't needed.

Comment 6

[Thomas D. (:thomas8)]

Attachment: 1429726_showCommandInSpecialFolder_polish.patch

I put the curly braces because I thought
> document.getElementById(cmdId).setAttribute("hidden", !inSpecialFolder);
is a statement, not an expression. What exactly makes it an expression?

Comment 7

[Thomas D. (:thomas8)]

Attachment: 1429726_showCommandInSpecialFolder_polish.patch

Forgot to refresh the patch

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8941979 [details] [diff] [review]
1429726_showCommandInSpecialFolder_polish.patch

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

::: mail/base/content/mailWindowOverlay.js
@@ +606,5 @@
> +  if (typeof aCommandIds === "string")
> +    aCommandIds = [aCommandIds];
> +
> +  aCommandIds.forEach(cmdId =>
> +    document.getElementById(cmdId).setAttribute("hidden", !inSpecialFolder);

Could the submitter and the reviewer have the kindness to actually TRY what they are submitting or reviewing???

Had I landed this syntax error, I would have create a HEAP of bustage since not even the mail window shows.

Grrrr :-(((

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a32ea71ed256
Polish showCommandInSpecialFolder() function to accept commands array. r=aceman

Comment 10

[:aceman]

I did try it locally before posting the suggestion. Yes, the trailing semicolon must be removed when removing the brackets.

Comment 11

[:aceman]

Thanks for catching it.