Re: [PATCH] Multiple perspectives support

[Dodji Seketeli <dodji seketeli org>]

Hello Fabien,

I apologize sorry for taking so much time to review this.

I know we discussed this already, but I think it's worth saying that I
like the general idea of the patch.  :-)

I have a few comments to make about details here and there, though.

> diff --git a/src/main.cc b/src/main.cc
> index 2ce9320..e55481e 100644
> --- a/src/main.cc
> +++ b/src/main.cc

[...]


> +static gchar *gv_tool = (gchar*) "dbgperspective";

As the tool name is going to be typed by the user on the command line
(sometimes at least) maybe a name like "debugger" would be more
intuitive?

[...]


> +        &gv_tool,
> +        _("Use the nemiver tool named <name>"),
> +        "<name>"
>      },

Or add a new command line options --list-tools to list the name of the
tools and what they do.  And change the "<name>" string to something
like "<name>; use --list-tools to get the possibilities".


> @@ -275,6 +159,18 @@ init_option_context ()

Earlier in this function, there is this code:

    #if GLIB_CHECK_VERSION (2, 12, 0)
	g_option_context_set_summary (context.get (),
				      _("A C/C++ debugger for GNOME"));
    #endif

I am wondering if we shouldn't change this context summary to
something that is more generic, once we have more than one
perspective.  Of course, this is not for this patch.  I am just
mentioning it here, to see what you think about it.

[...]

