Merge lp:~mardy/location-service/providers-dir into lp:location-service/trunk
Status: | Work in progress |
---|---|
Proposed branch: | lp:~mardy/location-service/providers-dir |
Merge into: | lp:location-service/trunk |
Diff against target: |
890 lines (+420/-141) 20 files modified
include/location_service/com/ubuntu/location/default_provider_selection_policy.h (+4/-4) include/location_service/com/ubuntu/location/provider_collection.h (+34/-12) include/location_service/com/ubuntu/location/provider_selection_policy.h (+2/-3) src/location_service/com/ubuntu/location/CMakeLists.txt (+2/-0) src/location_service/com/ubuntu/location/default_provider_selection_policy.cpp (+10/-10) src/location_service/com/ubuntu/location/engine.h (+6/-25) src/location_service/com/ubuntu/location/filesystem_provider_loader.cpp (+97/-0) src/location_service/com/ubuntu/location/filesystem_provider_loader.h (+69/-0) src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.cpp (+2/-2) src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.h (+1/-1) src/location_service/com/ubuntu/location/provider_loader.h (+54/-0) src/location_service/com/ubuntu/location/provider_manifest.cpp (+45/-0) src/location_service/com/ubuntu/location/provider_manifest.h (+76/-0) src/location_service/com/ubuntu/location/service/daemon.cpp (+14/-70) src/location_service/com/ubuntu/location/service/daemon.h (+0/-4) src/location_service/com/ubuntu/location/service/default_configuration.cpp (+1/-6) src/location_service/com/ubuntu/location/service/default_configuration.h (+0/-1) tests/engine_test.cpp (+1/-1) tests/null_provider_selection_policy.h (+1/-1) tests/provider_selection_policy_test.cpp (+1/-1) |
To merge this branch: | bzr merge lp:~mardy/location-service/providers-dir |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Thomas Voß (community) | Needs Fixing | ||
Review via email: mp+280724@code.launchpad.net |
Description of the change
WIP
Manifest-based provider loading
Don't hardcode the list of available providers. Instead, scan some directories from the filesystem ($XDG_DATA_
The goal is to allow a simple way of configuring providers; for instance, we might have a file
/usr/
with these contents:
{
"plugin": "gls",
"configurat
"endpoint": "https:/
"api-key": "xyz"
}
}
which will cause the location service to load a "gls" plugin and initialize it with the given configuration, suitable for the Google service. Also, a user might create a file
~/.local/
with these contents:
{
"plugin": "gls",
"configurat
"endpoint": "https:/
"api-key": "abc"
}
}
By doing this, the user is asking ULS to create a second instance of the "gls" plugin, this time setup to use the similar service provided by Mozilla.
The ULS remembers which manifest files have been loaded (using the basename of the manifest file as a key), and will refuse to load the same provider twice. So, continuing our example above, the user could create a file
~/.local/
which would override the system one. This can be useful when a different configuration is desired (for example, we the plugin might accept a configuration option to decide which of WIFI, CellId, or GeoIP positioning methods are allowed, and the user might want to force a specific one).
Some providers are built into the location service; in order to enable them, their plugin name must be prefixed with a "@". So, to enable the GPS provider, we should create a file
/usr/
with contents:
{
"plugin": "@gps"
}
(note that the filename is irrelevant in this case: what matters is the value of the "plugin" key)
The current implementation assumes that if the plugin name does not start with a "@", the plugin must be loaded using the "remote::Provider"; so, in the "configuration" key one should store the options needed by the remote provider. So, for example:
{
"plugin": "espoo",
"configurat
"name": "com.ubuntu.
"path": "/com/ubuntu/
}
}
========== OPEN QUESTIONS =========
The current WIP implementation still supports passing provider options from the command line (and actually, requires that at least one provider is specified). Command line options do override those from the manifest file. However, despite having implemented this, I think that there is no point in having this code, and I'd propose removing it and letting all the provider configuration happen in the manifest. Do you agree?
Then, about remote providers, to make landing this branch as easy as possible, I'd propose (at least for the time being) to keep the existing mechanism of loading remove providers via the remote::Provider interface; we can actually get rid of the D-Bus name and path from the "configuration" key and always assume that they are in the form:
"com.
"/com/
where "<plugin>" is the value of the "plugin" key in the manifest files.
In the long term, I'd like to get rid of D-Bus and just use a socket (maybe with a p2p D-Bus server on top of it, to minimize changes -- but we should add this to dbuscpp first), because there's no need to route all location updates through the server when all we want is actually a p2p communication. But that's for the far future.
Unmerged revisions
- 238. By Alberto Mardegan
-
Don't return reference to temporary
- 237. By Alberto Mardegan
-
no providers from command line
- 236. By Alberto Mardegan
-
Add override clause
- 235. By Alberto Mardegan
-
Remove is_internal method
- 234. By Alberto Mardegan
-
Use boost::fs:path in public methods
- 233. By Alberto Mardegan
-
Make load_providers() stateless
- 232. By Alberto Mardegan
-
Pass dir list in constructor
- 231. By Alberto Mardegan
-
Simplify consdruction of path list
- 230. By Alberto Mardegan
-
Remove provider_options from load_providers
- 229. By Alberto Mardegan
-
Move provider_loader to src/
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 :)