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

[Daniel Burrows]

  This was going to be short, but ended up being long.  Sorry :-(

  * 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")

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

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

    - aptitude uniformly uses .cc for its C++ source inputs.  Please
      change your file-names accordingly.

    - aptitude generally uses get_foo or getFoo() for accessors, not
      foo().

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

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

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

  * 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

  * Doccomments:

    - Please write missing API documentation (i.e., doccomments) for
      your classes and methods.

  * Typos:

    - There's a stray backslash in package_delegate.cpp, line 167.
    - informaton -> information (package.cpp)

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

  * Other:

    - Please make your accessors const-qualified (e.g., everything in
      Package -- that's a classic use case for "mutable", too).

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

  That's everything I could spot in a few minutes, anyway.

  Thanks!

  Daniel