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

[Abhijit Apte]

Another point, I forgot to mention it in my last mail..
In skymap.cpp (Pl. refer to patch):
(1) we find the trixels which are needed to cover the rectangular region of
selection (right-click drag)
(2) once we have the trixels, we iterate thru the set of trixels to find the
stars and deep sky objects.

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?

On Wed, Feb 3, 2010 at 1:10 AM, Abhijit Apte <abhijit.apte at gmail.com> wrote:

> 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/03d4baff/attachment.htm