167152 - [xpfilepicker] need proper support for modeOpenMultiple for nsIFilePicker

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[(not reading, please use [e-mail] instead)]

[xpfilepicker] need proper support for modeOpenMultiple for nsIFilePicker

you need to override ::GetFiles()

see http://bugzilla.mozilla.org/show_bug.cgi?id=121122 for the windows impl.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Optimistically targetting at 1.2beta, but I may not get a network connection
back by then.. at that point someone else should pick this up.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Needless to say, if someone does this, that would rock. 

The basic idea is that you want to use the mSelection member of
xpfe/components/filepicker/src/nsFileView.cpp.  The getRangeCount() and
getRangeAt(in long i, out long min, out long max) methods on this will give you
the selected rows...  The rest of the work is changing the logic in
filepicker.js and nsFilePicker.js to deal with multiple files; in particular the
interaction between multiple file selection and the filename textfield needs to
be sane somehow...

Comment 3

[(not reading, please use [e-mail] instead)]

the fix will be in:

Index: xpfe/components/filepicker/res/content/filepicker.js
Index: xpfe/components/filepicker/src/nsFilePicker.js

see the patch in #121122 to see how I'm "faking" it now.

Comment 4

[(not reading, please use [e-mail] instead)]

oops, bz beat me to the comment about where to fix.

Comment 5

[(not reading, please use [e-mail] instead)]

once this is done, #43015 will work on the linux, too.

Comment 6

[Brian Ryner (not reading)]

Attachment: first-cut patch

I think this is done, but I need to test it some more.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> +  readonly attribute nsISupportsArray selectedFiles;

Could this use nsIArray instead?

> +  var dir = treeView.selectedFiles.GetElementAt(0).QueryInterface(nsIFile);

.queryElementAt() (for nsIArray, for nsISupportsArray it's .QueryElementAt())

> +  // ignore multiple selection if a directory is selected... is there something
> +  // better we can do?

We could disable the OK button if multiple things including a dir are selected... 

> +    return this.mFiles[this.mIndex++];

The API for nsISimpleEnumerator demands that we throw NS_ERROR_FAILURE when
there are no more elements (stupid, eh?).  In any case, we should throw it any
time mIndex >= length...

> +  var file = fileList.GetElementAt(0).QueryInterface(nsIFile);

again, queryElementAt()

>  ioService.newFileURI(this.file());

Why the '()' ?

> +        mDirList->QueryElementAt(itemIndex, NS_GET_IID(nsIFile),
> +                                 getter_AddRefs(curFile));

I know this is old code, but do_QueryElementAt reads nicer and doesn't involve
manual IID stuff...   ;)

To bryner, since he's the man with the patch and I will have no time to work on
this in the foreseeable future (though I promise to be a lot more prompt in my
reviews now that things are a little settled...)

Comment 8

[Brian Ryner (not reading)]

Attachment: updated patch

I think this should be ready to go in.	It's been baking in my local tree for 5
months.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 116178 [details] [diff] [review]
updated patch

See comment 7; all those review comments still apply (and let's please not add
new users of nsISupportsArray to the tree, ok?	Make it nsIArray.)

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 116178 [details] [diff] [review]
updated patch

When did nsISupportsArray become evil?	I must not have gotten the memo on
that.  I have an array of XPCOM objects, which itself needs to have an XPCOM
interface.

Comment 11

[Boris Zbarsky [:bzbarsky]]

It became evil when 1) it was realized that it can never be frozen and that no
one will ever fix the methods on it that claim to return nsresult but return
PRBool and 2) nsIArray and nsCOMArray, which cover the functionality of
nsISupportsArray, became available.  This is why the nsIFilePicker API to
multiple files is _not_ an nsISupportsArray.

fwiw, I did ask alecf to add a nice comment to nsISupportsArray explaining that
it's deprecated, but he doesn't seem to have had time to do it....

Comment 12

[Alec Flett]

sorry that is bug 166888, I'll land something today.

I posted to netscape.public.mozilla.xpcom about this a while back.
http://groups.google.com/groups?q=nsisupportsarray+group:netscape.public.mozilla.xpcom&hl=en&lr=&ie=UTF-8&oe=UTF-8&selm=3D779491.3050506%40netscape.com&rnum=2
http://groups.google.com/groups?q=nsisupportsarray+group:netscape.public.mozilla.xpcom&hl=en&lr=&ie=UTF-8&oe=UTF-8&selm=al8412%245ab2%40ripley.netscape.com&rnum=8

Comment 13

[Jan Varga [:janv]]

Comment on attachment 116178 [details] [diff] [review]
updated patch

following super reviewer :)
sorry, nsISupportsArray was deprecated long time ago

Comment 14

[Brian Ryner (not reading)]

Attachment: revised patch

Ok, let's try this one.  I got rid of nsISupportsArray, fixed it to disable the
ok button if we have a multiple selection and a directory is selected, and
fixed a bug in addToTextInput that was causing problems for > 2 files.

Comment 15

[Jan Varga [:janv]]

Comment on attachment 116263 [details] [diff] [review]
revised patch

looks good
r=varga

Comment 16

[Jan Varga [:janv]]

Attachment: diff -uw version of the patch

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 116263 [details] [diff] [review]
revised patch

gFilesEnumerator.mHasMore is no longer used; just eliminate it.

+    // Quote the existing text if needed,
+    // then append the new filename (quoted and escaped)
+    if (textInput.value[0] != '"')
+      newValue = '"' + textInput.value + '"';

Don't we need to escape the existing text?

sr=bzbarsky with that.

Comment 18

[Brian Ryner (not reading)]

checked in.

Comment 19

[(not reading, please use [e-mail] instead)]

thanks for fixing this bryner.  now linux users can take advantage of #43015

Comment 20

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.03.17.05 comm bits on linux rh8.0. i'm able to attach
multiple files via file picker (checked both ctrl+click and shift+click for
selecting files).