Hi Alexey,<br><br>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 class="im">

</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><br>Attached updated patch (this compiles this against the svn head -- commit <a href="http://websvn.kde.org/?view=revision&amp;revision=1083977" title="Revision 1083977">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><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><br>Sure, would take care of it next time onward, actually problem is with &#39;set autoindent&#39; and &#39;set cindent&#39; of vim. :-(<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. 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><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><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&lt;SkyObject*&gt; findObjectInArea(Area area)  {<br>
        QList&lt;SkyObject*&gt; 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&lt;SkyObject*&gt;&amp;, Area);<br>
};<br>
----------------<br></blockquote><div> <br>Even if we implemented the way you&#39;ve suggest, we&#39;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-&gt;findObjectInArea(myarea);<br>DeepSkyComponent* deepSkyComp;<br>QList&lt;DeepSkyObject*&gt; listOfDSO = deepSkyComp-&gt;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><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&#39;s the case<br>
just drop it. It&#39;s completely fine to have missing functionality for some time<br>
and add it gradually. It&#39;s much easier to reason about code that way.<br></blockquote><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><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>Yeah agree, didn&#39;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>