Addressed all points. click works fine. comments inline. will soon MP
branch after ensuring gtests still pass.
thx.
On 06/08/2016 10:18 AM, Stephen M. Webb wrote:
> Review: Needs Fixing
>
> Got a few inline comments again.
>
> 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-07 21:40:03 +0000
>> @@ -0,0 +1,96 @@
>> +/*
>> + * 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 .
>> + * * Authored by:
>> + * Kyle Nitzsche
>> + */
>> +
>> +#include "libertine-scope/action.h"
>> +#include "libertine-scope/config.h"
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +namespace usc = unity::scopes;
>> +
>> +Action::
>> +Action(const usc::Result &result,
>> + usc::ActionMetadata const& metadata,
>> + std::string const& action_id,
>> + std::string const &cache_dir)
>> + : usc::ActivationQueryBase(result, metadata),
>> + action_id_(action_id),
>> + cache_dir_(cache_dir){
>
> newline before {
fixed
>
>> +}
>> +
>> +usc::ActivationResponse
>> +Action::activate()
>> +{
>> +
>> + 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);
>> + cq.set_department_id(ROOT_DEPT_ID_NULL);
>> + return usc::ActivationResponse(cq);
>> + }
>> + else if (action_id_ == "show")
>> + {
>> + QStringList hidden;
>> + 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)
>> + {
>> + out << app << "\n";
>> + }
>> + hidden_f.flush();
>> + hidden_f.close();
>> + }
>> + }
>> + usc::CannedQuery cq(SCOPE_PKG + "_" + SCOPE_APP);
>> + cq.set_department_id(ROOT_DEPT_ID_NULL);
>> + 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-07 21:40:03 +0000
>> @@ -59,10 +99,17 @@
>> Query::
>> Query(usc::CannedQuery const& query,
>> usc::SearchMetadata const& metadata,
>> - Libertine::Factory const& libertine_factory)
>> + Libertine::Factory const& libertine_factory,
>> + std::string const& cache_dir,
>> + std::tuple const& blackwhiteLists
>> + )
>> : usc::SearchQueryBase(query, metadata)
>> , libertine_factory_(libertine_factory)
>> +, cache_dir_(cache_dir)
>> +, bwlists(blackwhiteLists)
>> {
>> + blacklist = std::get<0>(bwlists);
>> + whitelist = std::get<1>(bwlists);
>
> (1) The bwlists member variable is used nowhere other than in initializing other member variables in the constructor. It ends up duplicating data making things less cache-friendly. You can get rid of it entirely.
>
> (2) You should either initialize blacklist and whitelist in the initializer list using std::get<> or use std::tie in the constructor body. Using the initializer list is better because it avoids default-constructing the QStringLists and then using the assignment operator to replace their contents.
>
understood, and fixed using initializer list and dropping bwlists as
member variable
>> }
>>
>>
>> @@ -71,59 +118,141 @@
>> {
>> }
>>
>> -
>> unity::scopes::VariantMap
>> Query::settings() const
>> {
>> return SearchQueryBase::settings();
>> }
>>
>> -
>> -QStringList
>> -Query::blacklist() const
>> -{
>> - QStringList blacklistedApps;
>> - auto blacklist = settings()["blacklist"];
>> - if (!blacklist.is_null()) {
>> - blacklistedApps = QString::fromStdString(blacklist.get_string())
>> - .remove("\"")
>> - .split(";", QString::SkipEmptyParts);
>> - }
>> - return blacklistedApps;
>> -}
>> -
>> -
>> void Query::
>> run(usc::SearchReplyProxy const& reply)
>> {
>> - auto blacklistedApps = blacklist();
>> + auto hidden = get_hidden(cache_dir_);
>> + std::string root_dept_id = "root_dept";
>> + // make scope departments
>> + if (hidden.size() > 0 )
>> + {
>> + usc::Department::SPtr root_dept;
>> + usc::Department::SPtr hidden_apps_dept;
>> + usc::CannedQuery myquery("libertine-scope.ubuntu");
>> + myquery.set_department_id(root_dept_id);
>> + root_dept = std::move(usc::Department::create("", myquery, _("X Apps")));
>> + myquery.set_department_id("hidden-apps-dept");
>> + hidden_apps_dept = std::move(usc::Department::create("hidden", myquery, _("Hidden X Apps")));
>> + root_dept->add_subdepartment(hidden_apps_dept);
>> + reply->register_departments(root_dept);
>> + }
>> +
>> + std::pair bwlists;
>> + std::map excludes_map;
>> + QStringList excludes_by_filter;
>> QRegExp re(QString::fromStdString(query().query_string()), Qt::CaseInsensitive);
>> Libertine::UPtr libertine = libertine_factory_();
>>
>> + //only provide filters in root department: X Apps
>> + if (query().department_id() == "")
>
> Like I mentioned previously, it's better to use a named constant to indicate magic values instead of comments. There should be a named constant for the root department name.
yes, taken care of now
>
>> + {
>> + auto filter_state = query().filter_state();
>> + std::map filters;
>> +
>> + //make exclude scope filter for apps
>> + for (auto const& container: libertine->get_container_list())
>> + {
>> + std::string filter_title = std::string(_("Exclude Apps")) + ": " + container->name();
>> + filters[container->id()] = unity::scopes::OptionSelectorFilter::create(container->id(), filter_title, true);
>> + unity::scopes::OptionSelectorFilter::SPtr filter = filters[container->id()];
>> + // add exclude filter checkable items for each app that pass blacklisting + whitelisting
>> + for (auto const& app: container->app_launchers())
>> + {
>> + excludes_map = app_keys(app.uri());
>> + //only exlcude through blacklisting if not explicitly whitelisted
>> + QString app_id = QString::fromStdString(app.uri()).split("/")[3];
>> + QString key = QString::fromStdString(container->id()) + "/" + app_id;
>> + if (hidden.contains(key))
>> + continue;
>> + if (whitelist.contains(key))
>> + filter->add_option(excludes_map["key"].toStdString(), app.name());
>> + else if (!blacklist.contains(excludes_map["key"]) && !blacklist.contains("all/" + excludes_map["name"]))
>> + filter->add_option(excludes_map["key"].toStdString(), app.name());
>> + }
>> + //get exclude filter checked items
>> + if (filter->has_active_option(filter_state))
>> + {
>> + auto excludeTypes = filter->active_options(filter_state);
>> + for (auto const &opt: excludeTypes)
>> + {
>> + excludes_by_filter.append(QString::fromStdString(opt->id()));
>> + }
>> + }
>> + }
>> + //push filters
>> + std::list filters_v;
>> + for (const auto& filter: filters)
>> + {
>> + if (filter.second->options().size() > 0 )
>> + filters_v.push_back(filter.second);
>> + }
>> + reply->push(filters_v, query().filter_state());
>> + }
>> +
>> + //make and push results
>> for (auto const& container: libertine->get_container_list())
>> {
>> auto category = reply->register_category(container->id(),
>> container->name(),
>> "Application",
>> usc::CategoryRenderer(CATEGORY_APPS_DISPLAY));
>> + bool breaking = false;
>> for (auto const& app: container->app_launchers())
>> {
>> - if (!(re.isEmpty() || QString::fromStdString(app.name()).contains(re))
>> - || blacklistedApps.contains(QString::fromStdString(app.name())))
>> - {
>> - continue;
>> - }
>> -
>> - usc::CategorisedResult result(category);
>> - result.set_title(app.name());
>> - result.set_art(app.icon());
>> - result.set_uri(app.uri());
>> - result["description"] = app.description();
>> - if (!reply->push(result))
>> - {
>> - break;
>> - }
>> + //search
>> + if (!(re.isEmpty() || QString::fromStdString(app.name()).contains(re)))
>> + continue;
>> +
>> + excludes_map = app_keys(app.uri());
>> + //only check for blacklisted if not whitelisted
>> + QString app_id;
>> + if (QString::fromStdString(app.uri()).split("/").size() >= 4)
>> + app_id = QString::fromStdString(app.uri()).split("/")[3];
>> + QString key = QString::fromStdString(container->id()) + "/" + app_id;
>> + //don't display if blacklisted
>> + if (!whitelist.contains(key))
>> + if (blacklist.contains(excludes_map["key"]) || blacklist.contains("all/" + excludes_map["name"]))
>> + continue;
>> + //don't display apps excluded by filter
>> + if (excludes_by_filter.contains(excludes_map["key"]))
>> + continue;
>> +
>> + //if root department, don't display if it's a hidden app
>> + if (query().department_id() == "" || query().department_id() == root_dept_id)
>> + {
>> + if (hidden.contains(key))
>> + continue;
>> + }
>
> indent is off by 1 for the following code block
yes - fixed
>
>> + else
>> + {
>> + //don't display non hidden apps in hidden dept
>> + if (!hidden.contains(key))
>> + continue;
>> + }
>> + usc::CategorisedResult result(category);
>> + result.set_title(app.name());
>> + result.set_art(app.icon());
>> + result.set_uri(app.uri());
>> + result["description"] = app.description();
>> + result["app_id"] = key.toStdString();
>> + //add department id so we know which buttons to display on preview
>> + if (query().department_id().empty() || query().department_id() == root_dept_id)
>> + result["department_id"] = root_dept_id;
>> + else
>> + result["department_id"] = "hidden";
>> + if (!reply->push(result))
>> + {
>> + breaking = true;
>> + break;
>> + }
>> }
>> + if (breaking)
>> + break;
>> }
>> }
>> -
>>
>> === modified file 'libertine-scope/query.h'
>> --- libertine-scope/query.h 2016-05-04 17:56:13 +0000
>> +++ libertine-scope/query.h 2016-06-07 21:40:03 +0000
>> @@ -46,9 +47,12 @@
>> virtual unity::scopes::VariantMap settings() const;
>>
>> private:
>> - QStringList blacklist() const;
>> -
>> Libertine::Factory libertine_factory_;
>> + std::string scope_dir_;
>> + std::string cache_dir_;
>> + std::tuple bwlists;
>
> bwlists is unused and shouldn;t be a member variable.
yes, removed it
>
>> + QStringList blacklist;
>
> Member variables should have a trailing underscore for consistency with existing code.
done
>
>> + QStringList whitelist;
here too
>> };
>>
>> #endif // LIBERTINE_SCOPE_QUERY_H_
>>
>> === modified file 'libertine-scope/scope.cpp'
>> --- libertine-scope/scope.cpp 2016-01-19 21:10:09 +0000
>> +++ libertine-scope/scope.cpp 2016-06-07 21:40:03 +0000
>> @@ -28,27 +31,49 @@
>> 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)
>> +{
>> + if (line.startsWith("whitelist"))
>> + if (line.split("/").size() < 3)
>> + return false;
>> + else
>> + return true;
>> + else
>> + return false;
>> +}
>> +
>> +static QString
>> +get_whitelist_key(QString line)
>> +{
>> + QStringList parts = line.split("/");
>> + return parts[1] + "/" + parts[2].trimmed();
>> +}
>> +
>> +static std::pair
>> +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());
>> + if (is_whitelist(line))
>> + whitelist.append(get_whitelist_key(line));
>> + }
>> + blacklist_f.close();
>> + }
>> }
>> -
>> - usc::ActivationResponse::Status status = usc::ActivationResponse::Status::NotHandled;
>> -};
>> -
>> +
>> + std::pair bwlists(blacklist,whitelist);
>> + return bwlists;
>
> You do not need to create a named temporary for the return value. In fact it can mess up RVO. Just return std::make_pair(blacklist, whitelist) -- or use a std::tuple and return std::tie(blacklist, whitelist).
>
fixed with: return std::tie(...)
>> +}
>> } // anonymous namespace
>>
>>
>> @@ -79,7 +104,7 @@
>> search(usc::CannedQuery const& query,
>> usc::SearchMetadata const& metadata)
>> {
>> - return usc::SearchQueryBase::UPtr(new Query(query, metadata, libertine_factory_));
>> + return usc::SearchQueryBase::UPtr(new Query(query, metadata, libertine_factory_, cache_directory(), get_bwlists(scope_directory())));
done
>
> This is a really long line. Could you please break it up?
>
>> }
>>
>>
>
>