[Aptitude-devel] Review of 298143

[Daniel Burrows]

> +      package_filters_model::package_filters_model(QObject *parent)

  Pass the package_filters_manager here and save it in a member, so
other implementations can be substituted.  (you can provide a
convenience constructor that uses get_instance() as usual)

> +	: QAbstractListModel(parent)
> +      {
> +	package_filters_manager::get_instance()->connect_filter_about_to_be_added(
> +	    sigc::mem_fun(*this, &package_filters_model::filter_about_to_be_added));

  Wrapping doesn't match the usual style here.  You can save yourself
a lot of horizontal space by saving a pointer to the manager (but see
my note above):

    package_filters_manager * const manager = package_filters_manager::get_instance();

  Also, you can create slots on separate lines from the connect() calls...

    sigc::slot<void, package_filter_definition_ptr> added_slot =
      sigc::mem_fun(*this, &package_filters_model::filter_added);
    manager->connect_filter_added(added_slot);

  ...although TBH, I would probably just write some long lines instead.

> +      QVariant package_filters_model::headerData(int section, Qt::Orientation orientation, int role) const
> +      {
> +	if (role != Qt::DisplayRole)
> +	  return QVariant();
> +
> +	if (orientation == Qt::Horizontal)
> +	  return QString("Column %1").arg(section);

  Should this output something like "Filter" instead?

> +      /** \brief Class responsible for propagating package filers information

  "propagating package filter information to package filter views".

> +       *  to package filters views
> +       */
> +      class package_filters_model : public QAbstractListModel
> +      {
> +	Q_OBJECT
> +
> +	/** \brief Method called just before filter is added to manager.
> +	*
> +	*  This method starts adding of the given filter to view

  "just before a filter is added to the manager", "adding the given
filter to the view".  Also, pass by const reference.

> +	*/
> +	void filter_about_to_be_added(package_filter_definition_ptr filter);
> +
> +	/** \brief Method called just after filter is added to manager.
> +	 *
> +	 *  This method ends adding of the given filter to view

  Ditto

> +	 */
> +	void filter_added(package_filter_definition_ptr filter);
> +
> +	/** \brief Method called just before filter is removed to manager.
> +	 *
> +	 *  This method starts removing of the given filter from view

  Ditto

> +	 */
> +	void filter_about_to_be_removed(package_filter_definition_ptr filter);
> +
> +	/** \brief Method called just before filter is removed to manager.
> +	 *
> +	 *  This method ends removing of the given filter from view
> +	 */
> +	void filter_removed(package_filter_definition_ptr filter);

  Ditto.

> +      public:
> +	/** \brief Create a new package_filters_model. */
> +	explicit package_filters_model(QObject *parent = 0);

  As noted above, this should take a manager pointer.

> +	virtual ~package_filters_model();
> +
> +	/** \brief Retrieve a number of columns for the children of the given parent. */
> +	virtual int columnCount(const QModelIndex &parent) const;

  "the number of columns"

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

  "the number of rows"

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

  "Returns an attribute of the given index, selected by its role."

> +
> +	/** \brief Retrieve a data for the given role and section in the header
> +	 *  with the specified orientation.
> +	 */
> +	virtual QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const;

  "Returns an attribute of the column header."

> +	/** \brief Retrieve a pointer to a filer under the given index. */

  "a filer under the given index" -> "the filter at the given index".

> +	package_filter_definition_ptr filter_at_index(const QModelIndex &index) const;
> +
> +	/** \brief Retrieve a index in view for a the given filter. */
> +	int filter_index(package_filter_definition_ptr filter) const ;

  "Retrieve the index of the given filter", also pass by const
reference.

> +++ b/src/qt/models/packages_proxy_model.cc
> @@ -0,0 +1,135 @@
> +/** \file package_proxy_model.cc */

  \file doesn't match the file name (saw this in other files in this
patch too, not noting all of them).

> +      bool packages_proxy_model::lessThan(const QModelIndex &left, const QModelIndex &right) const
> +      {
> +	if(!source_packages_model)
> +	  return QSortFilterProxyModel::lessThan(left, right);
> +
> +	switch(left.column())
>
> +	{
> +	case 1:

  Lots of hardcoding and magic numbers.  Please at least put an enum
in here.

> +	case 2:
> +	  {
> +	    QString left_string = left.data(package_installed_version_role).toString();
> +	    QString right_string = right.data(package_installed_version_role).toString();
> +
> +	    return left_string > right_string;

  When comparing version numbers, you should use CmpVersion.  e.g.,

    return _system->GetVS()->CmpVersion(left_string, right_string) < 0;

> +	  }
> +	case 3:
> +	  {
> +	    QString left_string = left.data(package_candidate_version_role).toString();
> +	    QString right_string = right.data(package_candidate_version_role).toString();
> +
> +	    return left_string > right_string;

  Ditto.

> +      bool packages_proxy_model::filterAcceptsRow(int source_row, const QModelIndex &source_arent) const
> +      {
> +	package_ptr pkg = sourceModel()->index(source_row, 0).data(package_role).value<package_ptr>();
> +	Q_FOREACH(package_filter_definition_ptr filter, filters)

  Is Q_FOREACH just a fancy way of saying

    for(QList<package_filter_definition_ptr>::const_iterator it = filters.begin();
        it != filters.end(); ++it)
      {
        const package_filter_definition_ptr filter = *it;
        // ...
      }

  ?

> +      void packages_proxy_model::add_filter(package_filter_definition_ptr filter)

  Why do we take a definition and not a filter here?

> +      {
> +	filters.append(filter);

  Maybe this should be a set, so we can dedupe?

> +      void packages_proxy_model::remove_filter(package_filter_definition_ptr filter)
> +      {
> +	filters.removeAll(filter);
> +	invalidateFilter();
> +//	filter->connect_filter_changed(sigc::mem_fun(*this, &packages_model::handle_cache_reloaded));

  Is that stale code I see?

> +      void packages_proxy_model::invalidate()
> +      {
> +	QSortFilterProxyModel::invalidate();
> +      }

  What's the purpose of this method?  All it does is defer to the
superclass.

> +      /** \brief Class responsible for sorting and filtering data passed between package_model
> +       *  and package view widget
> +       *
> +       *  This class contains list of active filters. During package checking, matches method
> +       *  is called on all registered filters. When at least one filter matches tested package
> +       *  the package is accepted.

  I think I would prefer to just have a single filter and manage the
policy of OR, AND, XOR ( :) ), etc elsewhere, probably in whatever is
handling selections (i.e., create an "OR filter" that accumulates some
sub-filters).  Unless there's a benefit to managing the list here
(e.g., performance / design)?

> +	/** \brief Create new packages_proxy_model*/

  "a new", also "model*/" -> "model. */"

> +	explicit packages_proxy_model(QObject *parent = 0);
> +
> +	virtual ~packages_proxy_model();
> +
> +	/** \brief Sets the given sourceModel to be processed by this proxy model. */

  Maybe "Set the source model that will be filtered by this proxy."?
Also, needs wrapping.

> +	virtual void setSourceModel(QAbstractItemModel *source_model);
> +
> +	/** \brief Adds the given filter to list of used filters and invalidates filtering. */

  "to the list of active filters", "invalidates the filtering", wrap.

> +	void add_filter(package_filter_definition_ptr filter);
> +
> +	/** \brief Removes the given filter from list of used filters and invalidates filtering. */

  "from the list of active filters", "invalidates the filtering", wrap.

> +	void remove_filter(package_filter_definition_ptr filter);
> +
> +	/** \brief Method called when one of the active filters has changed.
> +	 *
> +	 *  This method also invalidates actual filtering

  "invalidates the filtering".

> +	 */
> +	void invalidate();
> +      };
> +    }
> +  }
> +}

> +      class package_filters_manager::package_filters_manager_impl : public package_filters_manager
> +      {
> +	bool loaded;
> +
> +	std::vector<package_filter_definition_ptr> filters;
> +
> +	//for future:
> +	void ensure_loaded();
> +	void load();
> +	void store();
> +
> +      public:
> +	explicit package_filters_manager_impl();
> +	virtual ~package_filters_manager_impl();
> +
> +	package_filter_definition_ptr by_index(unsigned int index);
> +	unsigned int count();
> +	const std::vector<package_filter_definition_ptr> get_filters();
> +	unsigned int index_of(package_filter_definition_ptr filter);
> +
> +	void add_filter(package_filter_definition_ptr filter);
> +	void remove_filter(package_filter_definition_ptr filter);
> +
> +	sigc::signal1<void, package_filter_definition_ptr> filter_about_to_be_added;
> +	sigc::signal1<void, package_filter_definition_ptr> filter_added;
> +	sigc::signal1<void, package_filter_definition_ptr> filter_about_to_be_removed;
> +	sigc::signal1<void, package_filter_definition_ptr> filter_removed;
> +
> +	sigc::connection connect_filter_about_to_be_added(const sigc::slot<void, package_filter_definition_ptr> &slot);
> +	sigc::connection connect_filter_added(const sigc::slot<void, package_filter_definition_ptr> &slot);
> +	sigc::connection connect_filter_about_to_be_removed(const sigc::slot<void, package_filter_definition_ptr> &slot);
> +	sigc::connection connect_filter_removed(const sigc::slot<void, package_filter_definition_ptr> &slot);
> +      };
> +
> +      package_filters_manager::package_filters_manager_impl::package_filters_manager_impl()
> +	: loaded(false)
> +      {
> +      }
> +
> +      package_filters_manager::package_filters_manager::package_filters_manager_impl::~package_filters_manager_impl()
> +      {
> +      }
> +
> +      void package_filters_manager::package_filters_manager_impl::ensure_loaded()
> +      {
> +	if(!loaded)
> +	{
> +	  //TODO: loaded should be after load()
> +	  loaded = true;
> +	  load();
> +	}
> +      }
> +
> +      void package_filters_manager::package_filters_manager_impl::load()
> +      {
> +      }
> +
> +      void package_filters_manager::package_filters_manager_impl::store()
> +      {
> +
> +      }
> +
> +      package_filter_definition_ptr package_filters_manager::package_filters_manager_impl::by_index(unsigned int index)
> +      {
> +	ensure_loaded();
> +
> +	if(index >= count())
> +	  return package_filter_definition_ptr();
> +
> +	return filters.at(index);
> +      }
> +
> +      unsigned int package_filters_manager::package_filters_manager_impl::count()
> +      {
> +	ensure_loaded();
> +	return filters.size();
> +      }
> +
> +      unsigned int package_filters_manager::package_filters_manager_impl::index_of(package_filter_definition_ptr filter)
> +      {
> +	ensure_loaded();
> +	for(unsigned int i=0;i<filters.size();++i)
> +	  if(filters[i] == filter)
> +	    return i;
> +
> +	return -1;
> +      }
> +
> +      const std::vector<package_filter_definition_ptr> package_filters_manager::package_filters_manager_impl::get_filters()
> +      {
> +	ensure_loaded();
> +	return filters;
> +      }
> +
> +      void package_filters_manager::package_filters_manager_impl::add_filter(package_filter_definition_ptr filter)
> +      {
> +	ensure_loaded();
> +
> +	if(find(filters.begin(), filters.end(), filter) == filters.end())
> +	  return;
> +
> +	filter_about_to_be_added(filter);
> +	filters.push_back(filter);
> +	filter_added(filter);
> +      }
> +
> +      void package_filters_manager::package_filters_manager_impl::remove_filter(package_filter_definition_ptr filter)
> +      {
> +	ensure_loaded();
> +
> +	int i;
> +	if((i = find(filters.begin(), filters.end(), filter) == filters.end()))
> +	  return;
> +
> +	filter_about_to_be_removed(filter);
> +	filters.erase(filters.begin() + i);
> +	filter_removed(filter);
> +      }
> +
> +      sigc::connection
> +      package_filters_manager::package_filters_manager_impl::connect_filter_about_to_be_added(const sigc::slot<void, package_filter_definition_p> tr> &slot)
> +      {
> +	return filter_about_to_be_added.connect(slot);
> +      }
> +
> +      sigc::connection
> +      package_filters_manager::package_filters_manager_impl::connect_filter_added(const sigc::slot<void, package_filter_definition_ptr> &slot)
> +      {
> +	return filter_added.connect(slot);
> +      }
> +
> +      sigc::connection
> +      package_filters_manager::package_filters_manager_impl::connect_filter_about_to_be_removed(const sigc::slot<void, package_filter_definition> _ptr> &slot)
> +      {
> +	return filter_about_to_be_removed.connect(slot);
> +      }
> +
> +      sigc::connection
> +      package_filters_manager::package_filters_manager_impl::connect_filter_removed(const sigc::slot<void, package_filter_definition_ptr> &slot)
> +      {
> +	return filter_removed.connect(slot);
> +      }
> +
> +      package_filters_manager *package_filters_manager::get_instance()
> +      {
> +	static package_filters_manager_impl instance;
> +
> +	return &instance;
> +      }
> +
> +      package_filters_manager::package_filters_manager()
> +      {
> +      }
> +
> +      package_filters_manager::~package_filters_manager()
> +      {
> +      }
> +    }
> +  }
> +}
> diff --git a/src/qt/package_filters_manager.h b/src/qt/package_filters_manager.h
> new file mode 100644
> index 0000000..c109799
> --- /dev/null
> +++ b/src/qt/package_filters_manager.h
> @@ -0,0 +1,112 @@
> +/** \file filter_manager.h */   // -*-c++-*-
> +//
> +// Copyright (C) 2010 Piotr Galiszewski
> +//
> +// This program is free software; you can redistribute it and/or
> +// modify it under the terms of the GNU General Public License as
> +// published by the Free Software Foundation; either version 2 of the
> +// License, or (at your option) any later version.
> +//
> +// This program is distributed in the hope that it will be useful, but
> +// WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +// General Public License for more details.
> +//
> +// You should have received a copy of the GNU General Public License
> +// along with this program; see the file COPYING.  If not, write to
> +// the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> +// Boston, MA 02111-1307, USA.
> +
> +#ifndef APTITUDE_QT_PACKAGE_FILTERS_MANAGER_H
> +#define APTITUDE_QT_PACKAGE_FILTERS_MANAGER_H
> +
> +// System includes
> +#include <boost/shared_ptr.hpp>
> +
> +#include <sigc++/connection.h>
> +#include <sigc++/slot.h>
> +
> +#include <vector>
> +
> +namespace aptitude
> +{
> +  namespace gui
> +  {
> +    namespace qt
> +    {
> +      class package_filter_definition;
> +      typedef boost::shared_ptr<package_filter_definition> package_filter_definition_ptr;
> +

  Thanks for breaking the generic stuff out into a separate class.

> +      /** \brief Manage all defined filters in application.

  Manages the defined filters.

> +       *
> +       *  package_filters_manager provides methods to adding and removing each
> +       *  of the package filters that the could be used in filter view.

    package_filters_manager maintains the set of package filters that
    can be used in filtered views.

  Or did you mean:

    package_filters_manager maintains the set of package filters that
    can be picked from filter lists.

> +       *  Every registered filter is displayed in each package filter view of the program

    Every registered filter is displayed in each list of available
    filters.

> +       *  Registered filters will be stored in configuration file and lazy loaded before
> +       *  first use of filters.

    All registered filters will be stored in the configuration file
    and lazily loaded before the first time that the list of filters
    is required.

> +	/** \brief Retrieve filter at the given index.

  Retrieve the filter at the given index.

> +	 *  If the index is out of range empty pointer is returned

  Usually I say "invalid pointer" instead of "empty pointer".

    If the index is out of range, an invalid pointer is returned.

> +	 */
> +	virtual package_filter_definition_ptr by_index(unsigned int index) = 0;
> +
> +	/** \brief Retrieve registered filters count. */

  Retrieve the number of registered filters.  It would be more
consistent with normal C++ usage to call this "size".

> +	virtual unsigned int count() = 0;
> +
> +	/** \brief Retrieve all registered filters. */
> +	virtual const std::vector<package_filter_definition_ptr> get_filters() = 0;

  Maybe instead export begin() and end()?  If not, return by const
reference.

> +	/** \brief Retrieve the index of the given filter or -1 if not found. */

  "or -1 if it is not present".

> +	virtual unsigned int index_of(package_filter_definition_ptr filter) = 0;
> +
> +	/** \brief Adds new filter to the filters list.
> +	 *
> +	 *  If filter is already added this method does nothing. On other situation

  "Adds a new filter"

  "On other situation" -> "Otherwise,"

> +	 *  filter_about_to_be_added and filter_added signals are emitted

  "the filter_about_to_be_added"

  "are emitted."

> +	 */
> +	virtual void add_filter(package_filter_definition_ptr filter) = 0;

  Pass by const reference.

> +	/** \brief Removes filter from the filters list.

  "Removes a filter"

> +	 *  If filter is not in list this method does nothing. On other situation

  "If the filter"

  "On other situation" -> "Otherwise,"

> +	 *  filter_about_to_be_removed and filter_removed signals are emitted

  "the filter_about_to_be_removed"

  "are emitted."

> +	 */
> +	virtual void remove_filter(package_filter_definition_ptr filter) = 0;

  Pass by const reference.

> +	/** \brief Connect to a signal emited just before filter is added to manager. */

  emitted.
  "just before a filter is added to the manager."

> +	virtual sigc::connection connect_filter_about_to_be_added(const sigc::slot<void, package_filter_definition_ptr> &slot) = 0;
> +
> +	/** \brief Connect to a signal emited just after filter is added to manager. */

  Ditto.

> +	virtual sigc::connection connect_filter_added(const sigc::slot<void, package_filter_definition_ptr> &slot) = 0;
> +
> +	/** \brief Connect to a signal emited just before filter is removed from manager. */

  Ditto.

> +	virtual sigc::connection connect_filter_about_to_be_removed(const sigc::slot<void, package_filter_definition_ptr> &slot) = 0;
> +
> +	/** \brief Connect to a signal emited just after filter is removed from manager. */

  Ditto.

> +	virtual sigc::connection connect_filter_removed(const sigc::slot<void, package_filter_definition_ptr> &slot) = 0;
> +      };
> +    }
> +  }
> +}
> +
> +#endif // APTITUDE_QT_PACKAGE_FILTERS_MANAGER_H


> +        enum search_by
> +        {
> +          /** \brief searched_text is searched only in package name. */
> +          search_by_name = 0,
> +          /** \brief searched_text is searched in package name and package descriptions. */
> +          search_by_name_and_description = 1,
> +          /** \brief searched_text is searched in package maintainer name */
> +          search_by_maintainer = 2,
> +          /** \brief searched_text is searched only in all available package versions */
> +          search_by_version = 3
> +                            };

  Doesn't this basically reimplement a small section of the match
patterns?  Please don't do that.  I'm not going to comment further on
this class unless you can convince me it should exist. :-)

  Daniel