[Kstars-devel] Review Request: My entire GSoC work till now. What's Interesting works for planets, bright stars and constellations now.

Sun Aug 5 09:56:07 UTC 2012

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > Again, some minor imperfections. Could you start writing comments to your code right now? Try to use Doxygen.
> > 
> > Apart from that, congratulations on your good work! Keep it that way!
> >

Yeah, I'm starting to comment my code now. And yes, thank you. :)

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/data/PlanetFacts.dat, line 1
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75499#file75499line1>
> >
> >     So, we're sure that we can use those descriptions?

I think so.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.h, line 31
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75507#file75507line31>
> >
> >     Be consistent with formatting.

Taken care of.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.h, line 32
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75507#file75507line32>
> >
> >     Also, if you're using "get*" prefix for getters, why should you use "return*" prefix here?

Right. Thanks for pointing this out. This is one of the earliest methods I wrote.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.cpp, line 136
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75508#file75508line136>
> >
> >     Stray whitespace.

Removed.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.cpp, line 71
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75508#file75508line71>
> >
> >     Use enum values instead of integers, it's more readable.

Should I be using (int)SkyObject::Type instead?

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.cpp, line 166
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75508#file75508line166>
> >
> >     Shouldn't we rather return 0 in that case? Now it's possible to have memleaks it this method is not used properly.

Yeah.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.cpp, line 170
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75508#file75508line170>
> >
> >     const QString &type?

Removed that method.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.cpp, line 197
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75508#file75508line197>
> >
> >     Will never reach here.

Have removed that.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/obsconditions.h, line 38
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75509#file75509line38>
> >
> >     m_*?

Yes

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/obsconditions.cpp, line 24
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75510#file75510line24>
> >
> >     Init list maybe?

Yes.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/obsconditions.cpp, line 29
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75510#file75510line29>
> >
> >     I don't really like switch(something = something_different), but that's only my opinion.

Initialised that in the init list.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/obsconditions.cpp, line 37
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75510#file75510line37>
> >
> >     Not really needed.

Removed that.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/obsconditions.cpp, line 45
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75510#file75510line45>
> >
> >     Inline maybe?

Yeah.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/wiview.h, line 39
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75529#file75529line39>
> >
> >     m_*

Done.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/skyobjitem.h, line 48
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75522#file75522line48>
> >
> >     Why not m_So? ;-)

Yes, why not? Fixed.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.h, line 36
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75507#file75507line36>
> >
> >     m_*?

Yes.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/modelmanager.h, line 39
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75507#file75507line39>
> >
> >     Fix this formatting ;-)

Done.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/obsconditions.h, line 26
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75509#file75509line26>
> >
> >     Have you thought about using parameters of user-defined equipment?

Yes I have thought about it. Though have done nothing to actually implement it. :|

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/skyobjitem.h, line 27
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75522#file75522line27>
> >
> >     Why should SkyObjItem inherit QObject?

SkyObjItem should not inherit QObject.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/skyobjlistmodel.h, line 25
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75524#file75524line25>
> >
> >     Comments.

Yes, adding them.

> On Aug. 2, 2012, 7:25 p.m., Rafal Kulaga wrote:
> > kstars/tools/whatsinteresting/wiview.h, line 26
> > <http://git.reviewboard.kde.org/r/105481/diff/2/?file=75529#file75529line26>
> >
> >     Comments.

Documentation done for class WIView.

- Samikshan

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

On July 31, 2012, 10:21 a.m., Samikshan Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105481/
> -----------------------------------------------------------
> 
> (Updated July 31, 2012, 10:21 a.m.)
> 
> 
> Review request for KStars, Rafal Kulaga and Akarsh Simha.
> 
> 
> Description
> -------
> 
> QML user interface for testing purpose ready. This shows planets, bright stars, constellations and deep-sky object. Description not yet available for bright stars and constellations. The details-view is not ready for DSOs. Also the list of DSOs displayed is not the filtered or interesting list.
> 
> 
> Diffs
> -----
> 
>   kstars/CMakeLists.txt 8c228de 
>   kstars/data/CMakeLists.txt 0dc591b 
>   kstars/data/Interesting.dat PRE-CREATION 
>   kstars/data/PlanetFacts.dat PRE-CREATION 
>   kstars/kstars.h c9e4261 
>   kstars/kstars.cpp 02661bb 
>   kstars/kstarsactions.cpp d2260ef 
>   kstars/kstarsinit.cpp 5bcefa2 
>   kstars/kstarsui-indi.rc d5a038a 
>   kstars/kstarsui-win.rc eb23e82 
>   kstars/kstarsui.rc ee56cf3 
>   kstars/tools/whatsinteresting/modelmanager.h PRE-CREATION 
>   kstars/tools/whatsinteresting/modelmanager.cpp PRE-CREATION 
>   kstars/tools/whatsinteresting/obsconditions.h PRE-CREATION 
>   kstars/tools/whatsinteresting/obsconditions.cpp PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/Button.qml PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/CMakeLists.txt PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/cluster.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/constellation.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/dso.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/galaxy.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/nebula.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/planets.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/stars.jpg PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/stripes.png PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/wiview.qml PRE-CREATION 
>   kstars/tools/whatsinteresting/skyobjitem.h PRE-CREATION 
>   kstars/tools/whatsinteresting/skyobjitem.cpp PRE-CREATION 
>   kstars/tools/whatsinteresting/skyobjlistmodel.h PRE-CREATION 
>   kstars/tools/whatsinteresting/skyobjlistmodel.cpp PRE-CREATION 
>   kstars/tools/whatsinteresting/wiusersettings.h PRE-CREATION 
>   kstars/tools/whatsinteresting/wiusersettings.cpp PRE-CREATION 
>   kstars/tools/whatsinteresting/wiusersettings.ui PRE-CREATION 
>   kstars/tools/whatsinteresting/wiview.h PRE-CREATION 
>   kstars/tools/whatsinteresting/wiview.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105481/diff/
> 
> 
> Testing
> -------
> 
> Testing done for planets, constellations, bright stars. Details-view not yet ready for DSOs.
> 
> 
> Thanks,
> 
> Samikshan Bairagya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kstars-devel/attachments/20120805/f078f1c9/attachment-0001.html>