[PATCH] Fix File::revealToUser() and startAsProcess()


#1

Hello Jules!

I have a few comments related to document/folder opening on Linux if you’d care to take a look:

File::revealToUser() is basically broken on Linux. revealToUser() ends up calling Process::openDocument for the parent directory of the file, or the file itself if it’s in fact a directory. However, Process::openDocument() will try to execute the directory using /bin/sh, which doesn’t work.

Similar problems occur when calling File::startAsProcess(). Process::openDocument doesn’t check to see if a file is really a regular executable file before trying to execute it. So it ends up trying to execute regular documents instead of launching them via helper applications as expected (xdb-open).

Below is an attempt at fixing these issues. Checking for executability, opening directories correctly + adding the two common google chrome browsers to the search list. It works well for me and I hope you might consider including it in your codebase. This is a patch against current git as of today:

diff --git a/modules/juce_core/native/juce_linux_Files.cpp b/modules/juce_core/native/juce_linux_Files.cpp
index 7cc17d2..c52ba71 100644
--- a/modules/juce_core/native/juce_linux_Files.cpp
+++ b/modules/juce_core/native/juce_linux_Files.cpp
@@ -306,12 +306,19 @@ bool Process::openDocument (const String& fileName, const String& parameters)
     String cmdString (fileName.replace (" ", "\\ ",false));
     cmdString << " " << parameters;
 
+    bool canExecuteFile = false;
+    struct stat buf;
+    if (stat(fileName.toUTF8(), &buf) == 0 && S_ISREG(buf.st_mode) && access(fileName.toUTF8(), X_OK) == 0)
+        canExecuteFile = true;
+
     if (URL::isProbablyAWebsiteURL (fileName)
          || cmdString.startsWithIgnoreCase ("file:")
-         || URL::isProbablyAnEmailAddress (fileName))
+         || URL::isProbablyAnEmailAddress (fileName)
+         || File(fileName).isDirectory()
+         || canExecuteFile == false)
     {
         // create a command that tries to launch a bunch of likely browsers
-        const char* const browserNames[] = { "xdg-open", "/etc/alternatives/x-www-browser", "firefox", "mozilla", "konqueror", "opera" };
+        const char* const browserNames[] = { "xdg-open", "/etc/alternatives/x-www-browser", "firefox", "mozilla", "konqueror", "opera", "google-chrome", "chromium-browser" };
 
         StringArray cmdLines;

Thank you!


#2

Thanks! I’ll take a look at this today…


#3

Nice to see you fix this in latest git, thank you!


#4

Jules, thinking more about this:

File::startAsProcess (http://www.rawmaterialsoftware.com/api/classFile.html#ab53026180cda413853c62a8a9aedcd00) has a String argument “parameters”. The same goes for Process::openDocument(). Wouldn’t it make sense to make them StringArray instead, and change Process::openDocument() to use ChildProcess::start? Get rid of /bin/sh all together?

juce$ grep -r “bin/sh” modules/
modules/juce_core/native/juce_linux_Files.cpp: const char* const argv[4] = { “/bin/sh”, “-c”, cmdString.toUTF8(), 0 };
modules/juce_core/native/juce_mac_Files.mm: const char* const argv[4] = { “/bin/sh”, “-c”, pathAndArguments.toUTF8(), 0 };

IMHO, it makes sense to use exec*() instead of going the extra way around a shell.


#5

I’m not sure that’s quite right - the startAsProcess method can also be used on documents, so I think it’s relying on the shell to open it in the correct app.


#6

I believe that is a misunderstanding. The shell, /bin/sh, has no such intelligence. It cannot choose correct documents. That logic is already in Process::openDocument(). Notably, xdg-open is used for this purpose.

Basically, this is what happens in the current implementation:
[list]
[]A file and optional parameters are sent to Process::openDocument. The file can be a www-url, a file:// url, a regular file path. The file could be a document of some sort which requires a helper app to open it. It could also be an executable which should/could be executed directly.[/]
[]Process::openDocument() does some checking to try and decide what to do with the file. It compiles a command string consisting of an optional helper app, the file itself and the optional arguments.[/]
[]Process::openDocument forks, and then execs /bin/sh -c with the command string.[/]
[]/bin/sh -c will just take the command string and exec it again. It does nothing useful in this context. It adds a layer of complexity requiring escaping/quoting of arguments. It makes the program considerably slower. Nothing else.[/][/list]
Letting Juce do the exec() directly will make no change to the logic, but will improve performance and remove the need for careful escaping of arguments.

I’ll be happy to send you some suggested code, but first it’s necessary that we see this the same way. Please believe me when I say that nothing relies on /bin/sh in the context of running commands/helper-apps from an already running process. The exec*() family of system calls are there for this very purpose.


#7

Although, correcting myself, ChildProcess is not the class to use, since it will keep waiting for the process and expect it to terminate with or before the Juce app.


#8

Ok, my brain is too full of other things right now to think about this properly, but if you want to suggest some code, I’d be happy to take a look, thanks!