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

[Khudyakov Alexey]

В сообщении от Вторник 02 февраля 2010 22:40:15 Abhijit Apte написал:
> Hi Alexey,
> 


> 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.
> 
I'll try it tomorrow.


> > 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)?
> 
Exactly. Idea is to keep all code related to some thing in one rather then 
spreaded all over codebase. It easier to work with it that way.


> > I think better solution is to add function to SkyComponent to search for
> > objects and use virtual dispatch. Here is some example code.
> >
> 
> 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
> 
No we shouldn't. SkyComposites will handle all calls to components. All is 
needed is to call:
> skyComposite()->findObjectsInArea(myarea)
It will find all objects

You can check implementation of functions `objectNearest' and `findByName' in 
SkyComposite. They provide similar functionalty.

1) call SkyMapComposite::findObjectInArea(...)
2) it calls SkyMapsComposite::objectsInArea(...)
3) which in turn calls all the subcomponents and subcomposites.

Downside of this approach is that you get list of SkyObject* and all 
infomation about types is lost. It's price of uniformity and nothing could be 
done about it. 

If one wants to type information back he could use RTTI, dynamic_cast and 
things like that.

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

If you think that this function is useful implement it (-:

> 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.
> 
I think you shouldn't merge _entire_ details dialog. Old code will interfere 
with your work. (It is "will" not "could" judging from my experience). 

Probably it's better to put minimal stubs just to makething compile and run. 
After that old code could be pulled gradually one feature after another. Point 
is to make process controlled and understandable.

Also it offers opportunity to clean up existing code. It is far from 
perfection.

This approach has added benefit. I could commit patches to SVN without having 
complete dialog. 

> 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. :-)
> 
This how it goes all the time. You begin to work on small feature and end up 
with huge patch. "Complexity managing" is just about such thing. Programmer 
has to work actively againist such thing. 

There is one candidate for separate patch already. It is code for searching 
for objects.