[Marble-devel] Review Request: Task 3 from Trello: separate IDL, poles and lineStringToPolygon into CylindricalProjections

[Thibaut Gridel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105261/#review14917
-----------------------------------------------------------


Good patch, here are some nitpicks and further simplification possible for the cylindrical case.


src/lib/Projections/CylindricalProjection.cpp
<http://git.reviewboard.kde.org/r/105261/#comment11713>

    Please check with your editor that you don't add extra space at the end of lines...



src/lib/Projections/CylindricalProjection.cpp
<http://git.reviewboard.kde.org/r/105261/#comment11714>

    Tab or extra spaces?



src/lib/Projections/CylindricalProjection.cpp
<http://git.reviewboard.kde.org/r/105261/#comment11712>

    Imho, globeHidesPoint could not happen in a cylindrical projection, so the method could simply ignore that value returned from screenCoordinates.
    There are other parts further down that do case for it.
    
    Please check before whether this could happen at all?



src/lib/Projections/CylindricalProjection.cpp
<http://git.reviewboard.kde.org/r/105261/#comment11715>

    tab?



src/lib/Projections/SphericalProjection.cpp
<http://git.reviewboard.kde.org/r/105261/#comment11711>

    Please remove those comments if not applicable anymore


- Thibaut Gridel


On June 16, 2012, 12:06 p.m., Cezar Mocan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105261/
> -----------------------------------------------------------
> 
> (Updated June 16, 2012, 12:06 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser, Torsten Rahn, and Thibaut Gridel.
> 
> 
> Description
> -------
> 
> Separate AbstractProjectionPrivate::lineStringToPolygon into CylindricalProjectionPrivate::lineStringToPolygon and SphericalProjectionPrivate::lineStringToPolygon in order to get rid of the corner cases. 
> It is not final, right now I have just commented the lines which need to be removed in my opinion. Also, I would need some advice regarding what else needs to be deleted from CylindricalProjectionPrivate::lineStringToPolygon. 
> 
> 
> Diffs
> -----
> 
>   src/lib/Projections/AbstractProjection_p.h d8f6063 
>   src/lib/Projections/CylindricalProjection.h affd1e6 
>   src/lib/Projections/CylindricalProjection.cpp c103a68 
>   src/lib/Projections/AbstractProjection.cpp 92ae3dd 
>   src/lib/Projections/CylindricalProjection_p.h PRE-CREATION 
>   src/lib/Projections/SphericalProjection.h 75ea797 
>   src/lib/Projections/SphericalProjection.cpp 0978af4 
>   src/lib/Projections/SphericalProjection_p.h PRE-CREATION 
>   src/lib/Projections/provisionalPatch.diff 958d079 
> 
> Diff: http://git.reviewboard.kde.org/r/105261/diff/
> 
> 
> Testing
> -------
> 
> Marble seems to work properly with all 3 projections after applying the patch. MercatorProjectionTest passes all the 5 tests. How else could I test the change?
> 
> 
> Thanks,
> 
> Cezar Mocan
> 
>

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