Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix Quick Open async-related bugs & performance problems #2548

Merged
merged 1 commit into from
Jan 15, 2013

Conversation

peterflynn
Copy link
Member

Fixes these issues:

Also, some code cleanup:

  • Move code that didn't need to be in keyup into _filterCallback()
  • Remove keydown handler now that it's unneeded
  • Add more docs on event ordering, etc.

I contemplated ditching Smart Autocomplete altogether, since we have been fighting with it so much. But ultimately, it didn't seem worth it for this fix because it wouldn't avoid most of the thorny issues:

  • Processing letter keys async after the key event is a must if we want to avoid performance problems from multiple key events queueing up (the staleness check in _filterCallback() won't work otherwise). So the async complexity there wouldn't be avoided.
  • Earlier I griped about Smart Autocomplete not noticing programmatic text field changes, which we'd had to work around in an earlier bug. But it turns out that's basically impossible to fix since programmatic changes don't dispatch any events.

Down the road we still may want to get rid of it: it would still make key handling a tad cleaner (e.g. simplifying the Enter key fix here), let us improve perf slightly more (by skipping list re-render when stale instead of only skipping re-filtering as here), let us fix memory leak #1550, and fix the odd bug where hitting right-arrow is treated as hitting Enter. But it didn't feel worth it to make that big migration yet.

- #1855 (Typing quickly in Go to Definition goes to wrong location on
  Enter) -- moved Enter/Esc handling to keyup and added async delay to
  parallel how letter keys are handled; register keyup handler earlier so
  it can block Enter from being processed by Smart Autocomplete (which we
  did before by killing the UI before the keyup could arrive)
- Remaining bit of #1627 (Quick Open doesn't scroll based on prepopulated
  text if opened via menu) -- moved code from keyup to new
  _handleShowResults() method
- Slow performance when typing qucikly in big files like codemirror.js --
  added staleness check in _filterCallback()

Also, some code cleanup:
- move code that didn't need to be in keyup into _filterCallback()
- remove keydown handler now that it's unneeded
- add more docs on event ordering, etc.
@peterflynn
Copy link
Member Author

Also, miraculously I think this and #2462 manage not to collide -- the merge within QuickOpen.js should be pretty trivial.

showResults: this._handleShowResults,
itemSelect: this._handleItemSelect,
itemFocus: this._handleItemFocus,
keyup: this._handleKeyUp, // it's important we register this BEFORE calling smartAutoComplete(); see handler for details
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the W3C spec does guarantee this: listeners on the same node run "in their order of registration."

@peterflynn
Copy link
Member Author

Note: for more context on the various async issues with Quick Open, see my comment thread in #1855. Additional background is in #1627 and #1384.

// the key event; checking DURING the key event itself would never yield a future value for the input field.
if (queryIsStale(query)) {
return getLastFilterResult();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this doesn't 100% guarantee no unnecessary filter processing:

  • As NJ pointed out back when the searchFileList() usage was added, the user might type an extra char & then backspace, causing two filter invocations with the same, "non-stale" query string.
  • Experimentation shows that Chromium will actually skip ahead if key event processing gets really behind (like > 1 second). The DOM text field value gets fast-forwaded to the latest value even while older keyup events are getting handled. The result is typically 2-3 filter invocations with the latest "non-stale" string instead of just one.

Cases like those would be pretty easy to fix if we ditched Smart Autocomplete. Short of that though, it'd be hackier (e.g. monitoring Smart Autocomplete's keyIn events to figure out what timeouts it's going to queue up that we'll receive later on) -- doesn't seem worth that level of effort.

@ghost ghost assigned dangoor Jan 15, 2013
@dangoor
Copy link
Contributor

dangoor commented Jan 15, 2013

The fix looks great (all of the comments are super helpful) and works well in my testing. I didn't spot anything I would change.

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