[Aptitude-devel] Review of 13edae

[Daniel Burrows]

>  	boost::unordered_map<tab_widget *, boost::unordered_set<packages_tab *> > active_packages_tabs;
>  
> -	/** \brief List of all opened package_info tabs.
> -	 *   \todo shouldn't this be unordered_map?
> -	 */
> -	boost::unordered_set<package_info_tab *> package_info_tabs;
> +	/** \brief List of all opened package_info tabs. */
> +	boost::unordered_map<package_ptr, package_info_tab *> package_info_tabs;

  Are you sure this is going to be effectively hashed?  I think you
might need a custom hash function.  We could probably drop a generic
one in src/generic/util/hash_smart_ptrs.h, something like this:

    template<typename T>
    std::size_t hash_value(const boost::shared_ptr<T> &ptr)
    {
      return hash_value(ptr.get());
    }

    // Similarly for scoped_ptr, shared_array, scoped_array.
    // weak_ptr should probably not be handled generically,
    // and intrusive_ptr shouldn't be used at all.

  That's assuming that you can treat package_ptrs as being effectively
singletons, which I think is true given your current setup.

> +	if(package_info_tabs.find(pkg) != package_info_tabs.end())
> +	{

  Wrong indentation -- should be indented 2 spaces from the if().
I spotted this in a few other places in this file.

> +	  tab *searched_tab = package_info_tabs[pkg];

  I'd prefer to save the iterator and use it again here (very minor
local optimization, but it's also the usual aptitude idiom):

    const boost::unordered_map<package_ptr, package_info_tab *>::const_iterator
        found = package_info_tabs.find(pkg);
    if(found != package_info_tabs.end())
      {
        tab * const searched_tab = found->second;
        activate_tab(searched_tab);
        return;
      }

> +	tab_widget *widget = window->get_tab_widget();
> +
> +	connect_tab_widget_signals(widget);
> +
> +	package_info_tab *new_tab = new package_info_tab(pkg);
> +	package_info_tabs[pkg] = new_tab;

  Since new_tab is a bare pointer, swap this with the setup on widget
to narrow the window during which new_tab has no owner.  i.e.,

    new_tab = new package_info_tab(pkg);
    widget->addTab(new_tab, ...);
    widget->setCurrentWidget(new_tab);
    package_info_tabs[pkg] = new_tab;

  Also, doesn't this need to either connect destroyed() or add a clause
in tab_destroyed() to ensure that stale tabs get purged from the map?
Otherwise you'll end up accessing freed memory and fall over dead, like
this: x_x.

> -	virtual void open_package_info_tab(QWidget *tab_context) = 0;
> +	virtual void open_package_info_tab(package_ptr pkg, QWidget *tab_context) = 0;

  Pass pkg by const reference here (and other places this happens).

  Daniel