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

[Daniel Burrows]

On Thu, Jul 15, 2010 at 4:47 PM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
> Thanks for the code! I am working on this now (will be finished
> tomorrow) and I have one question. You write that package_filter
> should be immutable. What about search filter? It will not be added to
> any view, so name is not not necessary. It looks for me that
> implementation of package_filter will be sufficient. The only problem
> is that searched text could change and in this situation signal should
> be emitted to proxy_model.

  I think my main concern here was that you're mixing filter logic
(the function that selects packages) with the UI code to react to
user actions.  My own design preference is for stuff to be as
immutable as is reasonable/practical, especially in the "logic"
layers of the program.

  Speaking more practically, the "I've been changed" signal
is a warning sign the way you've defined it above because by
definition, it can only be emitted when the internal structure
of a filter has changed, not when its basic type has changed
(since that requires the creation of a new filter).  So if you ever
have another way of filtering packages (say, by whether they
have an update), you won't be able to switch filter styles and
have the view update properly, unless you did all of your
filtering with one class type.

  I think we may be running into a confusion of terminology.
By "filter" I just mean a predicate that's used to choose a
sublist of a list.  Maybe the things I'm calling "filters" aren't
filters in the Qt sense?

  If a Qt filter is *supposed* to be modified when the filter it
represents changes (rather than just being replaced), then it's
part of the UI, and the problem is not that it's mutable, but
that it's absorbed part of the non-UI program logic.  In that
case I would have it hold a reference to the actual predicate,
and change that reference to a new one when a new predicate
is selected.

  This is effectively what all the other frontends do, except that
they only support one predicate (aptitude match expressions)
and they don't have a separate class for holding the search
predicate and updating the view when it changes.

  BTW: I would be perfectly happy with just having the filter hold
a match expression and use that to search.  These extra layers
of indirection are only needed because you seemed to want to
use non-match-expression predicates.


  So, assuming that filter is a "term of art" in Qt, how about

    "selector" for a predicate on packages (or alternatively, just
use boost::function<bool, PkgIterator>)

  and

    "filter" for the Qt filter concept.

  A filter simply holds a reference to a selector and announces
when the selector has been updated to a new value.

  Does that make sense?

  Daniel