2010/8/3 Daniel Burrows <[email protected]>: > On Tue, Aug 3, 2010 at 1:42 PM, Piotr Galiszewski <[email protected]> > wrote: >> 2010/8/3 Daniel Burrows <[email protected]>:
[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 _______________________________________________ Aptitude-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
