[Aptitude-devel] 002-qt-stubs review

[Daniel Burrows]

On Mon, Jul 12, 2010 at 3:20 PM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
> 2010/7/12 Daniel Burrows <dburrows at debian.org>:
>>  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)
>>
>
> To be fair, the includes were organized but in totally opposite way ;)
> I will fix this where it will be possible. The problem is that any
> include which uses sigc++ library has to be above first Qt include. In
> other situation there are nasty compile errors during compilation.

  Wow, apparently Qt #defines the word "emit".  That's kind of horrifying.
I guess it's the heritage of having started out as a hacky preprocessor
on top of an antique dialect of C++ from circa 1996...

  I think that the only really robust way to handle this is to declare by
fiat that <sigc++/sigc++.h> has to be included at the top of every
.cc file under qt/ that needs sigc++ or references a header that
needs sigc++.

  *NOTE THAT THIS MEANS THAT THE QT HEADERS WILL NOT BE
SELF-CONTAINED.*  This is pretty horrible, but putting sigc++ includes
in the headers will mean we have to worry about header-to-header
includes when figuring out a safe order.

  Now please pardon me while I go scoop my eyes out with a rusty
spoon...


>>  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.
>>
>
> Widgets' based approach is more natural for me, cause KDE and Qt uses this

  No problem.

>>  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.
>>
>
> OK. According to documentation GUI classes can be used only in main thread.

  I thought that would probably be the case.

>>> +      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).
>>
>
> But tabs will be opened from different places in the code, and most of
> them will not have informations about tab_widget.

  It doesn't necessarily have to be tab_widget.  You could hand them an
opaque interface that only tabs_manager is allowed to invoke methods
on, for instance.

  class tabs_manager
  {
     class tabs_view
     {
      public:
        virtual ~tabs_view();

      private:
        friend class tabs_manager;

        void addTab(QWidget *page, const QString &label) = 0;
        void setCurrentWidget(QWidget *page) = 0;
        void set_tab_closable(tab *tab, bool enabled) = 0;
     };

      // ...

      void tabs_manager::open_changes_preview_tab(tabs_view &hint_view);

  If you ever want to support multiple windows, you need some sort of
information about the context in which a tab is being opened.  Adding it
now will be pretty easy -- trying to retrofit it in later when you've relied on
always having a global pointer to the tabs list will be tough.

>>  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.
>>
>
> When designing GUI I have decided that there will be no separate home
> tabs, but there will be always opened package_tab. But calling
> open_packages_tab by main_window and later set_tab_closable will be
> prettier. On the other hand, this method is invoked only once, so
> probably I will add required code to packages_tab class

  Actually, the real problem here is that you want there to always be at
least one package_tab, right?  Why should the first one that's opened be
special and un-closable if you open a second one?

  How about just maintaining the invariant "a packages_tab is unclosable
if and only if it is the only packages_tab in the notebook"?

>>> +          tab_to_delete->hide();
>>> +      }
>>
>>  Shouldn't this actually destroy the tab if it's not one of the ones
>> that gets hidden?
>>
>
> Yes. The code is added in later patch (when adding package info tabs
> started to work)

  Ok, I guess that's fine for now.

>>  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)
>>
>
> I am not sure what do you mean, but slots can be abstract and signals
> are also not a problem. You are suggesting that only public members of
> the class should be declared in *.h files, and all other code should
> be in *.cc files? It will not be too natural for me, but (probably) it
> will work without problems

  I mean that the .h file would be something like:

  class tabs_manager : public QObject
  {
    Q_OBJECT
    Q_DISABLE_COPY(tabs_manager)

    tabs_manager();
    class impl;
    friend class impl;

  public:
    virtual ~tabs_manager();

    static tabs_manager *get_instance();

    virtual void open_packages_tab() = 0;
    virtual void open_package_info_tab() = 0;
    // ...
  };

  (actually, does this even need to be a Q_OBJECT if it has no public slots?)

  The advantages of doing this are:

    1) It means that the header file just defines the interface of the module
        and not a bunch of stuff about how it's implemented.

    2) It means that code which #includes this doesn't have to parse all the
        members and doesn't have to be recompiled if they change.

  (virtual interfaces also allow implementations to be swapped, of course, but
here that doesn't seem very important)

>>  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?
>>
>
> IIRC slots can't be static, so I have to create exactly one instance
> of this class. The singleton looks the most natural for me

  Note that you could have a hidden implementation object that is
a singleton.  The virtual interface I sketched out earlier could have
had a bunch of static methods instead of virtual methods and no
get_instance() call or "impl" type.

  class tabs_manager
  {
  public:
    static void open_packages_tab();
    // etc.
  };

  and in the .cc:

  void tabs_manager::open_packages_tab()
  {
    tabs_manager_impl::get_instance()->open_packages_tab();
  }

  I don't remember which ones offhand, but this trick is used in some
existing aptitude modules.

  That said, let's use the virtual interface with a singleton for now.
I think that'll give us a little more flexibility in the future.

>>> +           *
>>> +           *   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?
>>
>
> Yes, But package class is added later in the process. For now there is
> no package at all.

  Ah, I see.  Maybe the package class should come before the widgets?

  Daniel