gcmd-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [gcmd-dev] Patch for Review


From: Piotr Eljasiak
Subject: Re: [gcmd-dev] Patch for Review
Date: Sat, 12 Nov 2011 17:04:47 +0100

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

Attachment: gcmd-options-01__signals.patch
Description: Text Data

Attachment: gcmd-options-02__callbacks.patch
Description: Text Data

Attachment: gcmd-options-03__edit.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]