Hi Alexey,<br><br>Thanks for your comments, pl. find my comments inlined.<br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div class="h5">
</div></div>1. Please change signature of findObjectInArea to<br>
findObjectInArea(const SkyPoint&, const SkyPoint&)<br>
It will make obvious to caller that points are not going to be changed.<br>
<br>
This may require copying but it's not a problem. </blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
2. I'm not sure that SkyMapComposite::skyRegion is eligible for separate<br>
function. It's as simple as<br>
<br>
SkyPoint p3( p1.ra(), p2.dec() );<br>
SkyPoint p4( p2.ra(), p1.dec() ); // Or other way around<br>
QList<SkyPoint*> list;<br>
// "<<" is overloaded for QList<T><br>
list << &p1 << &p3 << &p2 << &p4;<br></blockquote><div><br>Two reasons:<br>1) Have added the function skyRegion() as a separate function because later on we can scale it up to support a user-defined region.. Then one has to do, is to write overloaded functions for skyRegion(). Say user wants to select from a circular region or a polygon for instance. I think it's fine to add this skyRegion() function for now.<br>
<br>2) The problem with making SkyPoint p1 and p2 const:<br>While filling up the list with operator<<, it'd become a problem since QList is instantiated with SkyPoint* whereas the arguments are of type "const SkyPoint*".<br>
/home/kde-devel/kde/src/KDE/refactor/kdeedu/kstars/kstars/skycomponents/skymapcomposite.cpp: In member function ‘QList<SkyObject*> SkyMapComposite::findObjectsInArea(const SkyPoint&, const SkyPoint&)’:<br>/home/kde-devel/kde/src/KDE/refactor/kdeedu/kstars/kstars/skycomponents/skymapcomposite.cpp:348: error: invalid conversion from ‘const SkyPoint*’ to ‘SkyPoint*’<br>
/home/kde-devel/kde/src/KDE/refactor/kdeedu/kstars/kstars/skycomponents/skymapcomposite.cpp:348: error: initializing argument 1 of ‘QVector<T>& QVector<T>::operator<<(const T&) [with T = SkyPoint*]’<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Please note that there is no need to create temporary variables. Also `dms'<br>
has size of double since it's double in fancy wrapping.<br>
<br></blockquote><div>Sure thanks for pointing out. I'll remove those dms temporaries. They add up to memory.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
3. It doesn't seem very reasonable to leave out nameless stars. They are vast<br>
majority after all. This will require to set up some magnitude cut-off to<br>
remove faint stars at low zoom.<br>
<br>
This is difficult task and separate from object finding so it's fine as it is by<br>
now.<br></blockquote><div><br>I agree. <br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
4. If object doesn't visible it shouldn't get listed. You can use `selected()'<br>
to determine whether component visible.<br></blockquote></div><br>You mean the objects below horizon? <br>I didn't quite understand this -- I noticed each component has this selected() function. Did you mean the following because the component views can be toggled from toolbar?? If you are referring to it, it's a good catch ! :)<br>
if( m_Stars->selected() ) m_Stars->findObjectsInArea(p1,p2)<br>if( m_DeepSky->selected() ) m_DeepSky->findObjectsInArea(p1,p2)<br><br>Rgds<br>-Abhi<br>