[Marble-devel] Review Request 123181: Added Right placement for labels and placemark sorting

[Dennis Nienhüser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123181/#review78214
-----------------------------------------------------------



src/lib/marble/PlacemarkLayout.cpp (line 43)
<https://git.reviewboard.kde.org/r/123181/#comment53590>

    I'd call it `hasRoomFor` for readability and pass both parameters as const references for performance.



src/lib/marble/PlacemarkLayout.cpp (line 53)
<https://git.reviewboard.kde.org/r/123181/#comment53588>

    directly `return false;` here?



src/lib/marble/PlacemarkLayout.cpp (line 57)
<https://git.reviewboard.kde.org/r/123181/#comment53589>

    and `return true;` here and spare the isRoom boolean?



src/lib/marble/PlacemarkLayout.cpp (line 667)
<https://git.reviewboard.kde.org/r/123181/#comment53591>

    Maybe instead *Check up to seven different vertical positions*?
    
    Is there an easy way to test this (see it in action)? I suspect plain Marble does not make use of this code path yet. Specifically I wonder if -3...+3 vertical distances might be too far away already.



src/lib/marble/PlacemarkLayout.cpp (line 672)
<https://git.reviewboard.kde.org/r/123181/#comment53592>

    add a note here that integer arithmetic is intended and needed?



src/lib/marble/geodata/data/GeoDataLabelStyle.h (line 39)
<https://git.reviewboard.kde.org/r/123181/#comment53593>

    @all: I don't like our flags here, seems inconsistent and also lacks documentation.


- Dennis Nienhüser


On March 30, 2015, 11:30 a.m., Adam Dabrowski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123181/
> -----------------------------------------------------------
> 
> (Updated March 30, 2015, 11:30 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> -Added Right position for placemark labels. Up to 7 positions are available for labels if placemarks overlap.
> -Added sorting of placemarks before layouting. Without this there would be labels "jumping" around as several moving placemarks overlap (now they are drawn in the same order and there is no jumping). Cost of sorting is miniscule compared to other operations performed when layouting.
> -Refactored code for checking if layout space is already occupied
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/PlacemarkLayout.cpp 6c0af30 
>   src/lib/marble/geodata/data/GeoDataLabelStyle.h 046f7bc 
>   src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp 5949c7d 
> 
> Diff: https://git.reviewboard.kde.org/r/123181/diff/
> 
> 
> Testing
> -------
> 
> Displayed 1, 2, 3 and 7+ items.
> Checked for no flickering.
> 
> 
> Thanks,
> 
> Adam Dabrowski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150330/fede319d/attachment.html>