Re: [gcmd-dev] Patch for Review

[Bhaskar Bhat]

Hi Piotr,

Thanks for the detailed walkthrough !
I agree with the points mentioned.

I am new to such kind of open source development project, so kindly let me
know if go wrong somewhere.

Also, the three split patches you sent, how to merge those ?
Should I continue to do the changes and send patch for the pending actions
you mentioned ?

Kindly suggest further tasks !

Regards
*Bhaskar*

On Sat, Nov 12, 2011 at 9:34 PM, Piotr Eljasiak <address@hidden> wrote:

> Hi Bharat,
>
> > Agree with the comments.
> > I have done further changes to fix the issues. Details as below: -
> >
> > 1. Connected new signals at the end in the function "create_layout_tab
> ()".
> >     As you told, this solves the flickering problem.
> >
> > 2. For this, I am using a new GnomeCmdData struct variable
> > (prev_gnome_cmd_data), defined in "gnome_cmd_data.h" and
> > "gnome-cmd-data.cc". This variable would store the existing preferences
> > when the Options dialog is opened. This helps in reverting back the
> changes
> > when the "Cancel" button is clicked.
>
>
> Thanks for the patch.
>
> IMHO a bit cleaner solution is not to add global prev_gnome_cmd_data
> var, but to use it locally in options_edit() using the following code
> snippet:
>
>
> void options_edit (GtkMenuItem *menuitem, gpointer not_used)
> {
>    GnomeCmdData::Options prev_cfg = gnome_cmd_data.options;
>
>    if (!gnome_cmd_options_dialog (*main_win, gnome_cmd_data.options))
>    {
>        gnome_cmd_style_create (prev_cfg);
>        main_win->update_style();
>        gnome_cmd_data.options = prev_cfg;
>    }
>    else
>        gnome_cmd_data.save();
> }
>
>
> I've already changed gnome_cmd_options_dialog() to use options passed as
> an argument (and not using gnome_cmd_data explicitly). As for
> GnomeCmdData is a monstrous thingy with lots of data allocated
> dynamically, copying as the whole struct is irrelevant for
> gnome_cmd_options_dialog(). Thus GnomeCmdData::Options - new substruct
> holding only options configurable via 'Options' dlg. I've already done
> 7/8 of the transition.
>
> The task is obviously bigger than it seemed before and there is quite a
> chance for introducing some regressions here, so I opt for postponing it
> till 1.4 (I hope not longer than 2-3 weeks)
>
> Before that I'll complete moving to GnomeCmdData::Options, so
> gnome_cmd_options_dialog() will work with parametrized options only.
>
>
> I've split your patch into 3 parts:
>
>      * gcmd-options-01__signals.patch
>      * gcmd-options-02__callbacks.patch
>      * gcmd-options-03__edit.patch
>
>
> Pending tasks:
>
>     1. rearranging g_signal_connect() in all tabs of Options dlg - for
>        uniformity ;o)
>     2. adding copy constructor and assignment operator = for
>        GnomeCmdData::Options (simple memcpy can't be used here, as some
>        members are pointers to data)
>     3. handling of changes in options tabs affecting gcmd layout:
>        Filters, General, Format, Layout and Tabs
>
>
> Please stay tuned.
>
> Piotr
>
> _______________________________________________
> gcmd-devel mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/gcmd-devel
>
>