Code review comment for lp://staging/~ted/ubuntu-app-launch/snappy-backend-no-snap

On Wed, 2016-08-24 at 18:53 +0000, Charles Kerr wrote:
> By some yardstick this was an easier review -- the code diff is
> smaller -- but I had a lot of questions about the type-specific
> methods.

I think a lot of your questions came from not knowing the different
AppID schemes, which really means we didn't do a good job of
documenting that. So I tried to do that with narrative text on each of
the implementations: r363

> > +    if (apps.size() == 0)
> apps is a std::list<> so size() has to walk the list.
>
> Since size is used twice in this function (here and in ONLY_LISTED
> below) better to store it in a temporary.
>
> In general, better to say "if (!foo.empty())" than "if (foo.size() ==
> 0)"

List size is constant since C++11: http://en.cppreference.com/w/cpp/con
tainer/list/size

But yes, empty would look better. r364

> > +    switch (card)
> > +    {
> > +        case AppID::ApplicationWildcard::FIRST_LISTED:
> > +            return *apps.begin();
> > +        case AppID::ApplicationWildcard::LAST_LISTED:
> out-of-scope for this review, but as an aside, what's the use case
> for LAST_LISTED?

Added for completeness, seemed odd to have first and only but not last.
I think almost everyone uses first.

> > +    catch (std::runtime_error& e)
> > +    {
> > +        return false;
> does e.what need to be logged here? Looks like the answer is 'no' but
> wanted to be sure

No, it gets checked a fair amount and would be too chatty.

> +bool Legacy::verifyPackage(const AppID::Package& package, const
> > std::shared_ptr<Registry>& registry)
> > +{
> > +    return package.value().empty();
> (needinfo) Wait, whaaaat? Why would a function named verifyPackage()
> return true iff no package?

For Legacy apps the package field is empty.

> > +    std::string desktop = std::string(appname) + ".desktop";
> redundant type information, suggest auto on lhs
> > >
> > +    std::function<bool(const gchar* dir)> evaldir =
> > [&desktop](const gchar* dir) {
> redundant type information, suggest auto on lhs

r365

> > +AppID::Version Legacy::findVersion(const AppID::Package& package,
> > +                                   const AppID::AppName& appname,
> > +                                   const
> > std::shared_ptr<Registry>& registry)
> > +{
> > +    return AppID::Version::from_raw({});
> (needinfo) Why does findAppname() throw but findVersion() returns
> empty?

So my thought there was that if you're looking for the version, the
answer is always "", but we can't ever find an application, so that's
an error.

> > +/** Checks the AppID by making sure the version is "0.0" and then
> > +    calling verifyAppname() to check the rest.
> (needinfo) Same "whaaaat" as above, I think I'm missing some context
> here. I don't understand what this is for, nor why a 0.0 test is
> useful

All Libertine applications have a version string of "0.0".

Using it as a test to reject things quickly. Since basically our
algorithm is "try all implementations" we end up with wanting to reject
as much as possible.

> > +    catch (std::runtime_error& e)
> > +    {
> > +        return false;
> is this normal, or should e.what() be logged?

Normal.

> > +        auto container = containers.get()[i];
> > +        if (std::string(container) == package.value())
> bool operator==( const CharT* lhs, const
> basic_string<CharT,Traits,Alloc>& rhs ) exists, so there's no need
> for the std::string temporary:
> > if (container == package.value())

r366

> +/** Basically we're making our own VTable of static functions.
> > Static
> > +    functions don't go in the normal VTables, so we can't use our
> > class
> > +    inheritance here to help. So we're just packing these puppies
> > into
> > +    a data structure and itterating over it. */
> 1. s/itterating/iterating/

r367

> 2. This is a really strange mechanism, I wish there was a better
> approach than looping through DiscoverTools instances

Yeah, but hopefully most users won't get AppIDs through discovery as
much as keep Application objects around and use those. Or just be
checking their versions which is relatively cheap. Most of the more
advanced functions here are as much syntactic sugar to make everything
feel available, even if it is costly.

> > +AppID AppID::discover(const std::shared_ptr<Registry>& registry,
> > +                      const std::string& package,
> > +                      const std::string& appname,
> > +                      const std::string& version)
> > +{
> > +    auto pkg = AppID::Package::from_raw(package);
> > +
> > +    for (auto tools : discoverTools)
> auto&, here and below in the other "for (auto tools : discoverTools)"
> uses

There is no way the compiler would need this ;-)
r368

> > +                    app = AppID::AppName::from_raw(appname);
> > +                    if (!tools.verifyAppname(pkg, app, registry))
> > +                    {
> > +                        throw std::runtime_error("App name passed
> > in is not valid for this package type");
> This isn't logged or anything down in the catch block, why not avoid
> the try-catch overhead and just call "continue" here?

So I went this route so that there was one control scheme going on in
the function. Everything is an exception whether it is from one of the
functions themselves or this code. I figured the overhead would be
minimal as the compiler is likely to optimize to a jump anyway. I put
the string in as a comment.