[Aptitude-devel] aptitude-qt VCS usage and code review

[Piotr Galiszewski]

2010/7/9 Daniel Burrows <dburrows at google.com>:
>   BTW, regarding the filter class, I have some more comments.  I still
> don't know if I can post the sample code I wrote at work yesterday, so
> I'm just going to include my comments and leave the code out.
>
>
>   Looking at this class reminds me of something else: I generally prefer to
> avoid concrete virtual classes in favor of interface-based inheritance.
>  There
> are a bunch of benefits to this; e.g., it usually gives you a conceptually
> cleaner
> design, it makes it much easier to cut dependencies between components (for
> testing, for instance), and it makes it a lot clearer exactly how two
> components
> interact (instead of the slippery business of self-calls).  This is
> different from
> how traditional widget sets (like Qt) tend to work, but I think it's worth
> the
> trouble to get the hang of this style of coding.
>   In this case, it looks to me like you have three things that are mixed
> together:
>     1. The filter function.
>     2. The name associated with the filter (always set when the filter is
> constructed
>         and never changed).
>     3. A holder that allows the filter function to be modified to another
> filter that's
>         implemented by the same class and that emits a signal when it's
> changed.
>   Item (1) could be accomplished simply by having a member variable of type
> boost::function<bool(Package *)>, or by using your own interface.  Probably
> a
> custom interface is a bit better?  I would avoid Qt dependencies in the
> filter
> interface (leaving aside Package), although obviously implementations could
> rely on Qt objects.  Filters should probably be immutable.
>   Item (2) confuses me -- what is the name for, exactly?  If it's just the
> name the user gave, why is it immutable?  I'm going to assume that
> was an oversight.

Name  is shown in filters_list on the right side of packages tab. It
is immutable, because I have forgotten to add setter. I planned to do
this when creating manage filters window. As  name is confusing
probably display or display_name would be better.

>   Item (3) can be handled at the same time as item (2), by creating
> a separate "holder" class that represents the user's definition of the
> filter and is modified when the definition is modified.  This has the
> extra advantage of letting you switch to any other filter, not just one
> implemented by the same concrete class.
>   i.e., you would have "package_filter", which is an interface for
> predicates
> on packages, and "package_filter_definition", which contains a name and a
> package_filter (pointer), both of which have mutators, and which has two
> signals corresponding to those fields, emitted when they change.
>   Daniel

If I understand correctly package_filter contains
accept_package(package) function and for example lists of accepted
tags/sections or other mutators. Later every package_filter_definition
sets proper values of this mutators? I have read it several times and
it is still not entirely clear for me. Some sample code will be really
nice :)
-- 
Regards
Piotr Galiszewski