[Aptitude-devel] aptitude-qt VCS usage and code review

[Piotr Galiszewski]

2010/7/9 Daniel Burrows <dburrows at google.com>:
>   (sorry about dropping the list -- I'm still figuring out the quirks of
> this
> mail interface)

Argh. That was me, who dropped the list. Sorry about that ;) Now, it
is nearly 2.30 AM in my timezone, so my words probably could chaotic
;) I have pushed my todays work to repository. Not all code have been
ported, but more will follow tomorrow. Currently there are 4 new
branches

> On Thu, Jul 8, 2010 at 1:05 PM, Piotr Galiszewski <piotr at galiszewski.pl>
> wrote:
>>
>> 2010/7/8 Daniel Burrows <dburrows at google.com>:
>> > On Thu, Jul 8, 2010 at 1:39 AM, Piotr Galiszewski <piotr at galiszewski.pl>
>> > wrote:Thanks for the informations. I am now rebasing and reworking my
>> previous commits. I have planned to firstly write features, and later
>> polish, cleanup and refactor code. As I see, this should change now,
>> and I should try write code that is working and ready for merging? If
>> so, I have to change my working plan a little, cause there is already
>> small delay due to this code reworking. Please forgive me this delay
>> ;)
>
>  Yeah, I know this will introduce some delays, at least for now.  The thing
> is, my experience is that deferring code quality often means you never
> get around to it.  And getting your code into good shape immediately has
> some benefits:
>   - It's a lot easier in the long run to have code that's well-formatted,
> well
>     documented, etc, to start with, rather than going back and spending
>     blocks of time doing nothing but documenting and formatting code (IMO).
>   - Making sure the design is sensible up-front can save you a *lot* of time
>     later; it's relatively cheap to change the design when you're first
> putting
>     it together, and a lot more expensive to do it after you've built a lot
> of
>     code.
>   Also, my experience with SoC (based on both times that I've been a
> mentor) is that it goes better if your code is ready for submission from
> early on.  It ensures that we catch any differences of opinion about design,
> syntax, etc, early on and not after you have three months worth of code
> written.  Plus, this is an educational program, and I think code reviews
> serve both to help you write better code, and to give you some experience
> with a more formal / professional style of development.

Great ;) . I have already learned a lot and I know that every
experiences are important. I was only little nervous about gsoc
timelines, cause I am far behind my original schedule :/

>>
>> I have two other questions: Do you prefer smaller commits or one
>> bigger, full feature commit for each branch? Also API docs should be
>> written now, or it could wait a little. Also how precise they should
>> be?
>
>   My general preference (especially when I'm reviewing them) is for smaller
> commits, balanced by my preference for commits that compile and work.
> i.e., try to split your commits up into small chunks that will compile, pass
> the unit tests, and run without major bugs (even if not all the features
> work yet).  So a good first (or maybe even second or third) stage for your
> Qt frontend would be enough code to initialize Qt and display a blank
> window.
>   But despite what I said, feel free to use your common sense -- e.g., if
> it's
> going to impose a lot of extra work to split a smallish commit into
> two even smaller ones, don't bother.

Ok. I also prefer smaller commits ;) . My first commits are big, but I
could split them

