-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add a REPL widget that supports the SciJava ScriptREPL #265
Conversation
BTW, one thing I noticed with this is that the |
I'm not sure that I'll have the chance to look at the code until Tuesday or so @kephale, but this looks really awesome - thanks for implementing it! One quick comment - have you considered adding the REPL to the bottom by default? I really like how the napari console sits at the bottom - naively this seems like it should go in the same place. |
I'm very open to where it goes. I have some concerns about all the consoles/repls getting confused. I was actually a little worried about making this icon so similar to the napari-console icon. |
Yeah, I thought about that too. As far as the button goes, maybe we could color it differently, or add the ImageJ Logo somewhere? Another option would be to just make the SciJava Script REPL a separate napari plugin - making it show up in the napari menus - then we don't have to worry about a button. Then there's the question of differentiating the two consoles - does the SciJava Script REPL have a Java UI that we could launch (i.e. launch a separate window), and if so, would that detract from your probing properties if it was in a different window? That could be one way to do it, and would require less Qt code to maintain. |
Yes, but I was particularly keen on this for the headless situation, so I do hope that the UI is on the Python side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code itself looks great @kephale, just had one minor suggestion.
I think some documentation, both in the GUI (tooltips) and in the docs would be warranted (maybe this could help in troubleshooting?)
@gselzer I emailed you 1 more figure for this addition to the docs: https://github.com/kephale/napari-imagej/blob/4ccab72c0d8e2890e9ed27d8e4b5e1bff3da547e/doc/Configuration.rst?plain=1#L106 I've added tooltips + a section in the docs about the REPL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at this again, I'm actually pretty happy with the visual appearance of the REPL - I don't think it's too easy to confuse with the napari console.
One thing that I did think would be cool would be to put a shadow of the napari-imagej logo behind the text edit for a visual reminder of which textedit is which, but I have no idea how you'd do that, so I'm just going to leave it here as an idea.
@gselzer It seems you started reviewing this as I was actively trying to fix the same problems you discovered. This PR needs some additional work to squash bugs before it can be merged. If you want to get looped into fixing it (which would be really helpful!), I can give you a primer on how the SciJava |
I'm still working on this. Need to change |
Progress: scijava/scijava-common@0f3bf44. Was going to release it, but updating scijava-common to pom-scijava 36.0.0 breaks some tests, so gotta sort that out first. Then we can start using the improved |
Progress today: scijava-common 2.95.0 was released. And I worked further on this branch, rebasing it on top of the latest mainline. Some tests pass now, but others fail. I need to scrutinize the design changes a bit more, and probably reshuffle the commit order to minimize the footprint of the commit implementing the changeover to the nij object. |
And use jc mechanism instead of jimport for REPL-related classes.
This was a long time coming @ctrueden @kephale, however I've just rebased this PR on top of the latest I've still got an error where the bindings aren't found, I likely just have to find the code adding them on the Java side and copy/call it, but beyond that this REPL performs identically to the one in Fiji 🪄 TODO for merge:
|
This provides a place to keep track of not only the ImageJ2 gateway, but also any affiliated data structures, such as our singleton ScriptREPL.
Dynamically recolors button on appearance changes
224dda6
to
df01d34
Compare
This PR adds a widget for the SciJava ScriptREPL, allowing users to interactively write SciJava friendly code directly within napari.
When I was exploring the compatibility of various ImageJ/Fiji plugins with napari-imagej, I frequently found myself wanting to poke at variables/state. That led me to writing this REPL widget so we can quickly poke around on the JVM instead of always needing to deal with jpipe/jimport stuff.
Caption: This is a screenshot of napari-imagej with the REPL widget active and some example output from Jython.
Now updated to have the corrected icon: