[de-xbl] convert the ruleaction binding (and the filterlistitem binding it is using) to <richlistitem is="ruleaction-richlistitem">
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: arshad, Assigned: khushil324)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 2•5 years ago
|
||
Likely the filterlistitem can just go (workaround for something that would appear not to be the case anymore)
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 4•5 years ago
|
||
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
Assignee | ||
Comment 5•5 years ago
|
||
Assignee | ||
Comment 6•5 years ago
|
||
(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•5 years ago
|
||
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
Assignee | ||
Comment 8•5 years ago
|
||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
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?
Assignee | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
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 */
Assignee | ||
Comment 14•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
I can't see a try run here. This looks like a larger patch.
Assignee | ||
Comment 16•5 years ago
|
||
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•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5496b4328be5
[de-xbl] convert the ruleaction binding to <richlistitem is='ruleaction-richlistitem'>. r=mkmelin DONTBUILD
Updated•5 years ago
|
Description
•