[Marble-devel] Review Request: Split class RunnerPlugin

Fri May 4 16:21:49 UTC 2012

> On May 4, 2012, 3:47 p.m., Dennis Nienhüser wrote:
> > Looks good to me from skimming over the changes. I'll test it when it's in master...
> > 
> > Some further thoughts (copied from IRC):
> > <Earthwings> hm, i wonder if we shouldn't have convenience setters for all these plugin properties like icon, name, guistring in the base class
> > <Earthwings> imho the code becomes much more readable if you don't have to implement those simple methods in each and every plugin
> > <Earthwings> i.e. you call setName( "foo" ); setGuiString( "bar" ); etc in the ctor of the plugin
> > <Earthwings> where each setter is protected and implemented in the base class
> > 
> > The automatic deactivation of the hostip plugin on smallscreen devices is lost by the patch, though this won't have an effect for the N900/N950/N9 as we don't install it there. We (or more general packagers) need to be aware of that though.
> >

Will fix the hostip plugin by overloading canWork().

> On May 4, 2012, 3:47 p.m., Dennis Nienhüser wrote:
> > src/lib/SearchRunnerPlugin.h, line 23
> > <http://git.reviewboard.kde.org/r/104840/diff/1/?file=62299#file62299line23>
> >
> >     wrong documentation

Fixed for all runner plugin types.

- Bernhard

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104840/#review13429
-----------------------------------------------------------

