[Aptitude-devel] 002-qt-stubs review

[Daniel Burrows]

  I only got through the first part of the patch so far.

  [snip]

> diff --git a/src/qt/qt_main.cc b/src/qt/qt_main.cc
> index 6b04c79..181b286 100644
> --- a/src/qt/qt_main.cc
> +++ b/src/qt/qt_main.cc
> @@ -19,8 +19,12 @@
>  // the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>  // Boston, MA 02111-1307, USA.
>  
> +#include <QtGui/QApplication>
> +
>  #include <signal.h>
>  
> +#include "windows/main_window.h"
> +
>  #include "qt_main.h"

  Could you organize the includes a bit more?  My current practice for
new code is

// Local includes
#include "header_for_this_cc_file.h"

[Remaining includes in the same source directory]

[Remaining internal includes]

// System includes

[System includes in alphabetical order]


  Same for other files in the patch.

  The reason for doing this is that since I didn't make the root of my
include hierarchy something like "aptitude", there's currently no
unambiguous indication of which include files belong to aptitude and
which don't.  (someone else who was new to the codebase pointed this
out to me when they couldn't figure out where to look for include
files)

>  namespace aptitude
> @@ -34,6 +38,12 @@ namespace aptitude
>          // Don't crash if a subprocess breaks a pipe.
>          signal(SIGPIPE, SIG_IGN);
>  
> +        QApplication app(argc,argv);
> +
> +        main_window *main = new main_window();
> +        main->show();
> +
> +        app.exec();
>          return true;
>        }
>      }

  Do we need to delete main_window or does Qt do that for us?

  [snip]

> +      tabs_manager * tabs_manager::instance = 0;
> +
> +      tabs_manager *  tabs_manager::get_instance()

  Normally I don't put spaces between "*" and the following function
or variable name when defining a pointer.  i.e.,

  const char *foo;
  const char *a, *b;

  The one exception is when there's a second "const":

  const char * const foo;


  Over time, I found it a bit annoying to have the top-level UI
organization of the GTK+ code welded to the representation of the
various tabs.  I've been slowly picking away at a more
model/view-based representation of the program structure, in
gtk/toplevel/, that you may want to take a peek at.

  This design is fine for now, though; I'm only pointing that out as
food for thought.

> +      {
> +        if(0 == instance)
> +          instance = new tabs_manager();
> +
> +        return instance;
> +      }

  I just read the latest edition Effective C++ a few months ago, and
he had a very nice suggestion for handling a singleton object: make it
a static variable in a function, so it gets instantiated automatically
when the function's scope is entered for the first time. i.e.,

  tabs_manager *tabs_manager::get_instance()
  {
    static tabs_manager instance;

    return &instance;
  }

  It still isn't threadsafe, but if the Qt GUI follows the same thread
hygiene as GTK+ (one and only one UI thread, with background threads
not touching UI objects), that should be fine.


