58311 - RFE: add "create directory" button to the file picker.

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

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

just like the native windows file picker, it would be nice to have a "create
directory" button.

Comment 1

[Boris Zbarsky [:bzbarsky]]

*** Bug 97660 has been marked as a duplicate of this bug. ***

Comment 2

[Ferdinand]

*** Bug 99833 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

*** Bug 104006 has been marked as a duplicate of this bug. ***

Comment 4

[R.K.Aa.]

*** Bug 109617 has been marked as a duplicate of this bug. ***

Comment 5

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 6

[James Paige]

Nominating for 1.0

The need to open an xterm or a file manager every time I want to save a file
from mozilla into a new directory is getting to be quite an inconvenience.

Comment 7

[Oliver C.]

Agreed.

Comment 8

[R.K.Aa.]

*** Bug 125212 has been marked as a duplicate of this bug. ***

Comment 9

[[e-mail]]

bug http://bugzilla.mozilla.org/show_bug.cgi?id=125133 has some discussion
and a proposal for an enhanced filepicker

Comment 10

[David Grant]

I agree that this bug should have its target moved to Mozilla 1.0.0  However, I 
think 1.0.0 release is coming fairly soon, so I guess Moz 1.0.1 will have to do.

Comment 11

[[e-mail]]

Brian since this is assigned to you, do you already have anything for this? 
Adding the button itself
isnt that of a problem (I already did it, see rightmost icon in attachment
#75569 [details]) but making it work is a bit harder.

My problem is that I dont know how to find out which XPCOM service to use
and how to use it correctly (including error handling) for creating a file.
Is it @mozilla.org/file/local;1 nsILocalFile, method create? What are the
two uint32 arguments (umask?). What is the correct protocol to do it -
first doing an  initWithUnicodePath  and then the create is sufficient?
What errors can occur for either method? 

Comment 12

[[e-mail]]

I have a patch for this, can anybody assign this to me? 

Comment 13

[André Dahlqvist]

Reassign bug to [e-mail] on his wish.

Comment 14

[[e-mail]]

Attachment: Gifs needed for the subsequent patch

These GIFs are stored in a zip archive complete with the necessary path
information relative to the chrome subdir in a binary distribution.

Comment 15

[[e-mail]]

Attachment: First patchmaker "chromediff" patch

This patch seems to work quite fine and allows to specify a relative or 
absolute path for the new directory to create, embedded 
".." or a "~" in the first position.

There are two things still missing: proper display of error details
if something goes wrong, and using the umask for the properties
of the newly created dir. I still need to find out how to do this.
Help welcome.

Comment 16

[[e-mail]]

I need the folloing help to complete the patch: 
* when catching XPCOM errors in javascript, how can I get a user friendly
  error message for the error? Can I access it directly from the object?
  Is there a localized repository of messages? Do I have to do it manually - how?
* how can I find out the umask of the user in the javascript code? This is 
  needed to find the correct properties for the new directory

Comment 17

[[e-mail]]

Attachment: Better chromediff patch with problems solved

OK, the umask gets observed by the XPCOM service, so 0755 are the correct 
permission flags.
Concerning the exceptions: I browsed through several dozens of other 
javascript files and found that nobody actually analyzes the exception object,
so I just do a special
treatment for the case that a file with the same path as the new dir already
exists and handle all other cases with a generic error message.
PS: can I get authorization to obselete my own obsolete patches? :)

Comment 18

[[e-mail]]

Attachment: patch in cvs diff -u format

This is the patch including changes to the jar.mn files in cvs diff -u 
format for the srouce tree.

Ready for review.

Comment 19

[[e-mail]]

Attachment: ZIP archive of the two gifs for source tree

This zip archive contains the two icons for modern and classic, 
with path info suitable to be expanded from the "mozilla" dir
of the source tree. Needed in addition to the "cvs diff -u format" patch.

Comment 20

[[e-mail]]

BTW answering my own question 1 in comment #16: nsILocalFile.create
doesnt return any useful information about the failure in the exception
object when it fails, so there is no point in doing that.
The current, sufficient as I believe, solution is to just
show a generic error message and to only deal with _existing_ files
before even trying to create the dir.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 78382 [details] [diff] [review]
patch in cvs diff -u format 

>+var newDirName = { value: "" };

could this be prefixed with "g" like "gNewDirName"?  Also, put a comment here
explaining why this is a global

>+  var dialogTitle =
>+    gFilePickerBundle.getFormattedString("promptNewDirTitle", [""]);
>+  var dialogMsg =
>+    gFilePickerBundle.getFormattedString("promptNewDirMessage", [""]);

It looks like you want to just use |getString|:

var dialogTitle =
  gFilePickerBundle.getString("promptNewDirTitle");

You may want to catch exceptions from the getString calls and set the title
and message to sane defaults....

>+    var file = sfile.clone().QueryInterface(nsILocalFile);
>+    if (!file) {
>+      dump("Couldn't clone\n");
>+      return false;
>+    }

Um... clone() should throw an exception any time it can't clone.  You want
to catch that exception and show an error dialog of some sort, then return

>+    if (input[0] == '/')   /* an absolute path was entered */
>+      file.initWithUnicodePath(input);
>+    else if ((input.indexOf("/../") > 0) ||
>+             (input.substr(-3) == "/..") ||
>+             (input.substr(0,3) == "../") ||
>+             (input == "..")) {
>+      /* appendRelativePath doesn't allow .. */
>+      try {
>+        file.initWithUnicodePath(file.unicodePath + "/" + input);

This is duplicating a bunch of code we have elsewhere.	Any chance of
refactoring into a function called from both places?

>+      } catch (e) {
>+        showErrorDialog("errorCreateNewDirTitle",
>+                        "errorCreateNewDirMessage",
>+                        file);
>+        return false;
>+      }

It looks like you could move the exception handling out to be around that
if statement, no?  That way you would not need two identical catch clauses...

>+      file.create(1,0755);

This should use a nice name for "1", like 
Components.interfaces.nsIFile.DIRECTORY_TYPE

It may be a good idea to make sure that the closest existing
ancestor of |file| is writable, as we do for the modeSave case elsewhere
in the filepicker.

>+    file.normalize(); // we need to do this in case a .. was used

>+errorNewDirDoesExistMessage=A file with name %S already exists, directory cannot be created.

How about "A file named %S ...." instead?

>+promptNewDirMessage=Name of new directory to create:

How about "Enter Directory Name:" ?

Comment 22

[[e-mail]]

Thanks for the valuable comments, just a few questions:
  - I didnt catch exception for getString because its not done anywhere else
    in existing code :) - is the principle here to catch exceptions for
    *all* XPCOM methods used? Even if failure will not depend on user input?
    How should I deal with such an error - I might alert the user using a
    non localized constant string, but this might fail too and what then?

  - the dump(...) stuff was also copied from other code in filepicker.js
    I dont even know what it does, but i gathered it is used for error 
    conditions that cannot easily communicated to the user ("general failuer
    in nsIFile.clone()"??). Is there already a bunch of localzed error messages
    for this somewhere for stuff like "internal error, and you can do nothing
    about it"? :)

  - the problem with testing for writability is that when just saving
    into a selected dir, we can be sure the dir exists and hence simply
    check writability. For creating a new dir the case is different:
    The create new dir allows arbitrary paths to be created, i.e. including
    many nested dirs and specified in all kinds of strange ways using "..".
    Since isWritable() needs an existing dir, we would have to loop 
    through all possible ancestors until we find one that exists and
    then check if that one is writable. 

  - Do you think the different ways of specifying a new dir that are
    possible should somehow be mentioned in the new dir dialog?
    Otherwise we would have a wonderful feature that isnt used to full
    extend because nobody knows it even works :)
    However I am not native english so I have a bit of difficulty finding
    a good wording for that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> I didnt catch exception for getString because its not done anywhere else
> in existing code :)

Well... it's done in places that really care.  There aren't many of them.
Catching this exception is only useful if the user has an incorrect localization
that does not have the strings you're trying to get.  It's nice to handle that
case, but you're right -- the browser will be unusable for such a user anyway...

> the dump(...) stuff was also copied from other code in filepicker.js
> I dont even know what it does,

It prints text to the console, assuming dump() is enabled in debug prefs.

> Is there already a bunch of localzed error messages for this somewhere for
> stuff like "internal error, and you can do nothing about it"? :)

Not that I know of....  Looking over this code, I guess it's ok to leave it
as-is (though still make it an exception-handler, not a check for the result of
clone()).

> we would have to loop through all possible ancestors until we find one that
> exists and then check if that one is writable. 

Yep.  See the code at
http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/res/content/filepicker.js#300
for what you want to do there...

> Do you think the different ways of specifying a new dir that are possible
> should somehow be mentioned in the new dir dialog?

Absolutely not.  :)  That would make the dialog incredibly cluttered, imo.

Thanks for doing this, by the way.

Comment 24

[[e-mail]]

Hmm looking at the code that already exists near 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/res/content/filepicker.js#205
I realize that new directories already where created silently
when entered in the file name field. 
So to make the interface clearer now that a new dir button exists, there
are several ways to handle this:
 1 - leave the old way to silently create dirs unchanged
 2 - keep the old way, but show a warning if a new dir would need to be
   created, since this could be due to a typing error 
 3 - only allow creation of new dirs using the button

I think there is a strong argument against 1): it allows to silently 
save to the wrong place because of a typo without the user even knowing what
happened.
I'd prefer 3 over 2, since it seperates the different tasks of creating
and specifying and existsing path in the interface and probably also makes the
code easier to understand and maintain.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Actually, I'd go with choice #1.  But then again, that's the one I think is the
most convenient... The whole thing of entering subdirectory names in the
filename field is completely undocumented and is not something anyone but power
users would be relying on.....

Any chance of getting one of the UI experts to comment here?  ccing them in the
hopes that they'll actually respond.

On another note I was thinking about the directory-creation behavior we should
implement from the button, and I think it should be the following:

modeSave:  Creating a directory calls goToDirectory() on it
modeOpen:  Create a directory buttons is invisible (or at least disabled)
modeGetFolder:  Creating a directory selects it in the outliner (I'm not sure
                how to do it offhand)

Comment 26

[Peter Trudelle]

In case they don't respond: If users could still accidentally create
subdirectories by adding the directory separator charactor for the platform
[/,\,:], then a warning would be appropriate. Adding a directory is uncommon
enough, having such a character already in the filename common enough, and the
consequences of doing so accidentally severe enough to require being a bit more
explicit, IMO.

Comment 27

[[e-mail]]

What do I have to do in Mozilla to test the filepicker with mode 
getfolder? 

