[Aptitude-devel] Review of 298143
Piotr Galiszewski
piotr at galiszewski.pl
Tue Aug 3 20:42:33 UTC 2010
2010/8/3 Daniel Burrows <dburrows at debian.org>:
>> + 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)
>
OK. It is another thing I have to remember about in all my patches
>> + : 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.
>
I have many problems with wrapping lines. Sometimes they are really long
>> + 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?
>
This will not be visible, but of course I can change that
[snip]
>
>> +++ 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).
>
Thanks. I will check another files
>> + 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.
>
OK.
>> + 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;
>
OK
>> + }
>> + 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;
> // ...
> }
>
> ?
>
Not exactly. There is also copy of the container created:
http://doc.trolltech.com/4.4/containers.html#the-foreach-keyword But
probably I will remove QList and use boost::unordered_map as in other
places of the code. Is using BOOST_FOREACH acceptable, or normal
iterators are better?
>> + void packages_proxy_model::add_filter(package_filter_definition_ptr filter)
>
> Why do we take a definition and not a filter here?
>
Filters are immutable, and I have to change a filter when searched
text (or search category) changes. So I created
searcf_filter_definitions, and I am changing package_filter when it is
necessary. When the filter is change appropriate signal is emitted,
and model uses this to invalidate filtering. I thought that it will be
better then removing filter from proxy and adding new filter at the
same time (I avoid double invalidating with this code. I do not think
that adding additional bool parameter to remove method is better
option).
>> + {
>> + 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?
>
Yes. I have forgotten to remove this line
>> + void packages_proxy_model::invalidate()
>> + {
>> + QSortFilterProxyModel::invalidate();
>> + }
>
> What's the purpose of this method? All it does is defer to the
> superclass.
>
it is a remains from Qt slot. With sigc++ this is probably not
necessary. This method restarts filtering of packages
>> + /** \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)?
>
It is hard to say ;) I have already used presented design in another
projects, so it was more natural for me. Also I am not sure if OR and
XOR will be used. Only AND was planned. What are you suggesting?
>> + /** \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.
OK. For the future: How long comments lines should be?
>> + 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.
>
OK
[snip]
> 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.
>
They are actually used :). So "that are used" is correct. Thanks
[snip]
>> + */
>> + 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".
>
OK.
>> + 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.
>
OK. I should use this in every future patch?
[snip]
>
>> + 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. :-)
>
Yes it is. It is used for fast searching for non advanced users. There
is a edit box where user is typing text and category combobox, when
user selects where the search should be run. I had written this before
I looked on match patterns. Probably they also can be used (I can
create pattern when user change text and create new matches_filter -
matches filter is added in one of the later patches). The speed
comparison is one questions (I really have to create some benchmarks
ASAP and posts some result - searching be description will be the best
test case). I can create pattern in code, when typed text is different
than "~" and "?"
> Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>
--
Regards
Piotr Galiszewski
More information about the Aptitude-devel
mailing list