Adds a FormEditEngine MenuItem for adding forms to Projects, Home, QuickCreate. Also adds an EditEngine typeahead that has token rendering issues currently.
Details
- Reviewers
epriestley - Commits
- rPe9243f22b92c: Add Form MenuItem, Fix EditEngine Typeahead
Set a normal form as a menu item, edit it, set the name. Set a custom form as a menu item, edit it, set a name.
Diff Detail
- Repository
- rP Phabricator
- Branch
- form-menuitem (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 15017 Build 19696: Run Core Tests Build 19695: arc lint + arc unit
Event Timeline
src/applications/transactions/typeahead/PhabricatorEditEngineDatasource.php | ||
---|---|---|
24–32 | Having trouble here, can't quite get this to work when editing a menu item, it the shows the token as "Unknown Object ????" |
Yeah, this doesn't appear to be me, but not sure the correct fix since it's in the token renderer in typeahead.
This doesn't have a check in PhabricatorEditEngineConfiguration->setBuiltinKey() (suggested in D17093). Did you just overlook that, or decide against it?
src/applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php | ||
---|---|---|
55–58 | This will probably never show up on a profile, but the runtime complexity is higher than it needs to be. I think it can be rewritten to have lower runtime complexity without a real change in readability. Here's the runtime complexity of the code as written:
So this has complexity O(number of items * number of builtin forms): for every item, we have to do something to every form. Neither of these is likely to be exceptionally large, but users could conceivably build a menu with 100 items and we might have 100 builtin forms in the future, so this would be ~10,000 operations. These operations are cheap, but I could imagine this taking ~5-10ms on a profile some day. And there's no upper bound to the number of items or forms that we could have. A version of this code with lower runtime complexity is:
That would look something like this, up top: $builtin_map = array(); foreach ($form_engines as $engine_key => $form_engine) { $builtin_map[$engine_key] = mpull($form_engine, null, 'getBuiltinKey'); } That computes the map once, which has cost O(number of builtin forms). Then, for each item, when we identify that the key is non-numeric: if (isset($builtin_map[$form->getEngineKey()][$form_key])) { ... } Each test is a hash check and we do one per item, so this is O(number of items). So our overall complexity is now O(number of items + number of builtin forms) -- the sum of the terms instead of their product -- or about 200 operations instead of 10,000 for the "100 of each" case. I think that code isn't any more complex or harder to read. That said, this is definitely premature optimization so take it or leave it. We can adjust it if it ever shows up on a profile. | |
112–117 | This stuff looks unused (you use $form_key instead, which is good). | |
src/applications/transactions/editengine/PhabricatorEditEngine.php | ||
53 |
| |
src/applications/transactions/typeahead/PhabricatorEditEngineDatasource.php | ||
23 | Can this be private? | |
47 | The name getIsDefault() is a little misleading, and we should probably change it to getIsCreateForm() or similar eventually. A form marked as a create form is not necessarily the default create form: the default create form is the first visible one in the ordered list of create forms. So if you mark three forms "A", "B" and "C" as create forms, and order them in "A, B, C" order, this will mark all three as "Default Create Form", when "B" and "C" are normally not default create forms. The default form is also viewer-dependent, so even if we label the first form, it might be misleading to label that particular form as "Default Create Form" when it really means "Your Default Create Form, As The Current Viewer". Maybe just "Create Form". |
This doesn't have a check in PhabricatorEditEngineConfiguration->setBuiltinKey() (suggested in D17093). Did you just overlook that, or decide against it?
Oh maybe I misunderstood, I can take another look. I added an exception in PhabricatorEditEngine.php and thought that was sufficient.
There are two different parts -- one key for the EditEngine itself (like maniphest.task) which you're checking, and one for the form itself, which isn't currently checked.
Today, all applications only have one builtin form, but we might ship multiple forms by default in the future (e.g., public vs private Calendar events) or new builtin forms might be provided by extensions in the future.
That is, if we decided to ship two default forms in Calendar, they might look something like this:
Engine Key | Form Builtin Key | Human-Readable Name |
---|---|---|
calendar.event | event.public | "Create New Public Event" |
calendar.event | event.private | "Create New Private Event" |
The "Engine Key" is being checked, but the "Form Builtin Key" isn't.
src/applications/transactions/editengine/PhabricatorEditEngine.php | ||
---|---|---|
52–59 | wait I think this is wrong |