I agree with Peter to show a warning, since it is quite easy to
accidently create a new dir when mistyping a name - especially on some
non-US keyboards where the slash is on a shifted number or letter key.
However I believe that filepicker.js is used for Linux only, so the
only possible directory seperator is the slash. Is this correct?
I believe Windows and Mac use their native filepickers, but is there
any other non-*nix OS this gets ported to where filepicker.js/xul is used?
(how about OS/2, OpenVMS?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> What do I have to do in Mozilla to test the filepicker with mode 
> getfolder?

Edit > Preferences > Advanced > Cache > click the "Choose Folder" button is a
good way.

> I agree with Peter to show a warning

Ok.  That makes sense.

> However I believe that filepicker.js is used for Linux only

No, but at the moment it basically assumes that a "/" is the directory separator
(see, eg,
http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/res/content/filepicker.js#221
or
http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/res/content/filepicker.js#214).
VMS _does_ use the XP filepicker, but "/" is the directory separator there. 
OS/2 and BeOS use native filepickers.

Ideally we'd like the XP filepicker to be XP, but that would require an XP way
to distinguish absolute and relative paths (and a fix to the Unix version of
appendRelativePath).

None of which matters for purposes of this, does it?  All you have to do if put
up a warning on modeSave if the user clicks "OK" and the parent of |file| does
not exist.

Comment 29

[Brendan Eich [:brendan]]

I'd like this for mozilla1.0 -- will there be a new patch attached soon?

/be

Comment 30

[[e-mail]]

Sorry for the delay, too much work. 
Concerning the behavior for modeGetFolder, I dont think just selecting the 
directory is possible. 
Also I found that there are some bugs or at least usability issues with
that mode: 
  - clicking on a dir will select it and put the name in the 
    "File name" entry field
  - double clicking on a dir will change into it and still put
    the name in the entry field - that is bad
  - clearing the entry field will deactivate the select button - thats
    unintuitive since one would expect that the current dir will then
    be picked. It also makes it impossible to select the root dir "/" or
    a directory whose parent has no read permission.
    It also unexpected since when the dialog comes up first the
    initial directory will be shown and cannot simply be "reselected".
  - navigating after a double click leaves the dirname in the entry field
  
I am fixing this so that the entry field gets cleared after each doubleclick
and the select button is also enabled when the entry field is empty,
so that the current directory can be selected.

Comment 31

[[e-mail]]

Attachment: New cvs -u patch

New patch that addresses the issues from comment #21 ff. 

Please review.

Changes:
- the fix for the strange behviour of modeGetFolder (comment #30) is included
- also fixes a problem that occurred before when a path was entered that
  had a ".." and didnt exist - in that case, it no error message was shown
  but nothing was done either
- the assumption that directories get silently created (comment #24) 
  was wrong - an error message was shown instead. 
  This behaviour has been kept.

btw, patch 78383 is obsoleted by that

Comment 32

[R.K.Aa.]

*** Bug 141739 has been marked as a duplicate of this bug. ***

Comment 33

[Boris Zbarsky [:bzbarsky]]

*** Bug 141910 has been marked as a duplicate of this bug. ***

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 81841 [details] [diff] [review]
New cvs -u patch

>-  okButton.disabled = true;
>+  if (filePickerMode != nsIFilePicker.modeGetFolder) {
>+    okButton.disabled = true;
>+  }

I assume this is supposed to address the issues you raise in comment 30?
I have to say that I would _not_ expect the "current directory" to be
picked if nothing is selected and nothing is entered in the text entry
field....

I cannot find a single Unix program which has a half-decent directory
picker (the one in NS4 is worse than ours) so I'd want to test how it works
on Mac or Windows before changing this behavior....

>-  okButton.disabled = !enable;
>+  if (filePickerMode != nsIFilePicker.modeGetFolder) {
>+    okButton.disabled = !enable;
>+  }

Same as above.

>-  okButton.disabled = (textInput.value == "");
>+  //!!!!!!
>+  if (filePickerMode != nsIFilePicker.modeGetFolder) {
>+    okButton.disabled = (textInput.value == "");
>+  }

And again.

>+    try {
>+      file.create(Components.interfaces.nsIFile.DIRECTORY_TYPE,0755);
>+    } catch (e) {
>+      showErrorDialog("errorCreateNewDirTitle",
>+                      "errorCreateNewDirMessage",
>+                      file);
>+    }
>+    file.normalize(); // ... in case ".." was used in the path
>+    gotoDirectory(file);
>+    // we remember and reshow a dirname if something goes wrong
>+    // so that errors can be corrected more easily. If all went well,
>+    // reset the default value to blank
>+    gNewDirName = { value: "" }; 
>+  }

Don't you only want to return from that catch block? Otherwise you execute
the code below that even if creation fails, which is not what you want at all.

>+  if (filePickerMode == nsIFilePicker.modeGetFolder) {
>+    textInput.value = "";
>+  }

See comments above.  :)

>+      dump("Couldn't appen path\n"+e);

"append"

>+errorCreateNewDirIsFileMessage=Path cannot be created, %S is a file
>+errorCreateNewDirPermissionMessage=Path cannot be created, %S not writable

You mean "Directory cannot be created, because %s is a file" ?
And likewise for "not writable".  "Path" never gets created, directories do.

>+errorPathProblemTitle=Error
>+errorPathProblemMessage=Problem with path %S

This message comes up for errors like failing to clone an nsIFile, no?	I
think a message like "An unknown error occured" is a lot more reasonable
than the one you give (the error you have places the blame on the path the
user typed in, whereas in reality that path may be just fine).

Comment 35

[[e-mail]]

Hmm I guess we need a comment from UI guys here. I must say I strongly
disagree with your disagreement on the behaviour of the filepicker in
directory slection mode: 
- what do you expect when pressing "select" and you are in directory x, other
  than that directory gets selected?
- note that this is the most consistent way to treat the case that
  you just want to repick whatever is currently picked: when the dialog
  comes up, the current directory is whatever the current value is 
  and "select" will just "reselect" it. Previously you would have to go
  up (if possible at all), mark the dir in the list and then select.
- note that it will not be possible to select directories in certain cases
  when using the old method
- note that there were other problems with the old method when the selected
  dirname remained in the entry field and you navigated up for instance

So if you dont like the way I do it, what do you suggest how to solve it?
Do you agree that previous behaviour was worse or not even that?

Comment 36

[Boris Zbarsky [:bzbarsky]]

> what do you expect when pressing "select" and you are in directory x, other
> than that directory gets selected?

If I have a directory selected in the file listing, I expect that selected
directory to be selected....  If I have nothing selected, I would expect the
button to be disabled.

Yes, this makes it impossible to select the root directory; I'm not quite sure
what to do about that problem yet....

> note that this is the most consistent way to treat the case that you just want
> to repick whatever is currently picked

I feel that our behavior on startup there is broken -- we should show the parent
of the dir that was picked last and hightlight the last picked dir.  That's not
this bug, though.

> note that it will not be possible to select directories in certain cases when
> using the old method

Except by typing the directory name in directly, yes.  That's true...

> note that there were other problems with the old method when the selected
> dirname remained in the entry field and you navigated up for instance

Agreed.  I just spent some time playing with this, and putting the dirname in
the entry field on doubleclick is just completely and utterly wrong.  So I
rescind my comment on the part of the patch where you set the entry field to ""
in modeGetFolder.... :)  That part of our behavior is completely bogus.

I have no useful suggestion on improving that yet... I'll try to find a Mac and
see what the Mac filepicker does in this case.

In either case, could we keep this patch focused on adding the "create
directory" button instead of trying to solve all the problems of modeGetFolder?
 That mode is much less common than modeSave, and modeSave is handled correctly
by this patch... My suggestion would be to fix the obvious issue you found with
modeGetFolder putting garbage in the text entry field, get this patch in, and
file a separate bug on the fact that modeGetFolder just sucks all around and
needs serious UI design applied to it (notice that I cced some UI people back on
April 14th in the hope that they'd say something...).

Comment 37

[Peter Trudelle]

This is hard to follow, impoossible for me from code alone.  Sounds like the
larger issues justify the need for a spec and some usability testing, however
informal.  

Comment 38

[Boris Zbarsky [:bzbarsky]]

Both of those would be very, very much appreciated.

Comment 39

[Peter Trudelle]

Obviously not a priority for this release, but I'll try to get some attention on
it for the point release.

Comment 40

[[e-mail]]

Attachment: corrected cvs -u patch

OK, I agree - lets take the modeGetFolder stuff to another bug. 
I have backed out everything in connection with this, 
except the clearing of the subdir name.

Corrected the issues with returning from exception, "append", and 
message texts and also updated so that the methods from nsIFile/nsILocalFile
that got obsoleted in the meantime are replaced.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 82379 [details] [diff] [review]
corrected cvs -u patch 

>+function newDir() {

This needs a |return true;| at the end (otherwise you get the "Function
does not always return a value" warning).

With that, r=bzbarsky

Comment 42

[[e-mail]]

Attachment: patch with correction from comment #41 added

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 82444 [details] [diff] [review]
patch with correction from comment #41 added

r=bzbarsky

Comment 44

[jag (Peter Annema)]

Comment on attachment 82444 [details] [diff] [review]
patch with correction from comment #41 added

Could you change your style of:

var promptService =
  Components.classes ...

to the style used in the rest of this file?

+  if (ret) {
+    file = processPath(gNewDirName.value);
+    if(!file) {
+      showErrorDialog("errorCreateNewDirTitle",
+		       "errorCreateNewDirMessage",

Please add a space between that |if| and the '('.

+      file.create(Components.interfaces.nsIFile.DIRECTORY_TYPE,0755);

Please add a space after that comma.

sr=jag with those nits fixed.

Comment 45

[[e-mail]]

Attachment: patch corrected according to comment #44

Comment 46

[Boris Zbarsky [:bzbarsky]]

Patch checked in on the trunk (I added the space before 0755 that you forgot, btw).

Ask for approval?

Comment 47

[Gilles Durys]

*** Bug 147555 has been marked as a duplicate of this bug. ***

Comment 48

[Kari Hurtta]

Small comment:

// from the current directory and whatever was entered
// in the entry field, try to make a new path. This
// uses "/" as the directory seperator, "~" as a shortcut
// for the home directory (but only when seen at the start
// of a path), and ".." to denote the parent directory.

Shorthand for user's home directory is usually ~/ and not ~

Consider path   ~foo/xxx

That on normally on unix shells means file xxx on user foo's
home directory and NOT file xxx on current user's subdirecory
foo.

Comment 49

[Boris Zbarsky [:bzbarsky]]

That's bug 147434 and this patch did not introduce this code, just moved it 
around.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Marking this fixed, since it is.  The button lives.

Comment 51

[Elmar Ludwig]

*** Bug 168166 has been marked as a duplicate of this bug. ***

Comment 52

[sairuh (rarely reading bugmail)]

"create directory" button now present on file picker for saving files. vrfy'd on
linux rh7.2, 2002.09.16.08.