Re: [PATCH] Support Copy/Paste/Reset on target terminal

[Dodji Seketeli <dodji seketeli org>]

Hello Fabien,

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

> 
> This patch will need some information:
>     - I used the glib C style signal to detect the right click because
> for a reason I don't know, connecting the signal
> "signal_button_press_event ()" from the "Gtk::Widget* widget" doesn't
> work.

I haven't really investigated this, but this workaround is OK for me
for the time being.

>     - The consequence is I cannot access all the Terminal::Priv
> data, so I pass what I needed to the C callback function through a
> tuple.

OK.  You could have also just created an ad-hoc struct, which members
would have been the Terminal::Priv data that you stuffed into the
tuple.  But OK.


>     - Fix bugs #577496, #561100, #533437, #656093.
>     - Fix partially #564992

OK.  When you commit this patch, please add these bug numbers to the
commit log's first line, like it is specified in this document[1].

[...]

> diff --git a/menus/terminalmenu.xml b/menus/terminalmenu.xml
> new file mode 100644
> index 0000000..78911e8
> --- /dev/null
> +++ b/menus/terminalmenu.xml
> @@ -0,0 +1,10 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> + <ui>
> +  <popup name='TerminalMenu'>
> +    <menuitem action='CopyAction'/>
> +    <menuitem action='PasteAction'/>
> +    <separator/>
> +    <menuitem action='ResetAction'/>
> +  </popup>
> + </ui>
> +

Please put this file into src/persp/dbgperspective/menus/ instead.

Also, I am thinking that at some point, some terminal related menu
actions should also be available from the global menu of the
application.  But then we'd possibly need to think where exactly in
the application menu to put these.

For now I am OK with this addition, though.

[...]

> diff --git a/src/uicommon/nmv-terminal.cc b/src/uicommon/nmv-terminal.cc
> index 9573237..1b6092d 100644
> --- a/src/uicommon/nmv-terminal.cc
> +++ b/src/uicommon/nmv-terminal.cc
> @@ -37,16 +37,62 @@
>  #endif
>  #include <unistd.h>
>  #include <iostream>
> +#include <tr1/tuple>
>  #include <gtkmm/bin.h>
>  #include <gtkmm/main.h>
>  #include <gtkmm/window.h>
>  #include <gtkmm/adjustment.h>
> +#include <gtkmm/menu.h>
> +#include <gtkmm/builder.h>
> +#include <gtkmm/uimanager.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"
>  
>  NEMIVER_BEGIN_NAMESPACE(nemiver)
>  
> +using namespace common;
> +
> +typedef std::tr1::tuple<VteTerminal*&,
> +                   Gtk::Menu*&,
> +                   Glib::RefPtr<Gtk::ActionGroup>&> TerminalPrivDataTuple;
> +

Why references to pointers (VteTerminal*& and Gtk::Menu*?)?  Why not
just pointers?

> +static bool on_button_press_signal (GtkWidget*,
> +                                    GdkEventButton *a_event,
> +                                    TerminalPrivDataTuple *a_tuple)
> +{

Please start the on_button_press_signal on its on line, i.e:

static bool
on_button_press_signal (GtkWidget*,
                        GdkEventButton *a_event,
                        TerminalPrivDataTuple *a_tuple)
{


This patch is OK with the changes above.  By the way, please commit it
to gtk2-branch and master.

Thank you very much for your time.

[1]: http://git.gnome.org/browse/nemiver/tree/README.git-commit-messages.txt

-- 
		Dodji