Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore addImports functionality #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Restore addImports functionality #67

wants to merge 3 commits into from

Conversation

hinerm
Copy link
Member

@hinerm hinerm commented Jan 13, 2016

Allows package detection of imports in Java in the script editor.

See http://fiji.sc/bugzilla/show_bug.cgi?id=817

The Reflections project (https://github.com/ronmamo/reflections) adds
support for scanning the classpath for various elements, including
annotations, types, methods, etc... with pattern filters.
When given a "raw" class (no package), addImport will now iterate over
all elements in the core ImageJ packages looking for potential matches
to the requested class.

All potential matches will be added as imports (e.g. net.imglib2.ImgPlus
and net.imglib2.meta.ImgPlus will both be added). If there are no
matches, the class will be imported without a package.

Addresses http://fiji.sc/bugzilla/show_bug.cgi?id=817
@hinerm
Copy link
Member Author

hinerm commented Jan 13, 2016

@ctrueden @imagejan - any complaints?

@ctrueden
Copy link
Member

Firstly: cool idea.

Questions and concerns:

  • Performance of classpath scanning en masse? I benchmarked a Spring-centric discovery mechanism that discovered components on the classpath using reflection, similar to this, and it was ~2000 times slower than SezPoz in 2011. But obviously (from your comments in commits and source) this approach is tested and working, so it can't be too bad here, right?
  • The auto-imports feature in general should be in a UI-agnostic layer in SciJava Common probably. Part of the ScriptLanguage, perhaps? And then each scripting-<foo> component can implement it the way it needs to—so scripting-java would have the reflections dependency. Does this make sense?
  • For that to happen, I guess we'd have to implement the TODO you mention: allowing extensions to the package prefixes which are searched. I think the current solution of hardcoding is a (small) problem regardless, since it hardcodes sc.fiji in a net.imagej component, which we try very hard not to do. Or: we could just can everything on the classpath. Isn't that the behavior the user expects anyway? Auto-import any available class?

@hinerm
Copy link
Member Author

hinerm commented Jan 13, 2016

Performance of classpath scanning en masse?

Slow. Takes a second or two. So you can tell it's happening but it doesn't feel ridiculous.

That said, I wrote it so that it only scans once and caches the discovered collection. Iterating the org-filtered collection of items is not really noticeable on subsequent adds.

Regardless, we could possibly add a progress bar.

The auto-imports feature in general should be in a UI-agnostic layer in SciJava Common probably.

Yep I agree with this plan.

since it hardcodes sc.fiji in a net.imagej component, which we try very hard not to do.

Agreed that this should be changed; that was a primary motivator for the TODO 😄 . I guess I can put a plugin for sc.fiji in https://github.com/fiji/fiji now that it has a src directory!

@ctrueden
Copy link
Member

I guess I can put a plugin for sc.fiji in https://github.com/fiji/fiji now that it has a src directory!

Or perhaps repurpose the fiji-scripting project. I dunno.

@hinerm
Copy link
Member Author

hinerm commented Jan 14, 2016

Isn't that the behavior the user expects anyway? Auto-import any available class?

Yes, I would rather do this. But on my computer it takes 9s to scan all resources without filters, and 1.5s with filters. Anyway if we use an extensible model we can easily add more packages as needed.

The auto-imports feature in general should be in a UI-agnostic layer in SciJava Common probably. Part of the ScriptLanguage, perhaps? And then each scripting- component can implement it the way it needs to—so scripting-java would have the reflections dependency. Does this make sense?

@ctrueden - after further thought here is my take on the architecture:

  • scijava-common would have a ResourceScannerService with a ScannedPackage singleton plugin type. The ScannedPackage plugins would have API to return a list of the package strings to include. The service would have a getReflections method which, the first time called, would combine all the packages from all ScannedPackage plugins discovered and use that to build the filter list for reflections - which would put the reflections dependency in SJC.
  • Also in SJC, the ScriptLanguage iface would gain a getImport(String) method which, given a class name, returns a properly formatted import statement.
  • each scripting-xxx component would implement getImport(String)
  • imagej-common, scijava-common, scifio, imagej-legacy and.. fiji-lib maybe? would each provide a ScannedPackage plugin.
  • in imagej-ui-swing I think the "add import" menu entry should be always enabled and just provide a message if the current language does not support adding imports.

any concerns?

@hinerm
Copy link
Member Author

hinerm commented Jan 21, 2016

Yes, I would rather do this. But on my computer it takes 9s to scan all resources without filters, and 1.5s with filters. Anyway if we use an extensible model we can easily add more packages as needed.

per @ctrueden's suggestion we can just run this initialization on a background thread at startup

@imagejan
Copy link
Member

The TextEditor now lives in scijava/script-editor, so we should move this PR over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants