121122 - nsIFilePicker should allow picking multiple files

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We should be able to ask the filepicker interface for a list of files, not just
one file.  This would be useful in several places.

Comment 1

[Peter Trudelle]

->bryner/future

Comment 2

[Luke Deller]

This bug is marked as blocking bug 43015 which currently:
- has 47 votes
- has a target milestone of 0.9.9 (I guess this will slip soon)
- is a dependency of Bug 92997 "Bugs that make Mozilla advocacy harder"

This is inconsistent with marking this bug "future".  Perhaps this inconsistency
should be fixed, either by adjusting this bug or the other bugs as appropriate?

Comment 3

[Boris Zbarsky [:bzbarsky]]

1)  Though fixing this bug would allow fixing bug 43015 in a fairly easy way, bug  
    43015 can possibly be solved without fixing this bug.
2)  This is an architecture change of a scope that is, in my opinion,
    inappropriate for pre-1.0 work at this point.
3)  The target of bug 43015 is unrealistic, imo, but that's an issue to take up
    with the developer who's working on that bug.

Comment 4

[scottputterman]

Boris, could you add comments to 43015 about how you think it might be solved
without this bug being fixed?

Comment 5

[Boris Zbarsky [:bzbarsky]]

If I had any decent ideas I would have commented....

Comment 6

[Mark Kaplun]

Isn't it the future already?

IMHO this kind of change will have an impact over every user of nsIFilePicker 
(tried to count them, but could not find an easy way to do it), in mozilla and 
any mozilla based application, therefor it will always be an high risk change 
and always easy to postpone.

My suggestion is to implement this change in two phase. The first will be to 
change the IDL of nsIFilePicker an make all necesery adjustments for the 
current functionality. The second phase will be to implement the actual multi 
selection. After the implementation of the first phase it will be easier to 
contribute solutions for the different platforms.

Another (uglier but easier) way of dealing with this bug can be to define a 
new interface nsIMultiFilePicker which imlements this functionality.

In any way this bug can't be an enhancment. It is the root cause for bug 43015 
which is a real UI bug (72 votes and rising).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Isn't it the future already?

No.  ;)

This is not being put off because it is risky or anything, but rather because
it's a lot of work and no one has the time to do it so far.  Making the IDL
change is trivial, but until we have an implementation for at least one of our
platforms, what's the point of doing it? It'll just make it look like the
functionality exists when in fact it does not...

Adding helpwanted keyword, since I (or bryner) can make this work in the XP
filepicker but someone else will likely need to do the Windows/Mac/OS2 ones....

Comment 8

[Mike Young]

marking 4xp and nominating for moz1.2 (as per the dependencies which some people
in mailnews would like to have and which is mostfreq [37 dups]) 

Since this is available in 4.x (for all platforms that I can test) this is not
an "enhancement" -- upping to normal since a normal bug depends on this. I
*would* say minor (workaround is: select one file at a time, multiple times) but
there seems to be a legitamite claim that there are certain instances where
multiple file-pickers are either not feasible, or not possible.

None of this means the bug will be fixed any faster, but it might just get on
some people's radars

Comment 9

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

Attachment: initial patch, works for windows, still needs some cleanup.

Comment 10

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

I'm going to take this from bryner, since I need it for #43015 and I have a 
patch (at least for windows).

My plan is to stub out the implementations on other platforms, so that 
modeOpenMultiple acts like modeOpen until those platforms get fixed (by people 
who know those platforms better than me.)

I'll work on cleaning up my unclean patch tomorrow.

Comment 11

[Boris Zbarsky [:bzbarsky]]

As I mentioned in email, I feel that nsISimpleEnumerator is a better choice than
nsISupportsArray for the file and url lists.  For one thing, if we ever want to
freeze nsIFilePicker we will likely need to eliminate its use of
nsISupportsArray....  For another, I can see of no use for the lists other than
to iterate over them as this patch does.

NS_NewArrayEnumerator is probably the simplest way to go here.

Comment 12

[Alec Flett]

ditto for what boris said. nsISimpleEnumerator is the way to go.

oh, and can you please not include GetFileURLs in your patch - I'm trying to
reduced dependencies by not supporting urls in the file picker. See bug 102013.

Comment 13

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

Attachment: patch

heeding bz's and alecf's advice.

next steps:

1) stub out other platfoms
2) fix mailnews js to use enumerator
3) don't use nsIFileURL (per alecf)

Comment 14

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

Attachment: patch to use enumerator

includes fix for #43015

Comment 15

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

accepting.  now to:

1) stub out other platfoms
2) don't use nsIFileURLs (per alecf)

hoping to make it for 1.2

Comment 16

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

Attachment: don't add another widget dependency on necko, per alecf.

now to stub other platforms, do some cleanup, and then get reviews.

Comment 17

[Alec Flett]

seth, I think you want to be using ioService.getURLSpecFromFile - instead of
creating the URI object yourself and then retrieving the spec.

Comment 18

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

Attachment: patch, make xpfilepicker and all the platforms do the right thing until they are fixed.

still need to test...

Comment 19

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

Attachment: unified, diff use helper method (thanks alecf!)

Comment 20

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

Can I get some r/sr reviewing from you guys for 
http://bugzilla.mozilla.org/show_bug.cgi?id=121122?

Here's what I have:

1)  nsIFilePicker has new attribute, nsISimpleEnumerator files, and a new 
mode:  modeOpenMultiple
2)  base impl (shared by beos, os2, mac) fakes GetFiles() to create an 
enumerator for the one file [those platforms, and the xp file picker] don't 
support this yet.
3)  the platform impls and the xp file picker currently treat modeOpenMultiple 
as modeOpen (this makes the base impl GetFiles() work)
4)  the windows implementation overrides GetFiles() and does the right thing
5)  compose js fixed to use the new mode.

This bug (37 dups, the top most dupped bug in mailnews that hasn't been fixed) 
is wanted for 1.2

Once the fix is reviewed and finished, I'll log a bug on the widget owners to 
override GetFiles() and support multiple file picking.

Note, with this patch, we have one less js dependency on the "readonly 
attribute nsIFileURL fileURL" in nsIFilePicker.

I'll also log a cleanup bug on this:

readonly attribute nsILocalFile file;
readonly attribute nsISimpleEnumerator files;

we should use files for both, no matter the mode.  Or maybe remove "file" and 
add a helper, "firstFile" or something.  I haven't thought it out yet.

Comment 21

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

platform specific bugs:

http://bugzilla.mozilla.org/show_bug.cgi?id=167149 [mac]
http://bugzilla.mozilla.org/show_bug.cgi?id=167150 [os/2]
http://bugzilla.mozilla.org/show_bug.cgi?id=167151 [cocoa]

for the xp file picker:
http://bugzilla.mozilla.org/show_bug.cgi?id=167152 [xpfilepicker]

Comment 22

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

alecf suggested I post to n.p.m.xpfe for help wanted about the xpfilepicker.

news://news.mozilla.org:119/alb0td$[e-mail]

Comment 23

[Sergei Dolgov]

Why we need both mFile and mFiles? For backward compatibility?

Comment 24

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

>Why we need both mFile and mFiles? For backward compatibility?

see http://bugzilla.mozilla.org/show_bug.cgi?id=121122#c20

"I'll also log a cleanup bug on this:

readonly attribute nsILocalFile file;
readonly attribute nsISimpleEnumerator files;"

Until that's cleaned up, we need both.

Comment 25

[Sergei Dolgov]

http://bugzilla.mozilla.org/show_bug.cgi?id=121122#c20
"we should use files for both, no matter the mode.  Or maybe remove "file" and 
add a helper, "firstFile" or something.  I haven't thought it out yet"

That's good idea. BeOS filePicker implementation already gets and saves list of
file entries (that's API for native FilePanel)- (only that it has restriction
for selection number set), and till now it used for mFileOpen just
"List->ElementAt(0);"
Only bug in BeOS part of patch i found is typo - mMode = modeOpenMultiple

@@ -80,7 +80,7 @@
     node_flavors = B_DIRECTORY_NODE;
     panel_mode = B_OPEN_PANEL;
   }
-  else if (mMode == modeOpen) {
+  else if (mMode == modeOpen || mMode = modeOpenMultiple) {
     node_flavors = B_FILE_NODE;
     panel_mode = B_OPEN_PANEL;
   }
should be comparison ==, not assignment =

Comment 26

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

that for the = vs == catch, new patch coming.

forgot to log the [beos] bug:
see http://bugzilla.mozilla.org/show_bug.cgi?id=167200

Comment 27

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

"that for the = vs == catch, new patch coming."

thanks for catch, I mean.

Comment 28

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

Attachment: updated patch, fixed problem in beos

wait, I forgot to attach them implementation for nsFilePicker.js

Comment 29

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

Attachment: implement xpfile picker, whoops

Comment 30

[Boris Zbarsky [:bzbarsky]]

The impl of the "files" getter in nsFilePicker.js needs to return an enumerator,
not an array...  Or does XPConnect automagically deal here?

Comment 31

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

Attachment: patch

Comment 32

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

"The impl of the "files" getter in nsFilePicker.js needs to return an 
enumerator, not an array...  Or does XPConnect automagically deal here?"

I attached the wrong patch, sorry bz.  see the latest patch.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 98231 [details] [diff] [review]
patch

Windows code:

+	   fileStr += current;

Does the dirname include the trailing '\' then?

This part of the code could be more efficient if it cached the strlen() result
and used nsDependentCStrings instead of nsCAutoStrings (and just passed
"dirName + currentFile" to InitWithNativePath).  I don't know whether we care
about not scanning the file strings twice multiple (3) times.

The single-file case assumes that the string still has two trailing nulls, even
though the filename and dir are a single chunk.  Is that correct? (seems
logical, but...)

filepicker.js:

+  mHasMore: true,

Wouldn't it make more sense to init this to "false" and set to true when mFile
is set?

Other than those two items, looks great!

Comment 34

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

>+	   fileStr += current;
>Does the dirname include the trailing '\' then?

yes, so I added a comment explaining that.

>The single-file case assumes that the string still has two trailing nulls, even
>though the filename and dir are a single chunk.  Is that correct? (seems
>logical, but...)

yes, it is correct.

new patch that covers the other issues coming...

Comment 35

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

Attachment: patch

Comment 36

[David :Bienvenu]

Comment on attachment 98247 [details] [diff] [review]
patch

sr=bienvenu

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 98247 [details] [diff] [review]
patch

sr=bzbarsky if you add that comment about the '\' on the dirName

Comment 38

[Boris Zbarsky [:bzbarsky]]

ugh.  more collisions... ;)  r=me then.

Comment 39

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

Comment on attachment 98247 [details] [diff] [review]
patch

r=bz

note to bz:

+	   // dirName contains a trailing slash
+	   rv = file->InitWithNativePath(nsDependentCString(dirName) +
nsDependentCString(current));
+	   NS_ENSURE_SUCCESS(rv,rv);

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 98247 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.2a

Comment 41

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

fixed.

now we need all the other widgets (and xpfilepicker) to be fixed.

Comment 42

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

sping off bug about removing some of the attributes and just 
using "nsISimpleEnumerator files" is bug 167282

Comment 43

[Sergei Dolgov]

As i noticed, this patch/bug was intended to allow multiple attachments in
mailnews, but it will be good to open multiple local files from FilePicker in
browser too (in tabs or windows - depending on user settings). But i suspect
that it is topic for another bug.

Comment 44

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

"As i noticed, this patch/bug was intended to allow multiple attachments in
mailnews, but it will be good to open multiple local files from FilePicker in
browser too (in tabs or windows - depending on user settings). But i suspect
that it is topic for another bug."

that's doable now, but it will only work on windows until the various platforms
implement support for modeOpenMultiple.

If there are browser bugs where multiple file open makes sense, feel free to log
those RFEs.

Comment 45

[Sergei Dolgov]

Seth, can you explain me how can i test those modifications.
I implemented OpenMultiple handling in beos/nsFilePicker recently (instead
stub), changed *.js for mailnews, recompiled, but cannot see any option in
MailNews GUI which allows that feature :).

Comment 46

[scottputterman]

To test in mail, open a mail compose window (Ctrl+M).  Click the attach button.
 Select more than one file and click ok.  If you are able to do this and see
both files in the attachment pane of this window, then it worked.  Before this
fix, you could only select one file at a time.

Comment 47

[Sergei Dolgov]

added patch for BeOS:
http://bugzilla.mozilla.org/attachment.cgi?id=99180&action=view.

"- currently in this patch multipleOpen and Open in BeOS nsFilePicker are
exclusive, but i had version where even for multipleOpen first list item was
assigned also to mFile. Maybe that approach was better?"

Comment 48

[sairuh (rarely reading bugmail)]

rs vrfy. works fine for me in mail compose on win2k (2002.09.18.08 comm trunk).

filed bug 169539 for browser case.

Comment 49

[Miloslaw Smyk]

Ok, but what about Linux version of this bug?

Comment 50

[Boris Zbarsky [:bzbarsky]]

That's bug 167152; has a detailed description of what needs to be done and a
first-cut patch.