Skip to content
This repository has been archived by the owner on Apr 1, 2020. It is now read-only.

Highlight first item in menus upon filtering #2632

Merged

Conversation

feltech
Copy link
Contributor

@feltech feltech commented Oct 14, 2018

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.

* 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.
* So reset the `selectedIndex` to zero whenever a menu's filter is
updated, meaning the first element in the menu will be highlighted.
* This method potentially suffers from issues of not updating the
`selectedIndex` after filtering the menu items, causing array
bounds errors.
* However, the related methods are completely unused, so just remove
them.
@feltech
Copy link
Contributor Author

feltech commented Oct 14, 2018

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
Copy link

codecov bot commented Oct 14, 2018

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%) ⬆️
browser/src/Services/Menu/MenuActionCreators.ts 27.77% <ø> (-1.96%) ⬇️
browser/src/Services/ContextMenu/ContextMenu.tsx 18.84% <ø> (+0.26%) ⬆️

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.

@feltech feltech changed the title Highlight first item in menu upon filtering Highlight first item in menus upon filtering Oct 14, 2018
@akinsho
Copy link
Member

akinsho commented Oct 15, 2018

@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
Copy link
Contributor Author

feltech commented Oct 15, 2018

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 e2dc9bc as part of a refactor.

Copy link
Member

@akinsho akinsho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great going to bring this in now thanks for the PR 👍

@akinsho akinsho merged commit 2ae1224 into onivim:master Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants