Re: Hello ! It's been a long time ! - patch ready (almost) for pop up issue :)

[Dodji Seketeli <dodji seketeli org>]

Hello Seemanta,

Thanks for the patch. It looks good to me in general and from my
testing, it works like a charm! I have however some minor nits to
point out, so here go my comments.

[...]

> From 178c5e7277725137d3a92a7f2216229cd79c6dcf Mon Sep 17 00:00:00 2001
> From: Seemanta Dutta <seemanta gmail com>
> Date: Sun, 16 May 2010 13:55:22 +0530
> Subject: [PATCH] Adding a popup when quitting nemiver.
> 
> ---
>  src/persp/dbgperspective/nmv-dbg-perspective.cc |   20 ++++++++++++++
>  src/persp/dbgperspective/nmv-dbg-perspective.h  |    2 +
>  src/persp/nmv-i-perspective.h                   |   10 +++++++
>  src/workbench/nmv-workbench.cc                  |   32 ++++++++++++++++++++---
>  4 files changed, 60 insertions(+), 4 deletions(-)

It would have been nice to write a log describing the patch.  There is
a git-commit-messages.README file at the top level of hte source tree
that describes what the log should loo like. You can also do a git log
and see what the other commit logs look like.

> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective.cc b/src/persp/dbgperspective/nmv-dbg-perspective.cc
> index 5841f19..f09a0c8 100644
> --- a/src/persp/dbgperspective/nmv-dbg-perspective.cc
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc

[...]

> +bool
> +DBGPerspective::agree_to_shutdown()
> +{
> +    LOG_DD("In Veto function");

There should be a space before the parenthesis.

> +    UString message;
> +    message.printf (_("There is a program being currently debugged. \
> +                     Do you really want to exit from the debugger?"));

Declaring and setting this variable should probably better be done in
the if-block below.

> +    if( debugger ()->is_attached_to_target () ) {
> +        if (nemiver::ui_utils::ask_yes_no_question (message) == Gtk::RESPONSE_YES) {

This line is longer than 80 characters. Please keep lines shorter than
80 characters.

Also, there should be a space between the parenthesis and the if, and
no space between the parenthesis and debugger. There shouldn't be any
space between target () and the closing parenthesis of the if either.

These things are described in the coding-style.txt file at the top
level of the source directory.

[...]

> diff --git a/src/persp/nmv-i-perspective.h b/src/persp/nmv-i-perspective.h
> index fad359d..1cc32dd 100644
> --- a/src/persp/nmv-i-perspective.h
> +++ b/src/persp/nmv-i-perspective.h
> @@ -121,6 +121,16 @@ public:
>      virtual Gtk::Widget* load_menu (const UString &a_filename,
>                                      const UString &a_widget_name) = 0;
>  
> +    /// \brief method which will be called for each perspective before 
> +    /// workbench initiates a shutdown(). This is a chance given to

The \brief comment should stand on one line.
The next line would then be the begining of the longer form of the
brief, one-liner comment.

So it should probably be something like.

/// \brief Should returns true to allow shutdown.
//  This method is called for each perspective before the workbench
initiates a shutdown() ....

[...]

> +    virtual bool agree_to_shutdown() = 0;

> diff --git a/src/workbench/nmv-workbench.cc b/src/workbench/nmv-workbench.cc
> index 31f68bd..8e09f3e 100644
> --- a/src/workbench/nmv-workbench.cc
> +++ b/src/workbench/nmv-workbench.cc
> @@ -132,6 +132,7 @@ private:
>          WorkbenchStaticInit::do_init ();
>      }
>      bool on_delete_event (GdkEventAny* event);
> +    bool query_for_shutdown();

There should be a space between before the opening parenthesis.

> +bool
> +Workbench::query_for_shutdown()

Space before the open parenthesis.

> +{
> +    bool retval = true;
> +    list<IPerspectiveSafePtr>::const_iterator iter;
> +    for (iter = m_priv->perspectives.begin ();
> +         iter != m_priv->perspectives.end ();
> +         ++iter) {
> +         if((*iter)->agree_to_shutdown() == false) {

Space before the open parenthesis.

> +             retval = false;
> +             break;
> +         }
> +    }
> +    return retval;
> +}
> +
>  
>  //*********************
>  //signal slots methods
> @@ -197,6 +214,7 @@ bool
>  Workbench::on_delete_event (GdkEventAny* a_event)
>  {
>      LOG_FUNCTION_SCOPE_NORMAL_DD;
> +    bool retval = true;
>  
>      NEMIVER_TRY
>      // use event so that compilation doesn't fail with -Werror :(
> @@ -204,11 +222,15 @@ Workbench::on_delete_event (GdkEventAny* a_event)
>  
>      // clicking the window manager's X and shutting down the with Quit menu item
>      // should do the same thing
> -    on_quit_menu_item_action ();
> +    if(query_for_shutdown() == true) {

Space after the if.

So, would you mind sending an updated patch that addresses my comments
above? Or would you rather prefer that I do the changes myself?

In any case, thanks for your time and the nice patch.

Cheers.

        Dodji