[Aptitude-devel] Experimental package and package_pool implementation

[Piotr Galiszewski]

2010/7/13 Daniel Burrows <dburrows at google.com>:
> On Mon, Jul 12, 2010 at 2:03 PM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
>> Today I have been designing and writing new packages class
>> implementation. All work is available at 004.1-package branch on
>> gitorious.
>
>  Thanks!  Do you think you could rebase these onto master?  (probably
> renaming them 001.1 then)
>

Will do

>> Firstly I created new package class, This version doesn't contains any
>> reference to any Qt classes. As it was suggested in the discussion,
>> all members of the class are independently lazy loaded from the
>> pkgIterator object. Package name is an only exception, cause it would
>> be loaded for all package shortly after creating package_model. In
>> order to avoid discarding const qualifiers of getters, all class
>> variables are mutable and requires methods are const. I am not using
>> PackageInformation class from GTK frontend, but I have moved required
>> code to this new class. For a convenience I have added a typedef for
>> package's boost::shared_ptr. If the program is compiled with Qt
>> support this pointer is exported as metatype for use in packages_model
>
>  A couple comments here:
>
>    - What's a metatype?
>

Qt models use QVariant type for distributing object. QVariant requires
registered types which have public default constructor, a public copy
constructor, and a public destructor. More informations:
http://doc.qt.nokia.com/4.6/qmetatype.html

>    - Many of the values in this class can vary by version; maybe it would
>      be a good idea to have a "version" class to go with package?
>
>    - In that case, the list of versions should probably be a list of shared
>      version pointers.
>

it will work. Should I do this, and use the same code principle as here?

>    - I would return the members of package by const reference, to avoid
>      extra copies.  This is slightly dangerous, of course, but the idiom is
>      widely used inside aptitude; return by reference generally indicates
>      that you're getting a reference to part of an object and you should
>      copy it if you want it to survive longer than the parent object.
>

Sure, I was sure, that I have forgotten about something important :P

>    - I would probably do this for the list of versions:
>
>          typedef /* TBD */ version_type;
>
>        public:
>          typedef std::list<version_type>::const_iterator version_iterator;
>
>          version_iterator versions_begin();
>          version_iterator versions_end();
>
>      That insulates the client code a bit more from the details of how the
>      versions are stored.
>

OK

>    - Typically I declare "namespace cw = cwidget" at the top of files that use
>      cwidget names, to make names shorter.
>
>    - On namespacing: I've recently taken to pulling individual names in via
>       "using" at the top of the file; it's nice documentation of what a module
>       uses:
>
>           using cw::util::transcode;
>
>       This avoids the problem with "using namespace", where you pollute
>       your namespace with a huge amount of junk.
>
>       I generally don't do this for names in std::, because that's not too long
>       a prefix, and the names in std tend to be fairly generic.  I could be
>       convinced that it's a good idea, though.
>

OK

>    - Is the list of "tags" the debtags for the package?  I believe that tags.h
>      returns those as a std::set<tag>; it would probably make sense to just
>      store that value directly.
>

Yes it is. I planned to add this later

>    - Maybe inherit from boost::noncopyable to make it noncopyable?
>
>    - I would drop the virtual destructor here and make it impossible to
>      subclass package (by hiding its constructor and using a static factory).
>      That allows you to avoid a virtual method table for each instance (not
>      a huge deal even if we create tens of thousands of them, but not bad
>      either)
>

probably it is not an option, because it won't work with QVariant type

