----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125377/#review88693 -----------------------------------------------------------
Looks good and works fine, just some style nitpicking. src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchPlugin.cpp (line 44) <https://git.reviewboard.kde.org/r/125377/#comment60800> Copy-paste error, please adjust src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h (line 30) <https://git.reviewboard.kde.org/r/125377/#comment60801> Please move to the private section. The method can be marked const also. src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h (line 36) <https://git.reviewboard.kde.org/r/125377/#comment60802> Method can be const src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h (line 41) <https://git.reviewboard.kde.org/r/125377/#comment60803> Method can be const src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 22) <https://git.reviewboard.kde.org/r/125377/#comment60804> Please remove and use a local variable in isValidOLC instead: ``` // It must have only one SEPARATOR located at an even index in // the string. QChar const separator('+'); int separatorPos = olc.indexOf(separator); ... ``` src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 23) <https://git.reviewboard.kde.org/r/125377/#comment60805> Please remove and use a local variable in isValidOLC instead: ``` int const separatorPosition = 8; // It must be a full open location code. ... ``` src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 24) <https://git.reviewboard.kde.org/r/125377/#comment60806> Please remove and use a local variable in isValidOLC instead: ``` // Test the characters before the SEPARATOR. QChar const suffixPadding('0'); ``` src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 25) <https://git.reviewboard.kde.org/r/125377/#comment60807> Please remove and use a local variable in the ctor: ``` // initialize the charIndex map QString const acceptedChars = "23456789CFGHJMPQRVWX"; for(int index = 0; index < acceptedChars.size(); ++index) { charIndex[acceptedChars[index]] = index; } ``` src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 54) <https://git.reviewboard.kde.org/r/125377/#comment60808> GeoDataStyle is now used as a shared ptr, please replace with ``` GeoDataStyle::Ptr style = GeoDataStyle::Ptr(new GeoDataStyle()); ``` src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 164) <https://git.reviewboard.kde.org/r/125377/#comment60809> Please use curly brackets {} also for one-liners src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 167) <https://git.reviewboard.kde.org/r/125377/#comment60810> Please use curly brackets {} also for one-liners src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 171) <https://git.reviewboard.kde.org/r/125377/#comment60811> Please use curly brackets {} also for one-liners src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 179) <https://git.reviewboard.kde.org/r/125377/#comment60814> Please use curly brackets {} also for one-liners src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 181) <https://git.reviewboard.kde.org/r/125377/#comment60812> Please use curly brackets {} also for one-liners src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp (line 183) <https://git.reviewboard.kde.org/r/125377/#comment60813> Please use curly brackets {} also for one-liners - Dennis Nienhüser On Sept. 24, 2015, 8:17 p.m., Constantin Mihalache wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125377/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2015, 8:17 p.m.) > > > Review request for Marble. > > > Bugs: 351308 > http://bugs.kde.org/show_bug.cgi?id=351308 > > > Repository: marble > > > Description > ------- > > Implemented the open location code search plugin as described on the > bugtracker. > > > Diffs > ----- > > src/plugins/runner/CMakeLists.txt 005f1b6 > src/plugins/runner/openlocation-code-search/CMakeLists.txt PRE-CREATION > src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchPlugin.h > PRE-CREATION > > src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchPlugin.cpp > PRE-CREATION > src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.h > PRE-CREATION > > src/plugins/runner/openlocation-code-search/OpenLocationCodeSearchRunner.cpp > PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/125377/diff/ > > > Testing > ------- > > Tested with the codes found on Google's > [repository](https://github.com/google/open-location-code). > > > Thanks, > > Constantin Mihalache > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
