[Kstars-devel] Re: Review Request: Legend feature (currently used only for exported images)

[Rafal Kulaga]

> On July 11, 2011, 2:15 a.m., Akarsh Simha wrote:
> > kstars/legend.cpp, line 35
> > <http://git.reviewboard.kde.org/r/101912/diff/1/?file=26665#file26665line35>
> >
> >     KStars is now a single-instance class, so you can just use KStars::Instance(). Saves passing one parameter.

Done.


> On July 11, 2011, 2:15 a.m., Akarsh Simha wrote:
> > kstars/legend.cpp, line 54
> > <http://git.reviewboard.kde.org/r/101912/diff/1/?file=26665#file26665line54>
> >
> >     You could inline all of these right? Probably, the compiler will be smart enough to do that anyway, though. But it also removes some clutter if you put them as one-liners in the header file.

Done. I think that in this case compiler wouldn't inline this methods, because AFAIK in C++ you have to define an inline function in every compilation unit that is using it - this requirement will be satisfied if function body is defined in header file, but not when it's defined in *.cpp file. 


> On July 11, 2011, 2:15 a.m., Akarsh Simha wrote:
> > kstars/legend.cpp, line 470
> > <http://git.reviewboard.kde.org/r/101912/diff/1/?file=26665#file26665line470>
> >
> >     Maybe you should instead do a Q_ASSERT(false) so that we crash (I usually think it's better to crash and make sure the programmer fixes the problem than returning a non-result that might be harder to detect :D)

Good point, I'll do this :-)


> On July 11, 2011, 2:15 a.m., Akarsh Simha wrote:
> > kstars/skymap.h, line 267
> > <http://git.reviewboard.kde.org/r/101912/diff/1/?file=26666#file26666line267>
> >
> >     Is this still used? Or can it be done away with?

We'll see - now it's only convenience method for SkyMap::exportSkyImage( SkyQPainter *painter ), which enables exporting without explicit creation of SkyQPainter instance.


> On July 11, 2011, 2:15 a.m., Akarsh Simha wrote:
> > kstars/skyqpainter.cpp, line 411
> > <http://git.reviewboard.kde.org/r/101912/diff/1/?file=26671#file26671line411>
> >
> >     Makes sense!

Yep, that was needed to avoid creating "fake objects" and drawing them, which isn't a good idea IMO.


- Rafal


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


On July 10, 2011, 10:38 p.m., Rafal Kulaga wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101912/
> -----------------------------------------------------------
> 
> (Updated July 10, 2011, 10:38 p.m.)
> 
> 
> Review request for KStars, Victor Carbune and Akarsh Simha.
> 
> 
> Summary
> -------
> 
> Attached diff adds the option to include legends in exported sky images. Both vertical and horizontal orientations are supported; there are two types of legend: full-blown (symbol descriptions, star magnitudes and scale) and scale-only. Some changes will inevitably be made to this code in a few days - please note that there are some hard-coded values in legend.cpp which will be gone after integration with the functionality I am developing now (FOV representation exporting).
> 
> Any comments are welcome, be they look&feel or code-related.
> 
> 
> Diffs
> -----
> 
>   kstars/CMakeLists.txt 0c335b6 
>   kstars/dialogs/exportimagedialog.h PRE-CREATION 
>   kstars/dialogs/exportimagedialog.cpp PRE-CREATION 
>   kstars/dialogs/exportimagedialog.ui PRE-CREATION 
>   kstars/kstarsactions.cpp e917cac 
>   kstars/kstarsdcop.cpp 42dcb0f 
>   kstars/legend.h PRE-CREATION 
>   kstars/legend.cpp PRE-CREATION 
>   kstars/skymap.h e7a7f56 
>   kstars/skymapdrawabstract.h c94a745 
>   kstars/skymapdrawabstract.cpp cf44fc5 
>   kstars/skypainter.h 1340568 
>   kstars/skyqpainter.h df7cc9b 
>   kstars/skyqpainter.cpp 87719b2 
> 
> Diff: http://git.reviewboard.kde.org/r/101912/diff
> 
> 
> Testing
> -------
> 
> Done some testing, everything worked (and looked) fine for me.
> 
> 
> Thanks,
> 
> Rafal
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kstars-devel/attachments/20110712/46c59fcd/attachment-0001.htm