Add autocompletion framework #264

hadim · 2017-04-18T03:06:06Z

Add a framework for an autocompletion framework. Each scripting language is free to implement an AutoCompleter to provide autocompletion.

If they don't the default AdaptedAutoCompleter will just return no results when an autocompletion is asked.

hadim · 2017-04-18T03:08:04Z

@ctrueden let me know what do you think. Even if autocompletion is not implemented yet in the scripting languages, it's fine since all the changes are backward compatible.

ctrueden · 2017-04-18T03:21:43Z

Great, this is something I've been wanting to add to the SciJava script framework. (The other major missing thing is a "script recorder" type feature, which given a SciJava module execution, generates the line of code needed to reproduce it in that script language.)

I will try to find time to review the details later this week. Ping me again if I'm holding you up.

hadim · 2017-04-18T03:26:05Z

I don't think I'll have time to implement autocompletion for scripting languages any time soon (need at least another free weekend :-)) but as I said this PR should hold the main foundation of autocompletion.

So feel free to merge after reviewing it.

I'll ping you here at the end of this weekend :-)

hadim · 2017-04-18T03:28:30Z

Also once it's done that work could be used in the Scijava TextEditor :-D

imagejan · 2017-04-18T12:49:18Z

Just for reference: Icy has a nice auto-completion here:
https://github.com/tprovoost/Script-Editor/tree/master/javascripter/src/plugins/tprovoost/scripteditor/completion

It was discussed shortly at NEUBIAS2017 and suggested by @ctrueden that this could be ported into https://github.com/scijava/script-editor

It didn't look deep into this code, but maybe we could still profit from their implementations for JS and Python...

hadim · 2017-04-19T01:27:35Z

Well, I am surprised to see how easy it was to implement a first "naive" working autocompletion !

That implementation should provide autocompletion for all the scripting languages.

I am pretty happy with that for now. The framework should provide an easy way to extend autocompletion being language specific or not.

About the architecture, I am sure it can be improved (I did my best but IJ coding standard are pretty high ! (which is a good thing)). So I am waiting for @ctrueden comments about that :-)

ctrueden · 2017-04-20T19:26:17Z

src/test/java/org/scijava/script/ScriptInfoTest.java

