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

Samikshan Bairagya samikshan at gmail.com
Tue Jul 31 10:25:04 UTC 2012 


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/indi/indielement.h, line 48
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71811#file71811line48>
> >
> >     Are you sure you should remove this?

I do not know why this was included in the diff. I have not made this change. These two lines was added by Kevin Kofler on master branch through revision 93be2ba.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/kstarsactions.cpp, line 288
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71813#file71813line288>
> >
> >     Cool, but what if wi already points to a a WIView instance? Memleak I guess.

Fixed


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/WIView.h, line 31
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71820#file71820line31>
> >
> >     Variable naming conventions. I also can't find definition of that method.

Removed.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/WIView.cpp, line 25
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71821#file71821line25>
> >
> >     Where are you deleting this object? Also, I can think of many batter names for it.

Will talk about this on IRC.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/WIView.cpp, line 27
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71821#file71821line27>
> >
> >     Why don't you do: QDeclarativeView *baseView = new QDeclarativeView(); ?

Really ridiculous on my part. Fixed.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/WIView.cpp, line 35
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71821#file71821line35>
> >
> >     Things are just horribly complicated here. I am sure you can find a better solution.

catListObj = baseObj->findChild<QObject*>("catListObj"); ?


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/WIView.cpp, line 51
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71821#file71821line51>
> >
> >     Aren't enums better here?

Again I should discuss this with you on IRC.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/WIView.cpp, line 74
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71821#file71821line74>
> >
> >     This works, but it's just really complicated. There are better ways of doing that. Poke me on the IRC.

Okay.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/modelmanager.h, line 28
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71822#file71822line28>
> >
> >     Big memleak here. You're not freeing allocated memory.

Fixed.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/modelmanager.cpp, line 41
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71823#file71823line41>
> >
> >     This method is just too long. Try to refactor it.

Okay.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/modelmanager.cpp, line 63
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71823#file71823line63>
> >
> >     I don't see why this is required.

Really was a foolish thing to do! Fixed it.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.h, line 30
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71824#file71824line30>
> >
> >     Where is this enum used?

It was supposed to be used. Removed.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/modelmanager.cpp, line 195
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71823#file71823line195>
> >
> >     I don't think this method belongs to this class. I am also pretty sure that this is repeated code. Investigate that, please :-)

Fixed


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.cpp, line 68
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71825#file71825line68>
> >
> >     Use + operator instead of QString::append.

Done


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.cpp, line 99
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71825#file71825line99>
> >
> >     This method is basically an one-liner.

Done


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.cpp, line 53
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71825#file71825line53>
> >
> >     Const. And maybe put it somewhere else?

Done


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.cpp, line 22
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71825#file71825line22>
> >
> >     Use initialization lists.

Done


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.h, line 49
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71824#file71824line49>
> >
> >     Naming conventions, maybe?

Done


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.h, line 40
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71824#file71824line40>
> >
> >     This getter methods should be const, I guess.

Yeah, done.


> On July 13, 2012, 8:59 a.m., Rafal Kulaga wrote:
> > kstars/tools/WhatsInteresting/skyobjitem.cpp, line 73
> > <http://git.reviewboard.kde.org/r/105481/diff/1/?file=71825#file71825line73>
> >
> >     Enums, please.

Yeah, fixed that.


- Samikshan


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


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/20120731/ad191f1e/attachment-0001.html>

More information about the Kstars-devel mailing list