Another point, I forgot to mention it in my last mail..<br>In skymap.cpp (Pl. refer to patch):<br>(1) we find the trixels which are needed to cover the rectangular region of selection (right-click drag)<br>(2) once we have the trixels, we iterate thru the set of trixels to find the stars and deep sky objects.<br>
<br>As per your inputs, we might even think of moving the code which does (1) to a new public method in SkyMesh. What do your take?<br><br><div class="gmail_quote">On Wed, Feb 3, 2010 at 1:10 AM, Abhijit Apte <span dir="ltr"><<a href="mailto:abhijit.apte@gmail.com">abhijit.apte@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Hi Alexey,<br><br>My comments inlined.<br><br><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div>
</div>1. Patch fails to apply cleanly to current svn head. Please update it. Also<br>
compilation fails so I cannot try your last patch.<br></blockquote></div><div><br>Attached updated patch (this compiles this against the svn head -- commit <a href="http://websvn.kde.org/?view=revision&revision=1083977" title="Revision 1083977" target="_blank">1083977</a>). I think you/somebody have committed some changes into dialogs/detaildialog.cpp which were to be pull into dialogs/objectlist.cpp.<br>
<br></div><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
2. Current convention is to use 4 space for indentation and do not use tabs at<br>
all. Please fix it.<br></blockquote></div><div><br>Sure, would take care of it next time onward, actually problem is with 'set autoindent' and 'set cindent' of vim. :-(<br> </div><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
3. Currently you search for objects in skymap.cpp. I think it outright bad<br>
design. You have to enumerate all components by hands. This is error prone and<br>
changes in skycomponents are likely break your code.<br></blockquote></div><div><br>So, are you suggesting to distribute (move) the code which does the searching of objects to the respective components (i.e. the worker function you suggested below)?<br>
<br></div><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
I think better solution is to add function to SkyComponent to search for<br>
objects and use virtual dispatch. Here is some example code.<br>
<br>
-- Code snippet ---<br>
<br>
class SkyComponent {<br>
public:<br>
// Function which returns list of objects in given area.<br>
QList<SkyObject*> findObjectInArea(Area area) {<br>
QList<SkyObject*> list;<br>
objectsInArea(list, area)<br>
return list;<br>
}<br>
private:<br>
// This is worker function. It adds objects to list if they are in the<br>
// area. It should be reimplemented in subclasses.<br>
virtual void objectsInArea(QList<SkyObject*>&, Area);<br>
};<br>
----------------<br></blockquote></div><div> <br>Even if we implemented the way you've suggest, we'll still have to invoke the findObjectInArea() for each component (like StarComponent, DeepSkyComponent etc -- see below) from skymap.cpp right??<br>
<br>StarComponent* starComp;<br>StarList listOfStars = starComp->findObjectInArea(myarea);<br>DeepSkyComponent* deepSkyComp;<br>QList<DeepSkyObject*> listOfDSO = deepSkyComp->findObjectInArea(myarea);<br>
//.. similarly for planets, etc???<br></div><div><br>Pl. shed more light on point#3 as this is little unclear to me. :P <br><br></div><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
4. Are there code from DetailsDialog in the SkyObjectList? If it's the case<br>
just drop it. It's completely fine to have missing functionality for some time<br>
and add it gradually. It's much easier to reason about code that way.<br></blockquote></div><div><br>Yes, absolutely! Since, the detailed tabbed representation was to be merged with the list view (as per your earlier comments), I had to merge the entire code of DetailsDialog (detailsdialog.cpp and detailsdialog.h) into SkyObjectList (objectlist.cpp and objectlist.h). <br>
<br>The reason I did this was, going further, deprecate the existing detaildialog.* with objectlist.* and rename the latter SkyObjectList to DetailDialog.<br><br></div><div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
5. Patch is huge. It would be nice if you split it into several smaller ones.<br>
More below.<br>
<br></blockquote></div><div>Yeah agree, didn't want to make it this huge, but as I went on making changes, realized there were far too many files that I had to modify than actually thought. :-)<br></div><div> <br>Rgds<br>
-Abhi<br>
</div></div>
</blockquote></div><br>