scijava / script-editor

Script Editor and Interpreter for SciJava script languages
http://imagej.net/Script_Editor
BSD 2-Clause "Simplified" License
12 stars 12 forks source link

File system tree for the Script Editor #28

Closed acardona closed 5 years ago

acardona commented 5 years ago

Adds a JTree that shows nested folders from the file system; in a collapsible panel. Top-level folders can be added and removed with the (+) and (-) buttons. The "Edit - Save preferences" will store which top folders the user had listed. Double-click on a leaf (a file) will open it in a Tab of the Script Editor.

Builds on top of the other PR #27 that added the incremental evaluation and REPL.

acardona commented 5 years ago

All the "errors" are very old issues with the documentation of methods in Script Editor classes.

ctrueden commented 5 years ago

Thanks for working on this! I will try to go through this PR as well as #27 on Sunday or Monday.

acardona commented 5 years ago

Thanks @ctrueden .

Just pushed a few more edits to make things look nicer and easier.

Also: it would be nice to open images in ImageJ/Fiji from the FileSystemTree by double-click. How would you do that without introducing a dependency on ImageJ? I'm happy to enable it via reflection by discovering classes at run time, but perhaps there is a better way.

imagejan commented 5 years ago

@acardona wrote:

All the "errors" are very old issues

Maven is a bit misleading here, as all those [ERROR] lines except one are merely Javadoc warning:s, and wouldn't break the build unless there was this single Javadoc error:

/home/travis/build/scijava/script-editor/src/main/java/org/scijava/ui/swing/script/FileSystemTree.java:245: error: no tag name after @

https://github.com/scijava/script-editor/blob/bcd4ebe2b12ea59aee41e5a4ff920163497f3679/src/main/java/org/scijava/ui/swing/script/FileSystemTree.java#L245


How would you do that without introducing a dependency on ImageJ?

I guess you could use SciJava's IOService to do that.

imagejan commented 5 years ago

I quickly tested this, and it looks really nice! However, the fact that the output pane on the right cannot shrink smaller than the width of the button bar is a bit inhibiting on small(ish) screens:

screen shot 2018-12-08 at 22 10 50

Also a suggestion, how about allowing drag-and-drop to add the path of a file in the tree into the currently open script, at the position where the file is being dropped?

acardona commented 5 years ago

Thanks for testing!

More commits, includes the drag and drop. We were thinking similarly while offline.

On small screens: the right split is now collapsible, but yes. we'll need a button to place the output/error and prompt elsewhere.

acardona commented 5 years ago

On the scijava IOService: the injected magic is hard to follow and hard to find documentation for. Pointers to actual examples, and ideally the source code of the IOService, would be appreciated.

On Dec 8, 2018, at 3:48 PM, Jan Eglinger notifications@github.com wrote:

@acardona wrote:

All the "errors" are very old issues

Maven is a bit misleading here, as all those [ERROR] lines except one are merely Javadoc warning:s, and wouldn't break the build unless there was this single Javadoc error:

/home/travis/build/scijava/script-editor/src/main/java/org/scijava/ui/swing/script/FileSystemTree.java:245: error: no tag name after @ https://github.com/scijava/script-editor/blob/bcd4ebe2b12ea59aee41e5a4ff920163497f3679/src/main/java/org/scijava/ui/swing/script/FileSystemTree.java#L245

How would you do that without introducing a dependency on ImageJ?

I guess you could use SciJava's IOService to do that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

acardona commented 5 years ago

So I found this DefaultDatasetIOService source file: https://github.com/scifio/scifio/blob/master/src/main/java/io/scif/services/DefaultDatasetIOService.java

Question: is this what you mean? Question 2: how to actually open in the Fiji UI, rather than merely loading the DataSet?

I can't seem to find the right google terms to make any documentation popup.

acardona commented 5 years ago

There's also this: https://javadoc.scijava.org/SCIFIO/io/scif/io/DatasetIOPlugin.html

Question: what's the difference with the DefaultDatasetIOService.java?

ctrueden commented 5 years ago

To open an image file and show in the Fiji UI:

#@ File file
#@ DatasetIOService datasetIO
#@ UIService ui
dataset = datasetIO.open(file.getPath())
ui.show(dataset)

See also the load-and-display-a-dataset tutorial.

what's the difference with the DefaultDatasetIOService.java?

There is a more general service called IOService that opens any kind of data, not only images. It works by managing IOPlugins that handle different kinds of data. For example, there is a TableIOPlugin that opens CSV files, as well as a DatasetIOPlugin (the one you found) that handles opening images via SCIFIO.

