Re: Proposed new interface for dialog boxes

[Ben Pfaff]

John Darrington <address@hidden> writes:

> Currently, the implementation of dialog box behaviour has been rather add 
> hoc.   I'm attaching a series of patches for review which proposes a 
> hopefully cleaner and more consistent way of doing things.
>
> Basically there is a new abstract class PsppireActionDialog (which is itself
> a subclass of GtkAction) and all dialog boxes will be implementations of this.
> It'll take some time before all existing ones are converted, but I think it'l
> be worth the effort.
>
> One of the advantages it should bring us, is the ability to easily popup a 
> dialog box remotely (ie through a script) for the purposes of testing, or 
> to create screenshots.  Looking ahead much further to the future, I think
> it will also help allowing the implementation of dialog boxes in a high level
> language.
>
> I had hoped the loc would become smaller with this change, but that is not
> the case.  However, the extra code is largely boilerplate  - at a pinch it
> could be autogenerated, but I don't currently think it worthwhile doing so.

It would have been easier to read the diffs if you have used "-M"
in the git-format-patch invocation, which is smarter about
renames.  You can add the following to ~/.gitconfig so that you
don't have to remember to add -M manually:

    [diff]
            renames = copies

"git am" pointed out some trailing white space and trailing blank
lines.

It seems reasonable to me to add abstract this behavior.  I agree
that it is a little cleaner.

It would be nice to add a high-level comment to
psppire-dialog-action.h that describes the purpose and intended
use of PsppireDialogAction.  Otherwise the reader is stuck
figuring it out for himself.

generate_syntax() in the new psppire-dialog-action-descriptives.c
file has a stray printf().

The FIXME in psppire_dialog_action_descriptives_activate()
implies that we might be able to factor out a little more code in
the future.  Is it worth doing that immediately, so that we don't
have to convert all the dialogs and then update all of them again?

We could reduce boilerplate by using G_DEFINE_TYPE,
G_DEFINE_ABSTRACT_TYPE, etc. in PSPPIRE.  For whatever reason
PSPPIRE avoids them.

The names are really long.  I don't have a better idea, though.

The headers put their "#include"s outside the top-level #ifndef
header guard.  That's unusual in my experience; it's customary to
put them inside.  I think that it also prevents the GNU C
preprocessor header double-include optimization described at
http://gcc.gnu.org/onlinedocs/gcc-4.6.2/cpp/Once_002dOnly-Headers.html#Once_002dOnly-Headers
-- 
Ben Pfaff 
http://benpfaff.org