[Aptitude-devel] Review of 376324

[Daniel Burrows]

  Just some minor nits.  Also, this doesn't seem to be dependent on its
parents.  Is it OK if I cherry-pick it onto master?  (you'll need to
rewrite its branch to drop this patch, then)  I did a quick test on my
machine, and this patch seems to be perfectly happy sitting on top of
current master instead of on 002.1-packages_tab.

> +      /** \brief A predicate on Package objects. */
> +      class package_filter
> +      {
> +      public:
> +	virtual ~package_filter() {}
> +
> +	/** \return \b true if this filter matches the given Package. */
> +	virtual bool matches(package_ptr pkg) = 0;

  Pass by const reference.

> +      class package_filter_definition_impl : public package_filter_definition
> +      {
> +	std::string name;
> +
> +	package_filter_ptr filter;
> +
> +      public:
> +	package_filter_definition_impl(const std::string &name, const package_filter_ptr &filter);
> +	~package_filter_definition_impl();
> +
> +	/** \return the name of the filter defined by this object. */
> +	const std::string &get_name() const;
> +
> +	/** \brief Modify the name of the filter defined by this object.
> +	 *
> +	 *  Causes any slots registered by connect_name_changed() to be invoked
> +	 *  if the new name is different from the current name.
> +	 */
> +	void set_name(const std::string &new_name);
> +
> +	/** \brief Register a slot to be invoked when the defined name changes. */
> +	sigc::connection connect_name_changed(const sigc::slot<void, std::string> &slot);
> +
> +
> +	/** \return the filter function associated with this filter. */

  "with this object" for consistency with earlier comments.  (also,
this mixes up filter definitions with filters)

> +	/** \brief Modify the filter function associated with this filter.
> +	 *
> +	 *  Causes any slots registered by connect_filter_changed() to be invoked
> +	 *  if the new filter function is different from the current one.

  Suggest "is a different object" instead of "different" for clarity
(i.e., this compares by address, not by value).  Also, you don't need
to duplicate the comments from the header here.

> +	/** \brief Register a slot to be invoked when the defined filter changes. */
> +	sigc::connection connect_filter_changed(const sigc::slot<void> &slot);
> +
> +	sigc::signal1<void, std::string> name_changed_signal;
> +	sigc::signal0<void> filter_changed_signal;

  Would it make sense to pass along the filter with this signal, for
consistency?

> +      /** \brief A filter in the list of filters presented to the user.

  Maybe "an entry" instead of "a filter"?  (filter definitions have
filters, but they aren't filters)

> +	/** \brief Register a slot to be invoked when the defined name changes. */
> +	virtual sigc::connection connect_name_changed(const sigc::slot<void, std::string> &slot) = 0;
> +
> +
> +	/** \return the filter function associated with this filter. */

  "this filter" -> "this object" (yes, I'm repeating myself, sorry)

> +	virtual package_filter_ptr get_filter() = 0;
> +
> +	/** \brief Modify the filter function associated with this filter.

  "this filter" -> "this object"

> +	 *  Causes any slots registered by connect_filter_changed() to be invoked
> +	 *  if the new filter function is different from the current one.

  Same note as in the .cc file.

> +      /** \brief Create a package_filter_definition.
> +       *
> +       *  \param name   The name of the new filter definition.
> +       *  \param filter The filter defined by the new filter definition.
> +       */
> +      package_filter_definition_ptr
> +	  make_package_filter_definition(const std::string &name,
> +					 const package_filter_ptr &filter);
> +    }

  Per our recent discussion, maybe this should be a static ::create() method?

  Daniel