[Kstars-devel] Review Request: Review request for merging gsoc2012-samxan branch to master.

Sun Sep 23 20:13:28 UTC 2012

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

Could you please address issues listed here before merging?

kstars/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106371/#comment15309>

    Stray whitespace.

kstars/Messages.sh
<http://git.reviewboard.kde.org/r/106371/#comment15310>

    Is i18n working right now?

kstars/data/Interesting.dat
<http://git.reviewboard.kde.org/r/106371/#comment15314>

    Can't argue.

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

    I guess we should include that information also in the user interface to be 100% fair.

kstars/kstars.cpp
<http://git.reviewboard.kde.org/r/106371/#comment15311>

    Inconsistent code formatting.

kstars/kstarsactions.cpp
<http://git.reviewboard.kde.org/r/106371/#comment15312>

    Formatting.

kstars/kstarsinit.cpp
<http://git.reviewboard.kde.org/r/106371/#comment15313>

    Should we change this name? "What's interesting in the sky" or something like that would sound better IMO ;-)

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

    Formatting.

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

    I guess you should check line length first (!= 0) to be safe.

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

    I'd use switch-case here.

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

    Use enums instead of ints.

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

    Why Telescope = 0? Is that fact used somewhere else?

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

    If you use m_Something style, use it also in other files.

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

    Somewhat ugly.

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

    Somewhat more ugly. Why don't use QMap<int, double> to implement the lookup table?

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

    Nice job. I will add a few comments to that part as soon as you address issued mention in that request.

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

    Are you using QObject somewhere here?

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

    I don't like this way of doing stuff. Why do we introduce a few enum types that essentialy represent the same stuff?

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

    You're walking on a razor's edge in that line ;-)

kstars/tools/whatsinteresting/wiusersettings.h
<http://git.reviewboard.kde.org/r/106371/#comment15328>

    Please be consistent - if you use argument names, do it everywhere.

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

    Isn't it a better idea to use index instead of current text?

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

    Some of those includes are not needed here, forward declarations will suffice. 

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

    Naming conventions.

- Rafal Kulaga

On Sept. 7, 2012, 5:14 p.m., Samikshan Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106371/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2012, 5:14 p.m.)
> 
> 
> Review request for KStars, Rafal Kulaga and Akarsh Simha.
> 
> 
> Description
> -------
> 
> This is the request to review the mergeabilty of the gsoc2012-samxan branch which introduces the "What's Interesting..." feature in KStars.
> 
> This feature suggests sky-objects to beginner astronomers depending on such parameters like light pollution and equipment specifications,
> in a QML user interface.
> 
> Added new "whatsinteresting" directory under kstars/tools/ containing all the source code related to this new feature.
> kstars/tools/whatsinteresing/qml comtains all files for the QML UI.
> 
> 
> Diffs
> -----
> 
>   kstars/CMakeLists.txt 2c99bb1 
>   kstars/Messages.sh 7d56711 
>   kstars/data/CMakeLists.txt 0dc591b 
>   kstars/data/Interesting.dat PRE-CREATION 
>   kstars/data/PlanetFacts.dat PRE-CREATION 
>   kstars/kstars.h ad4b52f 
>   kstars/kstars.cpp 1512b67 
>   kstars/kstarsactions.cpp 4f03e30 
>   kstars/kstarsinit.cpp 3baec6c 
>   kstars/kstarsui-indi.rc 1b44e5f 
>   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/CMakeLists.txt PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/ScrollBar.qml PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/dialog_close.png PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/leftArrow.png PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/next.png PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/previous.png PRE-CREATION 
>   kstars/tools/whatsinteresting/qml/settingsIcon.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/106371/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Samikshan Bairagya
> 
>

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