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
_______________________________________________
Aptitude-devel mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
