Bugzilla – Full Text Bug Listing
| Summary: | Make sure 'Open Declaration' does something sensible when there is more than one implementation | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Steve Northover <steve_northover> |
| Component: | JS Tools | Assignee: | Olivier Thomann <Olivier_Thomann> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | curtis.windatt.public, Michael_Rennie, Olivier_Thomann, Silenio_Quarti |
| Version: | 10.0 | ||
| Target Milestone: | 11.0 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| See Also: |
https://git.eclipse.org/r/64072 https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=c86d441e7c7081d01a72f2be28c97c8c115fb6d9 https://git.eclipse.org/r/64163 https://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=2aec2143c789d55ad5a260c410426053deb3e5c6 |
||
| Whiteboard: | |||
This is fundamental OO stuff which I expected that we would have tested for and at least issued an error message. Is my example code bogus? (In reply to Steve Northover from comment #1) > This is fundamental OO stuff which I expected that we would have tested for > and at least issued an error message. Is my example code bogus? Your code is not bogus, part of the problem here is that we have a form of guessing when we look for decls, and in this case thats what we did. The snippet from your steps: function getText(widget) { return widget.getText(); } has a param with no type infos, so we end up looking for 'an object' with the function getText defined in it. We find one (the last one decl'd which happens to be in Item.js) and return that as a match. Along with the match the 'guess' flag is set and we are not doing anything with it. My proposal would be to add in some code that if we find a decl and we guessed, pop something up (tooltip / hover) and tell the user we found decl(s) and are not sure they are right, and let them click a link if they still want to go there. For what its worth, adding type infos, like: /** * @param {Widget} widget */ function getText(widget) { return widget.getText(); } causes open decl to work just fine (in the above case there is no decl of getText in widget, and you are told so invoking the command). Thanks for the info. I'm not sure about the suggested fix. If the guess is good and the programmer knows it, then I am done. If the guess is bad, then I need to do a grep search or 'references' to find the one I want. I lose my context and the next time I want to navigate, I have the same problem (ie. the guess is still wrong). Some ideas: We could offer the guy a list of methods he can choose from, but if the guy doesn't know the code, he's still going to do a grep or do a 'references' search. If there are multiple implementations, we could automatically open a 'references' search. We could fail the search but offer him a button that opens a 'references' search. We could offer code assist to add typing information in the JSDoc. SSQ? This is an interesting problem. http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=48bcd16a52c8f4d563644067425fd385962608c8 Single file test case to demonstrate the guessing behaviour. Marked as skip because the current behaviour isn't desired. Mike and I were throwing around ideas of how to indicate guessing and multiple destinations. When we are dealing with a function param or another case that can be solved using JSDoc, we want to lead the user to fix the issue there. So what if when we are unsure of our navigation, we opened up a tooltip/dialog (preferably inline) that showed the possible destination options and gave an option to set the type in JSDoc. Any subsequent navigation would go to the 'right' place, but it is easy for the user to correct it if they made a mistake (delete the type info from JSDoc or put the correct type in). If they push the JSDoc change to source control other users would get the fix too. I understand and agree with the thinking that say we need to have a Quick Fix or lead the user to fix the problem but I believe the first step before doing any of this would be a minimal UI that let's the guy choose. There will always be situations where you do not know how to resolve a call site because it depends on the particular object that is used at runtime. So, can we start off with a minimal "menu-like" list of targets and let the guy pick from them? Either that or we need to fail and provide the list of possibilities (as links?) in the drop down message. Any other ideas that let the guy choose a declaration when multiple are possible so that he can continue navigating his code? (In reply to Steve Northover from comment #7) > Any other ideas that let the guy choose a declaration when multiple are > possible so that he can continue navigating his code? Our suggestion included a dialog to let the user choose. It wouldn't force the user to update their JSDoc. First cut wouldn't even need that ability. But I can tell you that making your choice 'permanent' will be the first feature people request once they see a dialog of choices. Then they would need to undo it if they told you the wrong thing. Let's do the simple chooser or the links in the drop down status message? SSQ? Thoughts? (In reply to Steve Northover from comment #9) > Let's do the simple chooser or the links in the drop down status message? > > SSQ? Thoughts? Agreed, just like I mentioned in comment 2. Sounds good. The only other alternative would be to put the links in the drop down message but the simple chooser seems better. (In reply to Steve Northover from comment #9) > Then they would need to undo it if they told you the wrong thing. Let's do > the simple chooser or the links in the drop down status message? > > SSQ? Thoughts? I agree and that should already be possible by returning a status object with markdown content. https://wiki.eclipse.org/Orion/Documentation/Developer_Guide/Plugging_into_the_editor#orion.edit.command Olivier, this is a particularly bad one in that the tooling takes you to the wrong place., I forget what the current status is but first off, if there are multiple possible implementations, we should probably fail and tell the guy rather than charging off to the wrong place. That's not a good answer, but at least we didn't mislead the guy (not sure if this is already implemented). Thoughts? Some UI work will be required to put the the choice box. Not sure if you've gotten into that code yet. The test code is on beta3 in projects widget-test1 and widget-test2. Right now, tern is only returning one possible solution for find declaration. I am looking at what could be done when guess is true to return more than one solution. So as Curtis advised it, I'll create a new plugin for tern to try to get multiple results. Once this is done, we can see how to present this into the UI. Modifying the existing tern implementation will prevent us from adopting new tern versions easily. (In reply to Olivier Thomann from comment #15) > Right now, tern is only returning one possible solution for find > declaration. I am looking at what could be done when guess is true to return > more than one solution. So as Curtis advised it, I'll create a new plugin > for tern to try to get multiple results. Once this is done, we can see how > to present this into the UI. > Modifying the existing tern implementation will prevent us from adopting new > tern versions easily. I would avoid making another plugin. Just modify Tern to see what is needed to return multiple results and we can open a bug / pull request against Tern for the official support. I started to modify the tern implementation to give me a list of potential matches when it finds some. Everything works pretty well except that I cannot display the error message in html as expected. The warning is shown in plain text. New Gerrit change created: https://git.eclipse.org/r/64014 New Gerrit change created: https://git.eclipse.org/r/64070 New Gerrit change created: https://git.eclipse.org/r/64072 Gerrit change https://git.eclipse.org/r/64072 was merged to [master]. Commit: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=c86d441e7c7081d01a72f2be28c97c8c115fb6d9 (In reply to Eclipse Genie from comment #21) > Gerrit change https://git.eclipse.org/r/64072 was merged to [master]. > Commit: > http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/ > ?id=c86d441e7c7081d01a72f2be28c97c8c115fb6d9 I NLS'd + did a bit of cleanup: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=638b2fc9c4876e057014eb94221439ebe42929a7 (In reply to Michael Rennie from comment #22) > I NLS'd + did a bit of cleanup: > > http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/ > ?id=638b2fc9c4876e057014eb94221439ebe42929a7 Removed the unnecessary registry arg: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=0155ae8f009f994d66d9585f096f8cc4f853c631 New Gerrit change created: https://git.eclipse.org/r/64163 Gerrit change https://git.eclipse.org/r/64163 was merged to [master]. Commit: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=2aec2143c789d55ad5a260c410426053deb3e5c6 Closing as FIXED. Further changes will be done as part of a new bug report if needed. |
1) get widget-test1 2) open main.js 3) click on getText() in line 8 4) Tools->Open Declaration 5) BUG: The tooling goes to item.js (Item.getText()) This example creates a hierarchy as follows: Widget Control Button Label Item Button, Label and Item all have getText() and they are all unrelated. The tooling needs to recognize this and either fail or give me an option to pick from one of the three.