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

[Dennis Nienhüser]

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

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