On 06/08/2016 12:23 PM, Larry Price wrote:
> Review: Needs Fixing
>
> Submitted another round of comments. We're getting close.
>
> Diff comments:
>
>>
>> === added file 'libertine-scope/action.cpp'
>> --- libertine-scope/action.cpp	1970-01-01 00:00:00 +0000
>> +++ libertine-scope/action.cpp	2016-06-08 15:46:18 +0000
>> @@ -0,0 +1,95 @@
>> +/*
>> + * Copyright (C) 2016 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 by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + * * Authored by:
>> + *	Kyle Nitzsche <email address hidden>
>> + */
>> +
>> +#include "libertine-scope/action.h"
>> +#include "libertine-scope/config.h"
>> +#include <unity/scopes/ActivationResponse.h>
>> +#include <unity/scopes/CannedQuery.h>
>> +#include <url-dispatcher.h>
>> +#include <QString>
>> +#include <QFile>
>> +#include <QTextStream>
>> +
>> +namespace usc = unity::scopes;
>> +
>> +Action::
>> +Action(const usc::Result &result,
>> +               usc::ActionMetadata const& metadata,
>
> the spacing on this parameter list should be lined up, which would also mean moving const to the other side of usc::Result to match the other params
done
>
>> +               std::string const& action_id,
>> +               std::string const &cache_dir)
>> +    : usc::ActivationQueryBase(result, metadata),
>> +      action_id_(action_id),
>> +      cache_dir_(cache_dir)
>> +{
>> +}
>> +
>> +usc::ActivationResponse
>> +Action::activate()
>> +{
>> +
>
> delete
done
>
>> +  if (action_id_ == "open")
>> +  {
>> +    url_dispatch_send(result().uri().c_str() , NULL, NULL);
>> +    return usc::ActivationResponse(usc::ActivationResponse::Status::NotHandled);
>> +  }
>> +  else if (action_id_ == "hide")
>> +  {
>> +    QFile file(QString("%1/%2").arg(QString::fromStdString(cache_dir_),"hidden"));
>> +    file.open(QIODevice::Append | QIODevice::Text);	
>> +    QTextStream out(&file);
>> +    out << QString::fromStdString(result()["app_id"].get_string());
>> +    endl(out);
>> +    file.close();
>> +    usc::CannedQuery cq(SCOPE_PKG + "_" + SCOPE_APP);
>> +    return usc::ActivationResponse(cq);
>> +  }
>> +  else if (action_id_ == "show")
>
> this method is a bit long for me - extends more than 1 screen of my laptop. could you extract some of this logic to another function?

done.

>
>> +  {
>> +    QStringList hidden;
>
> unused variable - delete
right - deleted
>
>> +    QFile hidden_f(QString("%1/%2").arg(QString::fromStdString(cache_dir_), QString::fromStdString("hidden")));
>> +    if (hidden_f.exists()) {
>> +      QStringList hidden;
>> +      if (hidden_f.open(QIODevice::ReadOnly | QIODevice::Text))
>> +      {
>> +        QTextStream in(&hidden_f);
>> +        while (!in.atEnd())
>> +        {
>> +          QString line(in.readLine());
>> +          hidden.append(line.trimmed());
>> +        }
>> +        hidden_f.close();
>> +      }
>> +      QString app_id = QString::fromStdString(result()["app_id"].get_string());
>> +      if (hidden.contains(app_id))
>> +      {
>> +        hidden.removeAll(app_id);
>> +        hidden_f.open(QIODevice::WriteOnly | QIODevice::Text);	
>> +        QTextStream out(&hidden_f);
>> +        for (auto app : hidden)
>
> this could probably be a const ref
const& it is

>
>> +        {
>> +          out << app << "\n";
>> +        }
>> +        hidden_f.flush();
>> +        hidden_f.close();
>> +      }
>> +    }
>> +    usc::CannedQuery cq(SCOPE_PKG + "_" + SCOPE_APP);
>> +    return usc::ActivationResponse(cq);
>> +  }
>> +  return usc::ActivationResponse(usc::ActivationResponse::Status::NotHandled);
>> +}
>>
>> === modified file 'libertine-scope/query.cpp'
>> --- libertine-scope/query.cpp	2016-05-04 17:56:13 +0000
>> +++ libertine-scope/query.cpp	2016-06-08 15:46:18 +0000
>> @@ -16,21 +16,62 @@
>>
>>   #include "libertine-scope/query.h"
>>   #include "libertine-scope/container.h"
>> +#include "libertine-scope/config.h"
>> +#include "localization.h"
>>   #include <unity/scopes/CategorisedResult.h>
>>   #include <unity/scopes/CategoryRenderer.h>
>>   #include <unity/scopes/QueryBase.h>
>>   #include <unity/scopes/SearchReply.h>
>> +#include <unity/scopes/Department.h>
>> +#include <unity/scopes/OptionSelectorFilter.h>
>> +#include <unity/scopes/FilterOption.h>
>>   #include <QString>
>>   #include <QStringList>
>>   #include <QRegExp>
>> -
>> +#include <QFile>
>> +#include <QDebug>
>> +#include <QTextStream>
>>
>>   namespace usc = unity::scopes;
>>
>> -
>>   namespace
>>   {
>>
>> +//returns map of apps with keys: container, app, name
>> +static std::map<QString,QString>
>> +app_keys(std::string uri)
>
> uri as const ref
grokked and done
>
>> +{
>> +  std::map<QString,QString> parts;
>> +  QStringList uri_parts = QString::fromStdString(uri).split("/");
>> +  if (uri_parts.size() < 4)
>
> this would be more readable for me with curly braces
Is this a standard in the group? If so I'd like to go through the whole 
project and find and fix all such usages once and for all.

>
>> +    return parts;
>> +  parts["container"] = uri_parts[2];
>> +  parts["name"] = uri_parts[3];
>> +  parts["key"] = QString("%1/%2").arg(parts["container"],parts["name"]);
>> +  return parts;
>> +}
>> +
>> +static QStringList
>> +get_hidden(std::string cache_dir)
>
> cache_dir as const ref
done
>
>> +{
>> +  QStringList hidden;
>> +  QFile hidden_f(QString("%1/%2").arg(QString::fromStdString(cache_dir), QString::fromStdString("hidden")));
>> +  if (hidden_f.exists())
>> +  {
>> +    if (hidden_f.open(QIODevice::ReadOnly | QIODevice::Text))
>> +    {
>> +      QTextStream in(&hidden_f);
>> +      while (!in.atEnd())
>> +      {
>> +        QString line(in.readLine());
>> +        hidden.append(line.trimmed());
>> +      }
>> +      hidden_f.close();
>> +    }
>> +  }
>> +  return hidden;
>> +}
>> +
>>   /**
>>    * A custom rendering layout brazenly stolen from the click scope, so they look
>>    * sorta similar.  At least until they change theirs.
>>
>> === modified file 'libertine-scope/scope.cpp'
>> --- libertine-scope/scope.cpp	2016-01-19 21:10:09 +0000
>> +++ libertine-scope/scope.cpp	2016-06-08 15:46:18 +0000
>> @@ -28,26 +31,48 @@
>>   namespace
>>   {
>>
>> -/**
>> - * @todo move this class into its own source file.
>> - */
>> -class ScopeActivation
>> -: public usc::ActivationQueryBase
>> -{
>> -public:
>> -  ScopeActivation(usc::Result const& result,
>> -                  usc::ActionMetadata const& metadata)
>> -  : ActivationQueryBase(result, metadata)
>> -  { }
>> -
>> -  usc::ActivationResponse
>> -  activate() override
>> -  {
>> -    return usc::ActivationResponse(status);
>> +static bool
>> +is_whitelist(QString line)
>
> line as a const ref
yes
>
>> +{
>> +  if (line.startsWith("whitelist"))
>
> i could really use some curly braces here. alternatively ternary operators.
curly braces added
>
>> +    if (line.split("/").size() < 3)
>> +      return false;
>> +    else
>> +      return true;
>> +  else
>> +    return false;
>> +}
>> +
>> +static QString
>> +get_whitelist_key(QString line)
>
> line as a const ref
fixed
>
>> +{
>> +  QStringList parts = line.split("/");
>> +  return parts[1] + "/" + parts[2].trimmed();
>
> what should happen in the case where parts cannot be split into at least 3  pieces? this case should be handled
this code is only called is is_whitelist() == true. and is_whitelist() 
verifies it can be split into 3 pieces. so this case does not arise.
>
>> +}
>> +
>> +static std::tuple<QStringList,QStringList>
>> +get_bwlists(std::string scope_dir)
>> +{
>> +  //Get blacklisted and whitelisted apps from "blacklist" file in scope dir
>> +  QStringList blacklist;
>> +  QStringList whitelist;
>> +  QFile blacklist_f(QString("%1/%2").arg(QString::fromStdString(scope_dir), QString::fromStdString("blacklist")));
>> +  if (blacklist_f.exists()) {
>> +    if (blacklist_f.open(QIODevice::ReadOnly | QIODevice::Text)){
>> +      QTextStream in(&blacklist_f);
>> +      while (!in.atEnd()) {
>> +        QString line(in.readLine());
>> +        if (!line.startsWith("#") && !line.startsWith("whitelist"))
>> +          blacklist.append(line.trimmed());
>
> curly braces in these if statements
done

>
>> +        if (is_whitelist(line))
>> +          whitelist.append(get_whitelist_key(line));
>> +      }
>> +      blacklist_f.close();
>> +    }
>>     }
>> -
>> -  usc::ActivationResponse::Status status = usc::ActivationResponse::Status::NotHandled;
>> -};
>> +
>> +  return std::tie(blacklist,whitelist);
>> +}
>>
>>   } // anonymous namespace
>>
>>
>> === modified file 'tests/test_query.cpp'
>> --- tests/test_query.cpp	2016-04-27 18:37:46 +0000
>> +++ tests/test_query.cpp	2016-06-08 15:46:18 +0000
>> @@ -160,50 +159,17 @@
>>   }
>>
>>
>> -// Query class with faked out Settings
>> -class QueryWithFakeSettings : public Query
>> -{
>> -public:
>> -  QueryWithFakeSettings(unity::scopes::CannedQuery const& query,
>> -        unity::scopes::SearchMetadata const&  metadata,
>> -        Libertine::Factory const&   libertine_factory)
>> -  : Query(query, metadata, libertine_factory)
>> -    , settings_()
>> -  {
>> -  }
>> -
>> -  unity::scopes::VariantMap settings() const override
>> -  {
>> -    return settings_;
>> -  }
>> -
>> -  unity::scopes::VariantMap settings_;
>> -};
>> -
>> -
>>   TEST_F(TestQueryFixture, ignoresAnyBlacklistedApps)
>>   {
>>     expect_registry();
>> -  expect_push_library();
>> -
>> -  QueryWithFakeSettings query(canned_query, metadata, []() {
>> -    return FakeLibertine::make_fake(LIBERTINE_OUTPUT_WITH_APPS);
>> -  });
>> -  query.settings_["blacklist"] = "LibreOffice;Linux";
>> -  query.run(proxy);
>> -}
>> -
>> -
>> -TEST_F(TestQueryFixture, stripsQuotationMarksFromBlacklist)
>> -{
>> -  expect_registry();
>>     expect_push_linux();
>>     expect_push_library();
>> -
>> -  QueryWithFakeSettings query(canned_query, metadata, []() {
>> +  QStringList blacklist = {"fake-container/libreoffice"};
>> +  QStringList whitelist = {};
>
> we should add a test for whitelisting applications

I tried. whitelisting works at runtime. but the following does not work 
at test time and I don't know why. can you tell me why?

https://pastebin.canonical.com/158344/

This is the only comment here I have not addressed.

>
>> +  std::tuple<QStringList,QStringList> lists(blacklist, whitelist);
>> +  Query query(canned_query, metadata, []() {
>>       return FakeLibertine::make_fake(LIBERTINE_OUTPUT_WITH_APPS);
>> -  });
>> -  query.settings_["blacklist"] = "\"LibreOffice\"";
>> +  }, "/tmp", lists);
>>     query.run(proxy);
>>   }
>>
>
>