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

[Daniel Burrows]

On Thu, Jul 8, 2010 at 1:39 AM, Piotr Galiszewski <piotr at galiszewski.pl>wrote:

> 2010/7/8 Daniel Burrows <dburrows at algebraicthunk.net>:
> >  * Actually, please organize your repository as I noted above for at
> >    least the first set of patches, so I can thoroughly vet your earlier
> >    patches in isolation. (you should be able to just tag branches with
> >    judicious use of "git branch")
> >
>
> OK. It will be my first task today. I will try to incorporate a the
> same time your other comments. The repository format I have chosen is
> a result of my bad habits from svn and Kadu repository, where is
> single master repository (topics branches are only for bigger
> reworking of the code)
>

  No problem -- I only came across this idea while I was trying to improve
my git-fu a couple months ago.  You can see my WIP feature branches
in my personal repository here:

git://git.debian.org/~dburrows/aptitude.git

  or on the Web:

http://git.debian.org/?p=users/dburrows/aptitude.git;a=summary

  Unlike the branches I publish in the canonical repository, the feature
branches are frequently rebased, amended, rewritten, etc, and I delete
them when they're safely merged.  i.e., don't try to base your work off of
them.

  We could use a longer-lived branch for Qt work, but I'd prefer to just
merge it into the master branch in small chunks, ASAP.

>  * In the patch that adds the configure tests:
> >
> >    - Please change AC_ARG_WITH to instead use AC_ARG_ENABLE.  This
> >      matches how GTK+ is enabled, and is more correct (--with- normally
> >      lets you specify a particular installation of the library, and
> >      we don't support that).
> >
> >    - Please support --disable-qt (or --without-qt).  You should be
> >      able to directly adapt the code that adds --enable-gtk and
> >      --disable-gtk.
> >
>
> OK. I will change that. When writing this rules I had a dilemma which
> option will be better. But earlier I had written that there will be
> --with configuration switch, so I had stick with this.
>

  Just to be clear, my biggest reason for wanting to use AC_ARG_ENABLE
is that --with-* conventionally lets you say where the library is.  I'd be
even happier if you supported that and used AC_ARG_WITH -- but I don't
know if it's worth the trouble at this stage.


> >  * Pointers/memory management:
> >
> >    - I see lots of bare pointers here.  Could you give me a quick
> >      summary of how memory management works in the Qt frontend?
> >      aptitude strongly prefers smart pointers wherever it's feasible
> >      to use them -- I guess widgets are probably managed by Qt itself,
> >      but the Package object looks to me like a candidate for some
> >      sort of smart pointer wrapper.
> >
>
> The GUI code will be managed just fine by the Qt machinery. The
> problem is with Filter and Package classes (to be fair Package class
> is really ugly now ;) ). I was planning to split this classes to e.g
> Package and PackageShared classes. All data will be available in
> PackageShared class which will be held in QSharedDataPointer object in
> Package classes. The data will be automatically destroyed when the
> last Package class will be deleted. Package class will be lightweight
> so they objects could be passes by value not by references. It works
> quite nice in Kadu messanger (Actually Kadu's solution is even more
> complicated, so I will need to simplify this. It is all documented
> here: http://www.kadu.net/~vogel/doc/html/group___storage.html)
>

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


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

  Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/aptitude-devel/attachments/20100708/28f7afb9/attachment.htm>