Arshad Khan [:arshad] Assignee
	Description • 6 years ago

      No description provided.

	Arshad Khan [:arshad] Assignee
	Comment 5 • 6 years ago

Comment on attachment 9018926 [details] [diff] [review]
patch-3_ruleactiontargetpriority.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +47,5 @@
>  
> +class MozRuleactiontargetPriority extends MozRuleactiontargetBase {
> +    connectedCallback() {
> +        super.connectedCallback();
> +        this.appendChild(MozXULElement.parseXULToFragment(`

using parseXULToFragment since menupopup has an xbl binding

	Magnus Melin [:mkmelin]
	Comment 10 • 6 years ago

Comment on attachment 9018926 [details] [diff] [review]
patch-3_ruleactiontargetpriority.patch

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

For this and others, remember 2 space indent

::: mailnews/base/search/content/searchWidgets.js
@@ +55,5 @@
> +            <menuitem value="5" label="FROM-DTD-highPriorityCmd-label"></menuitem>
> +            <menuitem value="4" label="FROM-DTD-normalPriorityCmd-label"></menuitem>
> +            <menuitem value="3" label="FROM-DTD-lowPriorityCmd-label"></menuitem>
> +            <menuitem value="2" label="FROM-DTD-lowestPriorityCmd-label"></menuitem>
> +          </menupopup>

for the labels, I think you need to move to javascript. and to using Fluent. 

Shold be something like this:

let l10n = new Localization([
  "foo/bar.ftl",
]);
let labeles = await this._l10n.formatMessages([{
     {id: "highest-priority-cmd"},
     {id: "high-priority-cmd"},
}]);

	Magnus Melin [:mkmelin]
	Comment 11 • 6 years ago

Comment on attachment 9018934 [details] [diff] [review]
patch-7_ruleactiontargetfolder.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +112,5 @@
> +        <menulist class="ruleactionitem folderMenuItem" displayformat="verbose" oncommand="this.parentNode.setPicker(event);">
> +          <menupopup type="folder" mode="filing" class="menulist-menupopup" showRecent="true" recentLabel="FROM-DTD-recentFolders-label" showFileHereLabel="true"></menupopup>
> +        </menulist>
> +        `));
> +        this.menulist = document.querySelector("menulist");

this.querySelector(...) ?

@@ +114,5 @@
> +        </menulist>
> +        `));
> +        this.menulist = document.querySelector("menulist");
> +
> +        ChromeUtils.import("resource:///modules/MailUtils.jsm", this);

you can import this at top of the file I think

	Magnus Melin [:mkmelin]
	Comment 13 • 6 years ago

Comment on attachment 9018925 [details] [diff] [review]
patch-2_ruleactiontargettag.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +25,5 @@
> +
> +        menuList.appendChild(menuPopup);
> +
> +        let tagArray = MailServices.tags.getAllTags({});
> +        for (let i = 0; i < tagArray.length; ++i) {

you could switch this to 
for (let taginfo of MailServices.tags.getAllTags({}))

	Magnus Melin [:mkmelin]
	Comment 14 • 6 years ago

Comment on attachment 9021805 [details] [diff] [review]
patch-8-ruleactionbindingsfix.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +167,5 @@
> +
> +    _updateAttributes() {
> +        const type = this.getAttribute("type");
> +
> +        if (type != null) {

for null, remote the node, and return early

@@ +169,5 @@
> +        const type = this.getAttribute("type");
> +
> +        if (type != null) {
> +            switch (type) {
> +                case "movemessage":

you could do just have a type-element mapping

 { 
   "movemessage" : "ruleactiontarget-folder",
   "copymessage" : "ruleactiontarget-folder",
...

	Arshad Khan [:arshad] Assignee
	Comment 15 • 6 years ago

addressed magnus's comments.

	Arshad Khan [:arshad] Assignee
	Comment 16 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aeb1cab24c568a3d4e978e905473e835b76241d8

	Magnus Melin [:mkmelin]
	Comment 18 • 6 years ago

Comment on attachment 9022368 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Some nits below. I tried it out and at least two of the actions are not working.

* ReplyWithTemplate: JavaScript error: chrome://messenger/content/searchWidgets.js, line 94: TypeError: this.closest(...).querySelector(...) is null; can't access its "getTemplates" property
* Tag message: no tag menu opens to select tag

::: mailnews/base/search/content/searchWidgets.js
@@ +33,5 @@
> +    }
> +
> +    // propagating a pre-existing hack to make the tag get displayed correctly in the menulist
> +    // now that we"ve changed the tags for each menu list. We need to use the current selectedIndex
> +    // (if its defined) to handle the case where we were initialized with a filter action already.

hacky indeed. can you check if this is still needed?

@@ +47,5 @@
> +
> +class MozRuleactiontargetPriority extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`
> +        <menulist class="ruleactionitem" flex="1">

still 4 space indention

@@ +54,5 @@
> +            <menuitem value="5" label="&highPriorityCmd.label;"></menuitem>
> +            <menuitem value="4" label="&normalPriorityCmd.label;"></menuitem>
> +            <menuitem value="3" label="&lowPriorityCmd.label;"></menuitem>
> +            <menuitem value="2" label="&lowestPriorityCmd.label;"></menuitem>
> +            </menupopup>

and this indention is off

@@ +140,5 @@
> +
> +    updateParentNode(this.closest(".ruleaction"));
> +  }
> +
> +  setPicker(aEvent) {

please make this event instead of aEvent while we're here

@@ +179,5 @@
> +
> +  _updateAttributes() {
> +    const type = this.getAttribute("type");
> +
> +

extra empty lines here

::: mailnews/base/search/content/searchWidgets.xml
@@ +327,5 @@
>        <xul:hbox class="ruleactiontype"
>                      flex="&filterActionTypeFlexValue;"/>
> +      <xul:ruleactiontarget-wrapper xbl:inherits="type=value"
> +                                class="ruleactiontarget"
> +                                flex="&filterActionTargetFlexValue;"/>

nit: strange alignment

@@ +368,5 @@
>          <body>
>            <![CDATA[
>              var filterActionStr;
>              var actionTarget = document.getAnonymousNodes(this)[1];
> +            var actionItem = actionTarget.ruleactiontargetElement;

you can make this "let" while you're touching this line

@@ +451,5 @@
>              ChromeUtils.import("resource:///modules/MailUtils.jsm", this);
>              var filterActionString = this.getAttribute('value');
>              var actionTarget = document.getAnonymousNodes(this)[1];
> +            var actionTargetLabel = actionTarget.ruleactiontargetElement &&
> +                                    actionTarget.ruleactiontargetElement.childNodes[0].value;

let

@@ +509,5 @@
>              var filterAction = aFilter.createAction();
>              var filterActionString = this.getAttribute('value');
>              filterAction.type = gFilterActionStrings.indexOf(filterActionString);
>              var actionTarget = document.getAnonymousNodes(this)[1];
> +            var actionItem = actionTarget.ruleactiontargetElement;

let

@@ +530,5 @@
>                case nsMsgFilterAction.Custom:
>                  filterAction.customId = filterActionString;
>                  // fall through to set the value
>                default:
> +                if (actionItem && actionItem.childNodes.length)

for counts I think it's nicer to write length > 0

@@ +551,5 @@
>              aActionStrings.push({
>                label: document.getAnonymousNodes(this.mRuleActionType)[0].label,
>                argument: actionItem ?
> +                        (actionItem.childNodes[0].label ?
> +                         actionItem.childNodes[0].label : actionItem.childNodes[0].value) : ""

yuck. perhaps define a variable for this above so we can avoid the double ternary

	Arshad Khan [:arshad] Assignee
	Comment 19 • 6 years ago

Hey, the replywithtemplate option still doesn't work. There is no error log but when you run the replywithtemplate filter it says that filter has failed. Could you please confirm other options and this one as well on your side?

	Magnus Melin [:mkmelin]
	Comment 20 • 6 years ago

Comment on attachment 9023594 [details] [diff] [review]
ruleactionbindingsfix.patch

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

Yeah something is broken. Can't save the new filter with this patch applied

JavaScript error: actionTarget.ruleactiontargetElement is undefined; can't access its "childNodes" property @ searchWidgets.xml:455

JavaScript error: chrome://messenger/content/searchWidgets.xml, line 525: TypeError: actionItem is undefined; can't access its "childNodes" property
 --> this line: filterAction.targetFolderUri = actionItem.childNodes[0].value;

                
                break;

	Arshad Khan [:arshad] Assignee
	Comment 22 • 6 years ago

Magnus, I tried ruleaction bindings on linux as well and I can not only save filters but run them as well. Could you please check again ? This time the reply-to filter also worked.

	Arshad Khan [:arshad] Assignee
	Comment 24 • 6 years ago

In the screenshot you can see the error that occurs sometimes when running replyto filter.. You can also see it succeded for some emails by looking at the notification popup on top right..

	Magnus Melin [:mkmelin]
	Comment 25 • 6 years ago

Comment on attachment 9025258 [details] [diff] [review]
ruleactionbindingsfix.patch

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

Yeah looks like this works now. r=mkmelin

	Arshad Khan [:arshad] Assignee
	Comment 26 • 6 years ago

(In reply to Magnus Melin [:mkmelin] from comment #25)
> Comment on attachment 9025258 [details] [diff] [review]
> ruleactionbindingsfix.patch
> 
> Review of attachment 9025258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah looks like this works now. r=mkmelin

could you please confirm that the error that I see in the screenshot is not because of the patch?

	Magnus Melin [:mkmelin]
	Comment 28 • 6 years ago

Well it says the server responded with "temporary system problem" so I don't think it's related.

	Jorg K (CEST = GMT+2)
	Comment 29 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=01dbc444e9e0c4f6e0c2b6a87ee0914031ea05af

	Pulsebot
	Comment 30 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c7cda013b0b4
Remove ruleactiontarget bindings. r=mkmelin

	:aceman
	Comment 31 • 6 years ago

It seems this is all busted. All filters that have "move/copy to", "reply with template" or "tag message" actions display blank menulists next to these actions in the filter editor. Even when those values are set and the display worked in the past. Can you please confirm?
Is it caused by this patch or a new change in m-c?

	Jorg K (CEST = GMT+2)
	Comment 32 • 6 years ago

Yes, pretty busted. If you have a filter "Move Message to" there is no folder to choose from. If you then select "Forward Message to" a box appears and the action changed to "Add Star".

I'll see whether can back it out. That turns out a little difficult due to changes on mailnews/base/search/content/searchWidgets.js/xml:
https://hg.mozilla.org/comm-central/log/tip/mailnews/base/search/content/searchWidgets.xml
https://hg.mozilla.org/comm-central/log/tip/mailnews/base/search/content/searchWidgets.js

	Jorg K (CEST = GMT+2)
	Comment 33 • 6 years ago

Backout seems to fix the problem:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d57b0d35db291e3fb1b73fc09ff8bd5b6b355701

Aceman, can you please check as well.

	Jorg K (CEST = GMT+2)
	Comment 34 • 6 years ago

This is the patch that landed, but rebased after the backout. Arshad, you can start with this patch and fix the issues we found.

	Pulsebot
	Comment 35 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/57b3ea524f1d
Backed out changeset c7cda013b0b4 for making the filter editor unusable. a=jorgk

	Arshad Khan [:arshad] Assignee
	Comment 39 • 6 years ago

Comment on attachment 9027460 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Tested this patch on mac, everything seems to work even on editing filter.

	:aceman
	Comment 40 • 6 years ago

Comment on attachment 9027460 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

OK, this one looks better, but I still get some empty menulists in action filters.
I'm not sure if they I caused by the errors I get in the console:
TypeError: actionItem is null; can't access its "childNodes" property, searchWidgets.xml:409:17
The line number differs depending on action type, there is also 413 and 425.

::: mailnews/base/search/content/searchWidgets.js
@@ +18,5 @@
> +
> +class MozRuleactiontargetTag extends MozXULElement {
> +  connectedCallback() {
> +    const menulist = document.createElement("menulist");
> +    const menuPopup = document.createElement("menupopup");

'const' on mutable elements??? How did this pass? Use 'let' here and elsewhere.

@@ +39,5 @@
> +}
> +
> +class MozRuleactiontargetPriority extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`

We are not allowed to use .innerHTML but this tag blob suddenly passes...

@@ +86,5 @@
> +
> +    this.appendChild(menulist);
> +
> +    document.getAnonymousElementByAttribute(this.closest(".ruleaction"), "class", "ruleactiontype")
> +      .getTemplates(true, menulist);

Dot under dot.

@@ +110,5 @@
> +class MozRuleactiontargetFolder extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`
> +      <menulist class="ruleactionitem folderMenuItem" flex="1" displayformat="verbose">
> +        <menupopup type="folder" mode="filing" class="menulist-menupopup" showRecent="true" recentLabel="&recentFolders.label;" showFileHereLabel="true"></menupopup>

Long line, split into each attribute on a line as we have in XUL files.

@@ +131,5 @@
> +
> +    // An account folder is not a move/copy target; show "Choose Folder".
> +    folder = folder.isServer ? null : folder;
> +
> +    // The menupopup constructor needs to finish first.

You copied this comment from where it made sense as there was a timeout. But here, how do you ensure the element is built? If just reaching this place means that, the comment is useless.

@@ +173,5 @@
> +    return elementName ? document.createElement(elementName) : null;
> +  }
> +
> +  _updateAttributes() {
> +    const type = this.getAttribute("type");

Only this 'const' is acceptable.

	Arshad Khan [:arshad] Assignee
	Comment 41 • 6 years ago

Comment on attachment 9027460 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +18,5 @@
> +
> +class MozRuleactiontargetTag extends MozXULElement {
> +  connectedCallback() {
> +    const menulist = document.createElement("menulist");
> +    const menuPopup = document.createElement("menupopup");

const mutablity will not be voided if we dont change binding of that particuclar element. I can change attributes/properties of an object or a js element even if it is a const variable. Obviously error will be flagged when I reassign the variable, in other words change the binding of that element. e.g., `const a = {}; a.val = 1;` is valid use of const in js. If i try to do `const a = {}; a = {val: 1}` then error will be flagged.

@@ +39,5 @@
> +}
> +
> +class MozRuleactiontargetPriority extends MozXULElement {
> +  connectedCallback() {
> +    this.appendChild(MozXULElement.parseXULToFragment(`

this is not innerHTML, parseXULToFragment uses parseXML method which is I suppose ok to use like https://searchfox.org/comm-central/source/calendar/base/modules/utils/calXMLUtils.jsm#139. For more - https://searchfox.org/comm-central/source/mozilla/toolkit/content/customElements.js#135

	Arshad Khan [:arshad] Assignee
	Comment 42 • 6 years ago

const immutability will not be voided*

	:aceman
	Comment 44 • 6 years ago

Comment on attachment 9028113 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Looks good now, thanks. No errors and no empty menulists.

However, when the "See execution order" link is clicked, I get
TypeError: actionItem is null; can't access its "childNodes" property, searchWidgets.xml:545:17

and no dialog with the action list appears. Possibly this method needs to be updated for the new location of the action values.

	Arshad Khan [:arshad] Assignee
	Comment 46 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=245f37ad7629695abc2269d66ceec578fd6fd780

	:aceman
	Comment 47 • 6 years ago

Comment on attachment 9028812 [details] [diff] [review]
ruleactiontargetbindingsunified.patch

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

Thanks, seems to work nicely now.

The commit message needs a fix to r=aceman and also to mention the actiontarget bindings are not just removed but replaced by custom elements.

	Pulsebot
	Comment 48 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6e49a3ec00cd
Replace ruleactiontarget bindings with custom element. r=mkmelin,aceman