ROI Manager recording woes

[dscho]

Dear Wayne,

please find below a patch for the ROI Manager that fixes a couple of bugs
discovered during a recent, high-profile workshop. (Since my carefully
crafted documentation of the changes traditionally do not make it into
your source code repository, for this fix I allowed myself a slightly
sloppier development style than I usually do.)

In particular, it addresses the following problems:

- things were recorded twice (since events were recorded both while
  adjusting the GUI and after it).

- a lot of opportunity for race conditions was removed by avoiding to call
  list.getSelectedIndices() over and over again.

- after returning when no ROIs are selected, a check for the count <= 1 is
  misleading; I replaced it with the check == 1.

- multiple selections were recorded incorrectly as single selections.

- unselections were not recorded at all.

There are many, many more issues with recording ROI manager actions (some
of which are caused by the dichotomy between the macro vs script vs Java
API), so I have no doubt that you will make completely different
modifications to address the issues (maybe even addressing the concern
that roiManager("Select", 1) shows the selection in the GUI while
roiManager("Select", newArray(1, 2)) does not).

All I ask is that the following works: interactive clicks in the ROI
manager should be reflected in the Recorder as macros or scripts or Java
commands that reproduce exactly what the user did. I could even live with
all of the commands being listed twice, but as I demonstrate in the patch,
it is actually easy to prevent that.

Ciao,
Johannes

diff --git a/ij/plugin/frame/RoiManager.java b/ij/plugin/frame/RoiManager.java
index 5ddeaf5..4f81de2 100755
--- a/ij/plugin/frame/RoiManager.java
+++ b/ij/plugin/frame/RoiManager.java
@@ -2073,23 +2073,40 @@ public void mouseEntered (MouseEvent e) {}
  public void mouseExited (MouseEvent e) {}
 
  public void valueChanged(ListSelectionEvent e) {
- int index = 0;
- if (getCount()==0)
+ if (e.getValueIsAdjusting()) return;
+ if (getCount()==0) {
+ if (Recorder.scriptMode()) {
+ Recorder.recordCall("rm.runCommand(\"Deselect\");");
+ } else
+ Recorder.recordString("roiManager(\"Deselect\");\n");
  return;
- if (list.getSelectedIndices().length==0)
+ }
+ int[] selected = list.getSelectedIndices();
+ if (selected.length==0) {
  return;
-        index = list.getSelectedIndices()[0];
+ }
 //        list.repaint();
- if (index<0) index = 0;
  if (WindowManager.getCurrentImage()!=null) {
- if (list.getSelectedIndices().length <=1) {
- restore(getImage(), index, true);
+ if (selected.length == 1) {
+ restore(getImage(), selected[0], true);
  }
  if (record()) {
- if (Recorder.scriptMode())
- Recorder.recordCall("rm.select(imp, "+index+");");
- else
- Recorder.record("roiManager", "Select", index);
+ String arg = Arrays.toString(selected);
+ if (!arg.startsWith("[") || !arg.endsWith("]")) return;
+ arg = arg.substring(1, arg.length() - 1);
+ if (Recorder.scriptMode()) {
+ if (selected.length == 1) {
+ Recorder.recordCall("rm.runCommand(\"Select\", \""+arg+"\");");
+ } else {
+ Recorder.recordCall("rm.setSelectedIndexes(new int[] { "+arg+" });");
+ }
+ } else {
+ if (selected.length == 1) {
+ Recorder.recordString("roiManager(\"Select\", " + arg + ");\n");
+ } else {
+ Recorder.recordString("roiManager(\"Select\", newArray(" + arg + "));\n");
+ }
+ }
  }
  }
 

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html

[dscho]

Dear Wayne,

On Fri, 17 May 2013, Rasband, Wayne (NIH/NIMH) [E] wrote:

> On May 16, 2013, at 12:18 PM, Johannes Schindelin wrote:
>
> > please find below a patch for the ROI Manager that fixes a couple of
> > bugs discovered during a recent, high-profile workshop. (Since my
> > carefully crafted documentation of the changes traditionally do not
> > make it into your source code repository, for this fix I allowed
> > myself a slightly sloppier development style than I usually do.)
> >
> > In particular, it addresses the following problems:
> >
> > - things were recorded twice (since events were recorded both while
> > adjusting the GUI and after it).
> >
> > - a lot of opportunity for race conditions was removed by avoiding to
> > call list.getSelectedIndices() over and over again.
> >
> > - after returning when no ROIs are selected, a check for the count <=
> > 1 is misleading; I replaced it with the check == 1.
> >
> > - multiple selections were recorded incorrectly as single selections.
> >
> > - unselections were not recorded at all.
>
> I applied and tested this patch, in the 1.47r9 daily build. I had to
> make a few changes to account for differences between JavaScript and
> Java arrays. In "JavaScript" mode, the recorder now records
>
>     rm.setSelectedIndexes([0,1]);
>
> when you select the first two ROIs and it records
>
>     rm.setSelectedIndexes(new int[]{0,1});
>
> in "Plugin" mode.

I saw your fix, but hoped for a more general solution...

> Thanks for fixing these problems!
>
> > There are many, many more issues with recording ROI manager actions
> > (some of which are caused by the dichotomy between the macro vs script
> > vs Java API), so I have no doubt that you will make completely
> > different modifications to address the issues (maybe even addressing
> > the concern that roiManager("Select", 1) shows the selection in the
> > GUI while roiManager("Select", newArray(1, 2)) does not).
>
> I was able to easily fix this problem because the ROI Manager now uses a
> JList, which has a setSelectedIndices(int[]) method.

It will just lead to problems in truly headless mode, but I guess that's
what we have to live with. :-)

Thanks for your fixes,
Johannes

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html