Skip to content

Multiple file input #27

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

Merged
merged 2 commits into from
Sep 22, 2017
Merged

Conversation

imagejan
Copy link
Member

@imagejan imagejan commented Jul 9, 2017

This PR depends on scijava/scijava-common#276.

It implements the chooseFiles method, adds a new SwingFileListWidget, and adds drag-and-drop support to both the new widget and SwingFileWidget.

The current remaining issue is that the returned value is a File array of size one, with the file name being the String representation of the desired file list.
@ctrueden could you have a look at why this doesn't return an array of all files in the list? I couldn't figure out what's wrong...

@ctrueden
Copy link
Member

ctrueden commented Jul 18, 2017

The current remaining issue is that the returned value is a File array of size one, with the file name being the String representation of the desired file list.
@ctrueden could you have a look at why this doesn't return an array of all files in the list? I couldn't figure out what's wrong...

Great progress, and sorry for the delay in my reply. I took a look. Here are the steps I took:

  1. Fetch Add method signatures and preprocessor to select multiple files scijava-common#276 and check it out.
  2. Fetch Multiple file input #27 (this PR) and check it out.
  3. In Eclipse, add the following main method:
    package org.scijava.ui.swing;
    
    import org.scijava.Context;
    import org.scijava.script.ScriptService;
    import org.scijava.ui.UIService;
    
    public class Main {
      public static void main(final String[] args) throws Exception {
        final Context context = new Context();
        context.service(UIService.class).showUI();
        final String path = "script.groovy";
        final String script = "" + //
          "#@File[] files\n" + //
          "#@OUTPUT String result\n" + //
          "import java.util.Arrays\n" + //
          "result = Arrays.toString(files)";
        context.service(ScriptService.class).run(path, script, true);
      }
    }
  4. In pom.xml, add the following dependency:
    <dependency>
      <groupId>org.scijava</groupId>
      <artifactId>scripting-groovy</artifactId>
      <scope>test</scope>
    </dependency>
  5. Run the main method, select multiple files, and observe the result window listing all of them.

So all works well so far. But then, if you add #@String name as a second input parameter, things get weird. The file chooser widget initially shows [Ljava.io.File;@463df763, which indicates that somewhere a toString() is being called on the File[] object itself. If you then click Browse and select some files, they appear correctly. But if you then click OK, the files get munged back into a similarly stringified version of the File[] again.

Investigating now, but I may run out of time for the day; we'll see! 💨

@ctrueden
Copy link
Member

One problem is caused by improper persistence of File[] objects by the PrefService. We need to fix it so that the entire list of files is persisted, rather than an erroneously stringified version.

After adding persist=false to the files parameter above as a workaround, the initial value is no longer wrong (instead it is null). The SwingFileListWidget then needs a null check in doRefresh() to guard against the null case. After that, things seem to work until OK is pressed, at which point the files variable is still null, rather than being populated with the chosen files as desired. My bus ride is over, so that's it from me for the moment.

@ctrueden
Copy link
Member

@imagejan Here is a patch for scijava-ui-swing which fixes the issues you are having:

diff --git a/src/main/java/org/scijava/ui/swing/widget/SwingFileListWidget.java b/src/main/java/org/scijava/ui/swing/widget/SwingFileListWidget.java
index 113d9ed..21592a3 100644
--- a/src/main/java/org/scijava/ui/swing/widget/SwingFileListWidget.java
+++ b/src/main/java/org/scijava/ui/swing/widget/SwingFileListWidget.java
@@ -92,6 +92,7 @@ public class SwingFileListWidget extends SwingInputWidget<File[]> implements
                        model.addElement(file);
                }
                paths.setModel(model);
+               updateModel();
        }

        // -- AbstractUIInputWidget methods ---
@@ -100,8 +101,10 @@ public class SwingFileListWidget extends SwingInputWidget<File[]> implements
        protected void doRefresh() {
                File[] files = (File[]) get().getValue();
                DefaultListModel<File> model = new DefaultListModel<>();
-               for (File file : files) {
-                       model.addElement(file);
+               if (files != null) {
+                       for (File file : files) {
+                               model.addElement(file);
+                       }
                }
                paths.setModel(model);
        }

And here is a patch for scijava-common which fixes a bug if you cancel the file chooser dialog:

diff --git a/src/main/java/org/scijava/module/DefaultModuleService.java b/src/main/java/org/scijava/module/DefaultModuleService.java
index f7a7f7a4..974e1845 100644
--- a/src/main/java/org/scijava/module/DefaultModuleService.java
+++ b/src/main/java/org/scijava/module/DefaultModuleService.java
@@ -296,6 +296,8 @@ public class DefaultModuleService extends AbstractService implements
 			return;
 		}
 