On the scijava IOService: the injected magic is hard to follow and hard to find documentation for.

While interface-driven design always adds one layer of complexity (and I think ImgLib2 proves that this sort of design is well worth it), I had an idea that might make it easier to find code. What if we add a "Browse source" action to the Script Editor? If a script parameter like #@ IOService io is selected, then the script editor asks the SciJava context for the concrete instance of IOService—which in the standard case will be an instance of DefaultIOService—and then opens the DefaultIOService source code on GitHub in the browser. We could have a similar command "Browse javadoc" that opens the concrete implementation currently in use on javadoc.scijava.org.

Do you think something like that would help you? Or not worth the effort? I think it would be relatively straightforward to implement, since we have similar things already with the "Source" button of the search bar.

Pointers to actual examples, and ideally the source code of the IOService, would be appreciated.

You could start with the https://github.com/imagej/tutorials repository. Both notebooks and maven-projects have examples of API usage, with maven-projects having Java code.

ctrueden commented 5 years ago

@acardona Do you want me to review and merge this ASAP? Or are you planning more commits before it is ready?

acardona commented 5 years ago

Would like to complete the IOservice-based opening of image files, but I haven't figured it out yet. It's opaque. I can read an image, getting an Object for it (?) and then I'm still missing how to open a window for it.

On Dec 10, 2018, at 4:12 PM, Curtis Rueden notifications@github.com wrote:

@acardona Do you want me to review and merge this ASAP? Or are you planning more commits before it is ready?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ctrueden commented 5 years ago

I'm still missing how to open a window for it.

Use the UIService:

uiService.show(myObject)

Would like to complete the IOservice-based opening of image files

Sounds good; just ping me here when you are ready! Or if any questions!

imagejan commented 5 years ago

