[Aptitude-devel] 002-qt-stubs review
Piotr Galiszewski
piotr at galiszewski.pl
Mon Jul 12 22:20:26 UTC 2010
2010/7/12 Daniel Burrows <dburrows at debian.org>:
> 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)
>
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.
>> 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?
>
I will change this code to:
main_window main;
> [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;
>
OK.
>
> 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
>> + {
>> + 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.
>
OK. According to documentation GUI classes can be used only in main thread.
>
>> + 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.
>
OK.
>> + 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. I do not feel
comfortable with giving access to tab_widget to other classes than
this manager. Even, I have planned to make a static inheritance from
QTabWidget, so no other classes than friend tabs_manager will be able
to add or remove packages
> 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.
>
I have not thought about many windows at all. For now I am not able to
say how it will work. But yes in this situation context will be
necessary
>> + {
>> + 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.
>
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
> (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();
>
Yes, that is better solution. Will be fixed
>> + 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)
>> + /** \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)
>
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
>> + {
>> + Q_OBJECT
[snip]
>> + 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?
>
Sune has already explained this. Of course I can use something different ;)
>> + QList<package_info_tab *> package_info_tabs;
>> +
>> + private Q_SLOTS:
[snip]
>> +
>> + 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?
>
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
>> + *
>> + * tabs_manager is a singleton. This method return
>> + * the instance of of the class and (if not exists)
>> + * create new one
>
[snip]
>
>> + *
>> + * 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.
>> + /** \brief Create resolver_tab and add it to the tab_widget
>
> /** \brief Display the resolver tab, creating it if it does not exist.
[snip]
>> + *
>> + * 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.
>
OK. I will fix this, and all remaining comments you suggested. Thank
you very much for the review. I will split and update patch in near
future.
> Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>
--
Regards
Piotr Galiszewski
More information about the Aptitude-devel
mailing list