Re: [PATCH] New Layout Manager and 3 additionals new layouts
- From: Dodji Seketeli <dodji seketeli org>
- To: The mailing list of the Nemiver project <nemiver-list gnome org>
- Subject: Re: [PATCH] New Layout Manager and 3 additionals new layouts
- Date: Mon, 18 Jul 2011 23:16:33 +0200
First of all, thank you for this patch. This is great work. I see
that it has been crafted with great care. You tried hard to follow
the coding conventions of the project as well as the general design
ideas that we discussed previously.
As usual, I have made a few observations that I'd like to share to
with you. So there we go.
[...]
> * configure.ac: Add directives to enable/disable the building of the
> dynamic layout.
> * data/schemas/gconf/nemiver-dbgperspective.schemas: Add new configuration
> keys for the new layouts.
> * data/schemas/gsettings/org.nemiver.gschema.xml: Likewise
> * src/confmgr/nmv-conf-keys.h: Likewise
> * src/confmgr/nmv-gconf-keys-defs.cc: Likewise
> * src/confmgr/nmv-gsettings-keys-defs.c: Likewise
> * src/persp/dbgperspective/Makefile.am: Add new source files
> * src/persp/dbgperspective/nmv-dbg-perspective-default-layout.cc: Default
> DBGPerspective Layout
> * src/persp/dbgperspective/nmv-dbg-perspective-default-layout.h: Likewise
> * src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.cc: A dynamic
> layout using gdlmm.
> * src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.h: Likewise
> * src/persp/dbgperspective/nmv-dbg-perspective-two-pane-layout.cc: A layout
> using two pane: a bottom one with the breakpoints and the context widgets
> and a right one with the register, terminal and memory widgets.
> * src/persp/dbgperspective/nmv-dbg-perspective-two-pane-layout.h: Likewise
> * src/persp/dbgperspective/nmv-dbg-perspective-wide-layout.cc: A Layout
> for very large screen where all status widget are on a vertical (right) pane.
> * src/persp/dbgperspective/nmv-dbg-perspective-wide-layout.h: Likewise.
> * src/persp/dbgperspective/nmv-dbg-perspective.cc: Adapt to the API Changes.
You could maybe describe the changes to nmv-dbg-perspective.cc a bit
more: list the new functions, mention the names of the changed
functions and describe what changed in them, say which function got
moved where, etc. This might looks tedious and redundant, but it
eases the review and can greatly help for code analysis that can
happen later.
> * src/persp/dbgperspective/nmv-dbg-perspective.h: New APIs to get status
> view widgets, IDs, titles, get the configuration manager and a signal to
> signal when the layout changed.
> * src/persp/dbgperspective/nmv-preferences-dialog.cc
> (PreferencesDialog::Priv::Priv): Take the Perspective and the LayoutManager
> as parameters instead of a Workbench.
> (PreferencesDialog::Priv::init): Add the LayoutSelector widget to the
> preferences dialog.
> (PreferencesDialog::Priv::conf_manager): Use the IPerspective to get the
> configuration manager instead of the workbench.
> (PreferencesDialog::PreferencesDialog): Take the Perspective and
> the LayoutManager as parameters instead of a Workbench.
> * src/persp/dbgperspective/nmv-preferences-dialog.h
> (PreferencesDialog::PreferencesDialog): Take the Perspective and
> the LayoutManager as parameters instead of a Workbench.
> * src/persp/dbgperspective/ui/Makefile.am: Remove bodycontainer.ui file
> * src/persp/dbgperspective/ui/bodycontainer.ui: Removed
> * src/persp/dbgperspective/ui/preferencesdialog.ui: Add Layout tab
> * src/persp/nmv-i-perspective.h: Add a signal when the layout (body) changed.
> * src/uicommon/Makefile.am: Add new source files
> * src/uicommon/nmv-layout-manager.cc: API to load/unload layouts
> * src/uicommon/nmv-layout-manager.h: Likewise
> * src/uicommon/nmv-layout-selector.cc: Widget to select a layout from
> a layout vector.
> * src/uicommon/nmv-layout-selector.h: Likewise
> * src/uicommon/nmv-layout.h: Interface for implementing a layout manageable
> by the LayoutManager.
> * src/workbench/nmv-workbench.cc
> (Workbench::on_perspective_body_changed_signal): Reload the body of the
> perspective page when the layout has changed.
This is a new function. Here in the ChangeLog, you should just say
that it's a new function. What it does should be put as a comment of
the function itself.
> (Workbench::do_init): Attach the signal 'body_changed_signal' when
> the layout changed to the Workbench::on_perspective_body_changed_signal
> method.
[...]
> index b5fdc2e..865c236 100644
> --- a/data/schemas/gconf/nemiver-dbgperspective.schemas
> +++ b/data/schemas/gconf/nemiver-dbgperspective.schemas
[...]
> @@ -2,6 +2,17 @@
> <gconfschemafile>
> <schemalist>
> <schema>
> + <key>/schemas/apps/nemiver/dbgperspective/layout</key>
> + <applyto>/apps/nemiver/dbgperspective/layout</applyto>
> + <owner>nemiver</owner>
> + <type>string</type>
> + <default>default-layout</default>
> + <locale name="C">
> + <short>Layout of the perspective</short>
> + <long>The layout that will be loaded by the debugging perspective.</long>
I believe the non-written custom is to /not/ put any dot at the end of
sentences in gconf schemas. So the dot at after perspective should be
removed, IMO. I realize that other keys' descriptive sentences might
have dots at their end, but I think it's a mistake.
Also, I'd rather have the text be "The layout that is loaded by the
debugging perspective". That is, use the present tense rather than
the future.
> diff --git a/data/schemas/gsettings/org.nemiver.gschema.xml b/data/schemas/gsettings/org.nemiver.gschema.xml
> index 836da13..4d5afb8 100644
> --- a/data/schemas/gsettings/org.nemiver.gschema.xml
> +++ b/data/schemas/gsettings/org.nemiver.gschema.xml
[...]
> @@ -1,5 +1,11 @@
> <schemalist>
> <schema id="org.nemiver" path="/apps/nemiver/">
> + <key name="dbg-perspective-layout" type="s">
> + <default>'default-layout'</default>
> + <summary>Layout of the perspective</summary>
> + <description>The layout that will be loaded by the debugging perspective.</description>
> + </key>
> +
Likewise.
> <key name="source-search-dirs" type="s">
> <default>'.'</default>
> <summary>source directory dirs</summary>
> @@ -72,10 +78,28 @@
> <description>The minimum height of the status widget.</description>
> </key>
>
> - <key name="status-pane-location" type="i">
> + <key name="default-layout-pane-location" type="i">
> + <default>-1</default>
> + <summary>The location of the status pane for the default layout</summary>
> + <description>The location of the status pane for the default layout.</description>
> + </key>
Please remove the dot at the end of this sentence as well.
> +
> + <key name="wide-layout-pane-location" type="i">
> + <default>-1</default>
> + <summary>The location of the status pane for the wide layout</summary>
> + <description>The location of the status pane for the wide layout.</description>
> + </key>
Likewise.
> +
> + <key name="two-pane-layout-vpane-location" type="i">
> + <default>-1</default>
> + <summary>The location of the vertical status pane for the two-pane layout</summary>
> + <description>The location of the vertical status pane for the two-pane layout.</description>
> + </key>
Likewise.
> +
> + <key name="two-pane-layout-hpane-location" type="i">
> <default>-1</default>
> - <summary>The location of the status pane</summary>
> - <description>The location of the status pane.</description>
> + <summary>The location of the horizontal status pane for the two-pane layout</summary>
> + <description>The location of the horizontalstatus pane for the two-pane layout.</description>
> </key>
Likewise.
> diff --git a/src/persp/dbgperspective/Makefile.am b/src/persp/dbgperspective/Makefile.am
> index 618f146..7852432 100644
> --- a/src/persp/dbgperspective/Makefile.am
> +++ b/src/persp/dbgperspective/Makefile.am
> @@ -67,7 +67,21 @@ $(h)/nmv-vars-treeview.cc \
> $(h)/nmv-call-function-dialog.h \
> $(h)/nmv-call-function-dialog.cc \
> $(h)/nmv-set-jump-to-dialog.h \
> -$(h)/nmv-set-jump-to-dialog.cc
> +$(h)/nmv-set-jump-to-dialog.cc \
> +$(h)/nmv-dbg-perspective-default-layout.cc \
> +$(h)/nmv-dbg-perspective-default-layout.h \
> +$(h)/nmv-dbg-perspective-two-pane-layout.cc \
> +$(h)/nmv-dbg-perspective-two-pane-layout.h \
> +$(h)/nmv-dbg-perspective-wide-layout.cc \
> +$(h)/nmv-dbg-perspective-wide-layout.h
> +
> +if BUILD_DYNAMICLAYOUT
> +dynamiclayout_sources = \
> +$(h)/nmv-dbg-perspective-dynamic-layout.cc \
> +$(h)/nmv-dbg-perspective-dynamic-layout.h
> +else
> +dynamiclayout_sources =
> +endif
>
> if BUILD_MEMORYVIEW
> memoryview_sources = \
> @@ -77,7 +91,8 @@ else
> memoryview_sources =
> endif
>
> -libdbgperspectiveplugin_la_SOURCES=$(sources) $(memoryview_sources)
> +libdbgperspectiveplugin_la_SOURCES=$(sources) $(memoryview_sources) \
> +$(dynamiclayout_sources)
> libdbgperspectiveplugin_la_LDFLAGS= -module -avoid-version -Wl,--as-needed
> libdbgperspectiveplugin_la_LIBADD= \
> @NEMIVERDBGPERSP_LIBS@ \
> @@ -93,4 +108,3 @@ INCLUDES=@NEMIVERDBGPERSP_CFLAGS@ -DENABLE_NLS=1 -DDATADIR=\"${datadir}\" \
> -I$(abs_top_srcdir)/src/workbench \
> -I$(abs_top_srcdir)/src/persp \
> -I$(abs_top_srcdir)/src/dbgperspective
The removal of this -I options from the INCLUDE variable is not
mentioned in the ChangeLog.
> -
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective-default-layout.cc b/src/persp/dbgperspective/nmv-dbg-perspective-default-layout.cc
> new file mode 100644
> index 0000000..b648968
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective-default-layout.cc
[...]
Maybe you could add a comment here to describe what makes this layout
specific. Something to present how the layout looks like. You don't
need to go as far as an ASCII art thing.
> +struct DBGPerspectiveDefaultLayout::Priv {
> + SafePtr<Gtk::Paned> body_main_paned;
> + SafePtr<Gtk::Notebook> statuses_notebook;
> +
> + bool command_view_is_visible;
> + bool target_output_view_is_visible;
> + bool log_view_is_visible;
> + bool context_paned_view_is_visible;
> + bool terminal_view_is_visible;
> + bool breakpoints_view_is_visible;
> + bool registers_view_is_visible;
> +#ifdef WITH_MEMORYVIEW
> + bool memory_view_is_visible;
> +#endif // WITH_MEMORYVIEW
> + IDBGPerspective *dbg_perspective;
As I said in a comment later in nmv-layout.h, I would rather see this
dbg_perspective be a reference, rather than a pointer. This would
remove one possibility of having a null pointer argument passed here.
> +
> + Priv () :
In that case, this constructor should have an IDBGPerspective& (like
in Priv (IDBGPerspective &a_persp)
parameter here. The code that instantiates
DBGPerspectiveDefaultLayout::Priv would then pass it the reference to
IDBGPerspective.
> + command_view_is_visible (false),
> + target_output_view_is_visible (false),
> + log_view_is_visible (false),
> + context_paned_view_is_visible (false),
> + terminal_view_is_visible (false),
> + breakpoints_view_is_visible (false),
> + registers_view_is_visible (false),
> +#ifdef WITH_MEMORYVIEW
> + memory_view_is_visible (false),
> +#endif // WITH_MEMORYVIEW
> + dbg_perspective (0)
In that case, this would become dbg_perspective (a_persp).
> + {
> + }
> +
> + void
> + set_show_command_view (bool a_show)
> + {
> + Gtk::Widget &command_view =
> + dbg_perspective->get_command_view_widget ();
> + if (a_show) {
> + if (!command_view.get_parent ()
> + && command_view_is_visible == false) {
> + command_view.show_all ();
> + int pagenum =
> + statuses_notebook->insert_page
> + (command_view,
> + COMMAND_VIEW_TITLE,
> + COMMAND_VIEW_INDEX);
> + statuses_notebook->set_current_page (pagenum);
> + command_view_is_visible = true;
> + }
> + } else {
> + if (command_view.get_parent ()
> + && command_view_is_visible) {
> + statuses_notebook->remove_page (command_view);
> + command_view_is_visible = false;
> + }
> + }
> + }
If I understand correctly, this function, got moved here from
DBGPerspective::set_show_command_view. But I see that it doesn't emit
any equivalent of the signal DBGPerspective::show_command_view_signal
() that the later function was emitting. I understand that signal is
now obsolete and should be removed, along with its signal handler.
But this patch doesn't seem to remove those. Am I missing something?
> +
> + void
> + set_show_target_output_view (bool a_show)
> + {
> + Gtk::Widget &target_output_view =
> + dbg_perspective->get_target_output_view_widget ();
> + if (a_show) {
> + if (!target_output_view.get_parent ()
> + && target_output_view_is_visible == false) {
> + target_output_view.show_all ();
> + int page_num =
> + statuses_notebook->insert_page
> + (target_output_view,
> + TARGET_OUTPUT_VIEW_TITLE,
> + TARGET_OUTPUT_VIEW_INDEX);
> + target_output_view_is_visible = true;
> + statuses_notebook->set_current_page (page_num);
> + }
> + } else {
> + if (target_output_view.get_parent ()
> + && target_output_view_is_visible) {
> + statuses_notebook->remove_page (target_output_view);
> + target_output_view_is_visible = false;
> + }
> + target_output_view_is_visible = false;
> + }
> + }
Likewise for the removal of the
DBGPerspective::show_target_output_view_signal and associated signal
handler, as well
> +
> + void
> + set_show_log_view (bool a_show)
> + {
> + Gtk::Widget &log_view =
> + dbg_perspective->get_log_view_widget ();
> + if (a_show) {
> + if (!log_view.get_parent ()
> + && log_view_is_visible == false) {
> + log_view.show_all ();
> + int page_num =
> + statuses_notebook->insert_page
> + (log_view, LOGS_VIEW_TITLE, LOGS_VIEW_INDEX);
> + log_view_is_visible = true;
> + statuses_notebook->set_current_page (page_num);
> + }
> + } else {
> + if (log_view.get_parent ()
> + && log_view_is_visible) {
> + LOG_DD ("removing log view");
> + statuses_notebook->remove_page (log_view);
> + }
> + log_view_is_visible = false;
> + }
> + }
Likewise for the removal of the DBGPerspective::show_log_view_signal
and associated signal handler.
Also, I think the Command View, Target Output view and Log View are
now obsolete and should be removed. Would you mind removing these or
do you want me to remove these after your patch? FWIW, I think
removing these as a separate patch /before/ this patch would be the
best course of action.
[...]
> +void
> +DBGPerspectiveDefaultLayout::do_init (IPerspective *a_perspective)
> +{
If Layout::do_init takes a reference to IPerspective, then this
signature would need to be adapted ...
> + m_priv.reset (new Priv);
... and this instantiation would become:
m_priv.reset (new Priv (dynamic_cast<IDBGPerspective&> (a_perspective)));
[...]
> + m_priv->dbg_perspective = dynamic_cast<IDBGPerspective*> (a_perspective);
> + THROW_IF_FAIL (m_priv->dbg_perspective);
And these two lines would become useless.
> +
> + m_priv->body_main_paned.reset (new Gtk::VPaned);
> + m_priv->body_main_paned->set_position (350);
> +
> + // set the position of the status pane to the last saved position
> + IConfMgr &conf_mgr = m_priv->dbg_perspective->get_conf_mgr ();
> + int pane_location = -1; // don't specifically set a location
> + // if we can't read the last location from gconf
> + NEMIVER_TRY
> + conf_mgr.get_key_value (CONF_KEY_DEFAULT_LAYOUT_STATUS_PANE_LOCATION,
> + pane_location);
> + NEMIVER_CATCH_NOX
Here you should use NEMIVER_CATCH. NEMIVER_CATCH_NOX is to be used in
contexts where we don't necessarily have any X server available, e.g,
in regression tests. The NEMIVER_CATCH macro is provided by the
nmv-ui-utils.h header. Note that the NEMIVER_CATCH macro displays a
dialog with the exception message.
> +
> + if (pane_location > 0) {
What if the pane_location is 0? Shouldn't the test be if
(pane_location >= 0) instead? I realize you have just copied this
code, though.
[...]
> + m_priv->set_show_command_view (false);
> + m_priv->set_show_target_output_view (false);
> + m_priv->set_show_log_view (false);
> + m_priv->set_show_terminal_view (true);
> + m_priv->set_show_context_view (true);
> + m_priv->set_show_breakpoints_view (true);
> + m_priv->set_show_registers_view (true);
> +#ifdef WITH_MEMORYVIEW
> + m_priv->set_show_memory_view (true);
> +#endif // WITH_MEMORYVIEW
> + m_priv->statuses_notebook->set_current_page (0);
I'd prefer this to be:
m_priv->statuses_notebook->set_current_page (TERMINAL_VIEW_INDEX);
There are several other instances of this in the patch.
> +}
> +
> +void
> +DBGPerspectiveDefaultLayout::do_uninit ()
> +{
> + m_priv.reset (0);
This is correct. Note however that m_priv.reset (); should do the
same. There are several instances of this in the patch.
> +}
> +
> +const UString&
> +DBGPerspectiveDefaultLayout::identifier () const
> +{
> + static UString s_id = "default-layout";
> + return s_id;
> +}
> +
> +const UString&
> +DBGPerspectiveDefaultLayout::name () const
> +{
> + static UString s_name = _("Default Layout");
Please use static const UString s_name here. (note the lack of
const).
> + return s_name;
> +}
> +
> +const UString&
> +DBGPerspectiveDefaultLayout::description () const
> +{
> + static UString s_description = _("Nemiver's default layout");
Likewise. There are several other similar instances of this in the
patch.
[...]
> +void
> +DBGPerspectiveDefaultLayout::show_view (int a_view, bool a_show)
> +{
> + THROW_IF_FAIL (m_priv);
> +
Here, I would cast a_view back into an enum ViewsIndex, so that the
switch/case below acts on that enum. The benefit would be to let the
compiler help us detect missed cases in the code below when someone
adds a new member to the enum ViewsIndex.
> + switch (a_view) {
[...]
> + default:
> + LOG_ERROR_DD ("Trying to show a widget with a null pointer");
> + break;
If a_view becomes an enum, I would remove the default case here, to
allow the compiler to warn if we miss a case, e.g, when someone adds a
new member to enum ViewsIndex.
> + }
> +}
> +
> +void
> +DBGPerspectiveDefaultLayout::save_configuration ()
> +{
> + THROW_IF_FAIL (m_priv && m_priv->dbg_perspective && m_priv->body_main_paned);
> +
If m_priv->dbg_perspective is a reference (see my relevant comment a
few lines above) , then it becomes meaningless assert that it is
non-null here.
> + // save the location of the status pane so
> + // that it'll open in the same place
> + // next time.
> + IConfMgr &conf_mgr = m_priv->dbg_perspective->get_conf_mgr ();
> + int pane_location = m_priv->body_main_paned->get_position();
> +
> + NEMIVER_TRY
> + conf_mgr.set_key_value (CONF_KEY_DEFAULT_LAYOUT_STATUS_PANE_LOCATION,
> + pane_location);
> + NEMIVER_CATCH_NOX
> +}
> +
> +NEMIVER_END_NAMESPACE (nemiver)
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.cc b/src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.cc
> new file mode 100644
> index 0000000..6107b75
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.cc
[...]
A comment presenting how the layout looks like would be nice.
> +struct DBGPerspectiveDynamicLayout::Priv {
> + SafePtr<Gtk::Box> main_box;
> +
> + SafePtr<Gdl::Dock> dock;
> + SafePtr<Gdl::DockBar> dock_bar;
> + Glib::RefPtr<Gdl::DockLayout> dock_layout;
> +
> + SafePtr<Gdl::DockItem> source_item;
> + SafePtr<Gdl::DockItem> command_item;
> + SafePtr<Gdl::DockItem> target_output_item;
> + SafePtr<Gdl::DockItem> log_item;
As I said earlier, we should remove the command view, target output view and
log view. This would make the three members above disappear.
> + SafePtr<Gdl::DockItem> context_item;
> + SafePtr<Gdl::DockItem> terminal_item;
> + SafePtr<Gdl::DockItem> breakpoints_item;
> + SafePtr<Gdl::DockItem> registers_item;
> + SafePtr<Gdl::DockItem> memory_item;
> +
> + IDBGPerspective *dbg_perspective;
> +
> + Priv () :
> + dbg_perspective (0)
> + {
> + }
> +
> + void
> + iconify_item_if_detached (const SafePtr<Gdl::DockItem>& a_item)
> + {
> + THROW_IF_FAIL (dock);
> +
> + if (!a_item) {
> + LOG_ERROR_DD ("Trying to iconify a widget with a null pointer");
> + return;
> + }
Is it possible to make this function take a reference instead of a
(smart) pointer? This would make the test above disappear.
> +
> + if (!a_item->get_parent_object ()) {
> + dock->add_item (*a_item, Gdl::DOCK_NONE);
> + a_item->iconify_item ();
> + }
> + }
> +
> + Gdl::DockItem*
> + item (int a_item_id) const
> + {
Similar to a comment I made earlier, I would like a_item_id to be
converted into enum ViewsIndex ...
> + switch (a_item_id) {
> + case COMMAND_VIEW_INDEX:
> + return command_item.get ();
> +
> + case CONTEXT_VIEW_INDEX:
> + return context_item.get ();
> +
> + case TARGET_TERMINAL_VIEW_INDEX:
> + return terminal_item.get ();
> +
> + case BREAKPOINTS_VIEW_INDEX:
> + return breakpoints_item.get ();
> +
> + case REGISTERS_VIEW_INDEX:
> + return registers_item.get ();
> +
> +#ifdef WITH_MEMORYVIEW
> + case MEMORY_VIEW_INDEX:
> + return memory_item.get ();
> +#endif // WITH_MEMORYVIEW
> +
> + case TARGET_OUTPUT_VIEW_INDEX:
> + return target_output_item.get ();
> +
> + case LOGS_VIEW_INDEX:
> + return log_item.get ();
> +
> + default:
> + return 0;
... and I would remove the default case here, to let the compiler warn
if we forget to handle a case when someone adds a new member to the
enum ViewsIndex.
[...]
> +void
> +DBGPerspectiveDynamicLayout::do_init (IPerspective *a_perspective)
> +{
> + m_priv.reset (new Priv);
> + THROW_IF_FAIL (m_priv);
> +
> + m_priv->dbg_perspective = dynamic_cast<IDBGPerspective*> (a_perspective);
> + THROW_IF_FAIL (m_priv->dbg_perspective);
> +
> +
> + IConfMgr &conf_mgr = m_priv->dbg_perspective->get_conf_mgr ();
> + int width = 100;
> + int height = 70;
> +
> + NEMIVER_TRY
> + conf_mgr.get_key_value (CONF_KEY_STATUS_WIDGET_MINIMUM_WIDTH, width);
> + conf_mgr.get_key_value (CONF_KEY_STATUS_WIDGET_MINIMUM_HEIGHT, height);
> + NEMIVER_CATCH_NOX
> +
> + LOG_DD ("setting status widget min size: width: "
> + << width
> + << ", height: "
> + << height);
> +
> + // Otherwise the widget takes all the vertical|horizontal place
> + // and we cannot reduce it.
> + m_priv->dbg_perspective->get_terminal_view_widget ()
> + .set_size_request (width, height);
Why not using set_default_size instead of set_size_request? Do you
want to prevent the user from shrinking the terminal view widget?
There are several other similar instances of this in the patch.
> +#ifdef WITH_MEMORYVIEW
> + m_priv->dbg_perspective->get_memory_view_widget ()
> + .set_size_request (width);
> +#endif // WITH_MEMORYVIEW
Also, I don't understand why you need to set_size_request just for the
terminal and memory view widgets and not for the other view widgets.
> +
> + m_priv->source_item.reset (
> + new Gdl::DockItem ("source",
> + _("Source Code"),
> + Gdl::DOCK_ITEM_BEH_CANT_CLOSE |
> + Gdl::DOCK_ITEM_BEH_LOCKED |
> + Gdl::DOCK_ITEM_BEH_CANT_ICONIFY |
> + Gdl::DOCK_ITEM_BEH_NO_GRIP));
Please let the parenthesis after reset be on the next line, near the
'new' keyword.
> + m_priv->command_item.reset
> + (new Gdl::DockItem ("command",
> + COMMAND_VIEW_TITLE,
> + Gdl::DOCK_ITEM_BEH_CANT_CLOSE));
> + m_priv->target_output_item.reset
> + (new Gdl::DockItem ("output",
> + TARGET_OUTPUT_VIEW_TITLE,
> + Gdl::DOCK_ITEM_BEH_CANT_CLOSE));
> + m_priv->log_item.reset
> + (new Gdl::DockItem ("log",
> + LOGS_VIEW_TITLE,
> + Gdl::DOCK_ITEM_BEH_CANT_CLOSE));
As I said earlier, we should just drop these three widgets.
[...]
> + m_priv->target_output_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> + m_priv->log_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> + m_priv->context_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> + m_priv->command_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> + m_priv->breakpoints_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> + m_priv->registers_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> +#ifdef WITH_MEMORYVIEW
> + m_priv->memory_item->dock_to (*m_priv->terminal_item, Gdl::DOCK_CENTER);
> +#endif // WITH_MEMORYVIEW
> +
What does it mean to use the Gdl::DOC_CENTER as a relative position to
the first widget in argument to GdlDockItem::dock_to function? The
Gdl documentation doesn't say. Maybe you could add a comment above
this code paragraph to explain?
> + m_priv->main_box.reset (new Gtk::HBox);
> + m_priv->main_box->pack_start (*m_priv->dock_bar, false, false);
> + m_priv->main_box->pack_end (*m_priv->dock);
> + m_priv->main_box->show_all ();
> +
> + show_view (COMMAND_VIEW_INDEX, false);
> + show_view (TARGET_OUTPUT_VIEW_INDEX, false);
> + show_view (LOGS_VIEW_INDEX, false);
As I said earlier, these three should be nuked.
[...]
> +const UString&
> +DBGPerspectiveDynamicLayout::identifier () const
> +{
> + static UString s_id = "dynamic-layout";
s_id should be const.
> + return s_id;
> +}
> +
> +const UString&
> +DBGPerspectiveDynamicLayout::name () const
> +{
> + static UString s_name = _("Dynamic Layout");
Likewise for s_name.
> + return s_name;
> +}
> +
> +const UString&
> +DBGPerspectiveDynamicLayout::description () const
> +{
> + static UString s_description = _("A layout which can be modified");
Likewise for s_description.
> + return s_description;
> +}
> +
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.h b/src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.h
> new file mode 100644
> index 0000000..9f1ad08
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective-dynamic-layout.h
[...]
> +class NEMIVER_API DBGPerspectiveDynamicLayout : public Layout {
> + //non copyable
> + DBGPerspectiveDynamicLayout (const Layout&);
> + DBGPerspectiveDynamicLayout& operator= (const Layout&);
> +
> + struct Priv;
> + SafePtr<Priv> m_priv;
> +
> +public:
> + DBGPerspectiveDynamicLayout ();
> +
> + void activate_view (int);
> +
> + void show_view (int, bool);
> +
> + Gtk::Widget* widget () const;
> +
> + void do_init (IPerspective *);
The parameter should be IPerspective&.
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective-two-pane-layout.cc b/src/persp/dbgperspective/nmv-dbg-perspective-two-pane-layout.cc
> new file mode 100644
> index 0000000..8125eb2
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective-two-pane-layout.cc
[...]
A comment presenting how this layout looks like would be nice here.
> +struct DBGPerspectiveTwoPaneLayout::Priv {
> + SafePtr<Gtk::Paned> vertical_paned;
> + SafePtr<Gtk::Paned> horizontal_paned;
> + SafePtr<Gtk::Notebook> horizontal_statuses_notebook;
> + SafePtr<Gtk::Notebook> vertical_statuses_notebook;
> +
> + bool command_view_is_visible;
> + bool target_output_view_is_visible;
> + bool log_view_is_visible;
The three widgets controlled by these booleans should be dropped.
> + bool context_paned_view_is_visible;
> + bool terminal_view_is_visible;
> + bool breakpoints_view_is_visible;
> + bool registers_view_is_visible;
> +#ifdef WITH_MEMORYVIEW
> + bool memory_view_is_visible;
> +#endif // WITH_MEMORYVIEW
> + IDBGPerspective *dbg_perspective;
This should be a reference, rather than a pointer ...
> +
> + Priv () :
And this constructor should take a reference to an IDBGPerspective ...
> + command_view_is_visible (false),
> + target_output_view_is_visible (false),
> + log_view_is_visible (false),
> + context_paned_view_is_visible (false),
> + terminal_view_is_visible (false),
> + breakpoints_view_is_visible (false),
> + registers_view_is_visible (false),
> +#ifdef WITH_MEMORYVIEW
> + memory_view_is_visible (false),
> +#endif // WITH_MEMORYVIEW
> + dbg_perspective (0)
... so that this member would be initialized with it here.
> + {
> + }
> + void
> + set_show_command_view (bool a_show)
> + {
[...]
> + }
> +
[...]
> + void
> + set_show_target_output_view (bool a_show)
> + {
[...]
> + }
> +
> + void
> + set_show_log_view (bool a_show)
> + {
[...]
> + }
These three functions should be nuked, as I said earlier.
[...]
> +void
> +DBGPerspectiveTwoPaneLayout::do_init (IPerspective *a_perspective)
> +{
This should take a reference, rather than a pointer.
> + m_priv->horizontal_statuses_notebook->set_current_page (0);
> + m_priv->vertical_statuses_notebook->set_current_page (0);
I'd prefer using the TARGET_TERMINAL_VIEW_INDEX enum value, rather
than '0'.
[...]
> +const UString&
> +DBGPerspectiveTwoPaneLayout::identifier () const
> +{
> + static UString s_id = "two-pane-layout";
This string should be const.
[...]
> +const UString&
> +DBGPerspectiveTwoPaneLayout::name () const
> +{
> + static UString s_name = _("Two Status Pane");
Likewise.
> + return s_name;
> +}
> +
> +const UString&
> +DBGPerspectiveTwoPaneLayout::description () const
> +{
> + static UString s_description = _("A layout with 2 status pane");
Likewise.
> + return s_description;
> +}
> +void
> +DBGPerspectiveTwoPaneLayout::activate_view (int a_view)
> +{
> + LOG_FUNCTION_SCOPE_NORMAL_DD;
> +
> + THROW_IF_FAIL (m_priv && m_priv->dbg_perspective);
> + THROW_IF_FAIL (m_priv->horizontal_statuses_notebook);
> + THROW_IF_FAIL (m_priv->vertical_statuses_notebook);
> +
> + Gtk::Notebook *status_notebook = 0;
> + Gtk::Widget& (IDBGPerspective::*widget) () = 0;
Like I said earlier, a_view should be casted into enum ViewIndex ...
> + switch (a_view) {
> + case COMMAND_VIEW_INDEX:
> + status_notebook = m_priv->horizontal_statuses_notebook.get ();
> + widget = &IDBGPerspective::get_command_view_widget;
> + break;
[...]
> +
> + default:
> + LOG_ERROR_DD ("Trying to activate an unknown status view");
> + break;
And the default case should be removed to let the compiler catch cases
that we might have forgotten.
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective-wide-layout.cc b/src/persp/dbgperspective/nmv-dbg-perspective-wide-layout.cc
> new file mode 100644
> index 0000000..cf813eb
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective-wide-layout.cc
[...]
It would be nice to write a comment here to present what the layout
looks like.
> +
> +struct DBGPerspectiveWideLayout::Priv {
> + SafePtr<Gtk::Paned> body_main_paned;
> + SafePtr<Gtk::Notebook> statuses_notebook;
> +
> + bool command_view_is_visible;
> + bool target_output_view_is_visible;
> + bool log_view_is_visible;
These three views should be nuked.
> + bool context_paned_view_is_visible;
> + bool terminal_view_is_visible;
> + bool breakpoints_view_is_visible;
> + bool registers_view_is_visible;
> +#ifdef WITH_MEMORYVIEW
> + bool memory_view_is_visible;
> +#endif // WITH_MEMORYVIEW
> + IDBGPerspective *dbg_perspective;
This one should be a reference ...
> +
> + Priv () :
... and this constructor should take a reference to a perspective in
parameter ...
> + command_view_is_visible (false),
> + target_output_view_is_visible (false),
> + log_view_is_visible (false),
> + context_paned_view_is_visible (false),
> + terminal_view_is_visible (false),
> + breakpoints_view_is_visible (false),
> + registers_view_is_visible (false),
> +#ifdef WITH_MEMORYVIEW
> + memory_view_is_visible (false),
> +#endif // WITH_MEMORYVIEW
> + dbg_perspective (0)
... to initialize this reference.
> + {
> + }
> +
> + void
> + set_show_command_view (bool a_show)
> + {
[...]
> + }
> +
> + void
> + set_show_target_output_view (bool a_show)
> + {
[...]
> + }
> +
> + void
> + set_show_log_view (bool a_show)
> + {
[...]
> +const UString&
> +DBGPerspectiveWideLayout::identifier () const
> +{
> + static UString s_id = "wide-layout";
This string should take a const.
> + return s_id;
> +}
> +
> +const UString&
> +DBGPerspectiveWideLayout::name () const
> +{
> + static UString s_name = _("Wide Layout");
Likewise.
> + return s_name;
> +}
> +
> +const UString&
> +DBGPerspectiveWideLayout::description () const
> +{
> + static UString s_description = _("A layout for very large monitors");
Likewise.
> + return s_description;
> +}
> +
[...]
> +void
> +DBGPerspectiveWideLayout::show_view (int a_view, bool a_show)
> +{
> + THROW_IF_FAIL (m_priv);
a_view should be casted into enum ViewIndex ...
> + switch (a_view) {
[...]
> + default:
> + LOG_ERROR_DD ("Trying to show a widget with a null pointer");
> + break;
... and this default case should be removed.
> + }
> +}
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective.cc b/src/persp/dbgperspective/nmv-dbg-perspective.cc
> index 7f12edc..d3b24ca 100644
> --- a/src/persp/dbgperspective/nmv-dbg-perspective.cc
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc
[...]
> @@ -487,6 +497,26 @@ public:
>
> Gtk::Widget* get_body ();
>
> + Gtk::Widget& get_command_view_widget ();
> +
> + Gtk::Widget& get_target_output_view_widget ();
> +
> + Gtk::Widget& get_log_view_widget ();
I believe we can just remove these as this three views are not used
anymore.
[...]
> + sigc::signal<void>& body_changed_signal ();
I'd prefer calling this layout_changed_signal (), as it returns the
layout_changed_signal of the layout manager. I'd find that less
confusing in this context.
[...]
> @@ -1774,7 +1751,8 @@ DBGPerspective::on_show_commands_action ()
> ("/MenuBar/MenuBarAdditions/ViewMenu/ShowCommandsMenuItem"));
> THROW_IF_FAIL (action);
>
> - set_show_command_view (action->get_active ());
> + THROW_IF_FAIL (m_priv);
> + m_priv->layout ().show_view (COMMAND_VIEW_INDEX, action->get_active ());
For this function in particular and all the others that got adapted to
the new API, it would be nice to mention the names of the functions
that got modified in the ChangeLog to say explicitly that they got
adapted to the use the layout(manager) based API. This gives the
reviewer (or anybody doing code archaeology a couple of years later)
quick and broad idea about the impact of the change in this key class.
[...]
> void
> -DBGPerspective::init_perspective_menu_entries ()
> -{
> - set_show_command_view (false);
> - set_show_target_output_view (false);
> - set_show_log_view (false);
> - set_show_terminal_view (true);
> - set_show_context_view (true);
> - set_show_breakpoints_view (true);
> - set_show_registers_view (true);
> -#ifdef WITH_MEMORYVIEW
> - set_show_memory_view (true);
> -#endif // WITH_MEMORYVIEW
> - m_priv->statuses_notebook->set_current_page (0);
> -}
The removal of this function should be mentioned in the ChangeLog.
[...]
> +Gtk::Widget&
> +DBGPerspective::get_target_output_view_widget ()
> +{
> + return get_target_output_view_scrolled_win ();
> +}
> +
> +Gtk::Widget&
> +DBGPerspective::get_log_view_widget ()
> +{
> + return get_log_view_scrolled_win ();
> +}
> +
> +Gtk::Widget&
> +DBGPerspective::get_terminal_view_widget ()
> +{
> + return get_terminal_box ();
> +}
> +
These three functions should be dropped now, because we don't use
these three views anymore.
[...]
> sigc::signal<void>&
> +DBGPerspective::body_changed_signal ()
> +{
> + THROW_IF_FAIL (m_priv);
> + return m_priv->layout_mgr.layout_changed_signal ();
> +}
> +
> +sigc::signal<void>&
> DBGPerspective::debugger_not_started_signal ()
> {
> return m_priv->debugger_not_started_signal;
> @@ -8964,7 +8743,7 @@ DBGPerspective::agree_to_shutdown ()
> {
> LOG_FUNCTION_SCOPE_NORMAL_DD;
> if (debugger ()->is_attached_to_target ()) {
> - UString message;
> + UString message;
I think this is not well indented. It wasn't well indented before, it
still isn't with your change. :-)
> message.printf (_("There is a program being currently debugged. "
> "Do you really want to exit from the debugger?"));
> if (nemiver::ui_utils::ask_yes_no_question (message) ==
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective.h b/src/persp/dbgperspective/nmv-dbg-perspective.h
> index 5e4a34b..e17ceac 100644
> --- a/src/persp/dbgperspective/nmv-dbg-perspective.h
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.h
[...]
> diff --git a/src/persp/dbgperspective/nmv-preferences-dialog.cc b/src/persp/dbgperspective/nmv-preferences-dialog.cc
> index c880804..2c54fcb 100644
> --- a/src/persp/dbgperspective/nmv-preferences-dialog.cc
> +++ b/src/persp/dbgperspective/nmv-preferences-dialog.cc
[...]
>
> -PreferencesDialog::PreferencesDialog (IWorkbench &a_workbench,
> +PreferencesDialog::PreferencesDialog (IPerspective *a_perspective,
I believe this parameter should be a reference.
> + LayoutManager &a_layout_manager,
> const UString &a_root_path) :
> Dialog (a_root_path,
> "preferencesdialog.ui",
> "preferencesdialog")
> {
> - m_priv.reset (new Priv (gtkbuilder (), a_workbench));
> + m_priv.reset (new Priv (gtkbuilder (), a_perspective, a_layout_manager));
> m_priv->update_widget_from_conf ();
> }
>
[...]
> diff --git a/src/persp/dbgperspective/nmv-preferences-dialog.h b/src/persp/dbgperspective/nmv-preferences-dialog.h
> index 7407a4e..10b6cfd 100644
> --- a/src/persp/dbgperspective/nmv-preferences-dialog.h
> +++ b/src/persp/dbgperspective/nmv-preferences-dialog.h
[...]
> diff --git a/src/persp/nmv-i-perspective.h b/src/persp/nmv-i-perspective.h
> index bc7d0fd..164fe93 100644
> --- a/src/persp/nmv-i-perspective.h
> +++ b/src/persp/nmv-i-perspective.h
> @@ -140,6 +140,10 @@ public:
> /// about its activation state (whether it is activated or not).
> virtual sigc::signal<void, bool>& activated_signal () = 0;
>
> + /// This signal is emited to notify the workbench when the
> + /// layout of the perspective change.
There is a missing "s" at the end of "change".
> + virtual sigc::signal<void>& body_changed_signal () = 0;
> +
[...]
> diff --git a/src/uicommon/nmv-layout-manager.cc b/src/uicommon/nmv-layout-manager.cc
> new file mode 100644
> index 0000000..54abb48
> --- /dev/null
> +++ b/src/uicommon/nmv-layout-manager.cc
[...]
Maybe you could add a comment here to present what the layout manager
does, and roughly present how it works.
> +typedef SafePtr<Layout, ObjectRef, ObjectUnref> LayoutSafePtr;
> +struct LayoutManager::Priv {
> + map<UString, LayoutSafePtr> layouts_map;
> + Layout *layout;
> + sigc::signal<void> layout_changed_signal;
> +
> + Priv () :
> + layout (0)
> + {
> + }
> +};
> +
> +LayoutManager::LayoutManager () :
> + m_priv (new Priv)
> +{
> +}
> +
> +LayoutManager::~LayoutManager ()
> +{
> + LOG_D ("deleted", "destructor-domain");
> +}
> +
> +void
> +LayoutManager::register_layout (Layout *a_layout)
> +{
This function lacks a comment about what it does, what registering a
layout means etc ...
Oh, I have just seen that you actually put these comments in the
header file that contains the declaration of this function. I prefer
that comment to be here, at the definition point of the function
instead. I gave the rationale of this at the declaration point of
the register_layout function.
> + THROW_IF_FAIL (m_priv);
> + THROW_IF_FAIL (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);
A space is missing before the opening parenthesis.
> +}
> +
> +void
> +LayoutManager::load_layout (const UString &a_identifier,
> + IPerspective *a_perspective)
> +{
> + THROW_IF_FAIL (m_priv);
> +
> + if (!is_layout_registered (a_identifier)) {
> + LOG_ERROR ("Trying to load a unregistered layout with the identifier: "
> + << a_identifier);
> + return;
> + }
> +
> + if (m_priv->layout) {
> + m_priv->layout->save_configuration ();
> + m_priv->layout->do_uninit ();
I am thinking maybe you should the do_uninit function should be
renamed do_cleanup_layout ...
> + }
> +
> + m_priv->layout = m_priv->layouts_map[a_identifier].get ();
> + THROW_IF_FAIL (m_priv->layout);
> +
> + m_priv->layout->do_init (a_perspective);
... and do_init should be called do_lay_out. I think tells more what
do_uninit and do_init are meant to do.
[...]
> diff --git a/src/uicommon/nmv-layout-manager.h b/src/uicommon/nmv-layout-manager.h
> new file mode 100644
> index 0000000..c6db8a0
> --- /dev/null
> +++ b/src/uicommon/nmv-layout-manager.h
[...]
> +/// \brief Manages Layouts
> +///
> +/// The LayoutManager is used to load/unload layout inside a IPerspective.
> +class LayoutManager {
> + //non copyable
> + LayoutManager (const LayoutManager&);
> + LayoutManager& operator= (const LayoutManager&);
> +
> + struct Priv;
> + SafePtr<Priv> m_priv;
> +
> +public:
> + LayoutManager ();
> +
> + /// \brief Register a layout inside the LayoutManager
> + /// \param a_layout Layout to register
> + void register_layout (Layout *a_layout);
Apart for abstract (pure virtual) class declarations, I prefer that
description comments of methods be put at the *definition* point of
the methods. This is because when a developer changes the code of the
function it is less likely to forget to update the descriptive comment
of the method than if she has to go look into the header file to
update it. Also, when you use etags/ctags and look symbol up, these
tools points you to the definition point of the symbol. So it's very
handy to have the descriptive comment of the symbol there there as
well.
This applies to all the descriptive comments of this file.
[...]
> diff --git a/src/uicommon/nmv-layout-selector.cc b/src/uicommon/nmv-layout-selector.cc
> new file mode 100644
> index 0000000..d1a5fdb
> --- /dev/null
> +++ b/src/uicommon/nmv-layout-selector.cc
[...]
> +
> +struct LayoutModelColumns : public Gtk::TreeModel::ColumnRecord {
> + LayoutModelColumns ()
> + {
> + add(is_selected);
> + add(name);
> + add(description);
> + add(identifier);
A space is missing before the opening parenthesis in the 4 function
calls above.
> + }
[...]
> +struct LayoutSelector::Priv {
> + IPerspective *perspective;
I would prefer this pointer here to be a reference instead. For the
same reasons that I made similar comments earlier.
> + Gtk::TreeView treeview;
> + LayoutModelColumns model;
> + LayoutManager &layout_manager;
> +
> + void
> + on_layout_toggled (const Glib::ustring &str)
> + {
The convention in Nemiver is to prefix parameter names with an 'a'.
That means "argument".
[...]
> + Glib::RefPtr<Gtk::TreeModel> tree_model = treeview.get_model ();
> + THROW_IF_FAIL (tree_model);
> +
> + Gtk::TreePath path (str);
> + Gtk::TreeModel::iterator iter = tree_model->get_iter (path);
> + THROW_IF_FAIL (iter);
> +
> + (*iter)[model.is_selected] = true;
Maybe you could add a comment here to say that the loop below is to
de-select every other layout but the one selected.
> + for (Gtk::TreeModel::iterator i = tree_model->children ().begin ();
> + i != tree_model->children ().end ();
> + ++i) {
> + if (*i == *iter)
> + continue;
> +
> + (*i)[model.is_selected] = false;
> + }
> +
> + UString identifier = (Glib::ustring) (*iter)[model.identifier];
> + layout_manager.load_layout (identifier, perspective);
> +
> + NEMIVER_TRY
> + THROW_IF_FAIL (perspective);
> + IConfMgrSafePtr conf_mgr = perspective->get_workbench ()
> + .get_configuration_manager ();
> + THROW_IF_FAIL (conf_mgr);
> + conf_mgr->set_key_value (CONF_KEY_DBG_PERSPECTIVE_LAYOUT, identifier);
> + NEMIVER_CATCH_NOX
Here again, you should use NEMIVER_CATCH, instead of NEMIVER_CATCH.
> + }
> +
> + void
> + on_cell_rendering (Gtk::CellRenderer *renderer,
> + const Gtk::TreeModel::iterator &iter)
> + {
Parameter names should be prefixed by "a_".
> + THROW_IF_FAIL (renderer);
> + THROW_IF_FAIL (iter);
> +
> + Gtk::CellRendererText *text_renderer =
> + dynamic_cast<Gtk::CellRendererText*> (renderer);
> + THROW_IF_FAIL (text_renderer);
> +
> + text_renderer->property_markup () =
> + Glib::ustring::compose (
> + "<b>%1</b>\n%2",
> + Glib::Markup::escape_text ((*iter)[model.name]),
> + Glib::Markup::escape_text ((*iter)[model.description]));
> + }
> +
> + Priv (LayoutManager &a_layout_manager, IPerspective *a_perspective) :
I'd prefer the perspective parameter to be a reference here, instead
of a pointer.
[...]
> diff --git a/src/uicommon/nmv-layout.h b/src/uicommon/nmv-layout.h
> new file mode 100644
> index 0000000..1d42372
> --- /dev/null
> +++ b/src/uicommon/nmv-layout.h
[...]
> +/// \brief The base class for Layouts
> +///
> +/// Perspectives can use the LayoutManager to manage different Layouts.
> +/// The LayoutManager handle object that inherit from the class Layout.
> +class Layout : public Object {
> + //non copyable
> + Layout (const Layout&);
> + Layout& operator= (const Layout&);
> +
> +protected:
> + Layout () {}
> +
> +public:
> + /// \brief initialize the layout
> + /// \param a_perspective The associated perspective.
> + virtual void do_init (IPerspective *a_perspective) = 0;
I would rather pass IPerspective here by reference here, instead of
passing a pointer. This will remove a chance of getting a null
pointer in code paths using that perspective.
[...]
> +
> + /// \brief show/hide a view
> + /// \param a_view_identifier The view to show/hide
> + /// \param a_show Show the view if true, hide otherwide
> + virtual void show_view (int a_view_identifier, bool a_show) = 0;
The comment should say 'otherwise', not otherwide.
> +
> + virtual ~Layout() {}
> +};
[...]
> diff --git a/src/workbench/nmv-workbench.cc b/src/workbench/nmv-workbench.cc
> index fbdf8b8..f4a911a 100644
> --- a/src/workbench/nmv-workbench.cc
> +++ b/src/workbench/nmv-workbench.cc
[...]
I have a few general observations about the patch.
With this design, I am a bit concerned about the burden of e.g, adding
a new view to the status view, for instance. This would require to
update all the different layouts by coding a new equivalent of e.g,
set_show_terminal_view for the new view. I understand that
eventually, when the GDL(MM) library is solid enough so that we can
depend on it, the static layouts are meant be dropped so that we only
keep the dynamic layout. But still.
So I am thinking that maybe you could devise something like a
Layout::add_item (Widget &a_item, int a_index) function that would add
a graphical item -- to be laid out at a given index -- to the
graphical container of the layout. Each particular layout would
implement Layout::add_item in their particular once for all. A
similar Layout::remove_item could be implemented as well. The
views to be laid out could be put in an array, somewhere else. Then
each particular layout could e.g, walk the array of views and call
Layout::add_item on them.
With a scheme like this, adding a new layout item would be just a
matter of adding the item to the array.
I haven't thought about all the corner cases of this scheme, but I
wanted to share this idea with you to see what you think about it.
Thank you for your work.
--
Dodji
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
