Page MenuHomephabricator

Load the menus from arrays, not numbered vars
ClosedPublic

Authored by kerberizer on Feb 20 2019, 2:02 PM.

Details

Summary

The EditToolbar currently loads the menus from variables with names (a)tpl1, (a)tpl2, (a)tpl3, etc. EditToolbar-core.js has a fixed number of menus defined (tpl1, atpl1, and atpl2), so the users can add their own menus by defining additional vars in their user JS with consecutive numbers: tpl2, atpl3, etc.

We would like to make this more flexible, allowing the menus to be added by gadgets—and in arbitrary user-decided combinations. The consecutively numbered variables restrict us, because: 1) each gadget should use a unique number that wouldn't collide with another gadget's var number and with any user-defined menus; 2) the current logic stops adding menus once an undefined var is encountered, which might easily happen if e.g. the user activates gadgets with vars tpl2, tpl3, tpl5. Because they hadn't activated the gadget with tpl4, the one with tpl5 won't be loaded as well.

We avoid those problems by replacing the consecutively numbered variables, each holding a menu, with a single array, where the menus are elements of the array. This actually makes the code cleaner as well, as we can simply iterate over the array elements, instead of using the unsafe eval to access the numbered variables.

For reasons of backwards compatibility, considering that some users may have defined custom menus in their user JS, we still check for defined (a)tpl{i} variables and load them into the new arrays. We do this for all i in {0..10}, that is we ignore missing vars (the old default tpl1, atpl2, and atpl2 are now gone) and we never check beyond (a)tpl10, which should be more that enough.

Test Plan

Not really sure what would be the best approach. If you have any ideas, please share them.

Diff Detail

Repository
rUI JS/CSS User Interface
Branch
enh/menu-arrays
Lint
Lint WarningsExcuse: "Eval" is inevitable with numbered variables.
SeverityLocationCodeMessage
WarningGadget-EditToolbar-core.js:108W061JSHintW061
Unit
No Unit Test Coverage
Build Status
Buildable 20
Build 20: arc lint + arc unit

Event Timeline

kerberizer requested review of this revision.Feb 20 2019, 2:02 PM
kerberizer created this revision.
kerberizer updated this revision to Diff 25.Feb 20 2019, 2:05 PM

Iterate over all i in {0..10}

This was actually the old behaviour: to exit the loop on first undefined numbered variable. Instead, we just ignore the undefined ones.

This is actually something I wanted to do for a long time, so I was pleasantly surprised to see it done. :-) I will review it in the coming days.

Borislav accepted this revision.Feb 23 2019, 9:00 PM
This revision is now accepted and ready to land.Feb 23 2019, 9:00 PM
This revision was automatically updated to reflect the committed changes.