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

_______________________________________________
Marble-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/marble-devel

Reply via email to