Bugzilla – Full Text Bug Listing
| Summary: | Content assist automatically fills in the "wrong thing" (assumes a single method and pastes it in) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Steve Northover <steve_northover> | ||||||
| Component: | Client | Assignee: | Michael Rennie <Michael_Rennie> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P2 | CC: | curtis.windatt.public, Silenio_Quarti | ||||||
| Version: | 12.0 | ||||||||
| Target Milestone: | 13.0 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows 7 | ||||||||
| See Also: | https://git.eclipse.org/r/69477 | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Steve Northover
Fix + tests: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=ff6736a6ddba920ca12d89c17558a5e2a77aeea4 The same problem is happening for me with app.js when you place the cursor just before ".use". It automatically puts in "listen". Can you confirm this is happening for you? Created attachment 260484 [details]
Working fine for me
No, I cannot reproduce. I always get the list.
> Get minesweeper-objects from GitHub
It is the .tern-project file thing. Without out it works, with one, it inserts "listen" because it saw "listen" once in the file. I thought that was changed by fixing this bug? Anyhow, I think because JavaScript is so fuzzy and gets things wrong, when there is one thing, we probably should not just insert it.
1) Root/org.eclipse.orion.client/modules/orionode/server.js 2) Line 121, put the cursor after "nextWindow." 3) Invoke Code Assist 4) BUG: nextWindow.webContents is automatically filled in New Gerrit change created: https://git.eclipse.org/r/69477 (In reply to Steve Northover from comment #5) > 1) Root/org.eclipse.orion.client/modules/orionode/server.js > 2) Line 121, put the cursor after "nextWindow." > 3) Invoke Code Assist > 4) BUG: nextWindow.webContents is automatically filled in This is happening because nextWindow has no type information - it is inferred as 'any' so the best we can do is offer something we have seen before. It doesn't have anything to do with .tern-project or any other setting, its that we don't know what 'new electron.BrowserWindow' makes. If you place the cursor right before 'return nextWindow;' on line 139, you'll see what I mean - you will get two proposals 'loadURL' and 'webContents'. The missing type information will be fixed with bug 481271 (properly looking up libs in node_modules). But showing proposals for things we have seen before is kind of handy (and better than nothing). > New Gerrit change created: https://git.eclipse.org/r/69477 The proposed fix simply torches the auto apply flag. I don't think we should be doing that. Silenio would know more. Ok, so we are getting rid of "if there is only one possibility, use it" because in a JavaScript world, there are almost always many possibilities. That sounds right. Created attachment 262349 [details]
proposed fix
This patch adds a provider-level option to allow / disallow auto-applying proposals
Curtis, Silenio, please review the patch. The patch works as stated (turns off auto apply). However, I'm not a big fan of this because a lot of the time the single completion available is the right one. I would prefer the behaviour remains as is until we have better UI around guessed proposals. I know it would be much harder to implement, but I would rather auto apply stays on, but doesn't happen if the single completion was a guess by Tern. This combined with tagging guess proposals with some indicator for the user would be awesome. Please determine what other editors do in this case for comparison. Please determine what other editors do in this case for comparison. (In reply to Steve Northover from comment #13) > Please determine what other editors do in this case for comparison. None of the other editors automatically apply a proposal - they all will show the single one. I chose to turn it off for the mentioned reason, the fact that when we show guesses we will have to show it, and the fact that auto-apply annoys me - I'm of the camp that I always want to see what is being proposed before it is applied. Ok, I think we are in agreement. I hate when the wrong thing gets filled in automatically and I have to delete it. Does the released code every auto-fill? It's hard to detangle the discussion (In reply to Steve Northover from comment #15) > Ok, I think we are in agreement. I hate when the wrong thing gets filled in > automatically and I have to delete it. > > Does the released code every auto-fill? It's hard to detangle the discussion The released code does not prevent the auto-fill. The patch adds a new provider-level option to enable or disable auto-fill (and also contains the change to the JS provider to not auto-fill). Please stop the auto-fill (unless we are absolutely sure) the content assist is correct. This is consistent with the other editors. Pushed the patch to master: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=3af99b860793c46aaa563df74f4c33d5602f5ec4 All tests pass (JS, core and UI). |