Closed Bug 1489172 Opened Last year Closed 11 months ago

[de-xbl] Remove ruleactiontarget bindings.

Categories

(Thunderbird :: Mail Window Front End, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 65.0

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(3 files, 21 obsolete files)

865.87 KB, image/png
Details
3.16 MB, image/png
Details
27.41 KB, patch
aceman
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Blocks: 1489175
Blocks: 1489174
Blocks: 1489173
Blocks: 1489176
Blocks: 1489177
Blocks: 1489179
Attached patch ruleactiontargetbase.patch (obsolete) — Splinter Review
Attachment #9018918 - Attachment is obsolete: true
Summary: [de-xbl] Remove ruleactiontarget-base binding. → [de-xbl] Remove ruleactiontarget bindings.
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
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"},
}]);
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
Attachment #9021805 - Flags: review?(mkmelin+mozilla)
Attachment #9018924 - Flags: review?(mkmelin+mozilla)
Attachment #9018925 - Flags: review?(mkmelin+mozilla)
Attachment #9018926 - Flags: review?(mkmelin+mozilla)
Attachment #9018930 - Flags: review?(mkmelin+mozilla)
Attachment #9018932 - Flags: review?(mkmelin+mozilla)
Attachment #9018934 - Flags: review?(mkmelin+mozilla)
Attachment #9018933 - Flags: review?(mkmelin+mozilla)
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({}))
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",
...
addressed magnus's comments.
Attachment #9021805 - Attachment is obsolete: true
Attachment #9021805 - Flags: review?(mkmelin+mozilla)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aeb1cab24c568a3d4e978e905473e835b76241d8
Attachment #9018924 - Attachment is obsolete: true
Attachment #9018925 - Attachment is obsolete: true
Attachment #9018926 - Attachment is obsolete: true
Attachment #9018930 - Attachment is obsolete: true
Attachment #9018932 - Attachment is obsolete: true
Attachment #9018933 - Attachment is obsolete: true
Attachment #9018934 - Attachment is obsolete: true
Attachment #9022176 - Attachment is obsolete: true
Attachment #9018924 - Flags: review?(mkmelin+mozilla)
Attachment #9018925 - Flags: review?(mkmelin+mozilla)
Attachment #9018926 - Flags: review?(mkmelin+mozilla)
Attachment #9018930 - Flags: review?(mkmelin+mozilla)
Attachment #9018932 - Flags: review?(mkmelin+mozilla)
Attachment #9018933 - Flags: review?(mkmelin+mozilla)
Attachment #9018934 - Flags: review?(mkmelin+mozilla)
Attachment #9022259 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #9022259 - Attachment is obsolete: true
Attachment #9022259 - Flags: review?(mkmelin+mozilla)
Attachment #9022368 - Flags: review?(mkmelin+mozilla)
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
Attachment #9022368 - Flags: review?(mkmelin+mozilla) → review-
Attached patch ruleactionbindingsfix.patch (obsolete) — Splinter Review
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?
Attachment #9022368 - Attachment is obsolete: true
Attachment #9023594 - Flags: review?(mkmelin+mozilla)
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;
Attachment #9023594 - Flags: review?(mkmelin+mozilla) → review-
Attached patch ruleactionbindingsfix.patch (obsolete) — Splinter Review
Attachment #9023594 - Attachment is obsolete: true
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.
Flags: needinfo?(mkmelin+mozilla)
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..
Comment on attachment 9025258 [details] [diff] [review]
ruleactionbindingsfix.patch

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

Yeah looks like this works now. r=mkmelin
Attachment #9025258 - Flags: review+
(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?
Attachment #9025258 - Attachment is obsolete: true
Attachment #9025309 - Flags: review+
Well it says the server responded with "temporary system problem" so I don't think it's related.
Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c7cda013b0b4
Remove ruleactiontarget bindings. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
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?
Flags: needinfo?(jorgk)
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
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jorgk)
Flags: needinfo?(arshdkhn1)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch backout.patch (obsolete) — Splinter Review
Backout seems to fix the problem:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d57b0d35db291e3fb1b73fc09ff8bd5b6b355701

Aceman, can you please check as well.
Flags: needinfo?(acelists)
Attached patch 1489172.patch (obsolete) — Splinter Review
This is the patch that landed, but rebased after the backout. Arshad, you can start with this patch and fix the issues we found.
Flags: needinfo?(acelists)
Keywords: leave-open
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
Attached patch ruleactionbindingsfix.patch (obsolete) — Splinter Review
Attachment #9027353 - Attachment is obsolete: true
Attachment #9027357 - Attachment is obsolete: true
Flags: needinfo?(arshdkhn1)
Attachment #9025309 - Attachment is obsolete: true
Attachment #9027458 - Attachment is obsolete: true
Attachment #9027459 - Attachment is obsolete: true
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.
Attachment #9027460 - Flags: review?(acelists)
Flags: needinfo?(mkmelin+mozilla)
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.
Attachment #9027460 - Flags: feedback+
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
const immutability will not be voided*
Attachment #9027460 - Attachment is obsolete: true
Attachment #9027460 - Flags: review?(acelists)
Attachment #9028113 - Flags: review?(acelists)
Attachment #9028113 - Flags: feedback+
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.
Attachment #9028113 - Flags: feedback+
Attachment #9028113 - Attachment is obsolete: true
Attachment #9028113 - Flags: review?(acelists)
Attachment #9028812 - Flags: review?(acelists)
Attachment #9028812 - Flags: feedback+
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.
Attachment #9028812 - Flags: review?(acelists)
Attachment #9028812 - Flags: review+
Attachment #9028812 - Flags: feedback+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6e49a3ec00cd
Replace ruleactiontarget bindings with custom element. r=mkmelin,aceman
Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Keywords: leave-open
Resolution: --- → FIXED
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.