[Aptitude-devel] Review of b598238

[Daniel Burrows]

  Back from vacation now and feeling great :).  Well, good.  Well,
passably decent.

  Anyway, my first post-vacation code review follows.  I wanted to
ask about a few minor things before I merge this.

> +      packages_model::packages_model(QObject *parent)
> +	: QAbstractListModel(parent)
> +      {
> +	package_pool::get_instance()->connect_cache_reloaded(sigc::mem_fun(*this, &packages_model::handle_cache_reloaded));
> +	package_pool::get_instance()->connect_cache_closed(sigc::mem_fun(*this, &packages_model::handle_cache_closed));
> +	package_pool::get_instance()->connect_cache_state_changed(sigc::mem_fun(*this, &packages_model::package_state_changed));
> +      }

  I wonder if you could take a package_pool pointer as a parameter,
for future use (i.e., in test cases)?  You can provide a convenience
constructor that passes package_pool::get_instance() to the real
constructor.

> +      QVariant packages_model::headerData(int section, Qt::Orientation orientation, int role) const

  So, this one produces the column labels?  Am I right there?

> +      {
> +	if(role != Qt::DisplayRole)
> +	  return QVariant();
> +
> +	if(orientation == Qt::Horizontal)

  Presumably the other direction is for if you have row labels running
down the side of the view?  Will we see that in practice or is this
just for completeness?

> +	{
> +	  switch(section)
> +	  {
> +	  case 0:
> +	    return "";
> +	  case 1:
> +	    return "Package";
> +	  case 2:
> +	    return "Installed";
> +	  case 3:
> +	    return "Candidate";
> +	  case 4:
> +	    return "Archive";
> +	  default:
> +	    return "";
> +	  }

  What do these hardcoded numbers represent?  They appear to be
numeric column IDs counting from zero?  Could we make an enum for this?

  Also, these should get _(), as should the string below.

> +	}
> +	else
> +	  return QString("Row %1").arg(section);
> +      }

  I wonder if there's a better string to plug in here -- maybe the
package name?  Is "Row N" fairly standard for Qt applications?

> +      }
> +
> +      package_ptr packages_model::package_at_index(const QModelIndex &index) const
> +      {
> +	if(!index.isValid())
> +	  return package_ptr();

  I can't help noticing that you're using tabs for indentation
again. :) Or maybe I'm just so far in the past that this is from
before you switched...

  [snip]

> +      /** \brief Class responsible for propagating packages information
> +       *  to appropriate view classes
> +       */
> +      class packages_model : public QAbstractListModel, public sigc::trackable

  I wonder if it's necessary to expose the packages_model declaration
in the header.  It looks to me like the code that uses this doesn't
need any special functionality from it, and you could get away with
moving the declaration to the .cc and leaving just this in the header:

QAbstractListModel *create_packages_model(QObject *parent);

  If this is likely to change, then it's OK to leave this in the
header; no point in taking it out if you'll just have to put it back
in again.

> +	/** \brief Retrieve a number of columns for the children of the given parent. */

  "the number of columns"
  "given parent -> given item"

> +	virtual int columnCount(const QModelIndex &parent) const;
> +
> +	/** \brief Retrieve a number of rows under the given parent. */

  Maybe "the number of children of the given item"?  Or does this
count all descendants?  (i.e., what happens if children have children)

  Aside from that, my comments on columnCount apply here.

> +	virtual int rowCount(const QModelIndex &parent = QModelIndex()) const;
> +
> +	/** \brief Returns a data stored under the given role for the item
> +	 *  referred to by the index.
> +	 */

  People who know Qt will know what this does without the comment, and
people who don't won't understand with the comment (because it uses a
bunch of Qt-specific terminology).  Also, the brief description gets a
little long here; I suggest using \param blocks to break it up.

  "Returns the value of one logical attribute of an item.

   \param index The index of the item in the model.
   \param role  The numeric ID of the attribute to extract."

> +	virtual QVariant data(const QModelIndex &index, int role) const;
> +
> +	/** \brief Retrieve a data for the given role and section in the header
> +	 *  with the specified orientation.
> +	 */

  Similarly.

  "Returns the value of one row or column heading for the model.

   \param section Describe the section here, I don't understand its purpose.
   \param orientation Qt::Horizontal to display column headings, Qt::Vertical
                      to display row headings.
   \param role Describe the role here, it seems to have a different purpose
               than in data() (e.g., maintainer is a section and not a role)."

> +	virtual QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const;
> +
> +	/** \brief Retrieve a pointer to package under the given index. */
> +	package_ptr package_at_index(const QModelIndex &index) const;

  Does this need to be public?  Nothing but the object itself seems to
call it.

  Daniel