Extensions: Fix file test when checking dependencies by type

For "executable" files only existence was checked.
(Glib::file_test needs a single FileTest)

Apply this properly when checking script <command>s and xslt <file>s
- interpreted scripts and xslt files -> only check existence
- un-interpreted scripts -> check for executable file

For Windows workarounds are implemented to yield desirable behavior:
- as there is no executable bit, only check existence
- as executables usually come with an extensions, scan for those
  as well.

3 files changed, 64 insertions, 36 deletions

diff --git a/src/extension/dependency.cpp b/src/extension/dependency.cpp
index 1ea134c36..69cb3199d 100644
--- a/src/extension/dependency.cpp
+++ b/src/extension/dependency.cpp
@@ -22,7 +22,7 @@ namespace Extension {
 
 // These strings are for XML attribute comparisons and should not be translated;
 // make sure to keep in sync with enum defined in dependency.h
-gchar const * Dependency::_type_str[TYPE_CNT] = {
+gchar const * Dependency::_type_str[] = {
     "executable",
     "file",
     "extension",
@@ -30,7 +30,7 @@ gchar const * Dependency::_type_str[TYPE_CNT] = {
 
 // These strings are for XML attribute comparisons and should not be translated
 // make sure to keep in sync with enum defined in dependency.h
-gchar const * Dependency::_location_str[LOCATION_CNT] = {
+gchar const * Dependency::_location_str[] = {
     "path",
     "extensions",
     "inx",
@@ -39,17 +39,19 @@ gchar const * Dependency::_location_str[LOCATION_CNT] = {
 
 /**
     \brief   Create a dependency using an XML definition
-    \param   in_repr     XML definition of the dependency
-    \param   extension   Reference to the extension requesting this dependency
+    \param   in_repr       XML definition of the dependency
+    \param   extension     Reference to the extension requesting this dependency
+    \param   default_type  Default file type of the dependency (unless overridden by XML definition's "type" attribute)
 
     This function mostly looks for the 'location' and 'type' attributes
     and turns them into the enums of the same name.  This makes things
     a little bit easier to use later.  Also, a pointer to the core
     content is pulled out -- also to make things easier.
 */
-Dependency::Dependency (Inkscape::XML::Node * in_repr, const Extension *extension)
+Dependency::Dependency (Inkscape::XML::Node * in_repr, const Extension *extension, type_t default_type)
     : _repr(in_repr)
     , _extension(extension)
+    , _type(default_type)
 {
     Inkscape::GC::anchor(_repr);
 
@@ -144,9 +146,15 @@ bool Dependency::check ()
         case TYPE_EXECUTABLE:
         case TYPE_FILE: {
             Glib::FileTest filetest = Glib::FILE_TEST_EXISTS;
+
+#ifndef _WIN32
+            // There's no executable bit on Windows, so this is unreliable
+            // glib would search for "executable types" instead, which are only {".exe", ".cmd", ".bat", ".com"},
+            // and would therefore miss files without extension and other script files (like .py files)
             if (_type == TYPE_EXECUTABLE) {
-                filetest |= Glib::FILE_TEST_IS_EXECUTABLE;
+                filetest = Glib::FILE_TEST_IS_EXECUTABLE;
             }
+#endif
 
             std::string location(_string);
             switch (_location) {
@@ -183,6 +191,8 @@ bool Dependency::check ()
                 /* The default case is to look in the path */
                 case LOCATION_PATH:
                 default: {
+                    // TODO: we can likely use g_find_program_in_path (or its glibmm equivalent) for executable types
+
                     gchar * path = g_strdup(g_getenv("PATH"));
 
                     if (path == nullptr) {
@@ -219,21 +229,33 @@ bool Dependency::check ()
                             return true;
                         }
 
-                        // give it a 2nd try with ".exe" added
-                        Glib::ustring final_name_exe = final_name + ".exe";
-                        if (Glib::file_test(final_name_exe, filetest)) {
-                            g_free(orig_path);
-                            _absolute_location = final_name;
-                            return true;
-                        }
-
-                        // and a 3rd try with ".cmd" added (mainly for UniConvertor)
-                        Glib::ustring final_name_cmd = final_name + ".cmd";
-                        if (Glib::file_test(final_name_cmd, filetest)) {
-                            g_free(orig_path);
-                            _absolute_location = final_name;
-                            return true;
+#ifdef _WIN32
+                        // Unfortunately file extensions tend to be different on Windows and we can't know
+                        // which one it is, so try all extensions glib assumes to be executable.
+                        // As we can only guess here, return the version without extension if either one is found,
+                        // so that we don't accidentally override (or conflict with) some g_spawn_* magic.
+                        if (_type == TYPE_EXECUTABLE) {
+                            static const std::vector<std::string> extensions = {".exe", ".cmd", ".bat", ".com"};
+
+                            std::string extension;
+                            size_t index = final_name.find_last_of(".");
+                            if (index != std::string::npos) {
+                                extension = final_name.substr(index);
+                            }
+
+                            if (extension.empty() ||
+                                    std::find(extensions.begin(), extensions.end(), extension) == extensions.end())
+                            {
+                                for (auto extension : extensions) {
+                                    if (Glib::file_test(final_name + extension, filetest)) {
+                                        g_free(orig_path);
+                                        _absolute_location = final_name;
+                                        return true;
+                                    }
+                                }
+                            }
                         }
+#endif
                     }
 
                     g_free(orig_path);
diff --git a/src/extension/dependency.h b/src/extension/dependency.h
index 1b4d5b819..e2c279112 100644
--- a/src/extension/dependency.h
+++ b/src/extension/dependency.h
@@ -23,17 +23,8 @@ class Extension;
             are different things that can be done in a dependency, and
             this class takes care of all of them. */
 class Dependency {
-    static constexpr const char *UNCHECKED = "---unchecked---";
-    
-    /** \brief  The XML representation of the dependency. */
-    Inkscape::XML::Node * _repr;
-    /** \brief  The string that is in the XML tags pulled out. */
-    const gchar * _string = nullptr;
-    /** \brief  The description of the dependency for the users. */
-    const gchar * _description = nullptr;
-    /** \brief  The absolute path to the dependency file determined while checking this dependency. */
-    std::string  _absolute_location = UNCHECKED;
 
+public:
     /** \brief  All the possible types of dependencies. */
     enum type_t {
         TYPE_EXECUTABLE, /**< Look for an executable */
@@ -41,9 +32,7 @@ class Dependency {
         TYPE_EXTENSION,  /**< Make sure a specific extension is loaded and functional */
         TYPE_CNT         /**< Number of types */
     };
-    /** \brief  Storing the type of this particular dependency. */
-    type_t _type = TYPE_FILE;
-
+    
     /** \brief  All of the possible locations to look for the dependency. */
     enum location_t {
         LOCATION_PATH,       /**< Look in the PATH for this dependency  - historically this is the default
@@ -54,6 +43,21 @@ class Dependency {
         LOCATION_ABSOLUTE,   /**< This dependency is already defined in absolute terms */
         LOCATION_CNT         /**< Number of locations to look */
     };
+    
+private:
+    static constexpr const char *UNCHECKED = "---unchecked---";
+    
+    /** \brief  The XML representation of the dependency. */
+    Inkscape::XML::Node * _repr;
+    /** \brief  The string that is in the XML tags pulled out. */
+    const gchar * _string = nullptr;
+    /** \brief  The description of the dependency for the users. */
+    const gchar * _description = nullptr;
+    /** \brief  The absolute path to the dependency file determined while checking this dependency. */
+    std::string  _absolute_location = UNCHECKED;
+
+    /** \brief  Storing the type of this particular dependency. */
+    type_t _type = TYPE_FILE;
     /** \brief  The location to look for this particular dependency. */
     location_t _location = LOCATION_PATH;
 
@@ -68,7 +72,7 @@ class Dependency {
     const Extension *_extension;
 
 public:
-    Dependency (Inkscape::XML::Node *in_repr, const Extension *extension);
+    Dependency (Inkscape::XML::Node *in_repr, const Extension *extension, type_t type=TYPE_FILE);
     virtual ~Dependency ();
     bool check();
     const gchar* get_name();
diff --git a/src/extension/extension.cpp b/src/extension/extension.cpp
index e0f4ed3fe..fedd0facc 100644
--- a/src/extension/extension.cpp
+++ b/src/extension/extension.cpp
@@ -125,14 +125,16 @@ Extension::Extension(Inkscape::XML::Node *in_repr, Implementation::Implementatio
         } else if (!strcmp(chname, "script")) { // TODO: should these be parsed in their respective Implementation?
             for (Inkscape::XML::Node *child = child_repr->firstChild(); child != nullptr; child = child->next()) {
                 if (child->type() == Inkscape::XML::ELEMENT_NODE) { // skip non-element nodes (see LP #1372200)
-                    _deps.push_back(new Dependency(child, this));
+                    const char *interpreted = child->attribute("interpreter");
+                    Dependency::type_t type = interpreted ? Dependency::TYPE_FILE : Dependency::TYPE_EXECUTABLE;
+                    _deps.push_back(new Dependency(child, this, type));
                     break;
                 }
             }
         } else if (!strcmp(chname, "xslt")) { // TODO: should these be parsed in their respective Implementation?
             for (Inkscape::XML::Node *child = child_repr->firstChild(); child != nullptr; child = child->next()) {
                 if (child->type() == Inkscape::XML::ELEMENT_NODE) { // skip non-element nodes (see LP #1372200)
-                    _deps.push_back(new Dependency(child, this));
+                    _deps.push_back(new Dependency(child, this, Dependency::TYPE_FILE));
                     break;
                 }
             }