[Aptitude-devel] 002-qt-stubs review

[Daniel Burrows]

On Wed, Jul 14, 2010 at 03:40:24PM +0200, Piotr Galiszewski <piotr at galiszewski.pl> was heard to say:
> 2010/7/14 Daniel Burrows <dburrows at debian.org>:
> >  If it's just a signal, could you leave it in the header despite the
> > rest of the class being abstract?  I guess that means that other code
> > could emit the signal, but other than that it should be OK for now...
> >
> 
> As I wrote above it is working now. Probably there will be no problems
> with this in the future. So another parts could also be written as
> abstract interface. Which classes should use this approach? Every
> widget?

  I would prefer to use abstract interfaces wherever we can get away
with it.

  The tricky part is often figuring out what the interfaces between
components should be.  Also, you need to know when to stop. :-)  It's
easy to end up with a bazillion tiny little interfaces for every piece
of your program (which might actually be good for industrial
development, but isn't worth it in a smaller project like this); on the
other hand, having a few really broad interfaces doesn't do anyone any
good.  Hopefully we can find some common-sensical middle ground.

  If I look over your status widget, for instance, I might split it up
like this (leaving public: virtual ~foo() out for concision).  Let me know if it seems irredeemably horrible :).

  Note that I split this into views and controllers -- the idea is that
the controller is completely free of GUI dependencies, so it's really
easy to write automated tests for it, while the view is some really
simple GUI code, so it's easy to verify by eye (if we had engineering
time to spare we might test the views too, but this is already
pushing it).

  You can find a fully worked example of this technique over in the
command-line code.  Look at cmdline_progress_display and
cmdline_download_progress_display, along with the interfaces they
implement, the code that uses them, and their test cases.  There's a
more GUI-oriented example in the search_input controller and view (in
src/generic/controllers and src/generic/views), plus the implementation
in src/gtk/view-impls.

  There is one catch regarding views and controllers, which is that I
picked up the paradigm from Java coders, and they have real garbage
collection.

  If the code goes down this path, we'll have to figure out how to
attach a controller to a view safely: specifically, how to keep the
controller alive for as long as it needs to exist, how to ensure that
it's destroyed eventually, and how to avoid problems when the view
is destroyed.  This is probably the biggest issue that I see with trying
to use a controller/view separation in your frontend.  Whatever the
solution is, it should be something that can be applied mechanically
to all the code (preferably using some common code module, maybe base
classes for Qt views and controllers).

  I hope this will be food for thought, anyway, even if it turns out
to be impractical to do exactly this -- the places where I chose to
split the code might still be interesting points to divide modules, even
if the overall arrangement ends up a bit different.

namespace aptitude::qt::models
{
  class background_job
  {
    virtual bool can_cancel() = 0;
    virtual QString get_name() = 0;
    // Ideally this would go away in favor of a less
    // widgetful way of announcing progress:
    virtual views::has_qt_widget *get_progress_widget() = 0;

  public Q_SIGNALS:
    void job_status(const QString &message, bool complete);
  };

  class active_background_jobs
  {
  public Q_SIGNALS:
    void job_started(const shared_ptr<job> &job);
  };
}

namespace aptitude::qt::views
{
  // Used so that Qt widgets can be hidden behind an abstract interface.
  // that 
  //
  // Normally get_qt_widget() will read "return this;", but this way we
  // can mock out the interface to test code that doesn't need the
  // widget.
  class QWidget;
  class has_qt_widget
  {
    virtual QWidget *get_qt_widget() = 0;
  };

  // I assume that a stackable wrapper is implemented that takes only
  // Qt widgets implementing displayable and asks the stackable widget
  // to display any widget that emits display().
  //
  // Technically that should get a controller, but I don't know if
  // the extra testability is worth the effort in this case.  I would
  // just leave a comment indicating that it could be split by someone
  // with time on their hands.

  // TODO: maybe this should inherit from aptitude::views::progress?
  class background_job_status
  {
    virtual void set_cancel_enabled(bool enabled);

    virtual void set_last_completed_activity(const QString &text) = 0;

    // Instead of taking a widget here, it would be preferable to
    // directly expose the capabilities of the widget that's placed
    // in the progress area.  I need to understand better how this is
    // supposed to actually work to eliminate that, though.
    virtual void add_progress_widget(has_qt_widget *w) = 0;

  public Q_SIGNALS:
    void cancel();
  };

  class show_changes_actions
  {
    virtual void set_show_changes_enabled(bool enabled) = 0;
    virtual void set_apply_changes_enabled(bool enabled) = 0;
    // NB: the current implementation will actually hide and show the
    // button instead of enabling and disabling it.
    virtual void set_resolve_dependencies_enabled(bool enabled) = 0;
    virtual void set_cancel_enabled(bool enabled) = 0;

  public Q_SIGNALS:
    void show_changes();
    void apply_changes();
    void resolve_dependencies();
    void cancel();
  };

  class status_area
  {
    virtual void show_changes_action_buttons() = 0;
    virtual void show_progress() = 0;

    virtual void set_status_title(const QString &text) = 0;
  };
}

namespace aptitude::qt::controllers
{
  // Controllers / logic class can potentially have methods -- it just
  // happens that these are only interested in reacting to signals.

  class background_job_status
  {
    // When a job is added to active_background_jobs,
    // the implementation will add it to the progress area
    // and connect to its update signal.  When it finishes, the
    // implementation will set the last_completed_job_label accordingly.
    // When it's canceled, the appropriate action will be taken (out of
    // scope for this example).
  };
  shared_ptr<background_job_status>
  create_background_job_status(views::background_job_status *,
                               const shared_ptr<active_background_jobs> &);

  class show_changes_actions
  {
    // This controller will listen for changes to the global package
    // state and update the state of the buttons accordingly.  It also
    // will listen for clicks on the buttons and do its own stuff, but
    // I don't have enough of the design in my head to see what that
    // stuff is.
  };
  shared_ptr<show_changes_actions>
  create_show_changes_actions(views::show_changes_actions &);

  class status_area_controller
  {
    // When a job is added to the active_background_jobs, the
    // implementation will override the current title with the job's
    // name.  When the job finishes, the normal title string will be
    // restored.
  };
  shared_ptr<status_area_controller>
  create_status_area_controller(views::status_area *,
                                const shared_ptr<active_background_jobs> &);
}

> Making this code multi windows aware needed more (sometimes tricked)
> functions, but it should work without problems

  Thanks for humoring me on that point, I know it's not too important
this early in the game. :)

> I have also split patches a little, but in different way than you
> suggested. I tried to make every patch compilable.

  I haven't had a chance to look at them yet, sorry.  Obviously I'll
let you know when I do look at them.

  Daniel