Code review comment for lp:~peppujols/stellarium/printsky

Revision history for this message
Fabien Chéreau (xalioth) wrote :

Hi, thanks Bogdan for pinging me again and again.. Pep I'm really sorry for not reviewing the code before..

So here are some stuff I saw (I didn't look in detail at the GUI code; but it's OK you're responsible for it):

1- You canot do that!
-PROJECT(Stellarium)
+PROJECT(Printsky)
and that:
- #SET(CMAKE_OSX_ARCHITECTURES "x86_64")
+ SET(CMAKE_OSX_ARCHITECTURES "x86_64")

2- Do you really need all the #pragma mark xxxx ?

3- You will need to merge from trunk (there are some minor conflicts). I know it's my fault because it tool me too long to review this code :(

4- The changes in StarMgr are not needed I think. All you need is to compute the radius for various stars magnitudes. You should only use the StelSkyDrawer::computeRCMag method for each magnitude steps (and maybe suppress the steps if the radius is too small/big).

In theory you could do everything without touching at the core's code, except the CMakefiles to declare your plugin.

Fabien

review: Needs Fixing

« Back to merge proposal