[Aptitude-devel] Review of b598238

[Piotr Galiszewski]

2010/8/2 Daniel Burrows <dburrows at debian.org>:
>  Back from vacation now and feeling great :).  Well, good.  Well,
> passably decent.
>

Hello again, and greetings from DebConf! :)

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

no problem

>> +      QVariant packages_model::headerData(int section, Qt::Orientation orientation, int role) const
>
>  So, this one produces the column labels?  Am I right there?
>

yes

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

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

True. Probably we cannot to this. It is a reimplementation of
appropriate virtual method;

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

I am not sure why getter is needed? This method is used internally. It
will not be used in normal code

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

This string is only for completeness and probably we will never see
this text ;) Row is quite standard, but I can use something diffrent

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

Yes, this patch is older ;)

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

package_at_index will be used externally in one of the later patches.
So declaration has to be in place. But as I said before it will be
rewritten as an abstract interface in the future (to be more specific,
as this patch needs some fixes, I will do this at the same time ;) )

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

OK

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

Yes it counts direct children. Will change that ;)

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

Yes. I have a big problem how to document this functions. They are
simple reimplementation of abstract methods, and they are used
internally by other Qt classes (by view in this situation).

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

OK

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

It called by later (much later) code :)

>  Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>
-- 
Regards
Piotr Galiszewski