[Merge] lp:~mardy/location-service/providers-dir into lp:location-service
Thomas Voß
thomas.voss at canonical.com
Fri Dec 18 10:34:13 UTC 2015
Review: Needs Fixing
Thanks for the proposal, the general approach looks fine, I left a couple of comments inline.
For your questions:
(1.) Let's not keep redundant code paths around and go all in.
(2.) Please keep the keys, and let's avoid implicit conventions only visible in code.
In addition, let's load providers with { plugin: remote; configuration { plugin ... }}.
While being a little more verbose, it is absolutely clear what is happening just from
reading the configuration file.
(3.) We can think about spawning a private bus instance, but I disagree that the service should
become a communication hub and take over responsibilities that are in the area of the bus
daemon or the respective kernel facilities once kdbus is widely available.
With (2.), I would also propose to get rid of the "@" prefix convention as indicated in the corresponding inline comment.
Looking forward to see this landing in the not-too-distant future :)
Diff comments:
>
> === added file 'include/location_service/com/ubuntu/location/provider_loader.h'
Please move this to src, this class is not meant for public consumption. And yes, the include dir has way too many headers right now :) Something that we should clean up.
> --- include/location_service/com/ubuntu/location/provider_loader.h 1970-01-01 00:00:00 +0000
> +++ include/location_service/com/ubuntu/location/provider_loader.h 2015-12-16 14:57:22 +0000
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser 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 Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Alberto Mardegan <alberto.mardegan at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_PROVIDER_LOADER_H_
> +#define LOCATION_SERVICE_COM_UBUNTU_PROVIDER_LOADER_H_
> +
> +#include <com/ubuntu/location/configuration.h>
> +
> +#include <memory>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +class ProviderCollection;
> +class ProviderLoader
> +{
> +public:
> + ProviderLoader(const ProviderLoader&) = delete;
> + virtual ~ProviderLoader() = default;
> +
> + ProviderLoader& operator=(const ProviderLoader&) = delete;
> + bool operator==(const ProviderLoader&) const = delete;
> +
> + /**
> + * @brief Loads available providers.
> + * @param base_dir The directory which will be scanned.
> + * @param provider_options Map of options for each provider.
> + * @param collection The provider collection where found providers will be added.
> + */
> + virtual void load_providers(const std::string& base_dir,
Adding base-dir to the top-level interface feels like a self-fulfilling prophecy :) I would propose that you remove base_dir here, and make it a construction time parameter of the concrete implementation assembling the provider config by iterating over directory contents.
> + const std::map<std::string, Configuration>& provider_options,
Let's get rid of this and make it either or. That should also address your question about removing the respective code paths.
> + const std::shared_ptr<ProviderCollection>& collection) = 0;
One of the MPs in flight (about to land) renders the loading of providers as an async operation. With that, I would prefer ProviderLoader to have an inner interface class Receiver { virtual void provider_finished_loading_succssfully(const Provider::Ptr&) = 0; virtual void provider_finished_loading_with_error(const Configuration& config, const Error& error), with Error being an abstract type providing a const std::string& name(), a const std::string& description() and an int code() for easy comparison. With that, we could change the signature to load_providers(const std::shared_ptr<Receiver>& receiver).
> +
> +protected:
> + ProviderLoader() = default;
> +};
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_PROVIDER_LOADER_H_
>
> === modified file 'src/location_service/com/ubuntu/location/engine.h'
> --- src/location_service/com/ubuntu/location/engine.h 2015-04-23 14:48:44 +0000
> +++ src/location_service/com/ubuntu/location/engine.h 2015-12-16 14:57:22 +0000
> @@ -150,28 +150,9 @@
> */
> virtual ProviderSelection determine_provider_selection_for_criteria(const Criteria& criteria);
>
> - /**
> - * @brief Checks if the engine knows about a specific provider.
> - * @return True iff the engine knows about the provider.
> - */
> virtual bool has_provider(const Provider::Ptr& provider) noexcept;
Mind adding override to the method signature?
> -
> - /**
> - * @brief Makes a provider known to the engine.
> - * @param provider The new provider.
> - */
> virtual void add_provider(const Provider::Ptr& provider);
Mind adding override to the method signature?
> -
> - /**
> - * @brief Removes a provider from the engine.
> - * @param provider The provider to be removed.
> - */
> virtual void remove_provider(const Provider::Ptr& provider) noexcept;
Mind adding override to the method signature?
> -
> - /**
> - * @brief Iterates all known providers and invokes the provided enumerator for each of them.
> - * @param enumerator The functor to be invoked for each provider.
> - */
> virtual void for_each_provider(const std::function<void(const Provider::Ptr&)>& enumerator) const noexcept;
Mind adding override to the method signature?
>
> /** @brief The engine's configuration. */
>
> === added file 'src/location_service/com/ubuntu/location/filesystem_provider_loader.h'
> --- src/location_service/com/ubuntu/location/filesystem_provider_loader.h 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/filesystem_provider_loader.h 2015-12-16 14:57:22 +0000
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser 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 Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Alberto Mardegan <alberto.mardegan at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_FILESYSTEM_PROVIDER_LOADER_H_
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_FILESYSTEM_PROVIDER_LOADER_H_
> +
> +#include <com/ubuntu/location/provider.h>
> +#include <com/ubuntu/location/provider_collection.h>
> +#include <com/ubuntu/location/provider_loader.h>
> +
> +#include <com/ubuntu/location/settings.h>
> +
> +#include <core/property.h>
> +
> +#include <functional>
> +#include <set>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +/**
> + * @brief The FilesystemProviderLoader class is used to enumerate and load all
> + * available providers into a ProviderCollection.
> + */
> +class FilesystemProviderLoader : public ProviderLoader
> +{
> +public:
> + typedef std::shared_ptr<FilesystemProviderLoader> Ptr;
> +
> + FilesystemProviderLoader();
This should take const std::set<boost::filesystem::path>& base_dir at construction time.
> +
> + FilesystemProviderLoader(const FilesystemProviderLoader&) = delete;
> + FilesystemProviderLoader& operator=(const FilesystemProviderLoader&) = delete;
> + virtual ~FilesystemProviderLoader();
> +
> + virtual void load_providers(const std::string& base_dir,
> + const std::map<std::string, Configuration>& provider_options,
> + const std::shared_ptr<ProviderCollection>& collection) override;
> +
> +private:
> + void load_provider(const std::string& fn,
> + const std::map<std::string, Configuration>& provider_options,
> + const std::shared_ptr<ProviderCollection>& collection);
> +
> + std::set<std::string> instantiated_providers;
Why does this have to be stateful?
> +};
> +
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_FILESYSTEM_PROVIDER_LOADER_H_
>
> === added file 'src/location_service/com/ubuntu/location/provider_manifest.h'
> --- src/location_service/com/ubuntu/location/provider_manifest.h 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/provider_manifest.h 2015-12-16 14:57:22 +0000
> @@ -0,0 +1,75 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser 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 Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Alberto Mardegan <alberto.mardegan at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_PROVIDER_MANIFEST_H_
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_PROVIDER_MANIFEST_H_
> +
> +#include <boost/property_tree/ptree.hpp>
> +
> +#include <com/ubuntu/location/configuration.h>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +/**
> + * @brief The Provider class is the abstract base of all positioning providers.
Copy'n'Paste artifact?
> + */
> +class ProviderManifest
> +{
> +public:
> + /**
> + * @brief Loads a provider manifest file.
> + */
> + ProviderManifest(const std::string& fn);
Please make this take a const boost::filesystem::path& p, std::string is too generic here. Also: I would prefer to have a ProviderManifest(std::istream& in), with ProviderManifest(const boost::filesystem::path&) just opening the file and passing in the std::ifstream to the other ctor.
> + ProviderManifest(const ProviderManifest&) = delete;
> + ProviderManifest& operator=(const ProviderManifest&) = delete;
> + virtual ~ProviderManifest() = default;
> +
> + std::string name() const { return name_; }
Please make name and plugin_name return const std::string& references. Also: I don't see name_ being set anywhere, seems to be a leftover.
> + std::string plugin_name() const;
> + bool is_internal() const;
Let's get rid of this and be explicit about remote vs. local providers. Also see my introductory comment.
> +
> + const Configuration &configuration() const;
> +
> +private:
> + /** Keys used in provider manifest files. */
> + struct Keys
> + {
> + /** Key for plugin name */
> + static constexpr const char* plugin
> + {
> + "plugin"
> + };
> + /** Key for provider configuration */
> + static constexpr const char* configuration
> + {
> + "configuration"
> + };
> + };
> + boost::property_tree::ptree tree;
> + std::string name_;
> + Configuration empty_configuration;
> +};
> +
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_PROVIDER_MANIFEST_H_
>
> === modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
> --- src/location_service/com/ubuntu/location/service/daemon.cpp 2015-04-16 10:03:29 +0000
> +++ src/location_service/com/ubuntu/location/service/daemon.cpp 2015-12-16 14:57:22 +0000
> @@ -240,6 +217,21 @@
> }
> };
>
> + // TODO: Use xdg::Data once available as a shared library
> + namespace fs = boost::filesystem;
> + std::vector<fs::path> data_dirs;
This is cosmetics, but auto data_dirs = { fs::path("/usr/share/location/providers"), fs::path{std::getenv("HOME")} / ".local/share/location/providers" }; looks a lot nicer to me :)
> + data_dirs.push_back("/usr/share/location/providers");
> + auto v = fs::path{std::getenv("HOME")} / ".local/share/location/providers";
> + data_dirs.push_back(v);
> +
> + FilesystemProviderLoader loader;
> + for (const auto& dir: data_dirs)
> + {
> + loader.load_providers(dir.string(),
> + config.provider_options,
> + configuration.engine);
> + }
> +
> auto location_service = std::make_shared<location::service::Implementation>(configuration);
> // We need to ensure that any exception raised by the executor does not crash the app
> // and also gets logged.
--
https://code.launchpad.net/~mardy/location-service/providers-dir/+merge/280724
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.
More information about the Ubuntu-reviews
mailing list