> +      void tabs_manager::open_changes_preview_tab()
> +      {
> +        if(!changes_preview)

  I usually try to write "!= NULL" for these tests, to be a little
more explicit.

  Also -- I know that this can't happen in your design, but I'd feel
better if this defensively NULLed out its members when the
corresponding tabs were destroyed.

> +      void tabs_manager::tab_widget_created(tab_widget *_tabs)

  Tabs can't be a constructor parameter because tabs_manager is a
singleton.  What if instead of being a singleton, it was held by the
main_window object?  This is also more flexible (e.g., in an imaginary
future where there are several main_window objects).

  On the other hand, maybe you want to enforce some tab invariants
across windows (e.g., only one copy of the perform changes tab).  In
that case, it would be better to leave tabs_manager as a singleton,
and change it to cleanly handle the possibility of multiple tab_widget
objects.

  I think that the only reason that you need to store a reference to
the tab_widget here is that you use it to decide where to open the
various tabs.  But if you have more than one window, then presumably
you can't know where a tab should open without more contextual
information anyway.  So all the open_* methods would change to:

    void tabs_manager::open_*_tab(tabs_context *);

  where tabs_context is probably actually a tab_widget or main_window
object.

  At that point, I don't think you need "tabs" at all, and this whole
method disappears.


  This is another part of the GTK+ interface's design that I would
change if I was starting from scratch.  I'm particularly noting the
multi-window issues because it's something that could be fairly easy
to accommodate now with a little thought, and I think it's a useful
feature.

> +      {
> +        if(tabs)
> +        {
> +          // something is wrong
> +          return;
> +        }
> +        tabs = _tabs;
> +
> +        connect(tabs, SIGNAL(tab_deletion_request(tab *)), this, SLOT(tab_deletion_requested(tab *)));
> +
> +        packages_tab *tab = new packages_tab();
> +        tabs->addTab(tab, "Packages");
> +        tabs->set_tab_closable(tab, false);

  I think I'd rather see the owner of the tabs_manager call
open_packages_tab().  I guess the difference is that this one makes
the new tab not closable?  There are two obvious ways that I see to
handle this:

  1. Make the first packages tab never closable (ew).

  2. Return the newly created or displayed tab from each open_()
     method.

  I'd also wonder if maybe there should instead be some sort of "home"
tab that's a singleton (per-window) and never closable.  Then you
don't have to mess around with making it impossible to close a package
tab.

  (I think that the tabs_manager, tabs_widget, and a "home" tab would
be a natural place to split this patch)

> +      }
> +
> +      void tabs_manager::tab_deletion_requested(tab *tab_to_delete)
> +      {
> +        // when for example performing changes tab is closed, we do not destroy it but only hide
> +
> +        if(tab_to_delete->get_type() == tab::tab_resolver || tab_to_delete->get_type() == tab::tab_perform
> +           || tab_to_delete->get_type() == tab::tab_preview)

  Maybe instead make this policy a property of the tab?

/** \brief What to do when the tab is closed. */
enum closing_policy
{
  /** Destroy the tab when it's closed. */
  closing_policy_destroy,
  /** Hide the tab when it's closed. */
  closing_policy_hide,
};

  and then say:

  if(tab_to_delete->get_policy() == closing_policy_hide)
    tab_to_delete->hide();

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

  Shouldn't this actually destroy the tab if it's not one of the ones
that gets hidden?

> +      /** \brief Manage all tabs in tab_widget.

    /** \brief Defines the tabs that can be displayed in the main
     *  window, and manages their lifetimes.
     *
     *  tabs_manager provides methods to create and/or activate each
     *  of the tabs that the main window can display.  It manages the
     *  lifetime of all the tabs that it creates, ensuring that they
     *  are added to the correct tabs_widget and are destroyed when
     *  appropriate.
     *
     *  tabs_manager is responsible for managing policies regarding
     *  tab lifetimes, such as ensuring that some tabs are singletons,
     *  or ensuring that some tabs are hidden rather than being
     *  deleted.
     *
     *  tabs_manager is a singleton, reflecting the fact that the
     *  invariants it enforces are global over all tabs in the
     *  program.
     */

> +      class tabs_manager : public QObject

  I don't know Qt well.  Is it possible to make this an abstract
interface so that other code doesn't get compile-time dependencies on
its member variables?  (I don't know if that works with the
signal/slot mechanism)

> +      {
> +        Q_OBJECT
> +        Q_DISABLE_COPY(tabs_manager)
> +
> +          /** \brief Instance of the tabs_manager. */

  /** The unique instance of tabs_manager */ (but as I noted above
this could be hidden inside get_instance()).

> +          static tabs_manager * instance;
> +
> +          /** \brief Create a new tabs_manager. */
> +          explicit tabs_manager();
> +
> +          /** \brief Destroy the tabs_manager. */
> +          virtual ~tabs_manager();

  Unless the destructor does something special, you don't need to
document it.  The default case in aptitude is to have an empty
destructor (as you do here) because members should manage their own
destruction.

> +          /** \brief Pointer to a managed tab_widget. */
> +          tab_widget *tabs;
> +
> +
> +          /** \brief Pointer to a chaneges_preview tab. */

  chaneges -> changes

  /** \brief Pointer to the globally unique changes_preview tab,
   *         or NULL if none exists.
   */

> +          changes_preview_tab *changes_preview;
> +
> +          /** \brief Pointer to a perform_changes tab. */

  /** \brief Pointer to the globally unique perform_changes tab,
   *         or NULL if none exists.
   */

> +          perform_changes_tab *perform_changes;
> +
> +          /** \brief Pointer to a resolver tab. */

  /** \brief Pointer to the globally unique resolver tab,
   *         or NULL if none exists.
   */

> +          resolver_tab *resolver;
> +
> +          //TODO: maybe QMap will be better
> +          /** \brief List of all opened package_info tabs.
> +           *   \todo shouldn't this be QMap?
> +           */

  What's the benefit of QList/QMap over a standard container type?

> +          QList<package_info_tab *> package_info_tabs;
> +
> +        private Q_SLOTS:
> +
> +          /** \brief Slot invoked when user requests closing
> +           *   on of the opened tabs.

  /** \brief Slot invoked when the user asks to close a tab
   *  managed by this object.
   */

> +           *   This slot checks if a received tab could be closed.
> +           *   If yes the tab is deleted. If closing is not acceptable,
> +           *   i.e close of changes_preview_tab was requested, tab is
> +           *   hidden


  * This slot checks whether the given tab should be closed.  If it
  * is, the tab is deleted; otherwise, it is hidden.

> +           *
> +           */
> +          void tab_deletion_requested(tab *tab_to_delete);
> +
> +        public:
> +
> +          /** \brief Return the single instance of the class.

  I'd say "globally unique" instead of "single" here and
"tabs_manager" instead of "the class".

  /** \brief Return the globally unique instance of tabs_manager. */

  I tried to find somewhere else to crib this language from and I
couldn't -- mostly, if some functionality is global, aptitude just
exposes free functions or static member functions to access it.  Is
that an option here?

> +           *
> +           *  tabs_manager is a singleton. This method return
> +           *  the instance of of the class and (if not exists)
> +           *  create new one

  Redundant; just stick to the brief description.

> +           */
> +          static tabs_manager * get_instance();
> +
> +          /** \brief Create packages_tab and add it to the
> +           *   tab_widget.

  /** \brief Create a new packages_tab. */

  I think we can leave it as given that the tabs get added to the
tab_widget.  (if this grows a tab_widget argument, then it should be
mentioned)

> +           *   This method always create new tab and add it to view
> +           *   Many packages tabs are allowed to be opened at the same
> +           *   time

  Redundant with the brief description.

> +           */
> +          void open_packages_tab();

> +          /** \brief Create package_info_tab for given package and add it
> +           *   to the tab_widget

  /** \brief Display the package_info_tab corresponding to the given
   *  package.
   *
   *  If a package_info_tab is already displayed for this package, it
   *  is reused; otherwise, a new tab is created and displayed.
   */

> +           *
> +           *   Only one package_info_tab could be opened at the same
> +           *   time for each package. This slots checks if appropriate
> +           *   tab has been already opened, and if yes activates given tab in
> +           *   the tab_widget
> +           */
> +          void open_package_info_tab();

  Shouldn't there be a package argument somewhere here?

> +          /** \brief Create resolver_tab and add it to the tab_widget

  /** \brief Display the resolver tab, creating it if it does not exist.
   *
   *  The resolver tab is globally unique.
   */

> +          void open_resolver_tab();
> +
> +          /** \brief Create changes_preview_tab and add it to the tab_widget

  /** \brief Display the changes_preview_tab, creating it if it does not exist.
   *
   *  The changes_preview_tab is globally unique.
   */

> +          void open_changes_preview_tab();
> +
> +          /** \brief Create perform_changes_tab and add it to the tab_widget

  /** \brief Display the perform_changes_tab, creating it if it does not exist.
   *
   *  The perform_changes_tab is globally unique.
   */

> +          void open_perform_changes_tab();
> +
> +          /** \brief Inform manager that tab_widget was created

  /** \brief Set the tab_widget that this object should manage.
   *
   *  When new tabs are created, they will be added to the given
   *  tab_widget.  _tabs must not be NULL.
   *
   *  It is an error to invoke any other method of this object before
   *  invoking tab_widget_created(), or to invoke this method twice.
   *  This object holds an unsafe reference to the given tab_widget,
   *  and it is an error to invoke any method of this object after
   *  destroying the tab_widget.
   *
   *  This routine is normally invoked during startup, when the
   *  main_window is created.  \todo What if there are two instances
   *  of main_window?
   */

  (both of the error conditions are eliminated by some of my earlier
   suggestions, but I'm documenting the current state of affairs)

> +           *
> +           *   This method informs tabs_manager that tab_widget was created.
> +           *   It is normally executed by main_window after creating necessary
> +           *   widgets. Only one tab_widget could be added to manager.
> +           */
> +          void tab_widget_created(tab_widget *_tabs);

  aptitude only uses the _foo convention for constructors; here i'd
use new_tabs.

  Daniel