1500887 - [de-xbl] convert the ruleaction binding (and the filterlistitem binding it is using) to <richlistitem is="ruleaction-richlistitem">

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/mailnews/base/search/content/searchWidgets.xml#287

Comment 2

[Magnus Melin [:mkmelin]]

Likely the filterlistitem can just go (workaround for something that would appear not to be the case anymore)

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1500887_de-xbl_ruleaction-richlistitem.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9074842 [details] [diff] [review]
Bug-1500887_de-xbl_ruleaction-richlistitem.patch

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

Something is missing. When I click new or edit in the filter editor, the area to fill in new rules is all blank

::: mailnews/base/search/content/searchWidgets.js
@@ +1255,5 @@
>      { extends: "menulist" });
>  });
> +
> +
> +class MozRuleactionRichlistitem extends MozElements.MozRichlistitem {

Please add documentation.

@@ +1309,5 @@
> +        "chrome://messenger/locale/messenger.dtd",
> +        "chrome://messenger/locale/FilterEditor.dtd",
> +      ]));
> +
> +    this.mRuleActionType = this.childNodes[0];

maybe clearer to do this.querySelector("menulist");

@@ +1318,5 @@
> +
> +  set selected(val) {
> +    // This provides a dummy selected property that
> +    // the listbox expects to be able to call.
> +    // See bug 202036.

Can we remove this now?

@@ +1328,5 @@
> +
> +  _fireEvent(aName) {
> +    // This provides a dummy _fireEvent function that
> +    // the listbox expects to be able to call.
> +    // See bug 202036.

Can we remove this now?

@@ +1336,5 @@
> +    // We should only remove the initialActionIndex after we have been told that
> +    // both the rule action type and the rule action target have both been built since they both need
> +    // this piece of information. This complication arises because both of these child elements are getting
> +    // bound asynchronously after the search row has been constructed
> +

limit these to 80 ch

@@ +1414,5 @@
> +  }
> +
> +  validateAction() {
> +    // Returns true if this row represents a valid filter action and false otherwise.
> +    // This routine also prompts the user.

this looks like it should be the method documentation (jsdoc style, above)

@@ +1466,5 @@
> +    return !errorString;
> +  }
> +
> +  saveToFilter(aFilter) {
> +    // Create a new filter action, fill it in, and then append it to the filter.

this too

@@ +1499,5 @@
> +    aFilter.appendAction(filterAction);
> +  }
> +
> +  getActionStrings(aActionStrings) {
> +    // Collect the action names and arguments in a plain string form.

and this

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1500887_de-xbl_ruleaction-richlistitem.patch

Comment 6

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #4)

Can we remove this now?

Doing so, it is showing this: JavaScript error: chrome://global/content/elements/richlistbox.js, line 422: TypeError: setting getter-only property "selected"

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9077197 [details] [diff] [review]
Bug-1500887_de-xbl_ruleaction-richlistitem.patch

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

Seems to work now

::: mailnews/base/search/content/searchWidgets.js
@@ +5,5 @@
>  
>  /* global MozXULElement, gFilter, gFilterList, onEnterInSearchTerm, convertDateToString, initializeTermFromId,
>     convertPRTimeToString, convertStringToPRTime, UpdateAfterCustomHeaderChange, checkActionsReorder,
> +   initializeTermFrom, IdcheckActionsReorder, getScopeFromFilterList, gCustomActions, gFilterType, MozElements
> +   gFilterBundle, gFilterActionStrings, gFilterBundle, gFilterActionStrings */

gFilterBundle twice

@@ +1260,5 @@
> +/**
> + * The MozRuleactionRichlistitem is a widget which gives the options to filter
> + * the messages with following elements: ruleactiontype-menulist, ruleactiontarget-wrapper
> + * and button to add or remove the MozRuleactionRichlistitem. It is get added in
> + * filterActionList Richlistbox in the Filter Editor dialog.

grammar derailed, I don't understand the last sentence at all

@@ +1337,5 @@
> +
> +  _fireEvent(aName) {
> +    // This provides a dummy _fireEvent function that
> +    // the listbox expects to be able to call.
> +    // See bug 202036.

since that's apparently still the case, change the comment to "richlistbox", and make the comment use the full 80 ch

@@ +1427,5 @@
> +  /**
> +   * Function is used to check if the filter is valid or not. This routine
> +   * also prompts the user.
> +   *
> +   * @return {boolean} - true if this row represents a valid filter action else false.

can remove the "else false."

@@ +1520,5 @@
> +
> +  /**
> +   * Collect the action names and arguments in a plain string form.
> +   *
> +   * @param {String} aActionStrings - the action name as a sting.

apparently {String[]} 

but, wow, so it's actually using out params in js. please change this method to return the collected strings instead (and adjust the one caller)

@@ +1561,5 @@
> +    checkActionsReorder();
> +  }
> +
> +  /**
> +   * this.mListBox will fail after the row is removed, so save it.

this is not a method documentation, please move this comment down inside where it belongs. (so save it -> so safe a reference)

@@ +1574,5 @@
> +    checkActionsReorder();
> +  }
> +
> +  /**
> +   * When this action row is focused, store its index in the parent listbox.

richlistbox

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1500887_de-xbl_ruleaction-richlistitem.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9078339 [details] [diff] [review]
Bug-1500887_de-xbl_ruleaction-richlistitem.patch

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

::: mailnews/base/search/content/FilterEditor.js
@@ +535,3 @@
>  function showActionsOrder() {
>    // Fetch the actions and arguments as a string.
>    let actionStrings = [];

this would go

@@ +535,5 @@
>  function showActionsOrder() {
>    // Fetch the actions and arguments as a string.
>    let actionStrings = [];
> +  for (let index = 0; index < gFilterActionList.itemCount; index++) {
> +    actionStringsg = FilterActionList.getItemAtIndex(index)

and... typo!

::: mailnews/base/search/content/searchWidgets.js
@@ +1519,5 @@
> +  /**
> +   * Collect the action names and arguments in a plain string form.
> +   *
> +   * @param {Object[]} aActionStrings - the action objects with action name and argument.
> +   *

you would no longer take an input param. 

Just start a new array inside the method, fill that up and return it.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1500887_de-xbl_ruleaction-richlistitem.patch

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9078535 [details] [diff] [review]
Bug-1500887_de-xbl_ruleaction-richlistitem.patch

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

::: mailnews/base/search/content/FilterEditor.js
@@ +534,4 @@
>   */
>  function showActionsOrder() {
>    // Fetch the actions and arguments as a string.
> +  let actionStrings = gFilterActionList.getItemAtIndex(0).getActionStrings();

hmm, I wasn't very clear earlier, but of course you still need to loop, and add the added items to the final result, like

for (let i = 0; i < gFilterActionList.itemCount; i++) {
  let item = gFilterActionList.getItemAtIndex(i);
  actionStrings.push(...item.getActionStrings());
}

::: mailnews/base/search/content/searchWidgets.js
@@ +1522,5 @@
> +   * @return {Object[]} aActionStrings - the action objects with action objects with
> +   *                                     name and argument added for each filter.
> +   */
> +  getActionStrings() {
> +    let aActionStrings = [];

"a" is for argument, so please just name it actionStrings

@@ +1523,5 @@
> +   *                                     name and argument added for each filter.
> +   */
> +  getActionStrings() {
> +    let aActionStrings = [];
> +    for (let index = 0; index < gFilterActionList.itemCount; index++) {

while you're here changing it, make it i instead of index

@@ +1524,5 @@
> +   */
> +  getActionStrings() {
> +    let aActionStrings = [];
> +    for (let index = 0; index < gFilterActionList.itemCount; index++) {
> +      let FilterActionList = gFilterActionList.getItemAtIndex(index);

and this should be not be capitalized
also seems like the wrong name altogether? should be "ruleAction" I guess?

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1500887_de-xbl_ruleaction-richlistitem.patch

Actually, we can remove getActionStrings function from ruleaction-richlistitem and directly integrate into showActionsOrder. We just need an array with name of the each action and its argument.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9078700 [details] [diff] [review]
Bug-1500887_de-xbl_ruleaction-richlistitem.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +5,5 @@
>  
>  /* global MozXULElement, gFilter, gFilterList, onEnterInSearchTerm, convertDateToString, initializeTermFromId,
>     convertPRTimeToString, convertStringToPRTime, UpdateAfterCustomHeaderChange, checkActionsReorder,
> +   initializeTermFrom, IdcheckActionsReorder, getScopeFromFilterList, gCustomActions, gFilterType, MozElements
> +   gFilterBundle, gFilterActionStrings, gFilterActionStrings, gFilterActionList */

gFilterActionStrings is twice here

though it could be better to have 

/* import-globals-from FilterEditor.js */

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: Bug-1500887_de-xbl_ruleaction-richlistitem.patch

Comment 15

[Jorg K (CEST = GMT+2)]

I can't see a try run here. This looks like a larger patch.

Comment 16

[Khushil Mistry [:khushil324]]

Can you push a try run for me ? I am on a different machine and don't have configured key for try push.

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Khushil Mistry [:khushil324]]

Thanks :)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5496b4328be5
[de-xbl] convert the ruleaction binding to <richlistitem is='ruleaction-richlistitem'>. r=mkmelin DONTBUILD