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

Fri Jul 13 08:59:15 UTC 2012

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


OK, a few updates, after I've learned how to make inline comments on the review board :-) 


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

    Keep consistent file naming patterns.


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

    Earn some karma points by removing those stray whitespaces.


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

    I guess this will eventually land in the database?


kstars/indi/indielement.h
<http://git.reviewboard.kde.org/r/105481/#comment12341>

    Are you sure you should remove this?


kstars/kstarsactions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12337>

    Again, naming.


kstars/kstarsactions.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12338>

    Cool, but what if wi already points to a a WIView instance? Memleak I guess.


kstars/tools/WhatsInteresting/Base.qml
<http://git.reviewboard.kde.org/r/105481/#comment12342>

    I think you could improve some names here.


kstars/tools/WhatsInteresting/WIView.h
<http://git.reviewboard.kde.org/r/105481/#comment12343>

    Comments needed.


kstars/tools/WhatsInteresting/WIView.h
<http://git.reviewboard.kde.org/r/105481/#comment12344>

    Should WIView be an QObject or maybe a widget? Perhaps dockable one?


kstars/tools/WhatsInteresting/WIView.h
<http://git.reviewboard.kde.org/r/105481/#comment12345>

    Variable naming conventions. I also can't find definition of that method.


kstars/tools/WhatsInteresting/WIView.h
<http://git.reviewboard.kde.org/r/105481/#comment12346>

    Conventions again.


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12347>

    Conventions!


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12348>

    Where are you deleting this object? Also, I can think of many batter names for it.


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12349>

    Why don't you do: QDeclarativeView *baseView = new QDeclarativeView(); ?


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12350>

    Things are just horribly complicated here. I am sure you can find a better solution.


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12351>

    Aren't enums better here?


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12353>

    This works, but it's just really complicated. There are better ways of doing that. Poke me on the IRC.


kstars/tools/WhatsInteresting/WIView.cpp
<http://git.reviewboard.kde.org/r/105481/#comment12352>

    Unneeded whitespaces.


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

    Naming conventions.


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

    Big memleak here. You're not freeing allocated memory.


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

    This method is just too long. Try to refactor it.


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

    Put whitespaces around operators.


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

    I don't see why this is required.


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

    Switch-case is your friend :-)


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

    Why not use enums for that?


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

    Again, switch-case.


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

    I don't think this method belongs to this class. I am also pretty sure that this is repeated code. Investigate that, please :-)


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

    ? operator would look nice here, or just:
    
    visible = (sp.alt().Degrees() > 6.0);


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

    Why SkyObjItem should inherit QObject? I don't see any reasons for that.


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

    Where is this enum used?


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

    This getter methods should be const, I guess.


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

    Naming conventions, maybe?


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

    Use initialization lists.


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

    I know you can make it simpler ;-)


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

    Const. And maybe put it somewhere else?


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

    Use + operator instead of QString::append.


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

    Enums, please.


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

    This method is basically an one-liner.


- Rafal Kulaga


On July 8, 2012, 5:55 p.m., Samikshan Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105481/
> -----------------------------------------------------------
> 
> (Updated July 8, 2012, 5:55 p.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 1113333 
>   kstars/data/CMakeLists.txt 0dc591b 
>   kstars/data/Interesting.dat PRE-CREATION 
>   kstars/data/PlanetFacts.dat PRE-CREATION 
>   kstars/data/initWIList.dat PRE-CREATION 
>   kstars/indi/indielement.h 93be2ba 
>   kstars/kstars.h 959f042 
>   kstars/kstarsactions.cpp e2f8fc5 
>   kstars/kstarsinit.cpp bbc70fe 
>   kstars/kstarsui-indi.rc d2bbdb0 
>   kstars/kstarsui-win.rc 20a2fa0 
>   kstars/kstarsui.rc 1f28415 
>   kstars/tools/WhatsInteresting/Base.qml PRE-CREATION 
>   kstars/tools/WhatsInteresting/CMakeLists.txt PRE-CREATION 
>   kstars/tools/WhatsInteresting/WIView.h PRE-CREATION 
>   kstars/tools/WhatsInteresting/WIView.cpp PRE-CREATION 
>   kstars/tools/WhatsInteresting/modelmanager.h PRE-CREATION 
>   kstars/tools/WhatsInteresting/modelmanager.cpp 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 
> 
> 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/20120713/29910d6e/attachment-0001.html>