[Aptitude-devel] Review of 298143

[Piotr Galiszewski]

2010/8/3 Daniel Burrows <dburrows at google.com>:
> 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>:

[snip]

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

In this situation filtering is restarted.

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

Yes. It is mistake in comment

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

So there could be one more similar class like filters_manager?

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

Sorry. My mistake. Jet lag is still with me and my thoughts weren't
clear. I thought that this comment is about proxy model. In this case
your second proposal looks more correct

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

OK

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

I was talking about the same thing ;)

>> 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 know about IO. This is the reason I propose to use this for
measurements, as current design is using lazy loaded data. I will try
to test both cases

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

Ok thanks a lot.

>  Daniel
>

-- 
Regards
Piotr Galiszewski