Skip to content

Commit

Permalink
Avoid calling chooseFiles for non-autofill inputs
Browse files Browse the repository at this point in the history
This commit changes the API of ModuleService and DefaultModuleService to include an optional parameter 'acrossTypes' that allows checking for solitary unresolved parameters within all inputs/outputs independent of their type, while maintaining the backwards compatible method signatures.

We can then replace some duplicated logic in helper methods of FilePreprocessor and FileListPreprocessor by a call to 'moduleService.getSingleInput()', which makes them respect an 'autoFill=false' parameter annotation and avoid calling uiService.chooseFile/chooseFiles for those parameters.

The intended behavior is verified by two new tests that, before this commit, are failing on their last assertion.
  • Loading branch information
imagejan committed May 16, 2021
1 parent beb6de2 commit 4168b3e
Show file tree
Hide file tree
Showing 6 changed files with 285 additions and 76 deletions.
27 changes: 12 additions & 15 deletions src/main/java/org/scijava/module/DefaultModuleService.java
Original file line number Diff line number Diff line change
Expand Up @@ -260,27 +260,23 @@ public <M extends Module> M waitFor(final Future<M> future) {
}

@Override
public <T> ModuleItem<T> getSingleInput(final Module module,
final Class<T> type)
{
return getTypedSingleItem(module, type, module.getInfo().inputs());
public <T> ModuleItem<T> getSingleInput(final Module module, final Class<T> type, boolean acrossTypes) {
return getTypedSingleItem(module, type, module.getInfo().inputs(), acrossTypes);
}

@Override
public <T> ModuleItem<T> getSingleOutput(final Module module,
final Class<T> type)
{
return getTypedSingleItem(module, type, module.getInfo().outputs());
public <T> ModuleItem<T> getSingleOutput(final Module module, final Class<T> type, boolean acrossTypes) {
return getTypedSingleItem(module, type, module.getInfo().outputs(), acrossTypes);
}

@Override
public ModuleItem<?> getSingleInput(Module module, Collection<Class<?>> types) {
return getSingleItem(module, types, module.getInfo().inputs());
public ModuleItem<?> getSingleInput(Module module, Collection<Class<?>> types, boolean acrossTypes) {
return getSingleItem(module, types, module.getInfo().inputs(), acrossTypes);
}

@Override
public ModuleItem<?> getSingleOutput(Module module, Collection<Class<?>> types) {
return getSingleItem(module, types, module.getInfo().outputs());
public ModuleItem<?> getSingleOutput(Module module, Collection<Class<?>> types, boolean acrossTypes) {
return getSingleItem(module, types, module.getInfo().outputs(), acrossTypes);
}

@Override
Expand Down Expand Up @@ -478,24 +474,25 @@ private void assignInputs(final Module module,
}

private <T> ModuleItem<T> getTypedSingleItem(final Module module,
final Class<T> type, final Iterable<ModuleItem<?>> items)
final Class<T> type, final Iterable<ModuleItem<?>> items, boolean acrossTypes)
{
Set<Class<?>> types = new HashSet<>();
types.add(type);
@SuppressWarnings("unchecked")
ModuleItem<T> result = (ModuleItem<T>) getSingleItem(module, types, items);
ModuleItem<T> result = (ModuleItem<T>) getSingleItem(module, types, items, acrossTypes);
return result;
}

private ModuleItem<?> getSingleItem(final Module module,
final Collection<Class<?>> types, final Iterable<ModuleItem<?>> items)
final Collection<Class<?>> types, final Iterable<ModuleItem<?>> items, boolean acrossTypes)
{
ModuleItem<?> result = null;

for (final ModuleItem<?> item : items) {
final String name = item.getName();
if (!item.isAutoFill()) continue; // skip unfillable inputs
if (module.isInputResolved(name)) continue; // skip resolved inputs
if (acrossTypes && result != null) return null; // multiple unresolved items
final Class<?> itemType = item.getType();
for (final Class<?> type : types) {
if (type.isAssignableFrom(itemType)) {
Expand Down
48 changes: 44 additions & 4 deletions src/main/java/org/scijava/module/ModuleService.java
Original file line number Diff line number Diff line change
Expand Up @@ -272,26 +272,66 @@ <M extends Module> Future<M> run(M module,
* given type, returning the relevant {@link ModuleItem} if found, or null if
* not exactly one unresolved fillable input of that type.
*/
<T> ModuleItem<T> getSingleInput(Module module, Class<T> type);
default <T> ModuleItem<T> getSingleInput(Module module, Class<T> type) {
return getSingleInput(module, type, false);
}

/**
* Checks the given module for a solitary unresolved fillable input of the
* given type, returning the relevant {@link ModuleItem} if found, or null if
* not exactly one unresolved fillable input.
*
* If {@code acrossTpyes} is true, all inputs independent of type are taken
* into account when checking for singularity of the unresolved input.
*/
<T> ModuleItem<T> getSingleInput(Module module, Class<T> type, boolean acrossTypes);

/**
* Checks the given module for a solitary unresolved output of the given type,
* returning the relevant {@link ModuleItem} if found, or null if not exactly
* one unresolved output of that type.
*/
<T> ModuleItem<T> getSingleOutput(Module module, Class<T> type);
default <T> ModuleItem<T> getSingleOutput(Module module, Class<T> type) {
return getSingleOutput(module, type, false);
}

/**
* Checks the given module for a solitary unresolved output of the given type,
* returning the relevant {@link ModuleItem} if found, or null if not exactly
* one unresolved output.
*
* If {@code acrossTpyes} is true, all outputs independent of type are taken
* into account when checking for singularity of the unresolved output.
*/
<T> ModuleItem<T> getSingleOutput(Module module, Class<T> type, boolean acrossTypes);

/**
* As {@link #getSingleInput(Module, Class)} but will match with a set of
* potential classes, at the cost of generic parameter safety.
*/
ModuleItem<?> getSingleInput(Module module, Collection<Class<?>> types);
default ModuleItem<?> getSingleInput(Module module, Collection<Class<?>> types) {
return getSingleInput(module, types, false);
}

/**
* As {@link #getSingleInput(Module, Class, boolean)} but will match with a set of
* potential classes, at the cost of generic parameter safety.
*/
ModuleItem<?> getSingleInput(Module module, Collection<Class<?>> types, boolean acrossTypes);

/**
* As {@link #getSingleOutput(Module, Class)} but will match with a set of
* potential classes, at the cost of generic parameter safety.
*/
ModuleItem<?> getSingleOutput(Module module, Collection<Class<?>> types);
default ModuleItem<?> getSingleOutput(Module module, Collection<Class<?>> types) {
return getSingleOutput(module, types, false);
}

/**
* As {@link #getSingleOutput(Module, Class, boolean)} but will match with a set of
* potential classes, at the cost of generic parameter safety.
*/
ModuleItem<?> getSingleOutput(Module module, Collection<Class<?>> types, boolean acrossTypes);

/**
* Registers the given value for the given {@link ModuleItem} using the
Expand Down
34 changes: 6 additions & 28 deletions src/main/java/org/scijava/ui/FileListPreprocessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

import org.scijava.module.Module;
import org.scijava.module.ModuleItem;
import org.scijava.module.ModuleService;
import org.scijava.module.process.AbstractPreprocessorPlugin;
import org.scijava.module.process.PreprocessorPlugin;
import org.scijava.plugin.Parameter;
Expand All @@ -44,10 +45,13 @@ public class FileListPreprocessor extends AbstractPreprocessorPlugin {
@Parameter(required = false)
private UIService uiService;

@Parameter(required = false)
private ModuleService moduleService;

@Override
public void process(final Module module) {
if (uiService == null) return;
final ModuleItem<File[]> fileInput = getFilesInput(module);
if (uiService == null || moduleService == null) return;
final ModuleItem<File[]> fileInput = moduleService.getSingleInput(module, File[].class, true);
if (fileInput == null) return;

final File[] files = fileInput.getValue(module);
Expand All @@ -65,30 +69,4 @@ public void process(final Module module) {
module.resolveInput(fileInput.getName());
}

// -- Helper methods --

/**
* Gets the single unresolved {@link File} input parameter. If there is not
* exactly one unresolved {@link File} input parameter, or if there are other
* types of unresolved parameters, this method returns null.
*/
private ModuleItem<File[]> getFilesInput(final Module module) {
ModuleItem<File[]> result = null;
for (final ModuleItem<?> input : module.getInfo().inputs()) {
if (module.isInputResolved(input.getName())) continue;
final Class<?> type = input.getType();
if (!File[].class.isAssignableFrom(type)) {
// not a File[] parameter; abort
return null;
}
if (result != null) {
// second File parameter; abort
return null;
}
@SuppressWarnings("unchecked")
final ModuleItem<File[]> fileInput = (ModuleItem<File[]>) input;
result = fileInput;
}
return result;
}
}
35 changes: 6 additions & 29 deletions src/main/java/org/scijava/ui/FilePreprocessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

import org.scijava.module.Module;
import org.scijava.module.ModuleItem;
import org.scijava.module.ModuleService;
import org.scijava.module.process.AbstractPreprocessorPlugin;
import org.scijava.module.process.PreprocessorPlugin;
import org.scijava.plugin.Parameter;
Expand All @@ -53,12 +54,15 @@ public class FilePreprocessor extends AbstractPreprocessorPlugin {
@Parameter(required = false)
private UIService uiService;

@Parameter(required = false)
private ModuleService moduleService;

// -- ModuleProcessor methods --

@Override
public void process(final Module module) {
if (uiService == null) return;
final ModuleItem<File> fileInput = getFileInput(module);
if (uiService == null || moduleService == null) return;
final ModuleItem<File> fileInput = moduleService.getSingleInput(module, File.class, true);
if (fileInput == null) return;

final File file = fileInput.getValue(module);
Expand All @@ -75,31 +79,4 @@ public void process(final Module module) {
module.resolveInput(fileInput.getName());
}

// -- Helper methods --

/**
* Gets the single unresolved {@link File} input parameter. If there is not
* exactly one unresolved {@link File} input parameter, or if there are other
* types of unresolved parameters, this method returns null.
*/
private ModuleItem<File> getFileInput(final Module module) {
ModuleItem<File> result = null;
for (final ModuleItem<?> input : module.getInfo().inputs()) {
if (module.isInputResolved(input.getName())) continue;
final Class<?> type = input.getType();
if (!File.class.isAssignableFrom(type)) {
// not a File parameter; abort
return null;
}
if (result != null) {
// second File parameter; abort
return null;
}
@SuppressWarnings("unchecked")
final ModuleItem<File> fileInput = (ModuleItem<File>) input;
result = fileInput;
}
return result;
}

}
109 changes: 109 additions & 0 deletions src/test/java/org/scijava/ui/FileListPreprocessorTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
package org.scijava.ui;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;

import java.io.File;
import java.io.FileFilter;
import java.util.concurrent.ExecutionException;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.scijava.Context;
import org.scijava.command.Command;
import org.scijava.command.CommandInfo;
import org.scijava.module.Module;
import org.scijava.module.ModuleService;
import org.scijava.plugin.Parameter;
import org.scijava.ui.headless.HeadlessUI;

public class FileListPreprocessorTest {

public static final String DUMMY_FILE_NAME = "dummy_file";
private Context context;
private ModuleService moduleService;

@Before
public void setUp() {
context = new Context();
context.service(UIService.class).setDefaultUI(new CustomUI());
context.service(UIService.class).setHeadless(false);
moduleService = context.service(ModuleService.class);
}

@After
public void tearDown() {
context.dispose();
}

@Test
public void test() throws InterruptedException, ExecutionException {
// assert that single File[] parameters get filled by ui.chooseFiles()
CommandInfo info = new CommandInfo(SingleParameterFileListCommand.class);
Module mod = moduleService.run(info, true).get();
assertNotNull(mod);
assertEquals(DUMMY_FILE_NAME, ((File[])mod.getInput("inputFiles"))[0].getName());

// assert that the presence of any other unresolved parameters prevents
// calls to ui.chooseFiles()
CommandInfo info2 = new CommandInfo(ParameterFileListAndOthersCommand.class);
Module mod2 = moduleService.run(info2, true).get();
assertNotNull(mod2);
assertNull(mod2.getInput("inputFiles"));

// assert that 'autoFill = false' prevents calls to ui.chooseFiles()
CommandInfo info3 = new CommandInfo(NoAutofillParameterFileListCommand.class);
Module mod3 = moduleService.run(info3, true).get();
assertNotNull(mod3);
assertNull(mod3.getInput("inputFiles"));
}

private class CustomUI extends HeadlessUI {

@Override
public File[] chooseFiles(File parent, File[] files, FileFilter filter, String style) {
return new File[] { new File(DUMMY_FILE_NAME) };
}
}

public static class SingleParameterFileListCommand implements Command {

@Parameter(persist = false)
private File[] inputFiles;

@Override
public void run() {
// do nothing
}

}

public static class ParameterFileListAndOthersCommand implements Command {

@Parameter(persist = false)
private File[] inputFiles;

@Parameter(required = false)
private String dummyString;

@Override
public void run() {
// do nothing
}

}

public static class NoAutofillParameterFileListCommand implements Command {

@Parameter(autoFill = false, persist = false)
private File[] inputFiles;

@Override
public void run() {
// do nothing
}

}
}
Loading

0 comments on commit 4168b3e

Please sign in to comment.