onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.35k stars 299 forks source link

Highlight first item in menus upon filtering #2632

Closed feltech closed 5 years ago

feltech commented 5 years ago

If the user moves through the menu then resumes typing, the selectedIndex can end up larger than the contents of the menu. This can cause errors, or at least graphical glitches, and seems odd.

feltech commented 5 years ago

Some pics highlighting the issue

Unfiltered oni_selecteditem_range_before

Then type to filter: oni_selecteditem_range_after

You can see no option is highlighted, and the following exception occurs

vendor.bundle.js:925 Uncaught TypeError: Cannot read property 'rawCompletion' of undefined
    at EventEmitter.Ce._contextMenu.onSelectedItemChanged.subscribe.e (vendor.bundle.js:925)
    at emitOne (events.js:116)
    at EventEmitter.emit (events.js:211)
    at t.Event.dispatch (vendor.bundle.js:632)
    at Object.onSelectedItemChanged (vendor.bundle.js:987)
    at r (vendor.bundle.js:839)
    at vendor.bundle.js:839
    at vendor.bundle.js:824
    at n (vendor.bundle.js:632)
    at Object.filterMenu (vendor.bundle.js:632)

With the fix oni_selecteditem_range_fixed

codecov[bot] commented 5 years ago

Codecov Report

Merging #2632 into master will increase coverage by 0.05%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2632      +/-   ##
==========================================
+ Coverage   45.33%   45.38%   +0.05%     
==========================================
  Files         361      361              
  Lines       14572    14559      -13     
  Branches     1913     1910       -3     
==========================================
+ Hits         6606     6608       +2     
+ Misses       7741     7725      -16     
- Partials      225      226       +1
Impacted Files Coverage Δ
browser/src/Services/Menu/MenuReducer.ts 35.89% <ø> (+13.89%) :arrow_up:
browser/src/Services/Menu/MenuActionCreators.ts 27.77% <ø> (-1.96%) :arrow_down:
browser/src/Services/ContextMenu/ContextMenu.tsx 18.84% <ø> (+0.26%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ca6a4d1...35aa72b. Read the comment docs.

akinsho commented 5 years ago

@feltech I noticed that this PR removes the details code, I'm wondering if this impacts rendering of the documentation section for the autocomplete menu?

feltech commented 5 years ago

I believe the code responsible for processing a completionItem/resolve, which must be where the documentation comes from, was the bit I fixed in #2633. I cannot find any references to the string SET_DETAILED_MENU_ITEM and the related methods that I removed. Grepping the git history, it looks like the final reference to the top-level function updateItem was removed in e2dc9bcbfc11b2d73061deaada865fda26bed9ef as part of a refactor.