> @@ -345,51 +241,21 @@ parse_command_line (int& a_argc,
>          return false;
>      }
>  
> -    if (a_argv != inf_argv) {
> -        memmove (a_argv, inf_argv, inf_argc * sizeof (char*));
> -        a_argc = inf_argc;
> +    IPerspectiveSafePtr perspective = s_workbench->get_perspective (gv_tool);
> +    if (!perspective) {
> +        std::cerr << "Invalid tool name: " << gv_tool << std::endl;
> +        return false;
>      }

Following my earlier comment about having the user-visible tool name
be "debugger" rather than the internal "dbgperspective" that makes
just little sense for normal users,  I am thinking that maybe we
should just change the "dbgperspective" name to "debugger"?  It
follows that things like dbgperspective.conf.in name would be changed
to debugger.conf.in too.  But we'd keep the name of the
IDBGPerspective type intact, as well as the library name of the
plugin, which would stay libdbgperspectiveplugin.so.

What do you think about that?


> +    s_workbench->load_perspectives ();
> +
> +    if (!parse_command_line (a_argc, a_argv))
> +        return -1;
> +
> +    if (!process_non_gui_options ()) {
> +        return -1;
> +    }
>  
>      s_workbench->do_init (gtk_kit);

I have an issue with this.  Please let me re-state the spirit of the
::do_init function for dynmod interfaces.  It's intent is to be the
sole function the user -- who has loaded the object that implements
the interface -- need to call to initialize it and be able to use it.

In other words, IWorkbench::do_init should be the only function you
need to call to have the workbench object be fully functional and
usable.

Maybe I should update the comments of the function to make this
clearer.  And we badly need a hackers' documentation for this project.
Sigh.  So many things to do.

So I think that parse_command_line should be moved into the workbench,
that process_non_gui_options should be moved there and called by
parse_command_line, that process_gui_options should get the same
treatment, and that IWorkbench::do_init should be passed argc,argv,
along gtk_kit, and that it should call load_perspective and
parse_command_line.

That way, main would just have to do workbench->do_init (gtk_kit,
a_argc, a_argv), and would even get rid of the s_workbench global
variable.

[...]

> --- a/src/persp/dbgperspective/nmv-dbg-perspective.cc

[...]


>  static void
> @@ -488,6 +620,14 @@ public:
>  
>      virtual ~DBGPerspective ();

[...]

> +    const UString& name () const;

I'd prefer that we don't multiply the names of the perspectives.
Following my comment regarding names earlier, we could just change the
"dbgperspective" name into "debugger".  That "debugger" name would
then be available from IPerspective::descriptor ()->name (). Note that
descriptor () is a method of the EntryPoint class, which the
IPerspective class derives from.

So I believe the name method is not necessary here.

[...]

> @@ -3014,7 +3163,8 @@ DBGPerspective::add_perspective_menu_entries ()
>      relative_path = Glib::build_filename ("menus", "memoryview-menu.xml");
>      THROW_IF_FAIL (build_absolute_resource_path
>                      (Glib::filename_to_utf8 (relative_path), absolute_path));
> -    workbench ().get_ui_manager ()->add_ui_from_file
> +    m_priv->memoryview_merge_id =
> +        workbench ().get_ui_manager ()->add_ui_from_file
>                                  (Glib::filename_to_utf8 (absolute_path));

> -void
> +std::list<Gtk::UIManager::ui_merge_id>
>  DBGPerspective::edit_workbench_menu ()
>  {
>      CHECK_P_INIT;
>  
>      add_perspective_menu_entries ();
> +    std::list<Gtk::UIManager::ui_merge_id> merge_ids;
> +    merge_ids.push_back (m_priv->menubar_merge_id);
> +    merge_ids.push_back (m_priv->memoryview_merge_id);

Why pushing memoryview_merge_id only?  Why not the other merge_ids for
the other numerous menus that got merged in the ui manager by
add_perspective_menu_entries?

> +    return merge_ids;

I don't really like the fact that we return a list by copy here.  I'd
rather have edit_workbench_menu return a reference to a vector that is
kept by the perspective.  That looks more efficient to me.

>  }

> +GOptionGroup*
> +DBGPerspective::option_group () const
> +{
> +    THROW_IF_FAIL (m_priv);
> +    return m_priv->option_group;
> +}
> +

Please add a comment to this new function.

> +const UString&
> +DBGPerspective::name () const
> +{
> +    static const UString s_name (_("Debugger"));
> +    return s_name;
> +}
> +

As I said earlier, I believe this is not necessary.

> +bool
> +DBGPerspective::process_options (GOptionContext *a_context,
> +                                 int a_argc,
> +                                 char **a_argv)

Please add a comment to this new function.

[...]

> +bool
> +DBGPerspective::process_gui_options (int a_argc, char **a_argv)
> +{

Likewise, please a comment to this new function.

[...]

> --- a/src/persp/dbgperspective/nmv-dbg-perspective.h
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.h
> @@ -79,7 +79,7 @@ public:
>  
>      virtual IWorkbench& get_workbench () = 0;
>  
> -    virtual void edit_workbench_menu () = 0;
> +    virtual std::list<Gtk::UIManager::ui_merge_id> edit_workbench_menu () = 0;

Why does edit_workbench_menu need to return several ui merge ids?

>      virtual void open_file () = 0;
>  
> @@ -186,6 +186,8 @@ public:
>  
>      virtual sigc::signal<void>& layout_changed_signal () = 0;
>  
> +    virtual const UString& name () const = 0;
> +

Unnecessary.

[...]

> +++ b/src/persp/nmv-i-perspective.h
> @@ -65,6 +65,16 @@ protected:

[...]

> +    virtual const UString& name () const = 0;

Likewise.

[...]

> +++ b/src/workbench/nmv-i-workbench.h
> @@ -69,6 +69,7 @@ using nemiver::common::UString;
>  
>  class IWorkbench;
>  typedef SafePtr<IWorkbench, ObjectRef, ObjectUnref> IWorkbenchSafePtr;
> +typedef SafePtr<IPerspective, ObjectRef, ObjectUnref> IPerspectiveSafePtr;
>  

> +    virtual std::list<IPerspectiveSafePtr> perspectives () const = 0;

If possible, I'd prefer this function to be a reference to a vector that
is kept by the workbench.  That would be more efficient than returning
a list by copy like this.

[...]


> +    virtual void load_perspectives () = 0;

This should not be a public entry point like this.  Rather, it should
be just called by the do_init method of this class, as I said in a
comment earlier.

[...]

> +++ b/src/workbench/nmv-workbench.cc
> @@ -86,6 +86,7 @@ private:

[...]

> +    std::list<IPerspectiveSafePtr> perspectives () const;

I think it would be more efficient that this function returns a
reference to a vector own by the workbench.

[...]


> +void
> +Workbench::on_perspective_changed ()
> +{

Please provide a comment to this new function.  I know the other
functions of the file are not necessarily commented, but if we start
doing that for the new functions, maybe we'll get a better
comment/number of LOC ratio ;-)

> +    NEMIVER_TRY;
> +
> +    THROW_IF_FAIL (m_priv);
> +    THROW_IF_FAIL (m_priv->persp_selector_combobox);
> +
> +    UString name = m_priv->persp_selector_combobox->get_active_text ();
> +    std::list<IPerspectiveSafePtr>::iterator iter;
> +    for (iter = m_priv->perspectives.begin ();
> +         iter != m_priv->perspectives.end ();
> +         ++iter) {
> +        if ((*iter)->name () == name) {

I think this if condition could be:

           if ((*iter)->descriptor ()->name == name) {

For this to work, as I said earlier, the text node in the 'name'
element in the descriptor should be changed from dbgperspective to something like
"debugger", in src/persp/dbgperspective/dbgperspective.conf.in.

> +            select_perspective (*iter);
> +            return;
> +        }
> +    }
> +
> +    NEMIVER_CATCH;
> +}
> +

[...]

Thank you for your hard work.


-- 
		Dodji