search

imagej / imagej-legacy

ImageJ+ImageJ2 compatibility layer
https://imagej.net/libs/imagej-legacy
BSD 2-Clause "Simplified" License
17 stars 25 forks source link

MacroRecorder: Improve handling of org.scijava.widget.Button #239

Open frauzufall opened 4 years ago

frauzufall commented 4 years ago

Should fix https://forum.image.sc/t/scijava-command-macro-recording/35056

Would be nice to add a test, not sure how this would be done and I don't have more time, happy for pointers! :)

tischi commented 4 years ago
  1. I tested it from another repo and it works! It records the button as button=0 and then it does not show the UI when running the recorder command! I think even nicer might be that it does not record the button at all. Would that be possible?
  2. I tried to add a minimal example to src\test, but got stuck because I could not get the CommandService to run: https://github.com/imagej/imagej-legacy/pull/239/commits/bb5730148e293513f1678e06e29bf9447c84e29f
NicoKiaru commented 4 years ago

What are the use case of these buttons ?

frauzufall commented 4 years ago

@NicoKiaru In CSBDeep you can change the mapping between the input image and the network input tensor.

frauzufall commented 4 years ago

@ctrueden I see. I thought it was a legacy thing since I remember it working in a jython script already, but looking at my scripts I just avoid displaying the button by not calling the preprocessors:

commandService.run("command",false, ...);

Hope to find the time to work on this in scijava soon.

NicoKiaru commented 4 years ago

Ok, maybe I'm missing some points here, but if we set the flag isRequired of the Button to false, wouldn't it be acceptable that no UI is displayed if all inputs are not required ?

ctrueden commented 4 years ago

@NicoKiaru Studying the current logic, I don't think setting required to false will have the effect you desire here. Did someone test it? My guess is that it will have no impact on whether the dialog is shown.

NicoKiaru commented 4 years ago

No indeed, it's not working (sorry I should have mentioned it), I was just saying maybe it should / could work like this. Thanks for linking the logic.

uschmidt83 commented 4 years ago

FYI: We’ve also had the problem that the StarDist Command is not executable from IJ1 macro without pressing OK, even if all required parameters are provided.

I’ve decided to make a generic SciJava Command CommandFromMacro that (potentially/hopefully) allows to call any Command from IJ1 macro via the CommandService. The syntax is not ideal, but this is a workaround that works today, until the situation has been properly fixed.

You should be able to use this to macro-record/execute your own Command. You can get it by enabling the StarDist update site (it’s not the ideal place to put it, but that’s where it is for now). See this PR for more details: https://github.com/mpicbg-csbd/stardist-imagej/pull/7.

tferr commented 3 years ago

Hey guys (specially @ctrueden): 1 year later what is the best way to solve this? Is @uschmidt83 solution still the way to go?. This makes several commands from the Neuroanatomy update site non-recordable. And it feels silly to tell folks to use instead outdated plugins.

This does not affect only buttons. It affects also other more 'unusual' widgets such as ColorTable. Here is an example of a prompt in which all parameters have the attribute required = false. Left is the original dialog, Right is what gets displayed when the recorded macro is run:

image

@ctrueden, I read your notes above. I just wanted to say that (arguably minimal), the IJ1 GenericDialog also supports 1) extraneous buttons (the "Help" button can be renamed and formally hijacked for other functionality); 2) images (which is what a ColorTable is my example above), and 3) HTML messages with functional URLs that can open the browser, etc..

In all these cases, these type of IJ1 widgets were never recorded by the macro recorder. So, if what we are trying to strive here is backwards compatibility, then it would be perfectly fine to not record them. Alternatively, NON_RECORDABLE flavors of the existing ItemVisibility settings could be implemented?

imagejan commented 3 years ago

I hadn't noticed this whole issue until @tferr's post ten days ago. There's some overlap with other open issues, in particular https://github.com/scijava/scijava-common/issues/402 and https://github.com/scijava/scijava-common/issues/317.

I tried to come up with a less intrusive solution (specific to IJ1 macro calls, but not affecting macro recording behavior) in #262.

biovoxxel commented 2 years ago

Hi @ctrueden, @frauzufall, @imagejan, @tferr,

So, let me add one year to the counter and get back to the initial problem 😉
I have a use case in which I would like to add a button in a dialog which is only used while the dialog is open to refresh the display of an output image. This is not needed, once the run method is called by pressing "Ok". However, since the button widget looks for it and it is not recorded it will, as described before, make a dialog with this remaining, non-specified button. I think, if the button would have a value (like a String, Boolean, Integer,...) this might not even come up as an issue, but since I cannot assign any value it will be null and therefore call the dialog. Even adding it manually to the macro code with something like "buttonname=0" or similar does not solve it, as doesn't the "required=false" setting. I know that I could use DynamicCommand to update something, but that does it every time one parameter is changed, which can become somewhat annoying over time. Using a checkbox instead of a button to trigger an update seems to me non-elegant and counter intuitive for a user. Are there any plans to still modify this Button widget behavior? Or any workarounds except this one?

