BundleEditor: drag reorder, Miller arrows, dividers, category dialog

[tbates]

Follows the review in #64. Three commits: orphan-aware menu slot translation with serial tests; the BundleEditor work (drag reorder, Miller arrows, dividers, category add/delete/rename via dialog); review hygiene. Serial bundles suite green.

[dayglojesus]

Thanks, Tim. All seven points from #64 are addressed: validation now precedes materialisation (with the rejected.empty() test), the drag-session override is gone in favour of the data-source callback, the logging is failure-only, the in-place rename path is out, menuForEvent: is the single context-menu mechanism, and the housekeeping is done. Built on top of main, bundles suite green under the serial runner, and I re-ran the persistence checks on a scratch bundle.

One real problem in the new slot translation, plus two small things:

1. menu_indexes_for_pane_slot treats disabled items as undrawn (draws = !is_deleted && !is_disabled && !hidden), but the pane does draw them: menu_entry_t::entries() lists the menu through item_t::menu(true) (be_entry.cc:62), which keeps disabled items (query.cc:457), and the cell greys them (BundleEditor.mm:1011). So a disabled item above the slot puts every drop and insert one row low. Reproduced on a scratch bundle with items Alpha (isDisabled), Bravo, Charlie: "Insert Divider Here" on the Alpha row lands the divider below Bravo, and the saved plist reads [A, B, ------, C]. Unit form, which fails on this branch with (2 != 1):

   // entries {a, b, c}, a->initialize({{kFieldIsDisabled, true}}), slot 1
   OAK_ASSERT(bundles::menu_indexes_for_pane_slot(entries, 1, {}) == std::make_pair(1ul, 1ul));

   Disabled items are one checkbox away for any user ("Enable this item" in the properties pane), so this will be hit. Dropping !is_disabled(item) from the predicate makes the translation match the pane; deleted and hidden stay as they are.

2. kRealModifiers includes NSEventModifierFlagCapsLock. If that bit is set while Caps Lock is engaged, arrow navigation stops working with Caps Lock on. I have not verified the flag's behaviour for the engaged state, so treat this as a question; dropping it from the mask costs nothing.

3. NSMenuDelegate and NSTextFieldDelegate are still declared on the class extension but no delegate methods remain and nothing is assigned to them (upstream had neither). cmake/TextMateHelpers.cmake:130 has a whitespace-only line.

On the history: the PR shows 22 commits, since the three new ones sit on top of the original run rather than replacing it. We can squash-merge on our side, so no need to rewrite your main unless you prefer to.

[tbates]

Thanks Brian,

1. Nice catch of the disabled item miscount: I didn't have any. Fixed and verified now.
2. Capslock is now removed from the mask: verified arrow works whether it's up or down.
3. The relic of attempting in-item edit text before going with a dialog approach is now entirely clear (NSMenuDelegate and NSTextFieldDelegate gone. Whitespace line in cmake/TextMateHelpers.cmake:130 now a clean empty line.

We rewrote main at my end (now based off your current main) and crushed all of the previous push into one commit, and the update in response to this review into a second commit than replacing it.

Tim

[dayglojesus]

Thanks, Tim. Checked a32d37e on top of main:

• The disabled-row fix holds. Your regression test is the right shape (it fails on the previous predicate: slot 0 resolved to {2,2}), the serial bundles suite is green, and the in-app case from my review now comes out right: "Insert Divider Here" on a disabled first row puts the divider directly under it, and the saved plist reads [A, ------, B, C].
• CapsLock out of the mask, the two unused protocol conformances gone, whitespace line gone.
• The squashed commit is content-identical to the tree I reviewed, so nothing slipped in with the rewrite.

One nit you can take or leave: query.cc line 321 (bool draws = false;) lost a tab in the fix and sits one level out from the lines around it.

Approving. Two clean commits on top of main, so this can go in as a regular merge.