[PATCH 0/5] more var-type-dialog improvements

[Ben Pfaff]

I'm working on more but this is what's ready.

Comments?

Ben Pfaff (5):
  var-type-dialog: Avoid string copy setting up currency treeview.
  var-type-dialog: Fix possible memory leaks.
  var-type-dialog: Use G_TYPE_INT to store an int.
  format: Make fmt_date_template() human-friendly, respect field width.
  var-type-dialog: Use fmt_date_template() to reduce duplication.

 src/data/data-in.c           |   14 +++++---
 src/data/data-out.c          |   17 ++++------
 src/data/format.c            |   73 +++++++++++++++++++++++++++++++++--------
 src/data/format.h            |    2 +-
 src/ui/gui/var-type-dialog.c |   74 +++++++++++++++++++++---------------------
 5 files changed, 112 insertions(+), 68 deletions(-)

--
1.7.2.5


_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[Ben Pfaff]

The return value of fmt_name() is perfectly suitable here.
---
 src/ui/gui/var-type-dialog.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/src/ui/gui/var-type-dialog.c b/src/ui/gui/var-type-dialog.c
index 21d4966..786b0c4 100644
--- a/src/ui/gui/var-type-dialog.c
+++ b/src/ui/gui/var-type-dialog.c
@@ -502,11 +502,9 @@ var_type_dialog_create (GtkWindow *toplevel)
   for ( i = 0 ; i < 5 ; ++i )
     {
       enum fmt_type cc_fmts[5] = {FMT_CCA, FMT_CCB, FMT_CCC, FMT_CCD, FMT_CCE};
-      gchar text[4];
-      g_snprintf (text, 4, "%s", fmt_name (cc_fmts[i]));
       gtk_list_store_append (list_store, &iter);
       gtk_list_store_set (list_store, &iter,
-                          0, text,
+                          0, fmt_name (cc_fmts[i]),
   1, &cc_format[i],
   -1);
     }
--
1.7.2.5


_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[Ben Pfaff]

gtk_tree_model_get_value() documentation says:

    When done with value, g_value_unset() needs to be called to free
    any allocated memory.

but none of the users in this file did that.
---
 src/ui/gui/var-type-dialog.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/ui/gui/var-type-dialog.c b/src/ui/gui/var-type-dialog.c
index 786b0c4..aa46ca4 100644
--- a/src/ui/gui/var-type-dialog.c
+++ b/src/ui/gui/var-type-dialog.c
@@ -290,6 +290,8 @@ set_format_from_treeview (GtkTreeView *treeview, gpointer data)
   gtk_tree_model_get_value (model, &iter, 1, &the_value);
 
   dialog->fmt_l = *(struct fmt_spec *) g_value_get_pointer (&the_value);
+
+  g_value_unset (&the_value);
 }
 
 
@@ -313,6 +315,8 @@ set_format_type_from_treeview (GtkTreeView *treeview, gpointer data)
 
   dialog->fmt_l = custom_format;
   dialog->fmt_l.type = *(int*) g_value_get_pointer (&the_value);
+
+  g_value_unset (&the_value);
 }
 
 
@@ -589,6 +593,8 @@ select_treeview_from_format (GtkTreeView *treeview, const struct fmt_spec *fmt)
 
       spec = g_value_get_pointer (&value);
 
+      g_value_unset (&value);
+
       if ( 0 == memcmp (spec, fmt, sizeof (struct fmt_spec)))
  {
   break;
@@ -636,6 +642,8 @@ select_treeview_from_format_type (GtkTreeView *treeview,
 
       spec = * ((int *) g_value_get_pointer (&value));
 
+      g_value_unset (&value);
+
       if ( spec == fmt_type)
  break;
     }
--
1.7.2.5


_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[Ben Pfaff]

It seems more straightforward to store an int by value than by
pointer.
---
 src/ui/gui/var-type-dialog.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/ui/gui/var-type-dialog.c b/src/ui/gui/var-type-dialog.c
index aa46ca4..4ed27f0 100644
--- a/src/ui/gui/var-type-dialog.c
+++ b/src/ui/gui/var-type-dialog.c
@@ -314,7 +314,7 @@ set_format_type_from_treeview (GtkTreeView *treeview, gpointer data)
   gtk_tree_model_get_value (model, &iter, 1, &the_value);
 
   dialog->fmt_l = custom_format;
-  dialog->fmt_l.type = *(int*) g_value_get_pointer (&the_value);
+  dialog->fmt_l.type = g_value_get_int (&the_value);
 
   g_value_unset (&the_value);
 }
