Merge pull request #169 from end2endzone/feature-issue167

Merge branch feature-issue167.

Author: end2endzone

CHANGES

@@ -8,6 +8,7 @@ Changes for 0.10.0
 * Fixed issue #158: Compilation fails on Github Action: `CPack error : Problem running WiX.`.
 * Fixed issue #159: Unit test fails on Github Actions: TestPlugins.testServices().
 * Fixed issue #164: Fails to identify icon for HTML files.
+* Fixed issue #167: Improve the quality and accuracy of icon's fileextension attribute resolution (Icon::ResolveFileExtensionIcon()).
 
 
 Changes for 0.9.0

src/core/App.cpp

@@ -137,6 +137,16 @@ namespace shellanything
     return mRandom;
   }
 
+  void App::SetIconResolutionService(IIconResolutionService* instance)
+  {
+    mIconResolutionService = instance;
+  }
+
+  IIconResolutionService* App::GetIconResolutionService()
+  {
+    return mIconResolutionService;
+  }
+
   bool App::IsTestingEnvironment()
   {
     std::string process_path = ra::process::GetCurrentProcessPath();

src/core/App.h

@@ -30,6 +30,7 @@
 #include "IClipboardService.h"
 #include "IKeyboardService.h"
 #include "IRandomService.h"
+#include "IIconResolutionService.h"
 
 #include <string>
 
@@ -144,6 +145,21 @@ namespace shellanything
     /// <returns>Returns a pointer of the instance that is currently set. Returns NULL if no service is set.</returns>
     IRandomService* GetRandomService();
 
+    /// <summary>
+    /// Set the current application icon resolution service.
+    /// </summary>
+    /// <remarks>
+    /// If a service instance is already set, the caller must properly destroy the old instance.
+    /// </remarks>
+    /// <param name="instance">A valid instance of a the service.</param>
+    void SetIconResolutionService(IIconResolutionService* instance);
+
+    /// <summary>
+    /// Get the current application random service.
+    /// </summary>
+    /// <returns>Returns a pointer of the instance that is currently set. Returns NULL if no service is set.</returns>
+    IIconResolutionService* GetIconResolutionService();
+
     /// <summary>
     /// Test if application is loaded in a test environment (main's tests executable).
     /// </summary>
@@ -199,6 +215,7 @@ namespace shellanything
     IClipboardService* mClipboard;
     IKeyboardService* mKeyboard;
     IRandomService* mRandom;
+    IIconResolutionService* mIconResolutionService;
   };
 
 

src/core/CMakeLists.txt

@@ -18,6 +18,7 @@ set(SHELLANYTHING_CORE_HEADER_FILES ""
   ${CMAKE_SOURCE_DIR}/src/core/Environment.h
   ${CMAKE_SOURCE_DIR}/src/core/Icon.h
   ${CMAKE_SOURCE_DIR}/src/core/IObject.h
+  ${CMAKE_SOURCE_DIR}/src/core/IIconResolutionService.h
   ${CMAKE_SOURCE_DIR}/src/core/IKeyboardService.h
   ${CMAKE_SOURCE_DIR}/src/core/ILiveProperty.h
   ${CMAKE_SOURCE_DIR}/src/core/IClipboardService.h
@@ -62,6 +63,7 @@ add_library(sa.core SHARED
   IAttributeValidator.cpp
   Icon.cpp
   IObject.cpp
+  IIconResolutionService.cpp
   IKeyboardService.cpp
   ILiveProperty.cpp
   IClipboardService.cpp

src/core/IIconResolutionService.cpp

@@ -0,0 +1,38 @@
+/**********************************************************************************
+ * MIT License
+ *
+ * Copyright (c) 2018 Antoine Beauchamp
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *********************************************************************************/
+
+#include "IIconResolutionService.h"
+
+namespace shellanything
+{
+
+  IIconResolutionService::IIconResolutionService()
+  {
+  }
+
+  IIconResolutionService::~IIconResolutionService()
+  {
+  }
+
+} //namespace shellanything

src/core/IIconResolutionService.h

@@ -0,0 +1,77 @@
+/**********************************************************************************
+ * MIT License
+ *
+ * Copyright (c) 2018 Antoine Beauchamp
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *********************************************************************************/
+
+#ifndef SA_IICON_RESOLUTION_SERVICE_H
+#define SA_IICON_RESOLUTION_SERVICE_H
+
+#include "shellanything/export.h"
+#include "shellanything/config.h"
+#include "Icon.h"
+
+#include <stdint.h>
+
+namespace shellanything
+{
+  /// <summary>
+  /// Abstract icon resolution service class.
+  /// Used to decouple the core from the operating system.
+  /// </summary>
+  class SHELLANYTHING_EXPORT IIconResolutionService
+  {
+  public:
+    IIconResolutionService();
+    virtual ~IIconResolutionService();
+
+  private:
+    // Disable and copy constructor, dtor and copy operator
+    IIconResolutionService(const IIconResolutionService&);
+    IIconResolutionService& operator=(const IIconResolutionService&);
+  public:
+
+    /// <summary>
+    /// Resolve an icon's file extension to the system icon for this file extension.
+    /// </summary>
+    /// <param name="icon">The icon that have its fileextension's attribute set to resolve.</param>
+    /// <returns>Returns true if the operation is successful. Returns false otherwise.</returns>
+    virtual bool ResolveFileExtensionIcon(Icon & icon) = 0;
+
+    /// <summary>
+    /// Check if the given file extension have resolved before.
+    /// </summary>
+    /// <param name="file_entension">The file extension to check.</param>
+    /// <returns>Returns true if the file extension has previously resolve to a valid icon. Returns false otherwise.</returns>
+    virtual bool HaveResolvedBefore(const std::string& file_extension) const = 0;
+
+    /// <summary>
+    /// Check if the given file extension have previously failed to resolve.
+    /// </summary>
+    /// <param name="file_entension">The file extension to check.</param>
+    /// <returns>Returns true if the file extension has previously failed to resolve. Returns false otherwise.</returns>
+    virtual bool HaveFailedBefore(const std::string& file_extension) const = 0;
+
+  };
+
+} //namespace shellanything
+
+#endif //SA_IICON_RESOLUTION_SERVICE_H

src/core/Icon.cpp

@@ -23,19 +23,16 @@
  *********************************************************************************/
 
 #include "Icon.h"
-#include "SelectionContext.h"
-#include "PropertyManager.h"
-#include "Win32Registry.h"
+#include "App.h"
 #include "LoggerHelper.h"
+#include "PropertyManager.h"
+#include "SelectionContext.h"
 #include "SaUtils.h"
 
 #include "rapidassist/strings.h"
-#include "rapidassist/environment.h"
 
 namespace shellanything
 {
-  Icon::FileExtensionSet Icon::mUnresolvedFileExtensions;
-
   Icon::Icon() :
     mIndex(0) // As per documentation, "If the index is not specified, the value 0 is used." See Issue #17, #150 and #155.
   {
@@ -89,56 +86,30 @@ namespace shellanything
 
   void Icon::ResolveFileExtensionIcon()
   {
-    //is this menu have a file extension ?
+    IIconResolutionService* icon_resolution_service = App::GetInstance().GetIconResolutionService();
+    if (icon_resolution_service == NULL)
+    {
+      SA_LOG(ERROR) << "No Icon Resolution service configured for resolving file extensions to icon.";
+      return;
+    }
+
     shellanything::PropertyManager& pmgr = shellanything::PropertyManager::GetInstance();
     std::string file_extension = pmgr.Expand(mFileExtension);
-    if (!file_extension.empty())
+
+    const bool have_resolved_before = icon_resolution_service->HaveResolvedBefore(file_extension);
+    const bool have_failed_before = icon_resolution_service->HaveFailedBefore(file_extension);
+
+    //Do the actual resolution
+    bool success = icon_resolution_service->ResolveFileExtensionIcon(*this);
+
+    // Print a success/failure message once. Issue #98.
+    if (success && !have_resolved_before)
+    {
+      SA_LOG(INFO) << "Resolving icon for file extension '" << file_extension << "' to file '" << mPath << "' with index '" << mIndex << "'";
+    }
+    else if (!success && !have_failed_before)
     {
-      //check for multiple values. keep the first value, forget about other selected file extensions.
-      const std::string separator = pmgr.GetProperty(SelectionContext::MULTI_SELECTION_SEPARATOR_PROPERTY_NAME);
-      if (file_extension.find(separator) != std::string::npos)
-      {
-        //multiple values detected.
-        ra::strings::StringVector extension_list = ra::strings::Split(file_extension, separator.c_str());
-        if (!extension_list.empty())
-          file_extension = extension_list[0];
-      }
-
-      //try to find the path to the icon module for the given file extension.
-      Win32Registry::REGISTRY_ICON resolved_icon = Win32Registry::GetFileTypeIcon(file_extension.c_str());
-
-      //An icon with a negative index is valid from the registry.
-      //Only the special case index = -1 should be considered invalid (Issue #17).
-      //And ShellAnything accept positive (index) and negative index (resource id). (Issue #155, Issue #164).
-      //See issue #17, 155, 164.
-      if (Win32Registry::IsValid(resolved_icon))
-      {
-        //found the icon for the file extension
-        //replace this menu's icon with the new information
-        SA_LOG(INFO) << "Resolving icon for file extension '" << file_extension << "' to file '" << resolved_icon.path << "' with index '" << resolved_icon.index << "'";
-        mPath = resolved_icon.path;
-        mIndex = resolved_icon.index;
-        mFileExtension = "";
-      }
-      else
-      {
-        //failed to find a valid icon.
-        //using the default "unknown" icon
-        Win32Registry::REGISTRY_ICON unknown_file_icon = Win32Registry::GetUnknownFileTypeIcon();
-        mPath = unknown_file_icon.path;
-        mIndex = unknown_file_icon.index;
-        mFileExtension = "";
-
-        //show the message only once in logs
-        const bool is_already_in_log = mUnresolvedFileExtensions.find(file_extension) != mUnresolvedFileExtensions.end();
-        if (!is_already_in_log)
-        {
-          SA_LOG(WARNING) << "Failed to find icon for file extension '" << file_extension << "'. Resolving icon with default icon for unknown file type '" << unknown_file_icon.path << "' with index '" << unknown_file_icon.index << "'";
-
-          //remember this failure.
-          mUnresolvedFileExtensions.insert(file_extension);
-        }
-      }
+      SA_LOG(WARNING) << "Failed to resolve icon for file extension '" << file_extension << "'.";
     }
   }
 

src/core/Icon.h

@@ -119,8 +119,6 @@ namespace shellanything
     virtual void ToLongString(std::string& str, int indent) const;
 
   private:
-    typedef std::set<std::string /*file extension*/> FileExtensionSet;
-    static FileExtensionSet mUnresolvedFileExtensions;
     std::string mFileExtension;
     std::string mPath;
     int mIndex;

src/shellextension/dllmain.cpp

@@ -39,6 +39,7 @@
 #include "WindowsClipboardService.h"
 #include "WindowsKeyboardService.h"
 #include "PcgRandomService.h"
+#include "WindowsIconResolutionService.h"
 
 #include "shellanything/version.h"
 #include "shellanything/config.h"
@@ -55,6 +56,7 @@ shellanything::IRegistryService* registry_service = NULL;
 shellanything::IClipboardService* clipboard_service = NULL;
 shellanything::IKeyboardService* keyboard_service = NULL;
 shellanything::IRandomService* random_service = NULL;
+shellanything::IIconResolutionService* icon_resolution_service = NULL;
 
 class CShellAnythingModule : public ATL::CAtlDllModuleT< CShellAnythingModule >
 {
@@ -198,6 +200,10 @@ extern "C" int APIENTRY DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID lpRe
       random_service = new shellanything::PcgRandomService();
       app.SetRandomService(random_service);
 
+      // Setup an active icon resolution service in ShellAnything's core.
+      icon_resolution_service = new shellanything::WindowsIconResolutionService();
+      app.SetIconResolutionService(icon_resolution_service);
+
       // Setup and starting application
       app.Start();
 
@@ -218,11 +224,13 @@ extern "C" int APIENTRY DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID lpRe
       delete clipboard_service;
       delete registry_service;
       delete logger_service;
+      delete icon_resolution_service;
       random_service = NULL;
       keyboard_service = NULL;
       clipboard_service = NULL;
       registry_service = NULL;
       logger_service = NULL;
+      icon_resolution_service = NULL;
     }
   }
 

src/tests/TestIcon.cpp

@@ -240,7 +240,7 @@ namespace shellanything
       ASSERT_EQ(UNKNOWN_ICON_INDEX, icon.GetIndex());
 
       //act (issue #98)
-      for (int i = 0; i < 500; i++)
+      for (int i = 0; i < 100; i++)
       {
         //this should create multiple log entries if the feature is not properly implemented.
         Icon tmp_icon;
@@ -263,7 +263,16 @@ namespace shellanything
           ASSERT_TRUE(read) << "Failed to read log file: " << path;
 
           int count = CountString(content, UNKNOWN_FILE_EXTENSION);
-          ASSERT_LE(count, 1) << "Log file '" << path << "' contains " << count << " references to '" << UNKNOWN_FILE_EXTENSION << "'.";
+
+          // Define how many reference we should see in log file.
+          // With LegacyIconResolutionService class as active service, a maximum of 2 reference is allowed:
+          // One from LegacyIconResolutionService class and another from Icon::ResolveFileExtensionIcon().
+          // 
+          // With WindowsIconResolutionService class as active service, a maximum of 3 reference is allowed:
+          // Two from WindowsIconResolutionService class and another from Icon::ResolveFileExtensionIcon().
+          static const int MAX_COUNT = 3; // Issue #18. Issue #167.
+
+          ASSERT_LE(count, MAX_COUNT) << "Log file '" << path << "' contains " << count << " references to '" << UNKNOWN_FILE_EXTENSION << "'.";
         }
       }
     }