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

Thu Aug 2 19:25:20 UTC 2012

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

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!

kstars/data/CMakeLists.txt
<http://git.reviewboard.kde.org/r/105481/#comment13096>

    Please make it prettier by fixing those ugly whitespaces.

kstars/data/PlanetFacts.dat
<http://git.reviewboard.kde.org/r/105481/#comment13097>

    So, we're sure that we can use those descriptions?

kstars/tools/whatsinteresting/modelmanager.h
<http://git.reviewboard.kde.org/r/105481/#comment13098>

    Be consistent with formatting. 

kstars/tools/whatsinteresting/modelmanager.h
<http://git.reviewboard.kde.org/r/105481/#comment13099>

    Also, if you're using "get*" prefix for getters, why should you use "return*" prefix here?

kstars/tools/whatsinteresting/modelmanager.h
<http://git.reviewboard.kde.org/r/105481/#comment13100>

    m_*?

kstars/tools/whatsinteresting/modelmanager.h
<http://git.reviewboard.kde.org/r/105481/#comment13101>

    Fix this formatting ;-)

kstars/tools/whatsinteresting/modelmanager.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13102>

    Use enum values instead of integers, it's more readable.

kstars/tools/whatsinteresting/modelmanager.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13104>

    Stray whitespace.

kstars/tools/whatsinteresting/modelmanager.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13105>

    Shouldn't we rather return 0 in that case? Now it's possible to have memleaks it this method is not used properly.

kstars/tools/whatsinteresting/modelmanager.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13106>

    const QString &type?

kstars/tools/whatsinteresting/modelmanager.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13107>

    Will never reach here.

kstars/tools/whatsinteresting/obsconditions.h
<http://git.reviewboard.kde.org/r/105481/#comment13108>

    Have you thought about using parameters of user-defined equipment?

kstars/tools/whatsinteresting/obsconditions.h
<http://git.reviewboard.kde.org/r/105481/#comment13109>

    m_*?

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13110>

    Init list maybe?

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13111>

    I don't really like switch(something = something_different), but that's only my opinion.

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13112>

    Not really needed.

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13113>

    Inline maybe?

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13114>

    Same here.

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13115>

    break is not needed here.

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13116>

    I really like that. Try to include elaborate comments for all formulas that you use. Ideally, provide a link to some additional materials.

kstars/tools/whatsinteresting/obsconditions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13117>

    Try to split that a little bit.

kstars/tools/whatsinteresting/qml/wiview.qml
<http://git.reviewboard.kde.org/r/105481/#comment13124>

    Nice.

kstars/tools/whatsinteresting/skyobjitem.h
<http://git.reviewboard.kde.org/r/105481/#comment13094>

    Could you please start adding comments? It will be easier now.

kstars/tools/whatsinteresting/skyobjitem.h
<http://git.reviewboard.kde.org/r/105481/#comment13095>

    Why should SkyObjItem inherit QObject?

kstars/tools/whatsinteresting/skyobjitem.h
<http://git.reviewboard.kde.org/r/105481/#comment13093>

    Why not m_So? ;-)

kstars/tools/whatsinteresting/skyobjitem.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13118>

    Format your code consistently.

kstars/tools/whatsinteresting/skyobjlistmodel.h
<http://git.reviewboard.kde.org/r/105481/#comment13119>

    Comments.

kstars/tools/whatsinteresting/wiusersettings.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13120>

    Try to simplify this if you can.

kstars/tools/whatsinteresting/wiview.h
<http://git.reviewboard.kde.org/r/105481/#comment13121>

    Comments.

kstars/tools/whatsinteresting/wiview.h
<http://git.reviewboard.kde.org/r/105481/#comment13122>

    m_*

kstars/tools/whatsinteresting/wiview.cpp
<http://git.reviewboard.kde.org/r/105481/#comment13123>

    Again, use enum values.

- Rafal Kulaga

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/20120802/5398c6b2/attachment-0001.html>