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

[Piotr Galiszewski]

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

OK. Thanks for the clarification. Now I am understanding that

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

To be fair, it is not filter in Qt sens. It is my own implementation
of filters concept based on the code from Kadu IM (I am sure you are
bored by this Kadu, but it is first Open Source project I participated
(and I am still participating) , so I have learned a lot from its
codebase :)

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

No, it is not. It was only some "historical reasons" ;) Filter emits
signal when invalidated to QSortProxyModel, proxy model catches it and
starts validation of all items. It was also simpler to emit signal
from filter than replace filter.

Changing filter will also work. I will move reaction to changing
searched text to packages_tab, And there replace filter. It will work
:)

I moved this code to new branch on gitorious. But it still requires some work

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

It will be one of my next step to implement aptitude's search syntax.
Probably I will borrow some code from generic/apt/matches/match.cc and
make it work with package class

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

It is not necessary. I implemented first version. Thanks for answers
and sorry for trouble

P.S. Probably I am the most problematic and annoying student in this
year soc. Sorry about that ;)

>  Daniel
>
-- 
Regards
Piotr Galiszewski