+
+        @Override
+        public AutoCompleter getAutoCompleter(ScriptEngine scriptEngine) {
+            return new DefaultAutoCompleter(this, scriptEngine);


This override is unnecessary, no? Because AbstractScriptLanguage already defines this same logic?

Netbeans does not agree with you (only a warning). Should I just remove it ?

Weird. What is the warning? The whole point of having methods implemented in the abstract base class is so that you don't have to implement them that same way in the subclass. NetBeans warning about that does not make sense...

It's saying Add @Override Annotation. Note that the other methods above have @Override

IIUC, @ctrueden means you can leave away the whole method, not just the annotation, no?

Yes, just delete the whole overridden method. Lines 291 through 294.

ctrueden · 2017-04-20T19:26:49Z

src/test/java/org/scijava/script/ScriptFinderTest.java


+        @Override
+        public AutoCompleter getAutoCompleter(ScriptEngine scriptEngine) {
+            return new DefaultAutoCompleter(this, scriptEngine);


This override is unnecessary, no? Because AbstractScriptLanguage already defines this same logic?

ctrueden · 2017-04-20T19:27:03Z

src/test/java/org/scijava/script/ScriptEngineTest.java

+
+        @Override
+        public AutoCompleter getAutoCompleter(ScriptEngine scriptEngine) {
+            return new DefaultAutoCompleter(this, scriptEngine);


This override is unnecessary, no? Because AbstractScriptLanguage already defines this same logic?

ctrueden · 2017-04-20T19:27:16Z

src/test/java/org/scijava/menu/MenuServiceTest.java

 	@Test
 	public void testMenuCreation() {
-		menuService.getMenu();
+		//menuService.getMenu();


This needs more explanation.

I don't know why this test was failing. This is not the case anymore. I'll fix that.

ctrueden · 2017-04-20T19:29:57Z

src/main/java/org/scijava/script/autocompletion/AbstractAutoCompleter.java

+
+    @Override
+    public ScriptLanguage getScriptLanguage() {
+        return this.scriptLanguage;


Nothing actually uses this field in the base layer... do you use it in downstream implementations?

Nope. I'll remove it.

ctrueden · 2017-04-20T19:30:41Z

src/main/java/org/scijava/script/autocompletion/AbstractAutoCompleter.java

+
+    @Override
+    public ScriptEngine getScriptEngine() {
+        return this.scriptEngine;


Alternately, the autocompleter could be kept stateless, with the ScriptEngine instance being passed as another argument to the autocomplete methods. Which do you like better? (I don't feel super strongly; just pointing it out.)

Good point here and I like the idea. That will make the class less complicated.

ctrueden · 2017-04-20T19:32:05Z

src/main/java/org/scijava/script/AbstractScriptLanguage.java

+
+    @Override
+    public AutoCompleter getAutoCompleter(ScriptEngine scriptEngine) {
+        return new DefaultAutoCompleter(this, scriptEngine);


Now that we have Java 8, this could be a default method of ScriptLanguage interface itself, rather than in the abstract base class. That would potentially be "more backwards compatible", just in case anyone out there has implemented the ScriptLanguage interface directly.

I'll do that.

ctrueden · 2017-04-20T19:34:22Z

I made some comments. The only real concern I have is that failing test... otherwise, all looks great. We should decide once and for all if Autocompleter will be stateless, or will wrap a ScriptEngine instance the way you have now. Once we decide that, I'll merge it, and do some minor subsequent cleanup.

ctrueden · 2017-04-20T19:39:44Z

Relating to @imagejan's comments about Icy's autocompletion: one thing that Icy does is that it gives special priority to the Icy-specific classes. So you end up getting completions that are relevant to the context of Icy plugin development.

The current framework is very general, which is good, but I'm wondering what the best way forward is to make it possible to give contextual hints like that, but in an extensible way. (Icy just hardcodes everything.) One option would be to have some kind of new plugin type that identifies high-priority completions. It would be nice if it were not language-specific either, but rather a layer which operates sort of orthogonally with the actual completion strings. Maybe something as simple as "with such-and-such prefix, which classes make sense to autocomplete?" That will be the same across all script languages, and it will allow components to highlight "completion-friendly" classes. All of these thoughts are half-baked, of course, but wanted to jot this down before it's out of my head again.

hadim · 2017-04-20T22:48:08Z

All the comments have been addressed. Feel free to merge this one and we can still extend it later. I think it's good enough to start with.

About Icy code, I had a look but I didn't find useful things to steal so I just started from scratch.

The framework is simple for now but I definitely want to make something extensible. For example, we could have AutoCompleterPlugin discovered at runtime and used by AutoCompleter.

My last commit implements autocompletion by inspecting attributes/methods of an object (detected if the string ends by .). That work pretty well but it's not perfect. And of course reflection does not work with Jython (didn't try very hard).

hadim · 2017-04-20T22:48:27Z

Thank you both for your comments.

hadim · 2017-04-21T20:55:46Z

Once this has been merged, I'll cut a new release of the kernel, build conda packages and make an announcement on the forum (I'll also need a new scijava-common release).

ctrueden · 2017-04-21T20:58:02Z

Do you need a new pom-scijava, or is a new scijava-common sufficient?

I am currently reworking how status.imagej.net works (actually, it will become status.scijava.org), since the switch to Travis CI broke the old way of generating that page.

hadim · 2017-04-21T21:00:48Z

Well, I just need the last scijava-common version (to get this PR). But I guess you need to edit the version in the pom ?

ctrueden · 2017-04-21T21:56:08Z

Eventually we need to update the pom-scijava bill of materials to reference the new SciJava Common version. But in the meantime, you can likely override the scijava-common.version property to point to the forthcoming release.

I just asked because I can merge and release a new SJC soon (will try this weekend if I have time), but getting all my ducks in a row to do the full suite of releases and update pom-scijava to match is a larger endeavor which will take a few more days at least.

hadim · 2017-04-21T22:48:53Z

Oh, I see. So if you just cut a new release of scijava-common, it's fine. I'll override the version property.

ctrueden · 2017-05-04T21:39:24Z

Just a note that I haven't forgotten about this. I am planning to clean up and merge this branch soon (within a few days). I have to sort out how to make releases to OSS Sonatype though, now that our Jenkins configuration is hosed up. I want to instead do it from the CLI, without needing Jenkins anymore. Once I have that sorted, I'll try to get this merged, and then cut a new scijava-common release.

hadim · 2017-05-04T22:06:47Z

Thanks @ctrueden .

If you also have time to look at #265 , that would be nice ! Both PRs carry the final features before having a pretty stable scijava kernel.

Releasing from CLI is definitely the way to go. Good luck with that.

ctrueden · 2017-05-06T02:19:49Z

Cleaned up, fleshed out and merged!

Add autcompletion framework

1e3ba36

hadim force-pushed the add-autocompletion-framework branch from 7b900a5 to 1e3ba36 Compare April 18, 2017 03:18

hadim mentioned this pull request Apr 18, 2017

Add autocompletion support scijava/scijava-jupyter-kernel#24

Closed

hadim force-pushed the add-autocompletion-framework branch 2 times, most recently from 7b82a48 to c099152 Compare April 19, 2017 01:18

hadim mentioned this pull request Apr 19, 2017

Add autocompletion support scijava/scijava-jupyter-kernel#31

Merged

ctrueden reviewed Apr 20, 2017

View reviewed changes

hadim force-pushed the add-autocompletion-framework branch from 5417713 to 9a5aead Compare April 20, 2017 21:19

hadim mentioned this pull request Apr 20, 2017

Attributes auto-completion scijava/scijava-jupyter-kernel#33

Closed

hadim added 2 commits April 20, 2017 18:46

Add AutoCompletionResult class

40962f8

Add autocompletion by inspecting attributes/methods of an object.

e6cb31f

hadim force-pushed the add-autocompletion-framework branch from 6d541e5 to e6cb31f Compare April 20, 2017 22:46

Remove duplicates completion results

541f46b

ctrueden closed this in 5768490 May 6, 2017

hadim deleted the add-autocompletion-framework branch May 6, 2017 03:47

Add autocompletion framework #264

Add autocompletion framework #264

Uh oh!

Conversation

hadim commented Apr 18, 2017

Uh oh!

hadim commented Apr 18, 2017

Uh oh!

ctrueden commented Apr 18, 2017

Uh oh!

hadim commented Apr 18, 2017

Uh oh!

hadim commented Apr 18, 2017

Uh oh!

imagejan commented Apr 18, 2017

Uh oh!

hadim commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrueden Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hadim Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrueden Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hadim Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctrueden commented Apr 20, 2017

Uh oh!

ctrueden commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hadim commented Apr 20, 2017

Uh oh!

hadim commented Apr 20, 2017

Uh oh!

hadim commented Apr 21, 2017

Uh oh!

ctrueden commented Apr 21, 2017

Uh oh!

hadim commented Apr 21, 2017

Uh oh!

ctrueden commented Apr 21, 2017

Uh oh!

hadim commented Apr 21, 2017

ctrueden Apr 20, 2017 •

edited

Loading

hadim Apr 20, 2017 •

edited

Loading

ctrueden Apr 20, 2017 •

edited

Loading

hadim Apr 20, 2017 •

edited

Loading

ctrueden commented Apr 20, 2017 •

edited

Loading