@acardona how about putting the button bar below the script editor pane (i.e. keeping it attached to the script editing, as it was previously), instead of above the output pane? (didn't test latest commits, so please ignore if this has changed already...)

acardona commented 5 years ago

Hi @imagejan, I thought of that, but the "Show Errors" and "Clear" buttons, and the "persistence" checkbox, have direct effects on the output/error and prompt JTextArea, therefore, it makes more sense to me to keep them to the right.

What I could do is make the JButton insets smaller, but that's not very user friendly. Will think about it.

acardona commented 5 years ago

Hi @imagejan : I've added a buttom with a down-arrow / right-arrow that toggles the location of the screen/error + prompt. By default to the right (most modern screens are wider than they are taller), the toggle will enable those with small screens to manage. Notice as well the tiny arrows over the split bar that enable collapsing the subwindows altogether. They exist for both the file system tree and the screen/error + prompt.

Hi @ctrueden: barring errors, this is ready for merging. What is missing, though, is testing the prompt in multiple languages. I have tested with macros, javascript and jython only. In principle, it should work for all: the evaluation at the prompt is the same as the evaluation of selected text in the main editor window.

imagejan commented 5 years ago

@acardona that's a good point regarding the "Show Errors" and "Clear" being associated with the output/error pane. In the same line I would argue that "Run", "Batch" and "Kill" (unsure about "persistent") are associated more with the editor pane. So how about putting the button bar below (or above) both those panes, that it spans across two thirds of the screen space?

acardona commented 5 years ago

Hi @imagejan,

Placing the button bar on top incurs in other compromises. The current situation, with the button to toggle the location of the buttorn bar + output/error + prompt to the bottom of the main textarea, allows for returning to the former layout and that should do for now.

Also, I've returned from my travels and it will be tough to find minutes to further edit the layout. I'd leave it as is.

The whole Script Editor code is in need of substantial refactoring that would allow easier editing of its code. That's for the next window of time, in the far future.

This branch works, adds new much needed functionality and can be merged.

The new functions:

A. An "incremental" mode (activated by the checkbox) where the script engine is persisted beyond a single "Run" and therefore the results of the evaluation of the script can be edited by either re-runing the whole script or parts thereof (via execution of selected text), or from a prompt (a full-fledged scripting interpreter in the same language).

The commands entered in the scripting command prompt are persisted to a log file, with the last 1000 commands loaded when instantiated.

B. A file system tree that persists, via "Edit - Save preferences", the listed top-level folders.

As a side effect:

  1. Drag and drop from the file system tree onto the main ImageJ toolbar opens the image.

  2. When a language is selected and the "incremental" checkbox is selected, the command prompt acts as a full-fledged script interpreter, removing the need for any of the old scripting interpreters in the fiji-legacy update site.

All the above are significant advances that improve on existing workflows, removing the need for copy-pasting from the Script Editor into scripting interpreters, and allowing for more graceful script re-runs, particularly when the script failed after eg expensive operations like loading large images or running a multiview deconvolution.

On Dec 12, 2018, at 3:15 PM, Jan Eglinger notifications@github.com wrote:

@acardona that's a good point regarding the "Show Errors" and "Clear" being associated with the output/error pane. In the same line I would argue that "Run", "Batch" and "Kill" (unsure about "persistent") are associated more with the editor pane. So how about putting the button bar below (or above) both those panes, that it spans across two thirds of the screen space?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

imagejan commented 5 years ago

Unfortunately, the "incremental" mode (after checking "persistent") seems to be broken when using any script parameter.

I tried to run the following Groovy script:

#@ IOService io

img = io.open("https://imagej.net/images/baboon.jpg")

#@ UIService ui
ui.show(img)

and got this stack trace:

Started New_.groovy at Thu Dec 13 11:08:02 CET 2018
org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
Script3.groovy: 1: expecting '!', found '@' @ line 1, column 2.
   #@ IOService io
    ^

1 error

    at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:310)
    at org.codehaus.groovy.control.ErrorCollector.addFatalError(ErrorCollector.java:150)
    at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:120)
    at org.codehaus.groovy.control.ErrorCollector.addError(ErrorCollector.java:132)
    at org.codehaus.groovy.control.SourceUnit.addError(SourceUnit.java:350)
    at org.codehaus.groovy.antlr.AntlrParserPlugin.transformCSTIntoAST(AntlrParserPlugin.java:139)
    at org.codehaus.groovy.antlr.AntlrParserPlugin.parseCST(AntlrParserPlugin.java:110)
    at org.codehaus.groovy.control.SourceUnit.parse(SourceUnit.java:234)
    at org.codehaus.groovy.control.CompilationUnit$1.call(CompilationUnit.java:168)
    at org.codehaus.groovy.control.CompilationUnit.applyToSourceUnits(CompilationUnit.java:943)
    at org.codehaus.groovy.control.CompilationUnit.doPhaseOperation(CompilationUnit.java:605)
    at org.codehaus.groovy.control.CompilationUnit.processPhaseOperations(CompilationUnit.java:581)
    at org.codehaus.groovy.control.CompilationUnit.compile(CompilationUnit.java:558)
    at groovy.lang.GroovyClassLoader.doParseClass(GroovyClassLoader.java:298)
    at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:268)
    at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:254)
    at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:250)
    at org.scijava.plugins.scripting.groovy.GroovyScriptEngine.getScriptClass(GroovyScriptEngine.java:319)
    at org.scijava.plugins.scripting.groovy.GroovyScriptEngine.eval(GroovyScriptEngine.java:122)
    at javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:264)
    at org.scijava.script.ScriptModule.run(ScriptModule.java:160)
    at org.scijava.module.ModuleRunner.run(ModuleRunner.java:168)
    at org.scijava.module.ModuleRunner.call(ModuleRunner.java:127)
    at org.scijava.module.ModuleRunner.call(ModuleRunner.java:66)
    at org.scijava.thread.DefaultThreadService$3.call(DefaultThreadService.java:238)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)
acardona commented 5 years ago

Hi @imagejan, I am not surprised that the injected dependencies, as opposed to explicit ones, are broken in incremental mode. In the same way that you wouldn't use @Parameter in a Script Interpreter, they make no sense in incremental mode: incremental mode is merely a fancy name for continuous evaluation in a script interpreter.

ctrueden commented 5 years ago

I played around with this build of the Script Editor.

There are some UI kinks to work out before I am comfortable merging this. I think we should add menu items to toggle visibility of the tree, output and REPL panes. Some people might not want them, especially on smaller screens. On my laptop, the actual script editor pane is now only a couple of characters wide by default, which is too narrow. It also seems like the setting for the console/REPL pane being on right vs. bottom is not retained when new tabs are opened, so one has to keep clicking the console/REPL back to the bottom from its default right.

I also agree with @imagejan that we need to fix the support for script parameters in persistent mode. There is no reason these parameters cannot still be injected and retained—they are injected once at the beginning of the script, after all, so should not conflict with any form of incremental execution.

I will try to hack on this before end of year.

acardona commented 5 years ago

Hi @ctrueden, thanks for trying it out.

