Skip to content

Fix closing completion hint#3089

Closed
mloginov wants to merge 2 commits intocodemirror:masterfrom
mloginov:tern_close_completion_hint
Closed

Fix closing completion hint#3089
mloginov wants to merge 2 commits intocodemirror:masterfrom
mloginov:tern_close_completion_hint

Conversation

@mloginov
Copy link
Copy Markdown
Contributor

Sometimes hints don't disappear

  1. Open a JS file in the editor.
  2. Type fo to get a hint about for loops.
  3. Repeatedly and rapidly press backspace followed by o to make the hint load multiple times.
  4. You will get leftover hints that don't disappear.

It happens in FF often.

This PR fixes this problem.

@marijnh
Copy link
Copy Markdown
Member

marijnh commented Feb 17, 2015

I can not reproduce this issue. Does it happen in http://codemirror.net/demo/complete.html ?

@mloginov
Copy link
Copy Markdown
Contributor Author

No. It happens if completions update on each key press.
http://screencast.com/t/eCfHmUNxReI

@mloginov
Copy link
Copy Markdown
Contributor Author

Also fixed lagging if hints changes very often. Looks like this bugs appears on complex layouts.
Before
http://screencast.com/t/bOT0aelSm
After
http://screencast.com/t/uwssrWjc

@marijnh
Copy link
Copy Markdown
Member

marijnh commented Feb 18, 2015

You still didn't show me your code, but I assume you are starting new completions when the old one is still open. Don't do that. See this demo code for an example of how to do it properly.

@mloginov
Copy link
Copy Markdown
Contributor Author

No. I open only one completions like in your example. But I found where problem can be.
Looks like the problem in show-hint addon in update method.

on cursor activity -> update -> throw update signal -> getHints(async) -> finishUptate and throw select signal on new widget creation.

But if getHints gets a lot of time(for example if 100 docs was added to tern) you can get 2 update events in tern addon and then 2 select events for different positions.

Now, because tooltip stored in local scope it wouldn't be removed.

I added it to ts.completionHint just to be sure that tooltip would be removed in any case.

@marijnh
Copy link
Copy Markdown
Member

marijnh commented Feb 19, 2015

Okay, you are using an asynchronous hinter. That might have been good to include in the report. You are right that there is a race condition when using asynchronous hinting. I'll take a look.

@marijnh
Copy link
Copy Markdown
Member

marijnh commented Feb 19, 2015

Well, I still can't reproduce this. Please create a minimal test case that shows the issue. Here's my code:

<!doctype html>

<meta charset="utf-8">
<script src="lib/codemirror.js"></script>
<link rel=stylesheet href="lib/codemirror.css">
<link rel=stylesheet href="addon/hint/show-hint.css">
<script src="mode/javascript/javascript.js"></script>
<script src="addon/hint/show-hint.js"></script>
<script src="addon/hint/javascript-hint.js"></script>

<body>
<script>
  CodeMirror.registerHelper("hint", "foo", function(cm, f, options) {
    setTimeout(function() {
      f(CodeMirror.hint.javascript(cm, options));
    }, 400);
  });
  CodeMirror.hint.foo.async = true;

  var cm = CodeMirror(document.body, {mode: "javascript", lineNumbers: true});
  cm.on("inputRead", function() {
    if (!cm.state.completionActive)
      cm.showHint({globalScope: {abcd: {}, abef: {}, abcdef: {}}, hint: CodeMirror.hint.foo});
  });
</script>
</body>

@mloginov
Copy link
Copy Markdown
Contributor Author

You should use tern addon to reproduce it. Problem in tern tooltip that appears in 'hint' method.

Just use example from http://codemirror.net/demo/tern.html
But slightly change tern addon(getHint method) like this to emulate ternServer delay

this.getHint = function(cm, c) { setTimeout(function () { return hint(self, cm, c); }, 400); };

Then to reproduce.

  1. Clear textarea.
  2. Open hint by Ctrl+Space
  3. Type fast - 'e' - 'backspace' - 'e' - 'backspace' etc.

Test case http://jsfiddle.net/09aao7f9/8/
Video http://screencast.com/t/JyDDF9Teg

marijnh added a commit that referenced this pull request Feb 20, 2015
@marijnh
Copy link
Copy Markdown
Member

marijnh commented Feb 20, 2015

Thanks, that allowed me to reproduce it. The underlying problem is that show-hint gets confused when there are multiple pending async hint calls, and violates its event order contract when an async hinter finishes while another one is also running. Attached patch should fix this.

@marijnh marijnh closed this Feb 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants