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

Fix bug #1627 ("Go to Definition" prepopulated text doesn't work via menu) #1864

Merged
merged 1 commit into from
Oct 18, 2012

Conversation

peterflynn
Copy link
Member

Fix bug #1627 ("Go to Definition" prepopulated text doesn't work when activated via menu)

See my comment in the bug for details.

This also contains a minor cleanup to use KeyEvent consts for key codes.

activated via menu) -- There are two problems at play. First, smart-autocomplete
only listens for key events, not programmatic or other textfield changes,
so we need to kick it to generate results for the prepopulated text.

Second, the results are generated asynchronously while QuickOpen's auto-"focus"
is synchronous. That bug is not fixed here since it's related to a whole
tangle of async issues in #1855.

Fixing the first bug alone means that hitting Enter now goes to the correct
result (instead of doing nothing), but that the editor doesn't auto-scroll
to that result as soon as the search bar is opened.

This commit also cleans up QuickOpen to use KeyEvent consts for key codes.
@ghost ghost assigned redmunds Oct 17, 2012
@redmunds
Copy link
Contributor

Here's what I'm seeing:

  1. Using Ctrl+T, there is only 1 item in list and it is not selected, the function definition is immediately selected and scrolled to.
  2. Using Navigate > Go to Definition menu, there is only 1 item in list and it is not selected. If I select it and hit Enter, then it behaves the same as 1.
  3. Using Navigate > Go to Definition menu, there is only 1 item in list and it is not selected. If I press Escape (whether item is selected ot not), then the dialog is dismissed, and selection does not change.
  4. Using Navigate > Go to Definition menu, there is only 1 item in list and it is not selected. If I press Enter, then the dialog is dismissed, the function definition is selected, but editor does not scroll to selection.

Is this the intended behavior?

Seems like 4 would be easy to fix by calling ViewUtils.scrollElementIntoView() or something equivalent.

@peterflynn
Copy link
Member Author

Hmm, I hadn't noticed the scrolling problem before. We're doing the exact same thing in both cases 1 and 4 -- just setting the selection, which normally scrolls it into view automatically. Might be a CodeMirror bug... investigating now.

@peterflynn
Copy link
Member Author

The scrolling bug appears to be a regression due to all the focusedEditorChange changes that occurred this sprint -- in particular, the issues described in #1860. Any time an editor regains focus, this event is dispatched spuriously, and it causes the editor state to get blown away and refreshed three times. The refresh() essentially blows away the pending scroll position change that was just set by the Quick Open bar before it gave focus back to the editor.

peterflynn added a commit that referenced this pull request Oct 18, 2012
* Revert main-editor-swapping codepath back to how it used to work, mostly
disentangled from focus events.
* Don't fire focusedEditorChanged on the many times focus returns to the
same Editor that had it last (due to window reactivation, closing a dialog,
closing a search bar, etc.). All the current use cases for this event don't
care about those cases. Add more detailed docs on event cases.
* Simplify how Editor signals focus: trigger only from "onFocus" (a superset
of the other case), and remove _internalFocus flag that was guarding against
the overlap.
* Don't call resizeEditor() on every editor swap just because statusbar
*might* have been shown/hidden: only call when statusbar actually did
change (going to/from no editor). Fixes scrolling issue in #1864.

#1257 still isn't 100% fixed: there are at least two cases where getFocusedEditor()
lags behind the focusedEditorChanged event: opening an inline editor; and
moving focus from open inline editor to its host editor. These same cases -
and a number of others - were broken before this change, however.
@peterflynn
Copy link
Member Author

@redmunds: The patch in #1877 should fix the scrolling issue. But since it's relatively independent, we could go ahead and merge this fix sooner, knowing that for now it only works fully when jumping to the result doesn't require scrolling. Up to you...

@redmunds
Copy link
Contributor

Merging.

redmunds added a commit that referenced this pull request Oct 18, 2012
Fix bug #1627 ("Go to Definition" prepopulated text doesn't work via menu)
@redmunds redmunds merged commit 285bd6b into master Oct 18, 2012
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