Re: [PATCH] New Layout Manager and 3 additionals new layouts

[Fabien Parent <parent f gmail com>]

Hello,

To apply this patch, one must apply the path
0001-Remove-unused-status-widgets.patch first, but I pushed it
yesterday so updating its gtk2-branch is enough.

Quick overview of what change since the last patch:

* src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.cc:

 void
 DBGPerspectiveDynamicLayout::do_cleanup_layout ()
 {
-    // avoid a crash - see remove_view () to get more information.
-    for (std::map<int, DockItemPtr>::iterator item = m_priv->views.begin ();
-         item != m_priv->views.end ();
-         ++item) {
-        remove_view (item->first);
-    }
     m_priv.reset ();
 }


The Gdl::DockItem-s are now correctly handled with the SafePtr, we
don't need the remove them manually before destroying the Gdl::Dock to
avoid a crash.

@@ -270,11 +264,10 @@ DBGPerspectiveDynamicLayout::add_view (Gtk::Widget &a_widg
         a_widget.set_size_request (width, height);
     }

-    DockItemPtr item (new Gdl::DockItem (a_title,
-                                         a_title,
-                                         Gdl::DOCK_ITEM_BEH_CANT_CLOSE));
+    DockItemPtr item (Gtk::manage (new Gdl::DockItem
+            (a_title, a_title, Gdl::DOCK_ITEM_BEH_CANT_CLOSE)));
     THROW_IF_FAIL (item);
-
+    item->reference ();
     m_priv->dock->add_item (*item, Gdl::DOCK_BOTTOM);

     // Attach those widgets to the terminal_item.

To let the Gdl::DockItem be desallocated we manage the pointer and
take a reference.

@@ -297,16 +290,7 @@ DBGPerspectiveDynamicLayout::remove_view (int a_index)
         return;
     }

-    // Avoid a crash if we don't remove the child from the Gdl::DockItem
-    // before destroying it.
-    // Could be a bug in the gdl library where the DockItem forgot to unparent
-    // the child widget.
-    std::vector<Gtk::Widget*> children = m_priv->views[a_index]->get_children (
-    for (std::vector<Gtk::Widget*>::iterator child = children.begin ();
-         child != children.end ();
-         ++child) {
-        m_priv->views[a_index]->remove (**child);
-    }
+    m_priv->dock->remove (*m_priv->views[a_index]);
     m_priv->views.erase (a_index);
 }

With a correct Gdl::DockItem allocation there is no need to remove the
children from the Gdl::DockItem.
Remove also the Gdl::DockItem from the dock.

* src/persp/dbgperspective/nmv-dbg-perspective.cc:

>>  void
>> -DBGPerspective::activate_status_view (Gtk::Widget &a_page)
>> +DBGPerspective::on_layout_changed ()
>>  {
>> -    int pagenum = 0;
>>      LOG_FUNCTION_SCOPE_NORMAL_DD;
>>
>> +    NEMIVER_TRY
>> +
>>      THROW_IF_FAIL (m_priv);
>> -    THROW_IF_FAIL (m_priv->statuses_notebook);
>>
>> -    pagenum = m_priv->statuses_notebook->page_num (a_page);
>> -    if (pagenum != -1) {
>> -        if (m_priv->statuses_notebook->get_current_page () != pagenum)
>> -            m_priv->statuses_notebook->set_current_page (pagenum);
>> -        a_page.grab_focus ();
>> -    } else {
>> -        LOG_DD ("Invalid Pagenum");
>> -    }
>> +#ifdef WITH_MEMORYVIEW
>> +    m_priv->layout ().add_view (get_memory_view ().widget (),
>> +                                MEMORY_VIEW_TITLE,
>> +                                MEMORY_VIEW_INDEX);
>> +#endif // WITH_MEMORYVIEW
>> +    m_priv->layout ().add_view (get_registers_scrolled_win (),
>> +                                REGISTERS_VIEW_TITLE,
>> +                                REGISTERS_VIEW_INDEX);
>> +    m_priv->layout ().add_view (get_breakpoints_scrolled_win (),
>> +                                BREAKPOINTS_VIEW_TITLE,
>> +                                BREAKPOINTS_VIEW_INDEX);
>> +    m_priv->layout ().add_view (get_context_paned (),
>> +                                CONTEXT_VIEW_TITLE,
>> +                                CONTEXT_VIEW_INDEX);
>> +    m_priv->layout ().add_view (get_terminal_box (),
>> +                                TARGET_TERMINAL_VIEW_TITLE,
>> +                                TARGET_TERMINAL_VIEW_INDEX);
>> +
>> +    m_priv->layout ().do_init ();
>
> I think it would be appropriate to put this code that adds the views
> to the layout into a function DBGPerspective::add_views_to_layout,
> then call that function here.

Implemented

>> @@ -3863,13 +3779,27 @@ DBGPerspective::init_body ()
>>      get_breakpoints_scrolled_win ().add (get_breakpoints_view ().widget());
>>      get_registers_scrolled_win ().add (get_registers_view ().widget());
>>
>> -    //unparent the body_main_paned, so that we can pack it
>> -    //in the workbench later
>> -    m_priv->top_box->remove (*m_priv->body_main_paned);
>> -    m_priv->body_main_paned->show_all ();
>> +    m_priv->sourceviews_notebook.reset (new Gtk::Notebook);
>> +    m_priv->sourceviews_notebook->remove_page ();
>> +    m_priv->sourceviews_notebook->set_show_tabs ();
>> +    m_priv->sourceviews_notebook->set_scrollable ();
>> +#if GTK_CHECK_VERSION (2, 10, 0)
>> +    m_priv->sourceviews_notebook->signal_page_reordered ().connect
>> +        (sigc::mem_fun (this, &DBGPerspective::on_notebook_tabs_reordered));
>> +#endif
>> +
>> +    UString layout = DBG_PERSPECTIVE_DEFAULT_LAYOUT;
>> +    NEMIVER_TRY
>> +    conf_mgr.get_key_value (CONF_KEY_DBG_PERSPECTIVE_LAYOUT, layout);
>> +
>> +    // If the layout is not registered, we use the default layout
>> +    if (!m_priv->layout_mgr.is_layout_registered (layout)) {
>> +        layout = DBG_PERSPECTIVE_DEFAULT_LAYOUT;
>> +    }
>> +    NEMIVER_CATCH_NOX
>>
>> -    //must be last
>> -    init_perspective_menu_entries ();
>> +    m_priv->layout_mgr.load_layout (layout, *this);
>> +    on_layout_changed ();
>
> Instead of calling on_layout_changed here, it would make more sense
> (from a maintainability POV) to call the function add_views_to_layout
> I talked about earlier.  This would make what we are doing more
> self-explanatory.

Implemented

@@ -5056,11 +5064,15 @@ DBGPerspective::register_layouts ()
 {
     THROW_IF_FAIL (m_priv);

-    m_priv->layout_mgr.register_layout (new DBGPerspectiveDefaultLayout);
-    m_priv->layout_mgr.register_layout (new DBGPerspectiveTwoPaneLayout);
-    m_priv->layout_mgr.register_layout (new DBGPerspectiveWideLayout);
+    m_priv->layout_mgr.register_layout
+        (LayoutSafePtr (new DBGPerspectiveDefaultLayout));
+    m_priv->layout_mgr.register_layout
+        (LayoutSafePtr (new DBGPerspectiveTwoPaneLayout));
+    m_priv->layout_mgr.register_layout
+        (LayoutSafePtr (new DBGPerspectiveWideLayout));
 #ifdef WITH_DYNAMICLAYOUT
-    m_priv->layout_mgr.register_layout (new DBGPerspectiveDynamicLayout);
+    m_priv->layout_mgr.register_layout
+        (LayoutSafePtr (new DBGPerspectiveDynamicLayout));
 #endif // WITH_DYNAMICLAYOUT
 }

We are adding the new layout pointer to a LayoutSafePtr. Previously we
were creating the LayoutSafePtr directly in the register_layout method
but it wasn't safe against exception throwed in the beginning of the
register_layout. If an exception happens before the pointer is put
inside a LayoutSafePtr we would have lost the reference and the memory
would have been lost.
I could have just move the code that encapsulate the pointer into a
LayoutSafePtr to the head of the method, but if an exception was
throwed, the pointer would have been freed and the register_layouts
would have dangling pointer (even if right now it's not a problem
since the allocation is directly a parameter of layout_mgr.
With a LayoutSafePtr directly in the register_layouts method, we are
completely safe.

* src/uicommon/nmv-layout-manager.cc
void
-LayoutManager::register_layout (Layout *a_layout)
+LayoutManager::register_layout (const LayoutSafePtr &a_layout)
 {
     THROW_IF_FAIL (m_priv);
     THROW_IF_FAIL (a_layout);
@@ -77,7 +74,7 @@ LayoutManager::register_layout (Layout *a_layout)
     UString layout_identifier = a_layout->identifier ();
     THROW_IF_FAIL (!m_priv->layouts_map.count (layout_identifier));

-    m_priv->layouts_map[layout_identifier] = LayoutSafePtr (a_layout);
+    m_priv->layouts_map[layout_identifier] =  a_layout;
 }

Takes a LayoutSafePtr instead of a Layout*. (see above)

* src/workbench/nmv-workbench.cc
 void
+Workbench::disconnect_all_perspective_signals ()
+{
+    LOG_FUNCTION_SCOPE_NORMAL_DD;
+
+    list<IPerspectiveSafePtr>::iterator it;
+    for (it = m_priv->perspectives.begin ();
+         it != m_priv->perspectives.end ();
+         ++it) {
+        (*it)->layout_changed_signal ().clear ();
+    }
+}

A reference of the IPerspectiveSafePtr is hold by the layout manager
of the DBGPerspective. So when the workbench is destroyed the
DBGPerspective is not freed. By clearing the signal
layout_changed_signal () we drop the reference and the DBGPerspective
can be freed at exit.

 Workbench::~Workbench ()
 {
     remove_all_perspective_bodies ();
+    disconnect_all_perspective_signals ();
     LOG_D ("delete", "destructor-domain");
 }

Call the method described above.

Fabien