@@ -500,8 +500,7 @@ var_type_dialog_create (GtkWindow *toplevel)
        column);
 
 
-  list_store = gtk_list_store_new (2, G_TYPE_STRING,
- G_TYPE_POINTER);
+  list_store = gtk_list_store_new (2, G_TYPE_STRING, G_TYPE_INT);
 
   for ( i = 0 ; i < 5 ; ++i )
     {
@@ -509,7 +508,7 @@ var_type_dialog_create (GtkWindow *toplevel)
       gtk_list_store_append (list_store, &iter);
       gtk_list_store_set (list_store, &iter,
                           0, fmt_name (cc_fmts[i]),
-  1, &cc_format[i],
+  1, cc_format[i],
   -1);
     }
 
@@ -640,7 +639,7 @@ select_treeview_from_format_type (GtkTreeView *treeview,
 
       gtk_tree_model_get_value (model, &iter, 1, &value);
 
-      spec = * ((int *) g_value_get_pointer (&value));
+      spec = g_value_get_int (&value);
 
       g_value_unset (&value);
 
--
1.7.2.5


_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[Ben Pfaff]

The strings that fmt_date_template() returned were almost but not
quite the kind of strings that humans expect to see.  For one, they
always used "yy", even though the code uses 4-digit years ("yyyy")
when the field width is sufficient.  Similarly, they never included
seconds (i.e. omitted ":SS").  Finally, FMT_MOYR had an "X" where
one would expect a space.

This commit corrects all of the above issues.  Future commits will
make more use of fmt_date_template().
---
 src/data/data-in.c  |   14 ++++++---
 src/data/data-out.c |   17 +++++-------
 src/data/format.c   |   73 ++++++++++++++++++++++++++++++++++++++++----------
 src/data/format.h   |    2 +-
 4 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/src/data/data-in.c b/src/data/data-in.c
index e72d1b6..14a24ff 100644
--- a/src/data/data-in.c
+++ b/src/data/data-in.c
@@ -1,5 +1,5 @@
 /* PSPP - a program for statistical analysis.
-   Copyright (C) 1997-9, 2000, 2006, 2009, 2010, 2011 Free Software Foundation, Inc.
+   Copyright (C) 1997-9, 2000, 2006, 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -1123,7 +1123,7 @@ parse_date (struct data_in *i)
   double time = 0, date = 0;
   enum time_sign time_sign = SIGN_NO_TIME;
 
-  const char *template = fmt_date_template (i->format);
+  const char *template = fmt_date_template (i->format, 0);
   size_t template_width = strlen (template);
   char *error;
 
@@ -1180,14 +1180,18 @@ parse_date (struct data_in *i)
         case '-':
         case '/':
         case '.':
-        case 'X':
           error = parse_date_delimiter (i);
           break;
         case ':':
           error = parse_time_delimiter (i);
         case ' ':
-          parse_spaces (i);
-          error = NULL;
+          if (i->format != FMT_MOYR)
+            {
+              parse_spaces (i);
+              error = NULL;
+            }
+          else
+            error = parse_date_delimiter (i);
           break;
         default:
           assert (count == 1);
diff --git a/src/data/data-out.c b/src/data/data-out.c
index df447a6..2a0dd0a 100644
--- a/src/data/data-out.c
+++ b/src/data/data-out.c
@@ -1,5 +1,5 @@
 /* PSPP - a program for statistical analysis.
-   Copyright (C) 1997-9, 2000, 2006, 2009, 2011 Free Software Foundation, Inc.
+   Copyright (C) 1997-9, 2000, 2006, 2009, 2011, 2012 Free Software Foundation, Inc.
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -387,14 +387,11 @@ output_date (const union value *input, const struct fmt_spec *format,
   double number = input->f;
   int year, month, day, yday;
 
-  const char *template = fmt_date_template (format->type);
-  size_t template_width = strlen (template);
-  int excess_width = format->w - template_width;
+  const char *template = fmt_date_template (format->type, format->w);
 
   char tmp[64];
   char *p = tmp;
 
-  assert (format->w >= template_width);
   if (number == SYSMIS)
     goto missing;
 
@@ -411,6 +408,8 @@ output_date (const union value *input, const struct fmt_spec *format,
 
   while (*template != '\0')
     {
+      int excess_width;
+
       int ch = *template;
       int count = 1;
       while (template[count] == ch)
@@ -439,7 +438,7 @@ output_date (const union value *input, const struct fmt_spec *format,
             }
           break;
         case 'y':
-          if (count >= 4 || excess_width >= 2)
+          if (count >= 4)
             {
               if (year <= 9999)
                 p += sprintf (p, "%04d", year);
@@ -499,10 +498,7 @@ output_date (const union value *input, const struct fmt_spec *format,
                 }
               p += strlen (p);
             }
-          break;
-        case 'X':
-          *p++ = ' ';
-          break;
+          goto done;
         default:
           assert (count == 1);
           *p++ = ch;
@@ -510,6 +506,7 @@ output_date (const union value *input, const struct fmt_spec *format,
         }
     }
 
+ done:
   buf_copy_lpad (output, format->w, tmp, p - tmp, ' ');
   output[format->w] = '\0';
   return;
diff --git a/src/data/format.c b/src/data/format.c
index dc546af..58e16d3 100644
--- a/src/data/format.c
+++ b/src/data/format.c
@@ -1,5 +1,5 @@
 /* PSPP - a program for statistical analysis.
-   Copyright (C) 1997-9, 2000, 2006, 2010, 2011 Free Software Foundation, Inc.
+   Copyright (C) 1997-9, 2000, 2006, 2010, 2011, 2012 Free Software Foundation, Inc.
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -881,38 +881,81 @@ fmt_usable_for_input (enum fmt_type type)
   return fmt_get_category (type) != FMT_CAT_CUSTOM;
 }
 
-/* For time and date formats, returns a template used for input
-   and output. */
+/* For time and date formats, returns a template used for input and output in a
+   field of the given WIDTH.
+
+   WIDTH only affects whether a 2-digit year or a 4-digit year is used, that
+   is, whether the returned string contains "yy" or "yyyy", and whether seconds
+   are include, that is, whether the returned string contains ":SS".  A caller
+   that doesn't care whether the returned string contains "yy" or "yyyy" or
+   ":SS" can just specify 0 to omit them. */
 const char *
-fmt_date_template (enum fmt_type type)
+fmt_date_template (enum fmt_type type, int width)
 {
+  const char *s1, *s2;
+
   switch (type)
     {
     case FMT_DATE:
-      return "dd-mmm-yy";
+      s1 = "dd-mmm-yy";
+      s2 = "dd-mmm-yyyy";
+      break;
+
     case FMT_ADATE:
-      return "mm/dd/yy";
+      s1 = "mm/dd/yy";
+      s2 = "mm/dd/yyyy";
+      break;
+
     case FMT_EDATE:
-      return "dd.mm.yy";
+      s1 = "dd.mm.yy";
+      s2 = "dd.mm.yyyy";
+      break;
+
     case FMT_JDATE:
-      return "yyddd";
+      s1 = "yyddd";
+      s2 = "yyyyddd";
+      break;
+
     case FMT_SDATE:
-      return "yy/mm/dd";
+      s1 = "yy/mm/dd";
+      s2 = "yyyy/mm/dd";
+      break;
+
     case FMT_QYR:
-      return "q Q yy";
+      s1 = "q Q yy";
+      s2 = "q Q yyyy";
+      break;
+
     case FMT_MOYR:
-      return "mmmXyy";
+      s1 = "mmm yy";
+      s2 = "mmm yyyy";
+      break;
+
     case FMT_WKYR:
-      return "ww WK yy";
+      s1 = "ww WK yy";
+      s2 = "ww WK yyyy";
+      break;
+
     case FMT_DATETIME:
-      return "dd-mmm-yyyy HH:MM";
+      s1 = "dd-mmm-yyyy HH:MM";
+      s2 = "dd-mmm-yyyy HH:MM:SS";
+      break;
+
     case FMT_TIME:
-      return "H:MM";
+      s1 = "H:MM";
+      s2 = "H:MM:SS";
+      break;
+
     case FMT_DTIME:
-      return "D HH:MM";
+      s1 = "D HH:MM";
+      s2 = "D HH:MM:SS";
+      break;
+
     default:
       NOT_REACHED ();
     }
+
+  return width >= strlen (s2) ? s2 : s1;
 }
 
 /* Returns a string representing the format TYPE for use in a GUI dialog. */
diff --git a/src/data/format.h b/src/data/format.h
index 1744bec..def3cdc 100644
--- a/src/data/format.h
+++ b/src/data/format.h
@@ -134,7 +134,7 @@ bool fmt_usable_for_input (enum fmt_type) PURE_FUNCTION;
 int fmt_to_io (enum fmt_type) PURE_FUNCTION;
 bool fmt_from_io (int io, enum fmt_type *);
 
-const char *fmt_date_template (enum fmt_type) PURE_FUNCTION;
+const char *fmt_date_template (enum fmt_type, int width) PURE_FUNCTION;
 const char *fmt_gui_name (enum fmt_type);
 
 /* Format settings.
--
1.7.2.5


_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[Ben Pfaff]

I believe that it should be possible to merge more code together
later.
---
 src/ui/gui/var-type-dialog.c |   53 +++++++++++++++++++-----------------------
 1 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/src/ui/gui/var-type-dialog.c b/src/ui/gui/var-type-dialog.c
index 4ed27f0..1e86049 100644
--- a/src/ui/gui/var-type-dialog.c
+++ b/src/ui/gui/var-type-dialog.c
@@ -31,32 +31,26 @@
 #include "ui/gui/builder-wrapper.h"
 #include "ui/gui/var-type-dialog.h"
 
-struct format_opt {
-  gchar desc[21];
-  struct fmt_spec spec;
-};
-
-
-static const struct format_opt format_option[] =
+static const struct fmt_spec date_format[] =
   {
-    { "dd-mmm-yyyy", {FMT_DATE,  11, 0} },
-    { "dd-mmm-yy",   {FMT_DATE,   9, 0} },
-    { "mm/dd/yyyy",  {FMT_ADATE, 10, 0} },
-    { "mm/dd/yy",    {FMT_ADATE, 8, 0} },
-    { "dd.mm.yyyy",  {FMT_EDATE, 10, 0} },
-    { "dd.mm.yy",    {FMT_EDATE, 8, 0} },
-    { "yyyy/mm/dd",  {FMT_SDATE, 10, 0} },
-    { "yy/mm/dd",    {FMT_SDATE, 8, 0} },
-    { "yyddd",       {FMT_JDATE, 5, 0} },
-    { "yyyyddd",     {FMT_JDATE, 7, 0} },
-    { "q Q yyyy",    {FMT_QYR, 8, 0} },
-    { "q Q yy",      {FMT_QYR, 6, 0} },
-    { "mmm yyyy",    {FMT_MOYR, 8, 0} },
-    { "mmm yy",      {FMT_MOYR, 6, 0} },
-    { "dd WK yyyy",  {FMT_WKYR, 10, 0} },
-    { "dd WK yy",    {FMT_WKYR, 8, 0} },
-    { "dd-mmm-yyyy HH:MM", {FMT_DATETIME, 17, 0}},
-    { "dd-mmm-yyyy HH:MM:SS", {FMT_DATETIME, 20, 0}}
+    {FMT_DATE,  11, 0},
+    {FMT_DATE,   9, 0},
+    {FMT_ADATE, 10, 0},
+    {FMT_ADATE, 8, 0},
+    {FMT_EDATE, 10, 0},
+    {FMT_EDATE, 8, 0},
+    {FMT_SDATE, 10, 0},
+    {FMT_SDATE, 8, 0},
+    {FMT_JDATE, 5, 0},
+    {FMT_JDATE, 7, 0},
+    {FMT_QYR, 8, 0},
+    {FMT_QYR, 6, 0},
+    {FMT_MOYR, 8, 0},
+    {FMT_MOYR, 6, 0},
+    {FMT_WKYR, 10, 0},
+    {FMT_WKYR, 8, 0},
+    {FMT_DATETIME, 17, 0},
+    {FMT_DATETIME, 20, 0}
   };
 
 
@@ -177,7 +171,7 @@ on_toggle_2 (GtkToggleButton *togglebutton, gpointer user_data)
       break;
     case BUTTON_DATE:
       select_treeview_from_format (dialog->date_format_treeview,
-  &format_option[0].spec);
+  &date_format[0]);
       gtk_widget_hide (dialog->width_decimals);
       gtk_widget_show (dialog->date_format_list);
       break;
@@ -426,12 +420,13 @@ var_type_dialog_create (GtkWindow *toplevel)
 
   list_store = gtk_list_store_new (2, G_TYPE_STRING, G_TYPE_POINTER);
 
-  for ( i = 0 ; i < sizeof (format_option) / sizeof (format_option[0]) ; ++i )
+  for ( i = 0 ; i < sizeof (date_format) / sizeof (date_format[0]) ; ++i )
     {
+      const struct fmt_spec *f = &date_format[i];
       gtk_list_store_append (list_store, &iter);
       gtk_list_store_set (list_store, &iter,
-                          0, format_option[i].desc,
-  1, &format_option[i].spec,
+                          0, fmt_date_template (f->type, f->w),
+  1, f,
   -1);
     }
 
--
1.7.2.5


_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[John Darrington-4]

These all seem like improvements.  If the dialog still works feel free to push them.

On Mon, Jul 16, 2012 at 11:45:51PM -0700, Ben Pfaff wrote:
     I'm working on more but this is what's ready.
     
     Comments?
     
     Ben Pfaff (5):
       var-type-dialog: Avoid string copy setting up currency treeview.
       var-type-dialog: Fix possible memory leaks.
       var-type-dialog: Use G_TYPE_INT to store an int.
       format: Make fmt_date_template() human-friendly, respect field width.
       var-type-dialog: Use fmt_date_template() to reduce duplication.
     
      src/data/data-in.c           |   14 +++++---
      src/data/data-out.c          |   17 ++++------
      src/data/format.c            |   73 +++++++++++++++++++++++++++++++++--------
      src/data/format.h            |    2 +-
      src/ui/gui/var-type-dialog.c |   74 +++++++++++++++++++++---------------------
      5 files changed, 112 insertions(+), 68 deletions(-)
     
     --
     1.7.2.5
     
     
     _______________________________________________
     pspp-dev mailing list
     pspp-dev@...
     https://lists.gnu.org/mailman/listinfo/pspp-dev

--
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://keys.gnupg.net or any PGP keyserver for public key.



_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev

[Ben Pfaff]

Thanks.  It does still work, so I pushed these.

John Darrington <john@...> writes:

> These all seem like improvements.  If the dialog still works feel free to push them.
>
> On Mon, Jul 16, 2012 at 11:45:51PM -0700, Ben Pfaff wrote:
>      I'm working on more but this is what's ready.
>      
>      Comments?
>      
>      Ben Pfaff (5):
>        var-type-dialog: Avoid string copy setting up currency treeview.
>        var-type-dialog: Fix possible memory leaks.
>        var-type-dialog: Use G_TYPE_INT to store an int.
>        format: Make fmt_date_template() human-friendly, respect field width.
>        var-type-dialog: Use fmt_date_template() to reduce duplication.
>      
>       src/data/data-in.c           |   14 +++++---
>       src/data/data-out.c          |   17 ++++------
>       src/data/format.c            |   73 +++++++++++++++++++++++++++++++++--------
>       src/data/format.h            |    2 +-
>       src/ui/gui/var-type-dialog.c |   74 +++++++++++++++++++++---------------------
>       5 files changed, 112 insertions(+), 68 deletions(-)
>      
>      --
>      1.7.2.5
>      
>      
>      _______________________________________________
>      pspp-dev mailing list
>      pspp-dev@...
>      https://lists.gnu.org/mailman/listinfo/pspp-dev

_______________________________________________
pspp-dev mailing list
pspp-dev@...
https://lists.gnu.org/mailman/listinfo/pspp-dev