[Aptitude-devel] 002-qt-stubs-review round 2 part 1

[Daniel Burrows]

  First round of reviews on this.  I didn't have time to go through the
whole thing, but I think tabs_manager.cc is probably the most tricky
part.

> +	/** \brief Slot invoked when a tab_widget is destroyed
> +	 *
> +	 * This slot removes all references of destroyed tab_widget from manager data
> +	 */
> +	void tab_widget_destroyed();

  Would you mind adding periods at the end of these sentences?  Also
"of destroyed -> to the destroyed".

> +      void tabs_manager::tabs_manager_impl::update_packages_tabs_closability(tab_widget *tabs)
> +      {
> +	const boost::unordered_set<packages_tab *> &packages_tabs = active_packages_tabs[tabs];
> +
> +	if(packages_tabs.size() == 1)
> +	  tabs->set_tab_closable((*packages_tabs.begin()), false);
> +	else if(packages_tabs.size() == 2)
> +	{
> +	  for(boost::unordered_set<packages_tab *>::const_iterator it =
> +	      packages_tabs.begin(); it != packages_tabs.end(); ++it)
> +	    tabs->set_tab_closable(*it, false);
> +	}
> +      }

  Should this be setting closable to "true" in the second case?

> +      void tabs_manager::tabs_manager_impl::activate_tab(tab *tab_to_activate)
> +      {
> +	main_window *window = qobject_cast<main_window *>(tab_to_activate->window());

  A general comment on this cast and the ones later on: would you mind
if I wrote a post-patch that refactored this code so that the casts
weren't necessary?  I can do it without exposing the main_widget type
to the various tabs or allowing them to invoke methods on it, which I
think is what you're trying to avoid.

  I don't think this is a big enough deal to hold up the patch for it,
or for you to divert your attention from new code.

> +	if(window->isMinimized())
> +	  window->showNormal(); // unminimize
> +	window->raise();
> +	window->activateWindow();

  Normally the window should be unminimized (since the user is
interacting with it) -- if we show a tab because of some background
process, I think it would be better to display a notification instead
of popping the window to the foreground.  Also, if the user's window
manager is configured to not raise on click, we're overriding their
preferences here.

  In summary: is there a good reason to bring ourselves into the
foreground whenever a tab is displayed?  It seems impolite to me.

> +	if(changes_preview == NULL)
> +	{
> +	  tab_widget *widget = window->get_tab_widget();
> +
> +	  connect_tab_widget_signals(widget);
> +
> +	  changes_preview = new changes_preview_tab();
> +	  widget->addTab(changes_preview, _("Preview"));
> +	  return;
> +	}

  I wonder if you could factor this into a private method to save some
typing in the couple of places that follow this pattern.  Does this go
too far?

/** \brief If the given pointer variable is not NULL, displays the
 *  corresponding tab.
 *
 *  Otherwise, instantiates a new instance of its type using the
 *  default constructor and displays that instance with the given
 *  title, storing the new instance in the given variable.
 */
void create_or_display<TabType>(TabType &output, tab_widget *tabs, const char *name)
{
  if(output == NULL)
  {
    connect_tab_widget_signals(tabs);

    output = new TabType;
    tabs->addTab(output, name);
  }

  activate_tab(output);
}

> +      void tabs_manager::tabs_manager_impl::open_package_info_tab(QWidget *tab_context)
> +      {
> +	main_window *window = qobject_cast<main_window *>(tab_context->window());
> +	if(window == NULL)
> +	  return;
> +      }

  Presumably this is being implemented in another patch?

> +      void tabs_manager::tabs_manager_impl::tab_deletion_requested(tab *tab_to_delete, bool force)
> +      {
> +	// when for example performing changes tab is closed, we do not destroy it but only hide
> +	if(tab_to_delete->get_policy() == tab::closing_policy_hide)
> +	{
> +	  if(force)

  This logic is a little hairy.

  I almost would prefer a different signal/slot pair in this case, or
maybe connecting to the destroyed signal on the tab itself when we
attach it (in create_or_display<>).

> +	  else
> +	    tab_to_delete->hide();

  I wonder if it would be better to split this into two methods:
request_destroy(), which decides whether to destroy or hide a tab, and
tab_destroyed(), which removes backlinks to the tab when it's
destroyed.

  Also, shouldn't the policy be checked in the !force case?  And isn't
it irrelevant in the force case (since the tab will be deleted
anyway)?  Or am I missing a subtlety of the logic?

  Seems to me that the logic should be:

  if(force || tab->get_policy() == closing_policy_destroy)
  {
    destroy_tab(); // Maybe only if not forced?
    tab_destroyed(tab); // Should be invoked by the destroyed signal?
  }
  else
    tab->hide();

  Daniel