[Aptitude-devel] 002-qt-stubs-review round 2 part 1

[Piotr Galiszewski]

2010/7/19 Daniel Burrows <dburrows at debian.org>:
>  First round of reviews on this.  I didn't have time to go through the
> whole thing, but I think tabs_manager.cc is probably the most tricky
> part.
>
>> +     /** \brief Slot invoked when a tab_widget is destroyed
>> +      *
>> +      * This slot removes all references of destroyed tab_widget from manager data
>> +      */
>> +     void tab_widget_destroyed();
>
>  Would you mind adding periods at the end of these sentences?  Also
> "of destroyed -> to the destroyed".
>

OK.

>> +      void tabs_manager::tabs_manager_impl::update_packages_tabs_closability(tab_widget *tabs)
>> +      {
>> +     const boost::unordered_set<packages_tab *> &packages_tabs = active_packages_tabs[tabs];
>> +
>> +     if(packages_tabs.size() == 1)
>> +       tabs->set_tab_closable((*packages_tabs.begin()), false);
>> +     else if(packages_tabs.size() == 2)
>> +     {
>> +       for(boost::unordered_set<packages_tab *>::const_iterator it =
>> +           packages_tabs.begin(); it != packages_tabs.end(); ++it)
>> +         tabs->set_tab_closable(*it, false);
>> +     }
>> +      }
>
>  Should this be setting closable to "true" in the second case?
>

Yes

>> +      void tabs_manager::tabs_manager_impl::activate_tab(tab *tab_to_activate)
>> +      {
>> +     main_window *window = qobject_cast<main_window *>(tab_to_activate->window());
>
>  A general comment on this cast and the ones later on: would you mind
> if I wrote a post-patch that refactored this code so that the casts
> weren't necessary?  I can do it without exposing the main_widget type
> to the various tabs or allowing them to invoke methods on it, which I
> think is what you're trying to avoid.
>
>  I don't think this is a big enough deal to hold up the patch for it,
> or for you to divert your attention from new code.
>

Yes of course. I have not seen simpler way to do this, and this
qobject_cast should work (and they do not require RTTI ). I am willing
to learn better way to do it

>> +     if(window->isMinimized())
>> +       window->showNormal(); // unminimize
>> +     window->raise();
>> +     window->activateWindow();
>
>  Normally the window should be unminimized (since the user is
> interacting with it) -- if we show a tab because of some background
> process, I think it would be better to display a notification instead
> of popping the window to the foreground.  Also, if the user's window
> manager is configured to not raise on click, we're overriding their
> preferences here.
>
>  In summary: is there a good reason to bring ourselves into the
> foreground whenever a tab is displayed?  It seems impolite to me.
>

I have made an assumption, that tabs are only opened on user request.
Background processes shouldn't open new tabs. If they want to inform
user about something, they would use status_widget (now
active_background_jobs model) and add to widget "Show Details" button,
which will open new tab
I thought that if user decides to show some tabs, he want to use them
immediately. Of course I could be wrong and this code could be easy
fixed ;)

>> +     if(changes_preview == NULL)
>> +     {
>> +       tab_widget *widget = window->get_tab_widget();
>> +
>> +       connect_tab_widget_signals(widget);
>> +
>> +       changes_preview = new changes_preview_tab();
>> +       widget->addTab(changes_preview, _("Preview"));
>> +       return;
>> +     }
>
>  I wonder if you could factor this into a private method to save some
> typing in the couple of places that follow this pattern.  Does this go
> too far?
>
> /** \brief If the given pointer variable is not NULL, displays the
>  *  corresponding tab.
>  *
>  *  Otherwise, instantiates a new instance of its type using the
>  *  default constructor and displays that instance with the given
>  *  title, storing the new instance in the given variable.
>  */
> void create_or_display<TabType>(TabType &output, tab_widget *tabs, const char *name)
> {
>  if(output == NULL)
>  {
>    connect_tab_widget_signals(tabs);
>
>    output = new TabType;
>    tabs->addTab(output, name);
>  }
>
>  activate_tab(output);
> }
>

OK.

>> +      void tabs_manager::tabs_manager_impl::open_package_info_tab(QWidget *tab_context)
>> +      {
>> +     main_window *window = qobject_cast<main_window *>(tab_context->window());
>> +     if(window == NULL)
>> +       return;
>> +      }
>
>  Presumably this is being implemented in another patch?
>

Yes. It is implemented in 290af4033b7

>> +      void tabs_manager::tabs_manager_impl::tab_deletion_requested(tab *tab_to_delete, bool force)
>> +      {
>> +     // when for example performing changes tab is closed, we do not destroy it but only hide
>> +     if(tab_to_delete->get_policy() == tab::closing_policy_hide)
>> +     {
>> +       if(force)
>
>  This logic is a little hairy.
>
>  I almost would prefer a different signal/slot pair in this case, or
> maybe connecting to the destroyed signal on the tab itself when we
> attach it (in create_or_display<>).
>

OK. I said that this code is tricky ;) I thought about his and decided
that  it would be better to avoid series of objects cast in order to
decide which variable should be cleared. Should I create new method
for each tab_type (it is possible that there will be more types in the
future)

>> +       else
>> +         tab_to_delete->hide();
>
>  I wonder if it would be better to split this into two methods:
> request_destroy(), which decides whether to destroy or hide a tab, and
> tab_destroyed(), which removes backlinks to the tab when it's
> destroyed.
>
>  Also, shouldn't the policy be checked in the !force case?  And isn't
> it irrelevant in the force case (since the tab will be deleted
> anyway)?  Or am I missing a subtlety of the logic?
>
>  Seems to me that the logic should be:
>
>  if(force || tab->get_policy() == closing_policy_destroy)
>  {
>    destroy_tab(); // Maybe only if not forced?
>    tab_destroyed(tab); // Should be invoked by the destroyed signal?
>  }
>  else
>    tab->hide();
>

Yes. It is more readable. Logic looks correct. I will change that.

Thanks for the review!

>  Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>

-- 
Regards
Piotr Galiszewski