[Aptitude-devel] Review of 298143
piotr at galiszewski.pl
Tue Aug 3 23:18:36 UTC 2010
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>:
>> 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
> 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
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
> (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
>> 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.
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.
More information about the Aptitude-devel