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