[Marble-devel] Review Request 124511: Merged the decoration(124154) and the label placement(124498) review requests.

[Dávid Kolozsvári]

> 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?
> 
> Dennis Nienhüser wrote:
>     I'll drop the comments about changing the architecture. We can look into it later.

Ok, but I like the idea, I would like to work on this later :)


- Dávid


-----------------------------------------------------------
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
> 
>

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