Ubuntu Application Launcher

Merge lp:~mterry/ubuntu-app-launch/persistentid into lp:ubuntu-app-launch

persistentid
Merge into trunk.17.04

Proposed by Michael Terry on 2017-03-01

Status:	Merged
Approved by:	Ted Gould on 2017-04-04
Approved revision:	302
Merged at revision:	313
Proposed branch:	lp:~mterry/ubuntu-app-launch/persistentid
Merge into:	lp:ubuntu-app-launch
Diff against target:	216 lines (+113/-11) 5 files modified debian/changelog (+6/-0) libubuntu-app-launch/appid.h (+20/-3) libubuntu-app-launch/application.cpp (+62/-0) libubuntu-app-launch/helper.cpp (+1/-8) tests/libual-cpp-test.cc (+24/-0)
To merge this branch:	bzr merge lp:~mterry/ubuntu-app-launch/persistentid
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Ted Gould (community)		2017-03-01	Approve on 2017-04-04
unity-api-1-bot	continuous-integration		Needs Fixing on 2017-03-30
Review via email: mp+318682@code.launchpad.net

Commit message

Add persistentID and dbusID methods to ual::AppID

Description of the change

This branch adds two pieces of AppID API:

- persistentID which provides a string that is suitable for writing to disk and back again (i.e. no revision information)

- dbusID which is syntactic sugar for apps that may expose their appID over DBus paths or consume such paths by other apps.

With persistentID, other apps should no longer will have to embed information about appID formats (like manually combining package and command with an underscore). dbusID I care less about, but we talked about it before and since I was there...

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2017-03-01:

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

review: Needs Fixing (continuous-integration)

lp:~mterry/ubuntu-app-launch/persistentid updated on 2017-03-02

297. By Michael Terry on 2017-03-02: Drop prototype for removed function

Revision history for this message

Michael Terry (mterry) wrote on 2017-03-02:

Instead of homegrown escaping, we could use nih_dbus_path?

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2017-03-02:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Ted Gould (ted) wrote on 2017-03-07:

> Instead of homegrown escaping, we could use nih_dbus_path?

We're dropping deps on libnih, so no :-)

Revision history for this message

Ted Gould (ted) wrote on 2017-03-07:

Slight comment, but glad to get the dbus stuff C++-ified.

Revision history for this message

Michael Terry (mterry) wrote on 2017-03-08:

Fixing your isalpha comment. But I wanted to quickly make explicit that this new API is making a promise that UAL hasn't made yet, and just confirm we are cool with it.

So UAL is saying that if you write this AppID to disk, in a year or two, AppID::find() will definitely still recognize it. That's a longevity of format/parsing that I don't think we have made yet.

And since I'm doing this set of changes to consolidate parsing in UAL, and perhaps give us flexibility to change appID format in the future, when we do so, we'd have to carefully consider what that does to persistentIDs out in the wild.

Not a problem necessarily, just wanted to make sure you were OK with signing up for that.

(We already had this problem -- unity8 was already making its own persistentIDs. But now UAL is making that promise to everyone that uses its API.)

lp:~mterry/ubuntu-app-launch/persistentid updated on 2017-03-08

298. By Michael Terry on 2017-03-08: Use isalpha and isdigit

Revision history for this message

Michael Terry (mterry) wrote on 2017-03-08:

OK fixed to use isalpha and isdigit.

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2017-03-08:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Ted Gould (ted) wrote on 2017-03-09:

I think we have to, don't love it, but I think that we can handle that part of the persistent IDs.

Your isdigit() change dropped the i != 0 check, which is because dbus names can't have a digit as their first character.

lp:~mterry/ubuntu-app-launch/persistentid updated on 2017-03-13

299. By Michael Terry on 2017-03-13: Restore i != 0 check

Revision history for this message

Michael Terry (mterry) wrote on 2017-03-13:

OK, restored the i != 0 check.

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2017-03-13:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Terry (mterry) wrote on 2017-03-13:

Is that CI failure related? Doesn't look it to me, but if other branches are passing...

Revision history for this message

Ted Gould (ted) wrote on 2017-03-29:

Not worried about the CI as much as the conflict in the branch. I'm guessing there might be another with Pete's branches landing.

