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

Revision history for this message
Pep Pujols (peppujols) wrote :

Hi.

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

Yes, sorry.

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

No, I removed this code.

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

Done.

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

Ok. I removed the method from StarMgr and I have placed it in my
pluguin, with simplified code.

I resubmited to proposal to merge again.

Regards.

Pep.

« Back to merge proposal