Ticket #1020 (closed task: fixed)

Opened 7 years ago

Last modified 7 years ago

Use ItemIO.BOTH where relevant in plugins

Reported by: curtis Owned by: bdezonia
Priority: minor Milestone: imagej2-b2-bugfixes
Component: Plugins Version:
Severity: serious Keywords:
Cc: Martin.Horn@… Blocked By: #1157

Description (last modified by curtis) (diff)

Many plugins take a Dataset, DatasetView or Display as input, then mutate it and produce no explicit outputs.

We should be declaring such parameters with type = ItemIO.BOTH (rather than the default of ItemIO.INPUT). In the case of Displays, these plugins should then *not* explicitly invoke display.update(), since the DisplayPostprocessor will do it for all output displays.

This approach is more robust since someone might want to invoke multiple plugins programmatically in a workflow, and only update the display at the end.

This change will be useful for KNIP/KNIME, to more accurately model parameter inputs and outputs.

Change History

comment:1 Changed 7 years ago by curtis

  • Description modified (diff)

comment:2 Changed 7 years ago by bdezonia

Many of these plugins use Dataset::update() instead. Dataset::update() generates an event and sets the dirty flag. Many of them do not call display.update(). Determine what the best behavior is here before going forward.

comment:3 Changed 7 years ago by curtis

  • Blocked By 1157 added

comment:3 Changed 7 years ago by curtis

I am not sure of the best way to deal with Datasets.

Calling Dataset::update() will set off a chain of events that will eventually call Display::update() on all Displays containing that Dataset.

Currently, declaring the Dataset parameter as ItemIO.BOTH will cause DisplayPostprocessor to generate a second Display::update() for all Displays containing that Dataset, which is redundant and potentially wasteful.

However, it is vital that the Dataset be labeled as ItemIO.BOTH because it *is* also an output of the plugin, and downstream code needs to know that.

I can think of a couple different solutions, but none of them seems obviously and unequivocally best to me:

  1. All concrete implementations of Display become smart enough that calling update() twice in a row does nothing on the second call, because there is nothing left to do. This would entail more careful tracking of what has changed. It also makes doing a "forced refresh" of the display potentially less intuitive, as we would then need a separate method or toggle for doing that.
  1. The DisplayPostprocessor somehow becomes smart enough to avoid calling Display::update() on Displays that were already updated during plugin execution, either directly or as part of an event chain, such as when calling Dataset::update(). I am not entirely sure how we would track that though.

There is also another problem: see ticket #1157 for details.

comment:4 Changed 7 years ago by bdezonia

  • Milestone changed from imagej-2.0.0-beta3 to imagej-2.0.0-beta2

comment:5 Changed 7 years ago by bdezonia

  • Cc Martin.Horn@… added

With 853690ad1f0cdb7308982e915444c942630f39c0 all plugins with read/write @Parameters have been tagged with ItemIO.BOTH

comment:6 Changed 7 years ago by bdezonia

  • Status changed from new to closed
  • Resolution set to fixed

The remaining portions of this ticket have been hatched as new ticket #1180. Closing.

Note: See TracTickets for help on using tickets.