Re: [PATCH] 543488 Support renaming of sessions

[Dodji Seketeli <dodji seketeli org>]

Hello Fabien,

Thank you very much for this patch, and please find below my
observations.

[...]

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

[...]

> +    ISessMgr *session_manager;

Please make this be a reference instead of a pointer.

> +        ISessMgr *a_session_manager) :

Please make this a reference as well.

>              (*treeiter)[session_columns.name] =
> -                                        iter->properties ()["sessionname"];
> +                                    iter->properties ().count ("customname") ?
> +                                    iter->properties ()["customname"] :
> +                                    iter->properties ()["sessionname"];
>              (*treeiter)[session_columns.session] = *iter;
>          }

Why not initialize iter->properties()["customname"] to the value of
iter->properties ()["sessionname"] by default?  You would then
unconditionally set (*treeiter)[session_columns.name] to
iter->properties ()["customname"].  This makes the code simpler.  We
could also change the name "customname" into "captionname", which IMHO
reflects more what it's intended to be.

[...]


> +            THROW_IF_FAIL (session_manager);

This becomes unnecessary if session_manager is turned into a
reference.

> +            Transaction &transaction = session_manager->default_transaction ();
> +            if (transaction.begin ()) {
> +                session_manager->store_session (session, transaction);
> +                transaction.commit ();
> +                session_manager->load_sessions ();
> +            }
> +        }

Why this dance to just store a transation?  Normally,
session_manager.store_session (session, transaction) should be
enough to save the transaction.  If it's not then I think there is a
bug in the transaction saving code.  You really shouldn't mess up with
starting and committing transactions at this point.

Thanks.

-- 
		Dodji