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

Skip FileListPreprocessor for autofill=false parameters #423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Member Author

Choose a reason for hiding this comment

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

While working on this pull request, I noticed that the above line is specific to inputs only, so the method probably doesn't show the intended behavior when called with outputs.

The current unit tests seem to miss the method signatures specific to outputs (getSingleOutput).

I'll open a separate issue for this.

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