On May 3, 2012, 11:01 a.m., Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104840/
> -----------------------------------------------------------
> 
> (Updated May 3, 2012, 11:01 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> By splitting the class RunnerPlugin into separate classes for searching, routing, reverse geocoding and parsing, implementing a runner plugin will become more straight-forward. Before the patch, an implementer of e.g. a parsing plugin had to filter out unneccessary things such as which routing profile is supported.
> 
> Before the split could happen, the nominatim and gosmore runner plugins had to be split such that each new plugin had only one capability.
> 
> Please also review the user-visible strings of the nominatim and gosmore plugins.
> 
> Have a look at [1] to see the individual commits.
> 
> I'm looking forward to your reviews!
> 
> [1] https://github.com/shentok/marble/commits/RunnerPlugins-single-capability
> 
> 
> Diffs
> -----
> 
>   src/plugins/runner/gosmore/GosmoreRunner.cpp a5062be2a6ef55cae72b8fedfde675d89f73e53f 
>   src/plugins/runner/gosmore/GosmoreRunner.h 74b0400f8ee120328c7b94a4752436107d5c27b6 
>   src/plugins/runner/gosmore/CMakeLists.txt 984550686b4c184182d17027c44b966eda544e7c 
>   src/plugins/runner/gosmore/GosmorePlugin.h 6d9af2afea9ea21341b147e66320b298c6dd6c32 
>   src/plugins/runner/gosmore/GosmorePlugin.cpp 14a2d78b001bc203716c3402424657a19d9aace8 
>   src/plugins/runner/gosmore-routing/CMakeLists.txt PRE-CREATION 
>   src/plugins/runner/gosmore-routing/GosmoreRoutingPlugin.h PRE-CREATION 
>   src/plugins/runner/gosmore-routing/GosmoreRoutingPlugin.cpp PRE-CREATION 
>   src/plugins/runner/gosmore-routing/GosmoreRoutingRunner.h PRE-CREATION 
>   src/plugins/runner/gosmore-routing/GosmoreRoutingRunner.cpp PRE-CREATION 
>   src/lib/RunnerPlugin.cpp 31b89f6c2a88dba46cb6c7f6e2a8cc4ab796c548 
>   src/lib/RunnerTask.h a547f3caca23eefd88375718c85dcd9946b70cba 
>   src/lib/RunnerTask.cpp 5e2860e52dc2ff024cb49237d6e2a1073154c776 
>   src/lib/SearchRunnerPlugin.h PRE-CREATION 
>   src/lib/SearchRunnerPlugin.cpp PRE-CREATION 
>   src/lib/routing/RoutingManager.cpp 0e0d0ef49aa9ebffd720150907265068d6693d89 
>   src/lib/routing/RoutingProfileSettingsDialog.h 1402cc91904d9ac8cc825f64cc3a58bfd9bf1b7f 
>   src/lib/routing/RoutingProfileSettingsDialog.cpp 48f0f31ac070cbb16dc0fad7930f10b05370ceec 
>   src/lib/routing/RoutingProfilesModel.cpp 152b4ecd088e01cbb50536b4bec176a15884e7b8 
>   src/lib/routing/RoutingProfilesWidget.cpp 90bafb486f0cca8e34c2e4f2f56117f0e93b0a50 
>   src/plugins/runner/CMakeLists.txt edf0422e48a9393f10c6200b8ddfd5614beaafb2 
>   src/plugins/runner/cache/CachePlugin.h 8ef176a48d7a7439c96846150123cf4e947aa07d 
>   src/plugins/runner/cache/CachePlugin.cpp ce32c97e74baa6eabf10fa46c643bfab27e1f674 
>   src/plugins/runner/gosmore-reversegeocoding/CMakeLists.txt PRE-CREATION 
>   src/plugins/runner/gosmore-reversegeocoding/GosmoreReverseGeocodingPlugin.h PRE-CREATION 
>   src/plugins/runner/gosmore-reversegeocoding/GosmoreReverseGeocodingPlugin.cpp PRE-CREATION 
>   src/plugins/runner/gosmore-reversegeocoding/GosmoreReverseGeocodingRunner.h PRE-CREATION 
>   src/plugins/runner/gosmore-reversegeocoding/GosmoreReverseGeocodingRunner.cpp PRE-CREATION 
>   src/lib/RoutingRunnerPlugin.h PRE-CREATION 
>   src/lib/RoutingRunnerPlugin.cpp PRE-CREATION 
>   src/lib/RunnerPlugin.h f3f972132a26b85474d4c0cc8a0118cbdb864336 
>   src/lib/ReverseGeocodingRunnerPlugin.h PRE-CREATION 
>   src/lib/ReverseGeocodingRunnerPlugin.cpp PRE-CREATION 
>   src/lib/ParseRunnerPlugin.cpp PRE-CREATION 
>   src/lib/PluginManager.h 3b1ab1e3c7859317f18532abf541becea2a61964 
>   src/lib/PluginManager.cpp a2510ed4feeafc246dd175c7524ee558e62f80c1 
>   src/lib/CMakeLists.txt 5210329fb771254a8dc5ab19f7020d7145057401 
>   src/lib/MarbleRunnerManager.cpp c546bd4a140d7e875c984b4049fde69d74a30ea3 
>   src/lib/ParseRunnerPlugin.h PRE-CREATION 
>   src/plugins/runner/gpx/GpxPlugin.h 5ebc5c1130bc07bdb0c84c1f91879ef110e30c91 
>   src/plugins/runner/gpx/GpxPlugin.cpp 25d8eb1969ba6bd7dc19f3c2a58b03be7be01ddd 
>   src/plugins/runner/hostip/HostipPlugin.h 6b13bd2408a70394e2fb9d3e5443642c35fd12f8 
>   src/plugins/runner/hostip/HostipPlugin.cpp 002417da327b7b68795c56d0d53671be60236b6a 
>   src/plugins/runner/kml/KmlPlugin.h 29136bae8732ac6cfdd75e56b27331ba0180d9da 
>   src/plugins/runner/kml/KmlPlugin.cpp 00023f46c746fef1314a05aa363091aeb98c3d0c 
>   src/plugins/runner/latlon/LatLonPlugin.h 542a5d1148a4cdc67a1b463ce3d186fcadb9b099 
>   src/plugins/runner/latlon/LatLonPlugin.cpp 5900fc54c97fae39677c4703c99f3d1b5c9dff65 
>   src/plugins/runner/local-osm-search/LocalOsmSearchPlugin.h 0225813628509203c1da044bf4e5e9c07451c07d 
>   src/plugins/runner/local-osm-search/LocalOsmSearchPlugin.cpp 871f4ef1219c13422130a1afb6f8cf7069ec1209 
>   src/plugins/runner/localdatabase/LocalDatabasePlugin.h 1fec957ef4eb0d90dfcc6c344dc6eb735a4da6ff 
>   src/plugins/runner/localdatabase/LocalDatabasePlugin.cpp 0977bf5a16bfec74a42cf5cce71e85b2e15843ba 
>   src/plugins/runner/log/LogPlugin.h e08d67f5faf15e39dc52461aab698de00d4e2fe0 
>   src/plugins/runner/log/LogPlugin.cpp 0f9361b3ae0580e4996ed4626295874bace5d125 
>   src/plugins/runner/mapquest/MapQuestPlugin.h 49b6046cc7adfb4d5dc950148906df7c1e687236 
>   src/plugins/runner/mapquest/MapQuestPlugin.cpp 3d4d49f88011befdd474ecf7d3276a8c6c90c785 
>   src/plugins/runner/monav/MonavConfigWidget.h c0b9c7f3f798c70424f57a15df8da1f7bbcba4c2 
>   src/plugins/runner/monav/MonavConfigWidget.cpp 70b016bca206f227b172216ff88f1541f9787a67 
>   src/plugins/runner/monav/MonavPlugin.h 56661936da2bd624002a46c2f1cadb4bdd59b6bd 
>   src/plugins/runner/monav/MonavPlugin.cpp bcab1dd9c30dc2b2e9a2956d14a49f4b2fba3f1b 
>   src/plugins/runner/nominatim-reversegeocoding/CMakeLists.txt PRE-CREATION 
>   src/plugins/runner/nominatim-reversegeocoding/NominatimReverseGeocodingPlugin.h PRE-CREATION 
>   src/plugins/runner/nominatim-reversegeocoding/NominatimReverseGeocodingPlugin.cpp PRE-CREATION 
>   src/plugins/runner/nominatim-reversegeocoding/OsmNominatimReverseGeocodingRunner.h PRE-CREATION 
>   src/plugins/runner/nominatim-reversegeocoding/OsmNominatimReverseGeocodingRunner.cpp PRE-CREATION 
>   src/plugins/runner/nominatim-search/CMakeLists.txt PRE-CREATION 
>   src/plugins/runner/nominatim-search/NominatimSearchPlugin.h PRE-CREATION 
>   src/plugins/runner/nominatim-search/NominatimSearchPlugin.cpp PRE-CREATION 
>   src/plugins/runner/nominatim-search/OsmNominatimSearchRunner.h PRE-CREATION 
>   src/plugins/runner/nominatim-search/OsmNominatimSearchRunner.cpp PRE-CREATION 
>   src/plugins/runner/nominatim/CMakeLists.txt 461e3a4eb662c33c5ef0b29a34db1c08512661eb 
>   src/plugins/runner/nominatim/NominatimPlugin.h 22d8d10e01f4cf61df7d628a0be6fc19dc5d60cc 
>   src/plugins/runner/nominatim/NominatimPlugin.cpp 2a4ffed4a568c6a38578f88a0cd630d62acfcee3 
>   src/plugins/runner/nominatim/OsmNominatimRunner.h 43b498aa5b06278b141cdd6946da9d3aaab3bafc 
>   src/plugins/runner/nominatim/OsmNominatimRunner.cpp 8d1d0461e6ff67c830af4f2cdcf84b993698c523 
>   src/plugins/runner/open-source-routing-machine/OSRMPlugin.h d906bfdf6e104aa70fcc2dfa05849b94abe7d20d 
>   src/plugins/runner/open-source-routing-machine/OSRMPlugin.cpp c415699266056a78698228f22c8f882600a95080 
>   src/plugins/runner/openrouteservice/OpenRouteServicePlugin.h bdcbcb18a28e3951c1eb739635635ad27b89120e 
>   src/plugins/runner/openrouteservice/OpenRouteServicePlugin.cpp 65bb12323cfd2bca28612d77ea596c2184c55589 
>   src/plugins/runner/osm/OsmPlugin.h 472d1894a36d96c537f6554a66263f3b17a04aa7 
>   src/plugins/runner/osm/OsmPlugin.cpp bd15538f0cba74b79188d1d7f57a7a18671e37ad 
>   src/plugins/runner/pnt/PntPlugin.h e705fde173c28acaedcafbbe8a8c9de61202c935 
>   src/plugins/runner/pnt/PntPlugin.cpp aaa47a6be600c124a78ddae4ba9067efd969c675 
>   src/plugins/runner/routino/RoutinoPlugin.h f65c2e66f5d6db08677d78afefbd75435807a4a9 
>   src/plugins/runner/routino/RoutinoPlugin.cpp 05395790a2747ff5cb4d07bd834e41caa508b952 
>   src/plugins/runner/shp/ShpPlugin.h 7bf2d60fca21e066430919a2285770a7075940dc 
>   src/plugins/runner/shp/ShpPlugin.cpp 8896e4964fb23ae0764f7e88a61e80f5e3792e16 
>   src/plugins/runner/yours/YoursPlugin.h 9c2ce0f4d7db9e3e9b3dc3ac85c57aaeb7fbf29d 
>   src/plugins/runner/yours/YoursPlugin.cpp e3758e2656bf998e294d04072624c990ed23ff10 
>   tests/PluginManagerTest.cpp 7adaf2e0fb6095f6369924ef9781cec13380ff62 
> 
> Diff: http://git.reviewboard.kde.org/r/104840/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bernhard Beschow
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20120504/d1b1b05e/attachment-0001.html>