review: Needs Fixing

lp:~mterry/ubuntu-app-launch/persistentid updated on 2017-03-30

300. By Michael Terry on 2017-03-30: Merge trunk
301. By Michael Terry on 2017-03-30: Fix tests now that click apps are gone
302. By Michael Terry on 2017-03-30: Fix version

Revision history for this message

Michael Terry (mterry) wrote on 2017-03-30:

Fixed the conflict.

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2017-03-30:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Ted Gould (ted) on 2017-04-04:

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Indicator Applet Developers

Michael Terry

 === modified file 'debian/changelog'
 --- debian/changelog	2017-03-21 03:29:09 +0000
 +++ debian/changelog	2017-03-30 13:56:23 +0000
@@ -1,3 +1,9 @@
++ubuntu-app-launch (0.12) UNRELEASED; urgency=medium
++
++  * Bump version for new AppID methods
++
++ -- Michael Terry <mterry@ubuntu.com>  Tue, 28 Feb 2017 15:59:05 -0500
++
  ubuntu-app-launch (0.11+17.04.20170321-0ubuntu1) zesty; urgency=medium
    [ Ted Gould ]
 === modified file 'libubuntu-app-launch/appid.h'
 --- libubuntu-app-launch/appid.h	2016-08-24 21:52:19 +0000
 +++ libubuntu-app-launch/appid.h	2017-03-30 13:56:23 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright © 2016 Canonical Ltd.
++ * Copyright © 2016-2017 Canonical Ltd.
+  *
   * This program is free software: you can redistribute it and/or modify it
   * under the terms of the GNU General Public License version 3, as published
@@ -82,6 +82,16 @@
          anyting other than debug messages. */
      operator std::string() const;
++    /** Turn the structure into a string suitable for use in DBus paths.
++        Turn this back into an AppID with parseDBusID(). This is otherwise not
++        a valid AppID string. */
++    std::string dbusID() const;
++
++    /** Turn the structure into a string without any revision information.
++        This is suitable for writing the AppID to disk. Turn this back into
++        an AppID with find(). This is otherwise not a valid AppID string. */
++    std::string persistentID() const;
++
      /** Empty constructor for an AppID. Makes coding with them easier, but generally
          there is nothing useful about an empty AppID. */
      AppID();
@@ -98,6 +108,13 @@
      */
      AppID(Package pkg, AppName app, Version ver);
