> On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote: > > src/lib/marble/osm/OsmPlacemarkData.cpp, line 181 > > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388971#file388971line181> > > > > Did you consider moving from the QHash to a QVector (where the keys are > > implicitly given as (0...vector.size()-1) now that int's are used keys? At > > least here this would be much easier. > > Just wondering, I'm fine with keeping it like this if you prefer.
I actually considered moving to QVector, but that imposed a bunch of restrictions i didn't want: boundaries had to be inserted in order, all boundaries had to be inserted ( no boundary can be missing at any given time ), there's no negative index ( -1 for outerboundary ). There were a few more restrictions i encountered using Qvector i can't really rember right now. I'd like to keep the Qhash for the moment. > On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote: > > src/lib/marble/osm/OsmPlacemarkData.h, line 91 > > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388970#file388970line91> > > > > For the five `*Reference` method variants here I'd prefer to > > distinguish them by their names also, not just by the parameter: > > > > containsReference(Coordinates) => containsNodeReference > > containsReference(int) => containsMemberReference > > removeReference(Coordinates) => removeNodeReference > > removeReference(int) => removeMemberReference > > changeReference(GeoDataCoordinates,GeoDataCoordinates) => > > changeNodeReference(GeoDataCoordinates,GeoDataCoordinates) > > > > If you prefer shorter names, we could also do referencesNode, > > referencesMember, removeNode, removeMember, moveNode. > > > > Ctrl+Shift+R does a quick rename refactoring in QtCreator. moved to a separate patch > On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote: > > src/lib/marble/osm/OsmPlacemarkData.cpp, line 194 > > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388971#file388971line194> > > > > Shouldn't this one check whether newKey equals oldKey? In that case > > we'd end up removing both. moved to separate patch > On Aug. 3, 2015, 11:22 a.m., Dennis Nienhüser wrote: > > src/plugins/render/annotate/AreaAnnotation.cpp, line 272 > > <https://git.reviewboard.kde.org/r/124570/diff/1/?file=388972#file388972line272> > > > > Seems odd to use a comma initializer and only initialize one of the > > variables. the initialOsmData is not a pointer, that's why i didn't initialize it. I separated the declaration from the initialization into 2 lines - Marius ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124570/#review83373 ----------------------------------------------------------- On Aug. 16, 2015, 4:55 p.m., Marius Stanciu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124570/ > ----------------------------------------------------------- > > (Updated Aug. 16, 2015, 4:55 p.m.) > > > Review request for Marble. > > > Repository: marble > > > Description > ------- > > In order to achieve this, the following has been done: > > Wherever geometries are altered within the annotate plugin, I also modified > the osmPlacemarkData objects accordingly > Eg: -a node is deleted =======> a reference within the osmData for that node > is removed > -a node is moved =======> the reference of that node within the osmData > is changed to the new coordinates > > I also realised there's one extra case at writing time i haven't thought of: > Placemarks might have semi-initialized osmData. > Eg. a polygon is loaded from an ".osm" file with complete OsmData. In the > editor, a new innerBoundary is added. This new > boundary does not yet have osmData ). > > In order to take this case in consideration, I modified the OsmObjectManager > to check for such a case. > > > Diffs > ----- > > src/lib/marble/osm/OsmPlacemarkData.h b73a6a0 > src/lib/marble/osm/OsmPlacemarkData.cpp 5665ce5 > src/plugins/render/annotate/AreaAnnotation.cpp fa3bd16 > src/plugins/render/annotate/PolylineAnnotation.cpp 4ae08e8 > src/plugins/runner/osm/OsmObjectManager.cpp 8bebf0c > src/plugins/runner/osm/translators/OsmDocumentTagTranslator.cpp 3591b6d > > Diff: https://git.reviewboard.kde.org/r/124570/diff/ > > > Testing > ------- > > Tested every case: loading placemarks from files, creating placemarks, > modifying loaded placemarks in every way that i thought of. The results are > okay. > There are a lot of ways to modify placemarks, I might have skipped a few > please report any bugs :) > > > Thanks, > > Marius Stanciu > >
_______________________________________________ Marble-devel mailing list Marble-devel@kde.org https://mail.kde.org/mailman/listinfo/marble-devel