ctrueden commented 2 years ago

@biovoxxel I don't have time to think about this deeply at the moment, but I wanted to respond immediately by reopening this issue so this issue doesn't get lost. I also added it to my community backlog list, which I try to work through all day every Friday, but (I'm sorry to say) that this list is quite long, so I don't know when I'll get back to it. But it's in my queue now! In the meantime, don't let me stop the rest of you from discussing and converging upon a specific technical solution which you can champion, which would accelerate progress here.

imagejan commented 2 years ago

As said above, I would favor my less intrusive suggestion https://github.com/imagej/imagej-legacy/pull/262, a simple change to MacroPreprocessor.

@biovoxxel would this work for you? Would you be willing to check out https://github.com/imagej/imagej-legacy/tree/fix-macro-call-button-commands and test?

biovoxxel commented 2 years ago

Sure, I'm on it to test, will get back to you with results

biovoxxel commented 2 years ago

Good Morning @imagejan, So, here is what I did:

I don't see that I made an obvious mistake in that process.

I will send you the current version of the BV 3D box jar, so you can test the Voronoi Threshold Labeler with the button in question, since that is still not publicly released.

biovoxxel commented 2 years ago

My bad. Ignore my last post. The cloned repo did not contain your changes regarding skipping the button an input. I changed it in the MacroProcessor and it works. I tested it with two different plugins containing such a button. So, I would strongly plea for adding this or a similar solution to the imagej-legacy! Thanks a lot!

tischi commented 2 years ago

@imagejan I wonder whether this PR could be extended to skip all required = false inputs? Maybe this would then also help with that issue: https://github.com/scijava/scijava-common/issues/391 at least when calling from IJ-Macro?

imagesc-bot commented 1 year ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/image-data-figure-creation-best-practices-example-for-collagen-secretion/84584/17

NicoKiaru commented 1 year ago

Just dropping this here even if it's not completely directly related:

I've added in a repo of mine a MessageResolverProcessor, which role is to skip the ui display if all remaining unresolved inputs visibility are MESSAGE). This work well so far.

tferr commented 1 year ago

Yet one more complain about this here: https://forum.image.sc/t/sholl-analysis-from-image-batch-processing/85421

It has been quite a while and I cannot remember the details of why this remains unsolved. What is exactly the hold-up?

@NicoKiaru, how does one can reuse your MessageResolverProcessor? How are macro calls aware of it?

NicoKiaru commented 1 year ago

@NicoKiaru, how does one can reuse your MessageResolverProcessor? How are macro calls aware of it?

Easy quick way: just try to activate the PTBIOP update site, and see if that solves anything.

tferr commented 1 year ago

Easy quick way: just try to activate the PTBIOP update site, and see if that solves anything.

Oh I see. It bumps itself so is picked up earlier than the default harvester. Super smart @NicoKiaru! Unfortunately, it does not seem to fix this use case because buttons and LUTs are still being displayed. Unfortunate. These components remain in a prompt.

image

Does anyone know if there is something stalling this that it is not obvious. I'll probably need to look into it, as it affects many SNT commands.

imagejan commented 1 year ago

@tferr does #262 solve this for you? It's less intrusive then this PR (in particular doesn't add a StringToButtonConverter), but also solves only the pre-processing side (i.e. running a macro with options that don't contain values for button inputs), not the post-processing (i.e. how the macro gets recorded when you run the plugin via the UI), and only addresses buttons.

Are there any other UI element input parameters you need to exempt from macro-recording, and resolve in a tolerant way when they're not present in the macro call?

imagesc-bot commented 1 year ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/sholl-analysis-from-image-batch-processing/85421/5

imagesc-bot commented 1 year ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/error-when-using-sholl-analysis-and-snt/85716/1

ctrueden commented 1 year ago

Does anyone know if there is something stalling this that it is not obvious.

Just me being slow. @tferr feel free to assign me issues and PRs that are especially bothersome to you; doing so gives it a bump in my priority queue. :+1:

imagesc-bot commented 1 year ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/error-when-using-sholl-analysis-and-snt/85716/3

imagesc-bot commented 8 months ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/automize-sholl-analysis-on-tiff-files-of-a-folder/92845/1

imagesc-bot commented 3 months ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/guidance-on-sholl-analysis/99943/4

imagesc-bot commented 2 months ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/trouble-with-sholl-analysis-using-neuroanatomy-in-fiji/101105/5