> On Aug. 13, 2015, 7:10 p.m., Dennis Nienhüser wrote: > > src/lib/marble/layers/GeometryLayer.cpp, line 277 > > <https://git.reviewboard.kde.org/r/124511/diff/4/?file=394341#file394341line277> > > > > Extending my previous comment wrt using qStableSort instead of qSort: I > > wonder if we should really sort the list here. Instead we could replace > > `items << items[i]->decorations();` > > above with something like > > ``` > > foreach ( GeoGraphicsItem* decoration, items[i]->decorations() ) { > > items.insert( i, decoration ); > > } > > ``` > > and iterate over items in the outer loop in reverse order (since we > > insert into it during traversal) > > > > The benefit is that we do not make any changes to the order except for > > the added decorations. Also we don't need a special zValue in the > > decorations, and the runtime here is reduced from O(n log n) due to the > > sorting to O(n). > > Dennis Nienhüser wrote: > Another thought on this: Wouldn't it be cleaner to have (instead of pure > virtual) > ``` > void GeoGraphicsItem::paint( GeoPainter *painter, const ViewportParams > *viewport ) > { > foreach( GeoGraphicsItem* decoration, decorations() ) { > decoration->paint( painter, viewport ); > } > } > ``` > and then change `GeoPolygonGraphicsItem::paint()` as well as > `GeoLineStringGraphicsItem::paint()` to call `GeoGraphicsItem::paint(...)` in > the first line. This seems even clearer as GeometryLayer needs no change at > all. > > Finally I really wonder if having the decoration as some sort of copy of > itself really is any beneficial. It could be done as private members just as > fine, no?
I'll drop the comments about changing the architecture. We can look into it later. - Dennis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124511/#review83780 ----------------------------------------------------------- On Aug. 13, 2015, 12:19 a.m., Dávid Kolozsvári wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124511/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2015, 12:19 a.m.) > > > Review request for Marble. > > > Repository: marble > > > Description > ------- > > I changed a little bit the decoration creating method, it now uses a QList to > store the decorations, so multiple decorations can be added this way. It was > an idea for the street labeling, but it makes sense without that too. > > > Diffs > ----- > > src/lib/marble/GeoPainter.h 7a757b9 > src/lib/marble/GeoPainter.cpp d04138c > src/lib/marble/GeoPainter_p.h f0c4f9b > src/lib/marble/MarbleGlobal.h cf2768f > src/lib/marble/geodata/data/GeoDataFeature.h ea23cd8 > src/lib/marble/geodata/data/GeoDataFeature.cpp 6f330fb > src/lib/marble/geodata/data/GeoDataFeature_p.h 496c356 > src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.h 4842809 > src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp 4320c07 > src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h f469dfb > src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp 81cfe9a > src/lib/marble/graphicsview/GeoGraphicsItem.h 4ca4727 > src/lib/marble/graphicsview/GeoGraphicsItem.cpp b8fa693 > src/lib/marble/graphicsview/GeoGraphicsItem_p.h 01becfc > src/lib/marble/layers/GeometryLayer.cpp 9eb3f50 > > Diff: https://git.reviewboard.kde.org/r/124511/diff/ > > > Testing > ------- > > It works on a freshly pulled version of Marble. > > > Thanks, > > Dávid Kolozsvári > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
