Closed Bug 1429726 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: Composition, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: thomas8, Assigned: thomas8)

References

Details

Attachments

(1 file, 4 obsolete files)

+++ 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? :)
Simple.
But untested.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8941798 - Flags: review?(acelists)
Nitfix error from automatic replacement
Attachment #8941798 - Attachment is obsolete: true
Attachment #8941798 - Flags: review?(acelists)
Attachment #8941799 - Flags: review?(acelists)
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.
(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.
Attachment #8941799 - Attachment is obsolete: true
Attachment #8941799 - Flags: review?(acelists)
Attachment #8941816 - Flags: review?(acelists)
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.
Attachment #8941816 - Flags: review?(acelists) → review+
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?
Attachment #8941816 - Attachment is obsolete: true
Attachment #8941977 - Flags: review+
Forgot to refresh the patch
Attachment #8941977 - Attachment is obsolete: true
Attachment #8941979 - Flags: review+
Attachment #8941977 - Flags: review+
Keywords: checkin-needed
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 :-(((
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a32ea71ed256
Polish showCommandInSpecialFolder() function to accept commands array. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
I did try it locally before posting the suggestion. Yes, the trailing semicolon must be removed when removing the brackets.
Thanks for catching it.
You need to log in before you can comment on or make changes to this bug.