Re: [PATCH] 565447 Add application icons in process list

[Dodji Seketeli <dodji seketeli org>]

Fabien Parent <parent f gmail com> a écrit:

> This small patch is adding the application icon if available in the
> process list dialog.

Thank you for working on this.  The patch looks good to me.  There are
only two small nits I'd like to see addressed:


> +++ b/src/persp/dbgperspective/nmv-proc-list-dialog.cc

[...]

> +    void
> +    proc_args_cell_data_func (Gtk::CellRenderer *a_cell,
> +                              const Gtk::TreeModel::iterator &a_iter)

It would be nice to add comments for every new method we ask from now
on.  I am following this rule myself to increase the documentation
coverage of the code base; maybe I should add that recommendation to the
README.hacking.txt file in the source tree.

[...]

> +        NEMIVER_TRY
> +
> +        THROW_IF_FAIL (a_cell);
> +        Gtk::CellRendererPixbuf &renderer =
> +            static_cast<Gtk::CellRendererPixbuf&> (*a_cell);
> +
> +        Glib::RefPtr<Gtk::IconTheme> theme = Gtk::IconTheme::get_default ();
> +        THROW_IF_FAIL (theme);
> +
> +        Glib::RefPtr<Gdk::Pixbuf> icon =
> +            theme->load_icon ("application-x-executable",
> +                              APP_ICON_SIZE,
> +                              Gtk::ICON_LOOKUP_USE_BUILTIN);
> +
> +        THROW_IF_FAIL (a_iter);
> +        IProcMgr::Process proc;
> +        if (proc_mgr.get_process_from_pid
> +                (a_iter->get_value (columns ().pid), proc)
> +            && proc.args ().size ()) {
> +            UString process_name = proc.args ().front ();
> +            std::vector<UString> split = str_utils::split (process_name, "/");
> +
> +            if (split.size ()) {
> +                process_name = split[split.size () - 1];
> +                try {
> +                    icon = theme->load_icon (process_name,
> +                                             APP_ICON_SIZE,
> +                                             Gtk::ICON_LOOKUP_USE_BUILTIN);
> +                } catch (Gtk::IconThemeError&) {
> +                }

Maybe add a catch (...) clause to avoid terminating the program if
theme->load_icon throws an exception that is not compatible with
Gtk::IconThemeError?

This patch is OK to commit with those changes.

Thank you.

-- 
		Dodji