On the UI: Swing layout is tricky. Perhaps what could help ensure that the main, center editor part is wide enough is to .setMinimumSize(new Dimension(WINDOW_WIDTH / 2, WINDOW_HEIGHT / 2)).

On the output/error + REPL in new tabs: in the same way that the font size of the currently opened tab is used for the new tab, the choice of position of these subpanels could also be used for a new tab.

On menu items to hide the tree: the JSplitPane is collapsible, has buttons on the split bar to collapse it. I am not sure where I'd put menu items to do that, or whether it is necessary.

On injecting parameters: given that the ScriptEngine is retained, it should have its injected parameters. It is surprising that injected parameters aren't available at the REPL: they should be (if the script has been run prior to any execution in the REPL). The command interpretation in the REPL is nothing other than the same execution routine used for a click to "Run" when there is text selected in the main script text area.

acardona commented 5 years ago

On the minimum size of the main script text area: the actual command would look like this:

editorPane.setPreferredSize(new Dimension(textEditor.getPreferredSize().width / 2, textEditor.getPreferredSize().height / 2));

... and would be placed inside the TextEditorTab constructor, after creating the editorPane.

But: the TextEditorPane class itself calls its method wrappedInScrollbars that already sets a preferred size of 800, 350. This should ensure that the main text area is large enough, rather than 2 characters.

What may be happening here is that, the first time you open the script, it remembered the window size that you had last (before this pull request), and the button bar of the output/error+REPL size (with its minimum horizontal size) makes the main window shrink to almost nothing.

The way forward, in my view, given that many Fiji users will have automatically saved preferences for the JFrame dimensions, is to make the right side panels be by default at the bottom, like @imagejan wanted. They are one click away from putting them where, in my view, they belong: the right side.

ctrueden commented 5 years ago

Sorry for the delay on following up with this. Currently sucked into some internal lab tasks. It's still on my agenda to get this merged before too long.

acardona commented 5 years ago

Thanks for keeping the Script Editor in your list. No hurry, at least I can use it myself and it is working great for me. Would be good to have it easily available for when I work in other people's computers which is often, but other than that it can wait.

With the latests commits that init it with the screen/output at the bottom the UI shouldn't be terrible for those that have width & height stored in prefs.

On Jan 3, 2019, at 4:46 PM, Curtis Rueden notifications@github.com wrote:

Sorry for the delay on following up with this. Currently sucked into some internal lab tasks. It's still on my agenda to get this merged before too long.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

acardona commented 5 years ago

Hi @ctrueden would appreciate an update. Thanks.

ctrueden commented 5 years ago

Occupied with urgent writing deadlines until January 25. I plan to work on this during next week, Jan 28 - Feb 1.

acardona commented 5 years ago

Hi @ctrueden, any update? I did see you merged master into this branch.

ctrueden commented 5 years ago

@acardona Working on it today! All I want to do before merging is to add toggles for the new panes to the menus, which are remembered with the preferences.

Do you think it makes sense to split out the portion of the Edit menu controlling fonts and such—i.e. the UI preferences—to a new View or Window menu? Or should I just add the pane toggles to Edit also? I am leaning toward a new menu.

acardona commented 5 years ago

Thanks!

If I understand you correctly, you are referring to the 4th subsection of the Edit menu. While this is not really part of this PR, indeed, moving it to a "Format" menu would make sense.

acardona commented 5 years ago

Or a "Style" menu.

ctrueden commented 5 years ago

I worked on this for a couple of hours. I split out the options-related Edit menu items into a new Options menu, intended to add some toggles for the various UI components. However, after further consideration, I think it is unnecessary to add any new menu items, since all the UI components are nicely one-click collapsable. So now all I have to add is persistence of the split pane states. I have to go AFK now but will try to finish that later tonight or tomorrow.

acardona commented 5 years ago

Thanks. A comment on lambdas: why use them? The old code is explicit about which events are used. The lambdas code isn't. Doesn't seem to add anything in these particular instances other than obfuscation.

ctrueden commented 5 years ago

Regarding lambdas: it's more about what they remove than what they add: 100 fewer lines of code in classes that are long and bulky. I personally find them easier to read since there is less nesting, and I doubt I am the only one. Sorry you find them more obfuscated.

ctrueden commented 5 years ago

I fixed some bugs, and added persistence of the split pane dividers. Will merge now.

acardona commented 5 years ago

Wonderful, thanks very much!

On Feb 5, 2019, at 5:29 PM, Curtis Rueden notifications@github.com wrote:

Merged #28 into master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.