[Kstars-devel] Refined patch for wishlist 178232: List all objects within a rectangle

[Abhijit Apte]

Hi Alexey,

My comments inlined.

1. Patch fails to apply cleanly to current svn head. Please update it. Also
> compilation fails so I cannot try your last patch.
>

Attached updated patch (this compiles this against the svn head -- commit
1083977 <http://websvn.kde.org/?view=revision&revision=1083977>). I think
you/somebody have committed some changes into dialogs/detaildialog.cpp which
were to be pull into dialogs/objectlist.cpp.

2. Current convention is to use 4 space for indentation and do not use tabs
> at
> all. Please fix it.
>

Sure, would take care of it next time onward, actually problem is with 'set
autoindent' and 'set cindent' of vim. :-(


> 3. Currently you search for objects in skymap.cpp. I think it outright bad
> design. You have to enumerate all components by hands. This is error prone
> and
> changes in skycomponents are likely break your code.
>

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)?


> I think better solution is to add function to SkyComponent to search for
> objects and use virtual dispatch. Here is some example code.
>
> -- Code snippet ---
>
> class SkyComponent {
> public:
>    // Function which returns list of objects in given area.
>    QList<SkyObject*> findObjectInArea(Area area)  {
>        QList<SkyObject*> list;
>        objectsInArea(list, area)
>        return list;
>    }
> private:
>    // This is worker function. It adds objects to list if they are in the
>    // area. It should be reimplemented in subclasses.
>    virtual void objectsInArea(QList<SkyObject*>&, Area);
> };
> ----------------
>

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??

StarComponent* starComp;
StarList listOfStars = starComp->findObjectInArea(myarea);
DeepSkyComponent* deepSkyComp;
QList<DeepSkyObject*> listOfDSO = deepSkyComp->findObjectInArea(myarea);
//.. similarly for planets, etc???

Pl. shed more light on point#3 as this is little unclear to me. :P

4. Are there code from DetailsDialog in the SkyObjectList? If it's the case
> just drop it. It's completely fine to have missing functionality for some
> time
> and add it gradually. It's much easier to reason about code that way.
>

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).

The reason I did this was, going further, deprecate the existing
detaildialog.* with objectlist.* and rename the latter SkyObjectList to
DetailDialog.

5. Patch is huge. It would be nice if you split it into several smaller
> ones.
> More below.
>
> 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. :-)

Rgds
-Abhi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kstars-devel/attachments/20100203/200b9218/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch3feb.diff
Type: text/x-patch
Size: 71240 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kstars-devel/attachments/20100203/200b9218/attachment-0001.diff