[Aptitude-devel] aptitude-qt VCS usage and code review
Piotr Galiszewski
piotr at galiszewski.pl
Thu Jul 8 08:39:59 UTC 2010
Hello Daniel
2010/7/8 Daniel Burrows <dburrows at algebraicthunk.net>:
> This was going to be short, but ended up being long. Sorry :-(
>
No problem. Thank you for all comments and suggestions! I really
appreciate them, and I will resolve this problems ASAP
> * You might want to consider using feature branches instead of a
> single mainline branch, to make it easier to maintain a clean
> history (as opposed to having patches whose commit says "this should
> be in the previous patch"). I'm still learning how to use git
> myself, but the approach of treating a branch as a staging area
> for patches seems quite powerful to me.
>
> In your case, you'd probably have a chain of feature branches that
> are based off each other, using rebasing to keep everything in sync.
> e.g.,
>
> 001-build-skeleton -> 002-qt-stubs -> 003-packages-tab -> ...
>
> The downside is that if an earlier patchset (that hasn't been merged
> yet) needs to be changed, you have to rebase all the rest of the
> changes.
>
> The big *advantage* is that if I ask for changes when reviewing your
> code, we could easily fix them up before the "bad" code gets into the
> tree, instead of having to carry around temporarily bad code and a
> fix. It also makes it easier for me to review your code if it's
> divided into relatively small logical changes, and it means that I
> don't have to merge code that I haven't reviewed in order to get
> fixes I requested.
>
> (this is somewhat exploratory -- I haven't used git like this for
> collaboration, but I've used it this way to organize my own patches
> and it's worked out pretty well)
>
> * 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)
> * 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.
> * Naming conventions:
>
> - Most of aptitude uses lower_case_names_with_underscores for
> classes and enum values. There are exceptions in the GTK+ code,
> but I'm planning to gradually eliminate them for the sake of
> consistency.
>
> As far as other stuff goes, aptitude is all over the map -- lower
> case with underscores is generally preferred, but I won't object
> to camelCasing if it sneaks in. Just please don't use
> lptrVxQHungarian.
>
> - aptitude does not use any special naming convention for member
> variables. (e.g., _size in package_delegate.h) Single underscores
> are used only and exclusively to denote constructor parameters,
> particularly those with the same name as a member variable.
>
OK. Will be done
> - aptitude uniformly uses .cc for its C++ source inputs. Please
> change your file-names accordingly.
>
I know about this. There were only a problem that Qt's moc doesn't
recognize .cc files. I was trying to resolve this for several hours
during writing rules, but I have not found any solutions. Maybe it is
my configuration fault, cause I do not think that it is a problem in
moc itself. I will also ask Sune about this on IRC
> - aptitude generally uses get_foo or getFoo() for accessors, not
> foo().
>
OK. It was another my habit from Kadu/KDE/Qt naming conventions. Will be fixed
> * Namespace:
>
> - I'd prefer aptitude::qt or even aptitude::gui::qt for your code.
> The GTK+ frontend uses ::gui, and I've never been happy about
> that. The long-term goal is to put all of aptitude in the
> "aptitude" namespace, and I don't want to get even farther away
> from that in the near term.
>
OK.
> * Indentation:
>
> - aptitude idents 2 spaces per level. Your code appears to be
> indented 3 spaces per level. Please fix.
>
> - package_delegate.h seems to have irregular indentation. This is
> just the first one I noticed; there might be more.
>
> - aptitude wraps function parameters and other lists to line up with
> the first element of the list, e.g.:
>
> void foo(int a,
> int b);
>
> rather than
>
> void foo(int a,
> int b);
>
> I might not do this if I was starting the project from scratch,
> but I feel it's better to be consistent for the time being.
>
I completely agree with you. Consistency is very important. I had been
counting spaces in gtk's frontend and it looked that there is 3
spaces. Probably I have made a mistake there ;) Will be fixed
> * Searching:
>
> - Could you use aptitude's search patterns for search_filter.cpp?
> See src/generic/apt/matching/ for all this code -- it should be
> really simple for you to adapt, and it'll give you much broader
> search capabilities instantly.
>
> (I'm willing to merge this in its current form and wait till
> later to support search patterns)
>
For now searching is done entirely on the previously laded tabs. It
does not use searching in the cache. But of course I will have an look
on this in the future
> * Parameters:
>
> - aptitude always passes non-basic types by const reference unless
> there's a really good reason not to. see, e.g., Package::Package
> and
>
OK
> * Doccomments:
>
> - Please write missing API documentation (i.e., doccomments) for
> your classes and methods.
>
I was planning to do this after the first week of coding and add
comments to all of the classes. I will try to do this now
> * Typos:
>
> - There's a stray backslash in package_delegate.cpp, line 167.
> - informaton -> information (package.cpp)
>
I will double check all the code today
> * 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)
> * Other:
>
> - Please make your accessors const-qualified (e.g., everything in
> Package -- that's a classic use case for "mutable", too).
>
OK
> - 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's everything I could spot in a few minutes, anyway.
>
> Thanks!
>
Again, thanks a lot for your comments and review! Probably I should
have asked about some of this issue earlier. Sorry about that, but my
last weeks have been totally crazy due to my studies :/
> Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>
--
Regards
Piotr Galiszewski
More information about the Aptitude-devel
mailing list