++    /** Parse a string from DBus and turn it into an AppID. This assumes that
++        the string was originally produced by dbusID(), the result is otherwise
++        undefined.
++
++        \param dbusid String from a DBus path
++    */
++    static AppID parseDBusID(const std::string& dbusid);
      /** Parse a string and turn it into an AppID. This assumes that the string is
          in the form: $(package)_$(app)_$(version) and will return an empty AppID
          if not.
@@ -106,7 +123,7 @@
      */
      static AppID parse(const std::string& appid);
      /** Find is a more tollerant version of parse(), it handles legacy applications,
--        short AppIDs ($package_$app) and other forms of that are in common usage.
++        persistent IDs and other forms of that are in common usage.
          It can be used, but is slower than parse() if you've got well formed data
          already.
@@ -117,7 +134,7 @@
      */
      static AppID find(const std::string& sappid);
      /** Find is a more tollerant version of parse(), it handles legacy applications,
--        short AppIDs ($package_$app) and other forms of that are in common usage.
++        persistent IDs and other forms of that are in common usage.
          It can be used, but is slower than parse() if you've got well formed data
          already.
 === modified file 'libubuntu-app-launch/application.cpp'
 --- libubuntu-app-launch/application.cpp	2017-02-25 15:58:27 +0000
 +++ libubuntu-app-launch/application.cpp	2017-03-30 13:56:23 +0000
@@ -28,7 +28,9 @@
  #include "registry.h"
  #include <functional>
++#include <iomanip>
  #include <iostream>
++#include <locale>
  #include <regex>
  namespace ubuntu
@@ -153,6 +155,66 @@
      return package.value() + "_" + appname.value() + "_" + version.value();
+ }
++std::string AppID::persistentID() const
++{
++    if (package.value().empty())
++    {
++        if (appname.value().empty())
++        {
++            return {};
++        }
++        else
++        {
++            return appname.value();
++        }
++    }
++
++    return package.value() + "_" + appname.value();
++}
++
++std::string AppID::dbusID() const
++{
++    std::string bytes = operator std::string();
++    std::string encoded;
++
++    for (size_t i = 0; i < bytes.size(); ++i) {
++        char chr = bytes[i];
++
++        if (std::isalpha(chr, std::locale::classic()) ||
++            (std::isdigit(chr, std::locale::classic()) && i != 0)) {
++            encoded += chr;
++        } else {
++            std::ostringstream hex;
++            hex << std::setw(2) << std::setfill('0') << std::hex;
++            hex << int(chr);
++            encoded += '_' + hex.str();
++        }
++    }
++
++    return encoded;
++}
++
++AppID AppID::parseDBusID(const std::string& dbusid)
++{
++    std::string decoded;
++
++    for (size_t i = 0; i < dbusid.size(); ++i) {
++        char chr = dbusid[i];
++
++        if (chr == '_' && i + 2 < dbusid.size()) {
++            int result;
++            std::istringstream hex(dbusid.substr(i + 1, 2));
++            hex >> std::hex >> result;
++            decoded += (char)result;
++            i += 2;
++        } else {
++            decoded += chr;
++        }
++    }
++
++    return parse(decoded);
++}
++
  bool operator==(const AppID& a, const AppID& b)
+ {
      return a.package.value() == b.package.value() && a.appname.value() == b.appname.value() &&
 === modified file 'libubuntu-app-launch/helper.cpp'
 --- libubuntu-app-launch/helper.cpp	2017-03-20 12:28:10 +0000
 +++ libubuntu-app-launch/helper.cpp	2017-03-30 13:56:23 +0000
@@ -277,7 +277,7 @@
                      skel);
                  /* Find a path to export on */
--                auto dbusAppid = dbusSafe(std::string{appid});
++                auto dbusAppid = appid.dbusID();
                  std::string path;
                  while (path.empty())
@@ -319,13 +319,6 @@
          timeout = timeoutin;
+     }
--    static std::string dbusSafe(const std::string& in)
--    {
--        std::string out = in;
--        std::transform(out.begin(), out.end(), out.begin(), [](char in) { return std::isalpha(in) ? in : '_'; });
--        return out;
--    }
--
      bool proxyCb(GDBusMethodInvocation* invocation)
+     {
          if (mirfd.get() == 0)
 === modified file 'tests/libual-cpp-test.cc'
 --- tests/libual-cpp-test.cc	2017-03-20 15:17:02 +0000
 +++ tests/libual-cpp-test.cc	2017-03-30 13:56:23 +0000
@@ -586,6 +586,30 @@
      return;
+ }
++TEST_F(LibUAL, DBusID)
++{
++    auto id = ubuntu::app_launch::AppID::parse("container-name_test_0.0");
++    ASSERT_FALSE(id.empty());
++    EXPECT_EQ("container_2dname_5ftest_5f0_2e0", id.dbusID());
++    EXPECT_FALSE(ubuntu::app_launch::AppID::valid(id.dbusID()));
++
++    auto parsed = ubuntu::app_launch::AppID::parseDBusID(id.dbusID());
++    ASSERT_FALSE(parsed.empty());
++    EXPECT_EQ(id, parsed);
++}
++
++TEST_F(LibUAL, PersistentID)
++{
++    auto id = ubuntu::app_launch::AppID::parse("container-name_test_0.0");
++    ASSERT_FALSE(id.empty());
++    EXPECT_EQ("container-name_test", id.persistentID());
++    EXPECT_FALSE(ubuntu::app_launch::AppID::valid(id.persistentID()));
++
++    auto found = ubuntu::app_launch::AppID::find(registry, id.persistentID());
++    ASSERT_FALSE(found.empty());
++    EXPECT_EQ(id, found);
++}
++
  TEST_F(LibUAL, ApplicationList)
+ {
      SnapdMock snapd{LOCAL_SNAPD_TEST_SOCKET, {u8Package, interfaces, u8Package}};