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

[Khudyakov Alexey]

В сообщении от Понедельник 01 февраля 2010 21:45:34 Abhijit Apte написал:
> Hi Alexey/Akarsh,
> 
> On Mon, Feb 1, 2010 at 2:21 PM, Abhijit Apte <abhijit.apte at gmail.com> wrote:
> > I've implemented a draft of the implementation of combining the list
> > functionality along-side the detail dialog. So in essence, we'll have
> > only one dialog which will operate in two modes -- Detailed and Summary.
> > 1) When a popup menu is invoked on a sky object and "Details..." option
> > is selected from the popup menu, the detail dialog will open in
> > "Detailed" mode by default. User has the option to switch to the
> > "Summary" view also. 2) When a right-click drag selection happens, the
> > objects from the rectangle are shown in the detail dialog. This time, the
> > detail dialog will open in "Summary" mode.
> 
> Here's the patch, I managed to fix most of the bugs I noticed while
>  testing. Right now, I've not modified DetailDialog
>  (dialogs/detaildialog.cpp and dialogs/detaildialog.h) and instead merged
>  (copied) all of its code into dialogs/objectlist.cpp and
>  dialogs/objectlist.h
> Pl. review and apply the patch and suggest me if you've any comments.
> 

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

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

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. 

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);
};
----------------

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. 


5. Patch is huge. It would be nice if you split it into several smaller ones. 
More below.


== Workflow

Here is some suggestion about workflow you might fin useful. In my experience 
it's quite difficult to write big feature in one big patch. Usually it could be 
separated in many smaller patches. Also it's difficult to review big patch with 
many changes. 

I know two tools which allows to work with multiple patches.

* Mercurial queues. It allows to work on stack of patches. One can 
add/remove/change/rearrange patches and constantly update patches againist svn 
head. Very handy if you mostly work on one feature.

* git-svn. It's easier to work on multiple features (git's branches are 
wonderful) but harder to change locally commited patch. Also it's still 
possible.