[PATCH] Fix FileChooser and ChildProcess


#1

Hello Jules!

FileChooser with useOSNativeDialogBox=true doesn’t work correctly on Linux:

  • dialogBoxTitle is displayed with surrounding double quotes
  • dialogBoxTitle breaks if you send it embedded double quotes
  • initialFileOrDirectory is not working, ie the file or directory is not selected in the dialog

Reasons for this are:

  • zenity, (the gtk dialog program which is used to display the file dialog), doesn’t take a full path to --filename=. One must chdir to the relevant directory, then give only the file basename as argument. (cd /etc/; zenity --file-selection --filename=passwd).

  • FileChooser::showPlatformDialog escapes the title and file arguments with double quotes, as if preparing to send them to a shell like /bin/sh. However, the assembled command is sent to ChildProcess::start() which uses execvp() to execute the command. execvp() sends the arguments verbatim to the new process, hence they should not be escaped or quoted.

This example produces a file-not-found-error, since there is no file called “/etc/passwd” (including the quotes):

char* arglist_quoted[] = {"/bin/ls", "-l", "\"/etc/passwd\"", NULL};
execvp ("/bin/ls", arglist_quoted);

ChildProcess::start() also splits the command on whitespace, producing the argument list for execvp(). This causes an error either way: if arguments are quoted, the split is correct but the quotes remain wrongly in the argument; if arguments are not quoted they are broken when containing whitespace. ChildProcess::start() tries to emulate commandline parsing but fails.

Some ways to go about this:

  1. Call /bin/sh -c with the arguments, ie keep the quoting/escaping and pass it on to the shell
  2. Alter ChildProcess::start() to accept a StringArray. This avoids the split on whitespace, and all arguments are sent verbatim to the new process.
  3. Add an optional separator-argument: ChildProcess::start(const String &command, const String &commandSeparator=String(" \t")). Use commandSeparator to split the argument list, and trust the caller to choose something which doesn’t interfer with the actual arguments.
  4. A variable argument list ChildProcess::start("/bin/ls", “-l”, “/etc/passwd”), but this is frowned upon in C++.

Of the above, 1 is the worst choice. The caller should not have to worry about correct quoting and escaping for /bin/sh. I prefer 2, it is to me the cleanest approach. Compose your StringArray with the exact arguments needed - no quoting/escaping - and you’re done.

The patch below, to current git, overloads start() with a StringArray argument. It also modifies FileChooser to use it, and fixes the other errors mentioned above. Code like this will now work correctly:

$ touch "/tmp/the file"
FileChooser("test \"a b c\" ; --q ", File("/tmp/the file")).browseForFileToOpen();
diff --git a/modules/juce_core/native/juce_posix_SharedCode.h b/modules/juce_core/native/juce_posix_SharedCode.h
index 80c2faf..aa57438 100644
--- a/modules/juce_core/native/juce_posix_SharedCode.h
+++ b/modules/juce_core/native/juce_posix_SharedCode.h
@@ -1069,7 +1069,13 @@ bool ChildProcess::start (const String& command)
 {
     StringArray tokens;
     tokens.addTokens (command, true);
-    tokens.removeEmptyStrings (true);
+    return start (tokens);
+}
+
+bool ChildProcess::start (const StringArray& command)
+{
+    StringArray tokens (command);
+    tokens.removeEmptyStrings (false);
 
     if (tokens.size() == 0)
         return false;
diff --git a/modules/juce_core/threads/juce_ChildProcess.h b/modules/juce_core/threads/juce_ChildProcess.h
index 454a6c6..7aa94b5 100644
--- a/modules/juce_core/threads/juce_ChildProcess.h
+++ b/modules/juce_core/threads/juce_ChildProcess.h
@@ -56,6 +56,7 @@ public:
         occurs, the method will return false.
     */
     bool start (const String& command);
+    bool start (const StringArray& command);
 
     /** Returns true if the child process is alive. */
     bool isRunning() const;
diff --git a/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp b/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp
index 39da863..ce0fa1c 100644
--- a/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp
+++ b/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp
@@ -45,15 +45,33 @@ void FileChooser::showPlatformDialog (Array<File>& results,
                                       FilePreviewComponent* previewComponent)
 {
     const String separator (":");
-    String command ("zenity --file-selection");
+    StringArray command (String("zenity"));
+    command.add (String("--file-selection"));
 
-    if (title.isNotEmpty())         command << " --title=\"" << title << "\"";
-    if (file != File::nonexistent)  command << " --filename=\"" << file.getFullPathName () << "\"";
-    if (isDirectory)                command << " --directory";
-    if (isSave)                     command << " --save";
-    if (selectMultipleFiles)        command << " --multiple --separator=" << separator;
+    if (title.isNotEmpty())         command.add (String("--title=") + title);
+    if (isDirectory)                command.add (String("--directory"));
+    if (isSave)                     command.add (String("--save"));
+    if (selectMultipleFiles)
+    {
+       command.add (String("--multiple"));
+       command.add (String("--separator=") + separator);
+    }
 
-    command << " 2>&1";
+    File previousWorkingDirectory = File::getCurrentWorkingDirectory();
+    if (file.exists())
+    {
+        if (file.isDirectory())
+        {
+            file.setAsCurrentWorkingDirectory();
+        }
+        else
+        {
+            file.getParentDirectory().setAsCurrentWorkingDirectory();
+            command.add (String("--filename=") + file.getFileName());
+       }
+    }
+
+    command.add (String("2>&1"));
 
     ChildProcess child;
     if (child.start (command))
@@ -75,4 +93,6 @@ void FileChooser::showPlatformDialog (Array<File>& results,
 
         child.waitForProcessToFinish (60 * 1000);
     }
+
+    previousWorkingDirectory.setAsCurrentWorkingDirectory();
 }

#2

Thanks, that makes a lot of sense! I’ll have a look through this stuff asap!


#3

Great - When you’re done, I’ll add support for kdialog (QT/KDE) for the cases when zenity (GTK/Gnome) is not installed.


#4

Thanks, I checked something in earlier today.


#5

@grebneke: thanks for the recent linux fixes!
I don’t use juce for my own projects, but I do use some linux plugins made with it.

If you’re going to add kdialog support for native file-chooser, please consider that KDE users might have zenity installed but still prefer to use kdialog.
KDE sets the env variable “KDE_FULL_SESSION=true”, so if that is set I believe kdialog should be used (it’s very-very likely to be available if that env var is set).


#6

[quote=“falkTX”]If you’re going to add kdialog support for native file-chooser, please consider that KDE users might have zenity installed but still prefer to use kdialog.
KDE sets the env variable “KDE_FULL_SESSION=true”, so if that is set I believe kdialog should be used (it’s very-very likely to be available if that env var is set).[/quote]
Great advice, thank you!

Here is an extended Linux native FileChooser with support for kdialog. Additionally, the logic for zenity is improved and fixes some corner cases. There are a lot of permutations when testing all options and possible arguments.

[code]diff --git a/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp b/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp
index 4721749…1541507 100644
— a/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp
+++ b/modules/juce_gui_basics/native/juce_linux_FileChooser.cpp
@@ -23,16 +23,25 @@

*/

-bool FileChooser::isPlatformDialogAvailable()
+static bool systemHasExecutable (const String& executable)
{
ChildProcess child;

  • const bool ok = child.start (“which zenity”)
  •                 && child.readAllProcessOutput().trim().isNotEmpty();
    
  • bool ok = child.start ("which " + executable)

  •    && child.readAllProcessOutput().trim().isNotEmpty();
    

    child.waitForProcessToFinish (60 * 1000);

  • return ok;
    }

+bool FileChooser::isPlatformDialogAvailable()
+{

  • if (systemHasExecutable (“zenity”))
  •    return true;
    
  • return systemHasExecutable (“kdialog”);
    +}

void FileChooser::showPlatformDialog (Array& results,
const String& title,
const File& file,
@@ -44,35 +53,75 @@ void FileChooser::showPlatformDialog (Array& results,
bool selectMultipleFiles,
FilePreviewComponent* previewComponent)
{

  • const String separator (":");
  • String separator;
    StringArray args;
  • args.add (“zenity”);

  • args.add ("–file-selection");

  • if (title.isNotEmpty()) args.add ("–title=" + title);

  • if (isDirectory) args.add ("–directory");

  • if (isSave) args.add ("–save");

  • if (selectMultipleFiles)

  • {

  •   args.add ("--multiple");
    
  •   args.add ("--separator=" + separator);
    
  • }

    const File previousWorkingDirectory (File::getCurrentWorkingDirectory());

  • if (file.isDirectory())

  • const bool isKdeFullSession = String (getenv(“KDE_FULL_SESSION”)).equalsIgnoreCase(“true”);
  • if (systemHasExecutable (“kdialog”) && (isKdeFullSession || ! systemHasExecutable (“zenity”)))
    {
  •    file.setAsCurrentWorkingDirectory();
    
  •    // use kdialog for KDE sessions or if zenity is missing
    
  •    args.add ("kdialog");
    
  •    if (title.isNotEmpty())         args.add ("--title=" + title);
    
  •    if (selectMultipleFiles)
    
  •    {
    
  •        separator = "\n";
    
  •        args.add ("--multiple");
    
  •        args.add ("--separate-output");
    
  •        args.add ("--getopenfilename");
    
  •    }
    
  •    else
    
  •    {
    
  •        if (isSave)                 args.add ("--getsavefilename");
    
  •        else if (isDirectory)       args.add ("--getexistingdirectory");
    
  •        else                        args.add ("--getopenfilename");
    
  •    }
    
  •    String startPath;
    
  •    if (file.exists() || file.getParentDirectory().exists())
    
  •    {
    
  •        startPath = file.getFullPathName();
    
  •    }
    
  •    else
    
  •    {
    
  •        startPath = File::getSpecialLocation (File::userHomeDirectory).getFullPathName();
    
  •        if (isSave)
    
  •            startPath += String("/" + file.getFileName());
    
  •    }
    
  •    args.add (startPath);
    

    }

  • else if (file.exists())
  • else
    {
  •    file.getParentDirectory().setAsCurrentWorkingDirectory();
    
  •    args.add ("--filename=" + file.getFileName());
    
  • }
  •    // zenity
    

+// FIXME: If neither kdialog or zenity is available - can we fall back on juce built-in dialog?

  •    args.add ("zenity");
    
  •    args.add ("--file-selection");
    
  •    if (title.isNotEmpty())         args.add ("--title=" + title);
    
  • args.add (“2>&1”);
  •    if (selectMultipleFiles)
    
  •    {
    
  •        separator = ":";
    
  •        args.add ("--multiple");
    
  •        args.add ("--separator=" + separator);
    
  •    }
    
  •    else
    
  •    {
    
  •        if (isDirectory)            args.add ("--directory");
    
  •        if (isSave)                 args.add ("--save");
    
  •    }
    
  •    if (file.isDirectory())
    
  •        file.setAsCurrentWorkingDirectory();
    
  •    else if (file.getParentDirectory().exists())
    
  •        file.getParentDirectory().setAsCurrentWorkingDirectory();
    
  •    else
    
  •        File::getSpecialLocation (File::userHomeDirectory).setAsCurrentWorkingDirectory();
    
  •    if (! file.getFileName().isEmpty())
    
  •        args.add ("--filename=" + file.getFileName());
    
  • }

    ChildProcess child;
    if (child.start (args))
    [/code]


#7

Cool stuff, thanks!


#8

Hi guys. Has this issue be resolved? Juce demo in the latest tip hangs when you try to open the file browser in the code editor demo. Ubuntu 12.10


#9

I cannot reproduce this, have tried both with kdialog and zenity.

Could you please:

  1. “which zenity”. If you find it, try running it manually to confirm that it works. “zenity --file-selection --title=‘Choose a new file’”
  2. start the demo, cause it to hang and then use something like “ps auxwf | grep zenity” and/or “ps auxwf | more” to try and find out if zenity is called at all.
  3. If you find zenity in the ps output, use strace to find out what it’s doing (along the lines of strace -s 200 -ff -p )

#10

So I was able to run zenity manually without any problems. So it’s probably not a zenity thing. I think it’s to do with the File class. I’ll start a new thread so as not to confuse…