Re: [PATCH] Add terminal settings to the preferences dialog

[Dodji Seketeli <dodji seketeli org>]

Hello Fabien,

Thank you for the great patch, and sorry for the delay in reviewing it.

Please find below my comments.

> --- a/src/persp/dbgperspective/nmv-dbg-perspective.cc
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc
> @@ -1029,14 +1029,90 @@ struct DBGPerspective::Priv {

[..]

> +    } else if (a_key == CONF_KEY_TERMINAL_BACKGROUND_COLOR) {
> +        m_priv->modify_terminal_color_theme ();
> +    } else if (a_key == CONF_KEY_TERMINAL_FOREGROUND_COLOR) {
> +        m_priv->modify_terminal_color_theme ();
> +    } else if (a_key == CONF_KEY_TERMINAL_USE_SYSTEM_COLOR_THEME) {
> +        m_priv->modify_terminal_color_theme ();

Maybe replace the above with:

       } else if (a_key == CONF_KEY_TERMINAL_BACKGROUND_COLOR
               || a_key == CONF_KEY_TERMINAL_FOREGROUND_COLOR
               || a_key == CONF_KEY_TERMINAL_USE_SYSTEM_COLOR_THEME) {
           m_priv->modify_terminal_color_theme ();

> +    } else if (a_key == CONF_KEY_TERMINAL_USE_SYSTEM_FONT) {
> +        m_priv->modify_terminal_font ();
> +    } else if (a_key == CONF_KEY_TERMINAL_CUSTOM_FONT_NAME) {
> +        m_priv->modify_terminal_font ();

Likewise here.

[...]


> --- a/src/persp/dbgperspective/nmv-preferences-dialog.cc
> +++ b/src/persp/dbgperspective/nmv-preferences-dialog.cc

[...]

> +    void
> +    on_system_theme_toggled_signal ()
> +    {
> +        update_system_theme_key ();
> +    }
> +
> +    void
> +    on_text_color_set_signal ()
> +    {
> +        update_text_color_key ();
> +    }
> +
> +    void
> +    on_background_color_set_signal ()
> +    {
> +        update_background_color_key ();
> +    }
> +
> +    void
> +    on_color_scheme_changed_signal ()
> +    {
> +        update_color_scheme_keys ();
> +    }
> +
> +    void
> +    on_terminal_system_font_toggled_signal ()
> +    {
> +        update_terminal_system_font_key ();
> +    }
> +
> +    void
> +    on_terminal_custom_font_set_signal ()
> +    {
> +        update_terminal_custom_font_key ();
> +    }
> +
> +    void
> +    on_palette_scheme_changed_signal ()
> +    {
> +        update_palette_scheme_keys ();
> +    }
> +
> +    void
> +    on_palette_color_set_signal ()
> +    {
> +        update_palette_color_key ();
> +    }
> +

I usually prefer that callback code like this be protected by
NEMIVER_TRY/NEMIVER_CATCH.  This is because if something throws there,
the exception can be propagated into C (gtk) code and abort() is going
to be called.  Could you please do add NEMIVER_TRY/NEMIVER_CATCH to
protect the code in these callbacks?  I see that many other callback
are not done properly.  I'll apply a patch to fix that, unless you
beat to it.  If you do so, please do it in an additional patch.  I
pre-approve such a patch.

>      void init ()
>      {

[...]

Please add a comment that says what the loop below does.  Also say
where BUILT_IN_PALETTE_SIZE comes from, the naming scheme of the
palette buttons, this kind of things.

> +        std::string palette_buttons_name = "palettecolorbutton";
> +        for (int i = 0; i < BUILT_IN_PALETTE_SIZE; i++) {
> +            palette_buttons[i] =
> +                ui_utils::get_widget_from_gtkbuilder<Gtk::ColorButton>
> +                    (gtkbuilder, palette_buttons_name + int_to_string (i + 1));

Also, please at least say str_utils::int_to_string here, so that the
reader has a hint about where int_to_string comes from.  So the "using
namespace str_utils" is not really useful, I believe.

> +            THROW_IF_FAIL (palette_buttons[i]);
> +
> +            palette_buttons[i]->signal_color_set ().connect
> +                (sigc::mem_fun
> +                    (*this,
> +                     &PreferencesDialog::Priv::on_palette_color_set_signal));
> +        }

I think at some point we need to break this jumbo init () function
down into smaller functions, each one init-ing a particular tab of the
preference dialog.  I am not asking you to do this in this patch,
though.

[...]

> diff --git a/src/uicommon/nmv-terminal.cc b/src/uicommon/nmv-terminal.cc
> index 4f65da3..cb4636f 100644
> --- a/src/uicommon/nmv-terminal.cc
> +++ b/src/uicommon/nmv-terminal.cc
> @@ -45,15 +45,103 @@
>  #include <gtkmm/menu.h>
>  #include <gtkmm/builder.h>
>  #include <gtkmm/uimanager.h>
> +#include <gdkmm/rgba.h>
>  #include <vte/vte.h>
>  #include <glib/gi18n.h>
>  #include "common/nmv-exception.h"
>  #include "common/nmv-log-stream-utils.h"
>  #include "common/nmv-env.h"
>  #include "nmv-ui-utils.h"
> +#include "nmv-terminal.h"
>  
>  NEMIVER_BEGIN_NAMESPACE(nemiver)
>  
> +const int BUILT_IN_COLORS_SIZE = 5;
> +BuiltInColor BUILT_IN_COLORS[BUILT_IN_COLORS_SIZE] =
> +{
> +    {_("Black on light yellow"), "rgb(0,0,0)", "rgb(255,255,221)"},
> +    {_("Black on white"),        "rgb(0,0,0)", "rgb(255,255,255)"},
> +    {_("Gray on black"),         "rgb(170,170,170)", "rgb(0,0,0)"},
> +    {_("Green on black"),        "rgb(0,255,0)",     "rgb(0,0,0)"},
> +    {_("White on black"),        "rgb(255,255,255)", "rgb(0,0,0)"}
> +};
> +

Where does this data got copied from?  Please state it in comments.

> +const int BUILT_IN_PALETTES_SIZE = 4;
> +BuiltInPalette BUILT_IN_PALETTES[BUILT_IN_PALETTES_SIZE] =
> +{
> +    {
> +        "Tango",
> +        {
> +            "rgb(0,0,0)",   "rgb(85,87,83)",
> +            "rgb(204,0,0)", "rgb(239,41,41)",
> +            "rgb(78,154,6)", "rgb(138,226,52)",
> +            "rgb(196,160,0)", "rgb(252,233,79)",
> +            "rgb(52,101,164)", "rgb(114,159,207)",
> +            "rgb(117,80,123)", "rgb(173,127,168)",
> +            "rgb(6,152,154)", "rgb(52,226,226)",
> +            "rgb(211,215,207)", "rgb(238,238,236)"
> +        }
> +    },
> +    {
> +        _("Linux console"),
> +        {
> +            "#000000000000", "#aaaa00000000",
> +            "#0000aaaa0000", "#aaaa55550000",
> +            "#00000000aaaa", "#aaaa0000aaaa",
> +            "#0000aaaaaaaa", "#aaaaaaaaaaaa",
> +            "#555555555555", "#ffff55555555",
> +            "#5555ffff5555", "#ffffffff5555",
> +            "#55555555ffff", "#ffff5555ffff",
> +            "#5555ffffffff", "#ffffffffffff"
> +        }
> +    },
> +    {
> +        "XTerm",
> +        {
> +            "#000000000000", "#cdcb00000000",
> +            "#0000cdcb0000", "#cdcbcdcb0000",
> +            "#1e1a908fffff", "#cdcb0000cdcb",
> +            "#0000cdcbcdcb", "#e5e2e5e2e5e2",
> +            "#4ccc4ccc4ccc", "#ffff00000000",
> +            "#0000ffff0000", "#ffffffff0000",
> +            "#46458281b4ae", "#ffff0000ffff",
> +            "#0000ffffffff", "#ffffffffffff"
> +        }
> +    },
> +    {
> +        "Rxvt",
> +        {
> +            "#000000000000", "#cdcd00000000",
> +            "#0000cdcd0000", "#cdcdcdcd0000",
> +            "#00000000cdcd", "#cdcd0000cdcd",
> +            "#0000cdcdcdcd", "#fafaebebd7d7",
> +            "#404040404040", "#ffff00000000",
> +            "#0000ffff0000", "#ffffffff0000",
> +            "#00000000ffff", "#ffff0000ffff",
> +            "#0000ffffffff", "#ffffffffffff"
> +        }
> +    }
> +};
> +

Likewise, please add comments about where this got copied from, or how
you came to it.

> +bool
> +BuiltInPalette::operator== (const std::list<UString>& palette)
> +{

Please name palette a_palette instead.

[...]

> --- a/src/uicommon/nmv-terminal.h
> +++ b/src/uicommon/nmv-terminal.h

[...]

I am thinking that all the new BuilIn* and BUILT_IN_* below should be
defined in the nemiver::Terminal namespace.  Otherwise, when you use
them in the preference dialog code, it's not obvious where they come
from.

> +struct BuiltInColor
> +{
> +    UString name;
> +    UString foreground;
> +    UString background;
> +};

Please add a comment saying what this data type describes exactly.

> +#define BUILT_IN_PALETTE_SIZE 16

Please use a constant variable here, instead of a macro.

> +struct BuiltInPalette
> +{
> +    UString name;
> +    UString colors[BUILT_IN_PALETTE_SIZE];
> +
> +    bool operator== (const std::list<UString>& palette);
> +};

Comments for this data type as well.

I have tested the patch and it works great for me.  I just have the
following warnings in the terminal:

    (nemiver:6617): Gtk-WARNING **: Unknown property: gtkmm__GtkGrid.n-rows
    (nemiver:6617): Gtk-WARNING **: Unknown property: gtkmm__GtkGrid.n-columns

I am using GNOME 3.0.1 of Fedora 15.  Do you see these as well?

-- 
		Dodji