PlugInFilterRunner breaks Undo for single-image TYPE_CONVERSION

[mountain-maennlein]

From @mountain-maennlein on May 9, 2018 18:7

PlugInFilterRunner breaks Undo for certain code paths.

I am looking at the code in PlugInFilterRunner.java, from:

https://imagej.nih.gov/ij/developer/source/ij/plugin/filter/PlugInFilterRunner.java.html

It appears that the bug is in line 115. Lines 115--120 of the code are:

                if ((flags&PlugInFilter.NO_CHANGES)==0) {   // (filters doing no modifications don't change undo status)
                    if (snapshotDone)
                        Undo.setup(Undo.FILTER, imp);
                    else
                        Undo.reset();
                }

Line 115 tests for PlugInFilter.NO_CHANGES NOT being set, which means that the plugin has stated that it WILL (at least potentially) change the image. If the condition tests true (that is, NO_CHANGES is not set), then the subsequent lines incorrectly clear the Undo information.

I do not understand all of the code paths here, but, simplistically, it would appear that replacing line 115 with:

                if ((flags&PlugInFilter.NO_CHANGES)!=0) {   // (filters doing no modifications don't change undo status)

would fix the bug.

As a workaround, a (single-image) TYPE_CONVERSION PlugInFilter can return, e.g.:

   return DOES_8G | NO_CHANGES;

from its setup() method (falsely saying that it won't change the image), in effect, reversing the incorrect NO_CHANGES test in PlugInFilterRunner, and preventing PlugInFilterRunner from clearing the Undo information.

Here is code for a plugin that illustrates the bug and the workaround (but not the proposed actual fix):

// PlugInFilterUndo.java

// contents of plugins.config file:
//
// MM, "PlugInFilterUndo", mm.PlugInFilterUndo("")
// MM, "PlugInFilterUndo -- NO_CHANGES", mm.PlugInFilterUndo("NO_CHANGES")

package mm;

import ij.ImagePlus;
import ij.Undo;
import ij.plugin.filter.PlugInFilter;
import ij.process.ImageProcessor;
import ij.process.ShortProcessor;

/**
 * example for broken undo in PlugInFilterRunner
 */

public class PlugInFilterUndo implements PlugInFilter {

  private ImagePlus imp = null;

  public int setup (String arg, ImagePlus image) {
    imp = image;  // remember image for run method
    if (arg.equals ("NO_CHANGES"))  return DOES_8G | NO_CHANGES;
    else                            return DOES_8G;
  }

  public void run (ImageProcessor ip) {
    ShortProcessor sp = imp.getProcessor().convertToShortProcessor();
    short pixels[] = (short[]) sp.getPixels();
    for (int i = 0; i < pixels.length; i++)
      pixels[i] = (short) (((i / 32) % 2 == 0) ? 16384 : 49152);
    Undo.setup (Undo.TYPE_CONVERSION, imp);
    imp.setProcessor (sp);
    return;
  }

}

To run this code, build and install the plugin (including the plugins.config given as comments in the above java code.) Then open (or create) an 8-bit image and run the command:

MM > PlugInFilterUndo -- NO_CHANGES

This version uses the "NO_CHANGES" workaround, and will convert the image to a 16-bit image (and change the values of the pixels). Now running:

Edit > Undo

will revert the image back to the 8-bit original.

Contrariwise, running the version without the workaround:

MM > PlugInFilterUndo

will perform the same conversion to a 16-bit image, but, because of the PlugInFilterRunner bug, subsequently running:

Edit > Undo

will now not work.

This is all analyzed and tested with:

ImageJ 2.0.0-rc-65/1.52b; Java 1.8.0_66 [64-bit]; Linux 4.4.0-121-generic;

on:

ubuntu 16.04 LTS, 64-bit

Thanks, mm

Copied from original issue: imglib/imglib2-ij#15

[mountain-maennlein]

From @imagejan on May 9, 2018 20:3

This issue belongs in the imagej/imagej1 repository.

[mountain-maennlein]

From @rasband on May 10, 2018 0:39

The PlugInFilterRunner appears to be working as expected. Run a plugin like this

public class Inverter_ implements PlugInFilter { private ImagePlus imp = null;

public int setup (String arg, ImagePlus image) { imp = image; return DOES_8G; } public void run (ImageProcessor ip) { ip.invert(); imp.updateAndDraw(); } }

on an 8-bit image and you can undo the inversion by pressing "z" (Edit>Undo). Change "return DOES_8G;" to "return DOES_8G|NO_CHANGES;" and pressing "z" does nothing because PlugInFilterRunner has not setup an undo buffer. The PlugInFilter interface is not designed to support undo of images converted to a different type. In this case it, is better to use the PlugIn interface.

[mountain-maennlein]

Wayne, thanks for your explanation. I will close this issue based on your statement that "The PlugInFilter interface is not designed to support undo of images converted to a different type."

I understand your "ip.invert()" example, and I think I now fully understand line 115 of PlugInFilterRunner. In my words I would say that my attempt to use Undo with a TYPE_CONVERSION PlugInFilter is not compatible with PlugInFilterRunner's built-in Undo support for FILTER-type filters.

I guess I was misled by the silence of the PlugInFilter (and PlugInFilterRunner) javadoc on TYPE_CONVERSION, and by this 2000-vintage document:

Writing ImageJ PlugIns – A Tutorial

which states:

   ij.plugin
      PlugIn
         This interface has to be implemented by plugins, that do not require an image as
         input (see section 3).
   ij.plugin.filter
      PlugInFilter
         This interface has to be implemented by plugins, that require an image as
         input (see section 3).

So I interpreted the tutorial as telling me that I should use PlugInFilter for all plugins that operate on an image, including plugins that perform TYPE_CONVERSION (and that the PlugInFilter javadoc was not telling that I shouldn't).

Going forward, I will stick to your advice and use regular PlugIns for everything except FILTER-type plugins.

Thanks, mm

[rasband]

You can use PlugInFilter for plugins that change the image type as long as the plugin specifies NO_CHANGES, which is appropriate in this case because no changes are made to the original ImageProcessor.

[mountain-maennlein]

Wayne, thank you for your follow-up. I'm a bit confused about best practices and PlugInFilter now. What's the imagej-approved way of doing things?

If I have a plug-in that operates on a single, already-open ImagePlus, and modifies it, possibly (or perhaps always) by replacing the ImagePlus's ImageProcessor, what would be the recommended approach? Should I package such a plug-in as a PlugIn or as a PlugInFilter?

Thanks, mm

[rasband]

I would use the PlugIn interface, which is simpler and easier to understand.

[mountain-maennlein]

Wayne, okay, the PlugIn interface it is. Works for me.

Thanks, mm