>    - I really wish boost::flyweight didn't suck so we could make these strings
>      into flyweights easily.
>
>> Second class is package_aware_object. It purpose is to notify relevant
>> classes about changing a state of package_pool class, which was added
>> in later patch. If some class want to be notified about this event, it
>> needs to inherit from this class and reimplement three methods. This
>> approach is commonly used instead of Qt signals in Kadu IM, so I'd
>> like to ask you about your opinions. package_info_tab and
>> status_widget will be another classes using this aware object
>
>  The normal way of doing this in aptitude would be to just connect to
> the relevant signals out of src/generic/apt.h and src/generic/aptcache.h.
> I don't see a problem with having a convenience base class that sets up
> these connections for you in its constructor.  And using sigc++ will give
> you automatic features like detaching signals when the target is
> destroyed (as long as you inherit from sigc::trackable, which you can do
> in the base class).  [ok, I read your comments below; I'll think this over
> some more]
>
>  You should store the object pointers in a hash table (e.g.,
> boost::unordered_set) so that removing them is efficient.
>
>  Also, the state-changed signal should probably take a set of package
> objects instead of accepting them one at a time.  That's a bit problematic
> since you'll have to implement operator< or a hash function; a list would
> be OK for now.
>

OK. I was little confused about this two signals. I thought that that
this will be better. Will be fixed

>  I do find it odd that the methods to trigger the signals are public, but
> the signals are protected.  If you set up connections to the global signals
> in the base class constructor, you shouldn't need public methods to trigger
> the notifications, and you can encapsulate all the signal-handling in one
> module.  Conversely, I don't see any harm in making these methods
> public.  aptitude generally doesn't use protected inheritance, although this
> is a reasonably sane use of it.
>
>  I know there are a bunch of examples of this in the aptitude source
> base.  e.g., look at how src/gtk/info.cc uses the cache_closed and
> cache_reloaded signals.  The package_states_changed signal is used
> in entityview.cc.
>
>  (an amusing variant would be to use *private* virtual methods here.
> I don't think there's any real point in deploying that particular C++ trick,
> though)
>
>  I know I said that I normally prefer fully abstract interfaces :).  This
> may be an exception -- for this sort of event handling it could be better
> to have default NOP implementations.
>
>> Third patch introduces package_pool. This class is responsible for
>> managing a list of packages, which was earlier held by packages_model
>> class. Updated packages_model was added in fourth patch. packages_pool
>> will be the only class which is directly connected to signals about
>> cache updating or reloading. It updates inner package list (which uses
>> boost::multi_index_conantainer) and later translates this signal and
>> propagates it by package_aware_object. I did it in this way, cause I
>> need to be sure that packages_pool is updated earlier than other
>> classes using package objects.
>
>  First, please put this behind an abstract interface -- it shouldn't be called
> often enough for that to matter performance-wise, and right now you're
> forcing everyone to instantiate a multi_index_container if they include it.
> (that will also help with unit tests -- see below)
>  Similarly, you should be able to forward-declare "package" instead of
> including it.
>

OK. I will try to do this tomorrow

>  I would probably do a reserve() on the random access index before
> stuffing the whole package cache into it, for efficiency.
>

OK

>
>  I think a more "aptitude"y way of handling events would be to expose
> signals (or better yet, abstract connect_ methods like you can find in
> some of my more recent code) from the package_pool corresponding
> to the signals that it emits.  It would then emit its own signals at the
> appropriate time, allowing you to handle signal ordering without adding
> a new abstraction just for handling callbacks.
>
>  I'm not really sure which of these approaches is "better" -- having an
> event-handler interface would be a bit more memory-efficient (less
> signal connections to store) and would add some uniformity to the
> code, but is different from how this is handled elsewhere in aptitude.
>
>  I also wonder how this would work with unit tests.  Ideally we would
> eventually be able to fully wrap the low-level apt cache in package_pool
> or some similar class, and then provide a fake test implementation
> that didn't require bringing up all of apt.  Using a singleton makes this
> somewhat tricky; it would also be nice if the connections weren't hidden
> inside the package_aware_object class.
>

This aware object was only a proposal, cause personally I like this
approach ;)  sigc++ signals can also be used.

To sum up, what should I do with this signals?

>  Daniel
>

-- 
Regards
Piotr Galiszewski