[Aptitude-devel] 002-qt-stubs review

[Piotr Galiszewski]

2010/7/13 Daniel Burrows <dburrows at google.com>:
> 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...
>

It is possible, that compiling program with -no-keyword option will help.

>
>>>  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);
>

I will see what can be done ;)

>  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.
>

OK

>>>  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"?

It should work too. I will change the code of packages_tab and add
there proper static method

>
>>>> +          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)
>

OK. Thanks for the clarification. I will look on other aptitude code
how it is working

>>>  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.

I have only one more question. Currenty tabs_manager sends also Qt
signals. The signal is used to determine which buttons should be
active on status_widget. For example, when changes preview tab will be
active, show changes button should be disabled. It will increase
usability for the users. The same situation is with resolver or
perform changes tab

>
>>>> +           *
>>>> +           *   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?
>

The repository is in git, so it could be done ;)

>  Daniel
>

-- 
Regards
Piotr Galiszewski