+		// FIXME: Convert to string, instead of just calling toString.
+		// Otherwise many things (e.g. File[]) are persisted improperly.
 		final String sValue = value == null ? "" : value.toString();
 
 		// do not persist if object cannot be converted back from a string
diff --git a/src/main/java/org/scijava/ui/UserInterface.java b/src/main/java/org/scijava/ui/UserInterface.java
index 9f298013..35329e32 100644
--- a/src/main/java/org/scijava/ui/UserInterface.java
+++ b/src/main/java/org/scijava/ui/UserInterface.java
@@ -192,8 +192,8 @@ public interface UserInterface extends RichPlugin, Disposable {
 	 *
 	 * @param files The initial value displayed in the file chooser prompt.
 	 * @param filter A filter allowing to restrict file choice.
-	 * @return The selected {@link File}s chosen by the user, or null if prompt is not
-	 *         available
+	 * @return The selected {@link File}s chosen by the user, or null if user
+	 *         cancels the prompt.
 	 */
 	default File[] chooseFiles(File[] files, FileFilter filter) {
 		throw new UnsupportedOperationException();
@@ -204,12 +204,13 @@ public interface UserInterface extends RichPlugin, Disposable {
 	 *
 	 * @param fileList The initial value displayed in the file chooser prompt.
 	 * @param filter A filter allowing to restrict file choice.
-	 * @return The selected {@link File}s chosen by the user, or null if prompt is not
-	 *         available
+	 * @return The selected {@link File}s chosen by the user, or null if user
+	 *         cancels the prompt. not available
 	 */
 	default List<File> chooseFiles(List<File> fileList, FileFilter filter) {
-		File[] files = fileList.toArray(new File[fileList.size()]);
-		return Arrays.asList(chooseFiles(files, filter));
+		final File[] initialFiles = fileList.toArray(new File[fileList.size()]);
+		final File[] chosenFiles = chooseFiles(initialFiles, filter);
+		return chosenFiles == null ? null : Arrays.asList(chosenFiles);
 	}
 
 	/**

@imagejan
Copy link
Member Author

Great, thanks a lot, @ctrueden!

I pushed your diffs (after having some issues merging them into my workspace somehow) as separate commits to the respective branches (and therefore the associated PRs).


// FIXME: Convert to string, instead of just calling toString.
// Otherwise many things (e.g. File[]) are persisted improperly.

Alternatively, we could disable persisting for list and array values by changing the return value of convertService.supports() here:

// do not persist if object cannot be converted back from a string
if (!convertService.supports(sValue, item.getType())) return;

(I actually wonder why it seems to support String -> File[] conversion, i.e. returns true at the above line...)

But of course, if we can make it properly persist lists of files and read them back, that would be more desirable.

@imagejan
Copy link
Member Author

imagejan commented Jul 19, 2017

Note: now I'm getting the following exception when testing with a groovy script in Fiji:

[ERROR] Failed to refresh widget: class org.scijava.ui.awt.widget.AWTTextWidget on EDT
java.lang.reflect.InvocationTargetException
	at java.awt.EventQueue.invokeAndWait(EventQueue.java:1321)
...
Caused by: java.lang.NullPointerException
	at org.scijava.ui.awt.widget.AWTTextWidget.doRefresh(AWTTextWidget.java:97)
	at org.scijava.ui.AbstractUIInputWidget$1.run(AbstractUIInputWidget.java:85)
	at org.scijava.thread.DefaultThreadService$2.run(DefaultThreadService.java:220)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:301)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:756)
	at java.awt.EventQueue.access$500(EventQueue.java:97)
	at java.awt.EventQueue$3.run(EventQueue.java:709)
	at java.awt.EventQueue$3.run(EventQueue.java:703)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:726)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

Somehow scijava-ui-awt seems to be picked up, where no FileListWidget is implemented yet...

@ctrueden
Copy link
Member

now I'm getting the following exception when testing with a groovy script in Fiji

Ping me again if you are still stuck on Monday, July 31, and I'll investigate some more. Unfortunately, I will not have time before that. In the meantime: good luck!

@imagejan
Copy link
Member Author

Running fine now.

Tests are failing because of the scijava-common snapshot dependency, but should be fine after scijava/scijava-common#276 and/or scijava/scijava-common#285 get merged.

@imagejan imagejan changed the title Multiple file input (WIP) Multiple file input Jul 21, 2017
This makes the following improvements:

* Implement UserInterface#chooseFiles method, for multi-file selection.
* Add SwingFileListWidget, to allow input harvesting for File[] inputs.
* Add drag and drop functionality for Swing file widgets.

These changes require scijava-common version 2.66.0.
@ctrueden
Copy link
Member

After intense hacking with @imagejan, this branch is good to go! 👍

@ctrueden ctrueden merged commit 993b164 into scijava:master Sep 22, 2017
@ctrueden ctrueden deleted the multiple-file-input branch September 22, 2017 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants