Merge lp:~larryprice/ubuntu-app-launch/some-icon-paths-need-merged into lp:ubuntu-app-launch/16.10

[Larry Price]

Some explicit icon paths need to be merged with the base path to find existing icon.

Some explicit icon paths need to be merged with the base path to find existing icon.

In particular, installing supertuxkart in libertine on a device with vivid+overlay will result in an icon path of "/usr/share/pixmaps/supertuxkart_128.png". IconFinder could not find this path because it did not exist on the system `/usr/share/pixmaps/`. The base search path for libertine apps includes `/usr/share` already, so a simple concatenation doesn't work. I've opted to iterate through the base path, figure out where the base path and icon name match, and merge the two together if a file exists at that location.

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:233
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/9/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/228/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/236
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=vivid+overlay/177
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=xenial+overlay/177
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=yakkety/177
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/106/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/106/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/106/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/106/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/106/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/106/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/106/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/106/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/106
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/106/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/9/rebuild

review: Needs Fixing (continuous-integration)

[Ted Gould (ted)]

We've discussed this a bit on IRC and it seems a bit more complex than we originally thought. We don't want to match all the way down the path as we don't want an application to be able reference an icon outside of the Libertine container, or the Click package, etc. We do want it to be able do a full reference inside that container.

So to do this it is likely that we'll have to create another string to pass into the Desktop() object that is the contained path for references. This seems like it could have other uses in the future as well. That contained path can then be passed to the icon finder so that it doesn't over extend its search.

review: Needs Fixing

Unmerged revisions

233. By Larry Price on 2016-07-01

Reverse the merge lookup to be more performant

232. By Larry Price on 2016-07-01

Take care of more cases

231. By Larry Price on 2016-07-01

Attempt to prepend basePath to explicit icon names

=== modified file 'libubuntu-app-launch/application-icon-finder.cpp'
--- libubuntu-app-launch/application-icon-finder.cpp	2016-05-04 19:07:02 +0000
+++ libubuntu-app-launch/application-icon-finder.cpp	2016-07-01 19:32:30 +0000
@@ -45,6 +45,60 @@
 constexpr auto ICON_TYPES = {".png", ".svg", ".xpm"};
 
 static const std::regex iconSizeDirname = std::regex("^(\\d+)x\\1$");
+
+static std::string tryMergeFilePaths(const std::string& parent, const std::string& child)
+{
+    auto slashPos = parent.find_last_of('/');
+    auto prevSlashPos = std::string::npos;
+    while (slashPos != std::string::npos)
+    {
+        if (child.find(parent.substr(slashPos)) == std::string::npos && prevSlashPos != std::string::npos)
+        {
+            auto pathWithBase = g_build_filename(parent.substr(0, prevSlashPos).c_str(), child.c_str(), nullptr);
+
+            std::string strPathWithBase(pathWithBase);
+            g_free(pathWithBase);
+
+            if (g_file_test(strPathWithBase.c_str(), G_FILE_TEST_EXISTS))
+            {
+                return strPathWithBase;
+            }
+            break;
+        }
+        prevSlashPos = slashPos;
+        slashPos = parent.find_last_of('/', slashPos - 1);
+    }
+    return child;
+}
+
+static std::string tryFindExplicitFile(const std::string& basePath, const std::string& iconName)
+{
+    if (g_file_test(iconName.c_str(), G_FILE_TEST_EXISTS))
+    {
+        return iconName;
+    }
+    else
+    {
+        auto pathWithBase = g_build_filename(basePath.c_str(), iconName.c_str(), nullptr);
+
+        std::string strPathWithBase(pathWithBase);
+        g_free(pathWithBase);
+
+        if (g_file_test(strPathWithBase.c_str(), G_FILE_TEST_EXISTS))
+        {
+            return strPathWithBase;
+        }
+        else
+        {
+            auto mergedIconName = tryMergeFilePaths(basePath, iconName);
+            if (mergedIconName != iconName)
+            {
+                return mergedIconName;
+            }
+        }
+    }
+    return "";
+}
 }  // anonymous namespace
 
 IconFinder::IconFinder(std::string basePath)
@@ -58,7 +112,11 @@
 {
     if (iconName[0] == '/')  // explicit icon path received
     {
-        return Application::Info::IconPath::from_raw(iconName);
+        auto explicitIconPath = tryFindExplicitFile(_basePath, iconName);
+        if (!explicitIconPath.empty())
+        {
+            return Application::Info::IconPath::from_raw(explicitIconPath);
+        }
     }
 
     /* Look in each directory slowly decreasing the size until we find

=== modified file 'tests/application-icon-finder.cpp'
--- tests/application-icon-finder.cpp	2016-05-04 19:07:02 +0000
+++ tests/application-icon-finder.cpp	2016-07-01 19:32:30 +0000
@@ -57,6 +57,22 @@
               finder.find(basePath + "/icons/hicolor/scalable/apps/app.svg").value());
 }
 
+TEST(ApplicationIconFinder, ReturnsIconWithPrependedBasePath)
+{
+    auto basePath = std::string(CMAKE_SOURCE_DIR) + "/data/usr/share";
+    IconFinder finder(basePath);
+    EXPECT_EQ(basePath + "/icons/hicolor/scalable/apps/app.svg",
+              finder.find("/icons/hicolor/scalable/apps/app.svg").value());
+}
+
+TEST(ApplicationIconFinder, ReturnsIconWithMergedBasePathAndIconName)
+{
+    auto basePath = std::string(CMAKE_SOURCE_DIR) + "/data/usr/share";
+    IconFinder finder(basePath);
+    EXPECT_EQ(basePath + "/icons/hicolor/scalable/apps/app.svg",
+              finder.find("/usr/share/icons/hicolor/scalable/apps/app.svg").value());
+}
+
 TEST(ApplicationIconFinder, ReturnsIconFromPixmapAsFallback)
 {
     auto basePath = std::string(CMAKE_SOURCE_DIR) + "/data/usr/share";