patchPSPP - Patches: patch #5636, Add callback feature to dictionary

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5636: Add callback feature to dictionary

Submitter:  John Darrington <jmd>
Submitted:  Wed 13 Dec 2006 04:48:56 AM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed

Sat 16 Dec 2006 03:42:24 AM UTC, comment #4: 

OK.  I've committed this patch.


I haven't addressed the enum alignment <--> GtkJustification conversion issue, which is probably a good idea but it really a seperate issue.

John Darrington <jmd>
Group administrator
Thu 14 Dec 2006 05:43:59 PM UTC, comment #3: 

Revised patch looks good.

dict_set_callbacks could use a comment.

Is there good reason that struct dictionary stores the callbacks by-value instead of by-pointer?  I'd normally expect the callbacks structures to be static and const...

The comparison in mv_is_resizable and in val_labs_can_set_width could use var_type_from_width.

I'm beginning to think we should just drop the "Written by <so-and-so>" lines at the top of each source file.  PSPP is a collaborative work now and most files have been touched by more than one person.  What do you think?

I noticed this comment:
   /* Kludge: Happily GtkJustification is defined similarly
      to enum alignment from pspp/variable.h */
Perhaps this comment should be on enum var_alignment, to discourage me from changing it, or perhaps we should add a real conversion here.

The following (in at least three places):
+           var_set_write_format (pv, &fmt);
+           var_set_print_format (pv, &fmt);
can be shorted to var_set_both_formats (pv, &fmt);

Ben Pfaff <blp>
Group administrator
Thu 14 Dec 2006 01:14:52 PM UTC, comment #2: 

Thanks for your comments.  A revised patch is attached.

(file #11525)

John Darrington <jmd>
Group administrator
Thu 14 Dec 2006 04:01:58 AM UTC, comment #1: 

This diff is a little hard to read because of several "changes" that appear to just change invisible white space.  It looks OK though.  Much cleaner GUI code in places.

The assertions in var_set_decimals are wrong.  I'd recommend simply setting the decimal places, then asserting on fmt_check_output.

This change in var_set_width should be unnecessary:
-          v->val_labs = NULL;
+          v->val_labs = val_labs_create (new_width);
If you put it in thinking that it would always make ->val_labs non-null, it is insufficient, because var_create sets ->val_labs to null.  But I don't think any code depends on that.

In src/ui/gui/missing-val-dialog.c:
-                          psppire_variable_get_type(dialog->pv) == VAR_NUMERIC)
;
+                          var_get_type(dialog->pv) == VAR_NUMERIC);
You could use var_is_numeric(dialog->pv) here.

Similarly for VAR_STRING ==  var_get_type (pv) in src/ui/gui/psppire-data-store.c.

The psppire_var_store_set_string function looks suspicious.  I don't know where its input is coming from, but it looks unsafe if it's not coming from someplace that's been validated, e.g. does the following change work?  var_set_name should assert-fail if the variable is in a dictionary; dict_rename_var is supposed to be used instead:
     case COL_NAME:
-      return psppire_variable_set_name(pv, text);
+      var_set_name (pv, text);
+      return TRUE;
       break;

Similarly, this is going to assert-fail if the decimals are not in the allowed range:
     case COL_DECIMALS:
       if ( ! text) return FALSE;
-      return psppire_variable_set_decimals(pv, atoi(text));
+      var_set_decimals (pv, atoi(text));
+      return TRUE;
       break;

The following check could be simplified to var_has_value_labels, which internally does the same thing:
+       if ( ! vls || 0 == val_labs_count (vls) )


Ben Pfaff <blp>
Group administrator
Wed 13 Dec 2006 04:48:56 AM UTC, original submission:  

This change adds optional callbacks that get invoked whenever a variable is added/deleted/mutated.

It also removes the caching of variable data in the gui, which with the recent vardict changes is now unnecessary and was a faulty concept anyway.  The upshot is, that the dictionary can now be changed "behind the GUI's back" without it getting very upset about it.


John Darrington <jmd>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #11525:  callbacks2.patch added by jmd (70KiB - text/x-patch)
file #11512:  callbacks.patch added by jmd (75KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by blp (Posted a comment)
  • -email is unavailable- added by jmd (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2006-12-16 jmd StatusWorks For Me Done
        Assigned tojmd None
        Open/ClosedOpen Closed
    2006-12-14 blp Assigned toblp jmd
    2006-12-14 jmd Attached File- Added callbacks2.patch, #11525
        Assigned tojmd blp
    2006-12-14 blp StatusReady For Test/Review Works For Me
        Assigned toblp jmd
    2006-12-13 jmd Attached File- Added callbacks.patch, #11512

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code