[Aptitude-devel] Review of eb4d4a

[Daniel Burrows]

  Just some minor things here and then I think this is ready to merge.

> +        /** \brief Method called after package pkg state has changed. */
> +        void package_state_changed(std::vector<package_ptr> pkgs);

  Pass by const reference.

> +
> +        /** \brief Method called after reloading package cache. */
> +        void handle_cache_reloaded();
> +
> +        /** \brief Method called after closing package cache. */
> +        void handle_cache_closed();
> +
> +      public:
> +        /** \brief Create a new package_model for the default package_pool. */
> +        explicit packages_model_impl(QObject *parent = 0);
> +
> +        /** \brief Create a new package_model for the given package_pool. */
> +        explicit packages_model_impl(package_pool *pkg_pool, QObject *parent = 0);

  This should also be exposed in the header (forward-declare
package_pool).

> +      packages_model_impl::packages_model_impl(QObject *parent)
> +        : packages_model(parent), pkg_pool(package_pool::get_instance())
> +      {
> +        connect_package_pool();
> +      }
> +
> +      packages_model_impl::packages_model_impl(package_pool *_pkg_pool,
> +                                               QObject *parent)
> +                                                 : packages_model(parent), pkg_pool(_pkg_pool)

  Wrong indentation for the initializers.  (the ones in the previous constructor are correct)

> +      package_ptr packages_model_impl::package_at_index(const QModelIndex &index) const
> +      {
> +        if(!index.isValid())
> +          return package_ptr();
> +
> +        if(index.row() < 0 || index.row() >= rowCount())
> +          return package_ptr();
> +
> +        return pkg_pool->get_package_at_index(index.row());
> +      }
> +
> +      void packages_model_impl::package_state_changed(std::vector<package_ptr> pkgs)
> +      {
> +        Q_UNUSED(pkgs)
> +        }

  Wrong indentation for the closing brace.

> +namespace aptitude
> +{
> +  namespace gui
> +  {
> +    namespace qt
> +    {
> +      const int aptitude_role = 1000;
> +
> +      const int package_role = aptitude_role + 0;
> +      const int package_name_role = aptitude_role + 1;
> +      const int package_short_description_role = aptitude_role + 2;
> +      const int package_state_role = aptitude_role + 3;
> +      const int package_installed_version_role = aptitude_role + 4;
> +      const int package_candidate_version_role = aptitude_role + 5;
> +      const int package_archive_role = aptitude_role + 6;
> +
> +      const int filter_role = aptitude_role + 7;

  I wonder if it would be valuable to use a macro here?  Not a
requirement, just an idle thought.

#define DEFINE_APTITUDE_ROLE(id, name) const int name = aptitude_role + id;

         DEFINE_APTITUDE_ROLE(0,  package_role);
         DEFINE_APTITUDE_ROLE(1,  package_name_role);

  ...etc.  The reason for doing this is just to make the list easier
to scan (that's why I put two spaces after each comma, so if there are
more than ten roles, everything still lines up).

  Daniel