[Aptitude-devel] Review of 298143

[Daniel Burrows]

On Tue, Aug 3, 2010 at 1:42 PM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
> 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

  Incidentally, this isn't just theoretical -- I will probably commit
unit tests on
top of this patch when I merge your code.

> I have many problems with wrapping lines. Sometimes they are really long

  Yeah, that's why I commented earlier that I would perhaps use hanging
indents (what you did here) in a new project.  But in general, I find that I can
fix long lines by giving local names to parts of the expression (which is good
practice for readability anyway if you have a big, complex expression).

>>  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?

  If c++0x is usable before the heat death of the universe[0], I'd love to use
its extended "for" construct here.  I do like the idea of using
BOOST_FOREACH in the meantime.  Go ahead and use it wherever
reasonable (but watch out for some of the caveats caused by its
macro-ness).

  [0] heat death may be void in your area, ask your local cosmologist for
       details.

>>> +      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.

  This gets at something I mentioned later, so I'll defer answering it.

> 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).

  So invalidating twice is more expensive than invalidating once?  I
usually expect invalidate to just mean "flip a switch saying we need to
recalculate this", so multiple calls to it are cheap as long as you don't
request the invalidated value in the meantime.  If invalidating actually
restarts a calculation, that could matter (but see below).

>>> +      /** \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?

  XOR was a joke, hence the smiley :).  But I can see both OR and AND being
supported (with a little tick-box saying whether to require all terms to match),
if you feel that it makes sense from a UI perspective.  Also, if you only wanted
AND, your doccomment is backwards.  (should say that the package is
accepted if all filters match, not if at least one matches)


  The natural design for me would be to have the proxy model have a
single filter, and have some other component keep track of what it is.  e.g.,
maybe the list of available filters has a signal saying "the filter
has changed",
which is emitted whenever the selection changes, and which automatically
bundles up all the selected filters into an OR / AND as appropriate.  That feels
more proper than having the package list know what multiple selected filters
mean.

>> 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?

  I usually wrap at 80 columns.  I make an effort to wrap code lines
at 72-80 columns, but I'm not religious about this, as I mentioned
above; I'd rather see a slightly long line than a really awkward
wrapping.

  (this is all cheating for me because Emacs wraps comments at 80
columns with a single keystroke :) )

>>  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

  I evidently don't understand what this is doing, then.  I thought it just
managed all the defined filters, and the user could select some to activate
in a particular packages_proxy_model?

>>> +     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?

  I think I would prefer begin() and end() methods when you're just exporting
a pile of objects that the client will want to iterate over.  If the
client will want
to do more than that, exporting the member could make sense.  Please feel
free to point out the places in the existing codebase that don't match this
pattern; they might indicate that it needs to be refined.

>>  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).

  I don't mean that the user has to enter a match pattern, but you can
create them programmatically.  I think that backslash_escape_nonalnum
(from util.h) will let you escape regex metacharacters, but you'll
want to verify that.

> 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).

  That would be interesting.  Actually, description might be the worst
test case, because it will tend to be I/O-bound.  Searching by name
will probably show more differences.

> I can create pattern in code, when typed text is different
> than "~" and "?"

  That shouldn't matter -- use pattern::make_name(),
pattern::make_maintainer(), etc.

  Daniel