Closed Bug 1546301 Opened 6 years ago Closed 5 years ago

[de-xbl] convert the ruleactiontype-menulist binding to <menulist is="ruleactiontype-menulist">

Categories

(Thunderbird :: Filters, task)

Product:
Thunderbird
Component:
Filters
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 69.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug-1546301_dexbl_ruleactiontype-menulist.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review

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

The ruleactiontype-menulist binding can be converted to a customized built-in.
Should be similar to e.g. menulist-charsetpicker-viewing.

Assignee: nobody → arshdkhn1
Attached patch ruleactiontypemenulistWIP.patch (obsolete) (deleted) — — Splinter Review
Assignee: arshdkhn1 → khushil324
Attachment #9069802 - Attachment is obsolete: true
Attachment #9071180 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9071180 [details] [diff] [review] Bug-1546301_de-xbl_ruleactiontype-menulist.patch Review of attachment 9071180 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work, so r=mkmelin with the below ::: mailnews/base/search/content/searchWidgets.js @@ +1038,5 @@ > + */ > + class MozRuleactiontypeMenulist extends customElements.get("menulist") { > + connectedCallback() { > + super.connectedCallback(); > + if (this.delayConnectedCallback()) { should protect against being run more than once, so add this.hasChildNodes() @@ +1087,5 @@ > + let scope = getScopeFromFilterList(gFilterList); > + > + // walk through the list of filter actions and hide any actions which aren't valid > + // for our given scope (news, imap, pop, etc) and context. > + let elements, i; please just declare the i everywhere it's used, not here @@ +1138,5 @@ > + } > + } > + > + /** > + * returns a hash containing all of the filter actions which are currently R
Attachment #9071180 - Flags: review?(mkmelin+mozilla) → review+
Summary: [de-xbl] convert the ruleactiontype-menulist binding to <menulist is="rule-action-type"> → [de-xbl] convert the ruleactiontype-menulist binding to <menulist is="ruleactiontype-menulist">

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

should protect against being run more than once, so add this.hasChildNodes()

We are not inserting the elements in the connected callback, the child elements are inserted in the XUL file only. So When connectedCallback will run, there will be child elements in the menulist. Any other to prevent connected callback for running 2nd time?

(In reply to Khushil Mistry [:khushil324] from comment #4)

We are not inserting the elements in the connected callback, the child elements are inserted in the XUL file only. So When connectedCallback will run, there will be child elements in the menulist. Any other to prevent connected callback for running 2nd time?

Got this with this.hasConnected.

Ok, you can use that. (But even if we don't insert children we should protect against running twice so we don't double up event handlers and such.)

Attachment #9071180 - Attachment is obsolete: true
Attachment #9073159 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e9bc2e1580a0
[de-xbl] convert the ruleactiontype-menulist binding to <menulist is='ruleactiontype-menulist'>. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

I pushed that with DONTBUILD since there was a green try and Daily will take care of it.

Target Milestone: --- → Thunderbird 69.0
Comment on attachment 9073159 [details] [diff] [review] Bug-1546301_dexbl_ruleactiontype-menulist.patch Review of attachment 9073159 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/search/content/searchWidgets.js @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +/* global MozXULElement, gFilter, gFilterList, onEnterInSearchTerm, convertDateToString, initializeTermFromId, > + convertPRTimeToString, convertStringToPRTime, UpdateAfterCustomHeaderChange, checkActionsReorder, > + initializeTermFrom, IdcheckActionsReorder, getScopeFromFilterList, gCustomActions, gFilterType */ Aren't the lines too long? @@ +9,2 @@ > > +var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); Why are the spaces better? @@ +91,4 @@ > > this.appendChild(menulist); > > + document.getAnonymousElementByAttribute(this.closest(".ruleaction"), "is", "ruleactiontype-menulist") Long line. @@ +1051,5 @@ > + }); > + > + this.addEventListener("popupshowing", (event) => { > + let unavailableActions = this.usedActionsList(); > + for (let index = 0; index < this.menuitems.length; index++) { Wouldn't this take 'for (let menu of this.menuitems)' ? @@ +1068,5 @@ > + // and creating a row which will be initialized with an action. > + if (!this.parentNode.hasAttribute("initialActionIndex")) { > + let unavailableActions = this.usedActionsList(); > + // Select the first one that's not in the list. > + for (let index = 0; index < this.menuitems.length; index++) { Again.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: