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

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

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

Conversation

frauzufall
Copy link
Member

@frauzufall frauzufall commented Mar 16, 2020

  • button in scijava command will be recorded as buttonName=0
  • macro call with buttonName=0 will not display widget with button

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! :)

* button in scijava command will be recorded as buttonName=0
* macro call with buttonName=0 will not display widget with button
@tischi
Copy link

tischi commented Mar 16, 2020

  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: bb57301

@ctrueden ctrueden self-requested a review March 16, 2020 13:45
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frauzufall @tischi Thanks for digging on this! Before proceeding further, let's spend a moment to consider some scenarios:

  1. Will someone ever want a command whose dialog box is nothing else than a button? I am inclined you say "Yes, someone might want that." It is after all a simple and UI agnostic way to generate functional button(s) with attached code.
  2. Will someone ever want a command whose dialog box is a mixture of inputs and buttons—but if all inputs are already resolved, they don't want the box to appear? I'm sure the answer is yes, since that is the scenario here. 😉
  3. Will someone ever want a command whose dialog box is a mixture of inputs and buttons—and if all inputs are already resolved, still wants the box to appear with only the buttons? I'm not sure, but similar to scenario 1 above, it might come up.

Therefore, I conclude that we need a way to declare the behavior of each button explicitly: a new module item property. We can have a sensible default (either 2 or 3 above), but some people might want the other behavior. The buttonName=0 hack enables people to toggle it running from a macro or script, but it is not fully general, since inputs can be filled from other sources e.g. custom preprocessors. Plus there may be other input types in the future that benefit from this new property. The property would indicate whether the item is intended to be displayed when there are no normal unresolved parameters left, and/or appropriate to be recorded from a macro.

Let's check the ModuleItem API! The visibility property looks like a good candidate. There are currently four settings for visibility:

  • INVISIBLE - Item is excluded from the history for the purposes of data provenance, and also excluded as a parameter when recording scripts.
  • MESSAGE - As INVISIBLE, and further indicating that the item's value is intended as a message to the user (e.g., in the input harvester panel) rather than an actual parameter to the module execution.
  • NORMAL - Item is included in the history for purposes of data provenance, and included as a parameter when recording scripts.
  • TRANSIENT - Item is excluded from the history for the purposes of data provenance, but still included as a parameter when recording scripts.

As an aside, here is the discussion from 2011 where these were discussed and created:

At first I thought it might work to set the Button to MESSAGE visibility, because I vaguely remembered that the input harvester might decline to pop a dialog when all unresolved parameters are MESSAGE visibility. But alas, it still pops up, just as a "question" style dialog rather that "information" one, because the original scenario was to use String message parameters to ask questions that way.

Regardless: setting a button to "message" is too unintuitive. I think none of the above four visibilities is appropriate, and we need a new one. Reasoning:

Right now, there are two methods isMessageOnly() and hasWidgets() of InputPanel which affect how the InputPanel/InputHarvester machinery behaves. If after processing all unresolved inputs, there are no widgets, then the dialog is skipped. Or if the only widgets are message visibility, the dialog becomes a question prompt.

Unfortunately, neither of those outcomes is what we want here. We want a way to say, for each item, whether it should—on its own—trigger a dialog to pop at all. While this is technically a distinct question from "should the item be macro-recorded", for our scenario, we never want button items to be recorded. But what about the general case? This leads to another scenario to consider:

  1. Will someone ever have a subset of unresolved module items which they don't want shown in a dialog alone, but which they do want to be macro recorded? I don't know...

So: please give it some more thought and come back with a proposal here, and we can then move forward. Here are the questions to ponder and resolve, in my view:

  • Is ItemVisibility really the best place for this new toggle? Or should it be a new boolean, independent of the visibility? After all, it really has to do with whether to do input harvesting, not whether to record history. (Or does it? How are these two concepts related? Is scenario 4 above a concern? I lean toward not... Thoughts?)
  • What to name it? I'm blanking on good names for this property. The best I have so far is "stealth" which is terrible. 😜
  • What should the default value be for buttons? Stealth, or not stealth?

Finally: some related discussion (as well as links to the SJC3 input harvesting prototype from late 2017) can be found at scijava/scijava-common#42. In that issue, @adaerr suggests:

I wonder whether it is not worth making a special case of purely UI related changes like graying out/disabling or even making invisible items which "aren't really parameters", such as Button widgets (possibly also Visibility=MESSAGE parameters ?

@NicoKiaru
Copy link
Contributor

NicoKiaru commented Mar 16, 2020

What are the use case of these buttons ?

@frauzufall
Copy link
Member Author

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

@frauzufall
Copy link
Member Author

@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.

@frauzufall frauzufall closed this Mar 16, 2020
@NicoKiaru
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

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: stardist/stardist-imagej#7.

@tferr
Copy link
Contributor

tferr commented Mar 31, 2021

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 added a commit that referenced this pull request Apr 10, 2021
When calling a command from a macro, we usually don't want to show a UI dialog.

If the called command contains inputs of type Button, we resolve these without setting them,
as we assume that buttons and their callbacks provide some interactivity that is not required
when running from macro.

See #239 for some discussion.
@imagejan
Copy link
Member

I hadn't noticed this whole issue until @tferr's post ten days ago. There's some overlap with other open issues, in particular scijava/scijava-common#402 and scijava/scijava-common#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
Copy link

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 ctrueden reopened this May 12, 2022
@ctrueden
Copy link
Member

@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
Copy link
Member

As said above, I would favor my less intrusive suggestion #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
Copy link

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

@biovoxxel
Copy link

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

  • cloned the indicated repo
  • build the imagej-legacy-0.38.2-SNAPSHOT.jar
  • exchanged the shipped one for the built one
  • ran my code while recording
  • tested the macro, however, the window with the lonely button still appears when the macro is called.

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
Copy link

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
Copy link

tischi commented May 16, 2022

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

@imagesc-bot
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

tferr commented Aug 24, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

tferr commented Aug 24, 2023

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
Copy link
Member

imagejan commented Aug 24, 2023

@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
Copy link

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
Copy link

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
Copy link
Member

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. 👍

@ctrueden ctrueden self-assigned this Aug 31, 2023
@imagesc-bot
Copy link

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
Copy link

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

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

Successfully merging this pull request may close these issues.

None yet

9 participants