>   As for API docs, please write them up-front.  My experience since I
> adopted
> this strategy has been that it's a really good way to flush out badly
> factored
> code and other poor designs (such code is usually hard to document, or you
> end up writing documentation that makes you feel so dirty and ashamed
> that you go and fix it :) ).  Also, it really helps when other people (ie,
> me) are
> reviewing your code, so they know what was intended and what's a bug.
>   The API documentation should be written as if you're documenting a
> library,
> because you are (every piece of aptitude is a library from the point of view
> of the rest of the program).  The most important thing to document is what
> the meaning of each piece is and how it fits into the overall picture.
>  Think
> about what a reader who encountered your code without knowing what you
> were doing would ask.  e.g., consider the Filter class:
>    class Filter : public QObject
>    {
>       Q_OBJECT
>          QString _name;
>       public:
>          Filter(QString name) : _name(name) { }
>          const QString &name() { return _name; }
>          virtual bool acceptPackage(Package *package) { return true; }
>       signals:
>          void filterChanged();
>    };
>   Questions:
>      - What is a Filter?
>      - How does Filter relate to Package?
>      - Under what conditions is filterChanged emitted?
>   Here's a stab at answering them, although I'm not sure that I got the
> right
> answers:
>   /** \brief A generic filter on packages for use with FiltersModel.
>    *
>    *  A Filter represents a predicate on Package objects, implemented
>    *  by acceptPackage().  Each filter has a name, which represents
>    *  something about the filter.  I'm not sure exactly what.  Maybe
>    *  it's intended to appear as a label when the user is selecting
>    *  filters?
>    *
>    *  Filters might change over time; if the logical value of a Filter
>    *  changes, it will emit filterChanged().
>    *
>    *  The main implementation of Filter is SearchFilter, which supports
>    *  simple string matches against several fields of a package.
>    */
>   class Filter : public QObject
>   {
>     Q_OBJECT
>     // \todo Shouldn't this be const?
>     QString name;
>   public:
>     /** \brief Create a new filter with the given name. */
>     Filter(const QString &_name) : name(_name)
>     {
>     }
>
>     /** \brief Retrieve the name of this filter. */
>     const QString &get_name() const { return name; }
>     /** \brief Test whether the given package matches this filter
>      *  and return \b true if it does.
>      *
>      *  The default implementation of this method matches all packages.
>      */
>     virtual bool acceptPackage(const Package *package)
>     {
>       return true;
>     }
>   signals:
>     /** \brief Emitted when this filter is changed to match a different
>      *  set of packages.
>      */
>     void filterChanged();
>   };

Thanks for the informations. I will do this after finishing porting my
previous code

>   I have some more design notes on this particular piece of
> code, but I'm waiting to verify that it's OK for me to post some
> example code I wrote at work.
>>
>> >   So, if I understand correctly, you intend to allocate shared Package
>> > objects and pass smart pointers to them?  Would you mind using
>> > boost::shared_ptr so we can have consistency across frontends?  (the
>> > backend objects that use this pattern already use that wrapper, and I'd
>> > rather not add even more types of smart pointers if we can avoid it).
>> >
>>
>> Not exactly, but yes. I will try to use boost::shared_ptr instead of
>> it. Should it be done during reintroduction of Package class, or it
>> can wait? Probably I will also create package_manager class for
>> managing packages. Reading cache in model is not perfect solution and
>> probably it will be problematic in the future
>
>   Is package_manager some sort of a global storage pool of Packages,
> then?  Could you maybe call it package_pool instead to avoid naming
> confusion?

yes it is. I will call it that

>   I don't really understand the whole purpose of the Package object.
> Is the goal here to save memory and CPU time by only copying package
> information into QStrings once per package?
>

For model I need an array of packages. And as the number of packages
is really big I have decided that custom class with lazy loading could
be a good idea. And my tests show that it is true (especially in case
of showing description on the list)

>>
>> >>
>> >> >    - Why are all of PackageDelegate's methods const when they modify
>> >> >      its state?  Something seems wrong here.  Is there a better way
>> >> >      to do this?
>> >> >
>> >>
>> >> I have only reimplemented functions from QAbstractItemDelegate. They
>> >> all are const there:
>> >> http://doc.trolltech.com/4.6/qabstractitemdelegate.html
>> >
>> >   That would kind of tie your hands.  And all the workarounds are really
>> > no better IMO.
>>
>> It is a normal behavior of this classes, so we have to live with this
>> ;) Qt Model Framework is really nice and powerful solution and there
>> are no better options for Qt.
>
>   Yeah.  Um, my language above was a little ambiguous, so I just wanted to
> say clearly that I agree that you have to do things like this given how Qt
> works.
>   Daniel

Thanks again, and now it is time to sleep. I have a lot of work to do tomorrow

-- 
Regards
Piotr Galiszewski