patchPSPP - Patches: patch #6358, Text import

 
 

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

patch #6358: Text import

Submitter:  Ben Pfaff <blp>
Submitted:  Sun 30 Dec 2007 07:38:56 AM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  None
Open/Closed:  Closed

Jump to the original submission

Sun 16 Mar 2008 08:06:40 PM UTC, comment #24: 

I checked this in.

Ben Pfaff <blp>
Group administrator
Sun 16 Mar 2008 07:58:23 PM UTC, comment #23: 

Thanks.  I applied all your suggested changes, except:

>...[in the separators array], shouldn't we use N_("") for the first element?


The first element is a widget name that gets passed to get_widget_assert, so no.

>A number of places which use xstrdup, we can use g_strdup instead.
>Similarly they can be freed with g_free.


I can see that that is equivalent in some cases here, but I don't see the benefit to changing it.

Ben Pfaff <blp>
Group administrator
Sun 16 Mar 2008 06:18:01 AM UTC, comment #22: 

Looks all very good to me.

1. Instead of
  gtk_button_new_with_label ("Reset");
can you use
 gtk_button_new_from_stock ("pspp-stock-reset");

2. gtk_window_set_title (GTK_WINDOW (a->assistant), "Importing Textual Data") needs the string to use _("...")

3. I'm guessing that the first element of
static const struct separator separators[] =
  {
    {"space", ' '},
    {"tab", '\t'},
    ...}
is what get's used to display the strings in the Choose Seperators page.  Therefore, shouldn't we use N_("") for the first element?



Some other minor gnits:

  • Change 2007 to 2008 in the copyright notices.
  • The #include directives in text-data-import-dialog.h are not necessary.
  • The first arg to apply_dict can be const.
  • A number of places which use xstrdup, we can use g_strdup instead. Similarly they can be freed with g_free.


John Darrington <jmd>
Group administrator
Sun 16 Mar 2008 05:02:07 AM UTC, comment #21: 

Without further ado, here's what I'm proposing to check in.

Comments?

(file #15273)

Ben Pfaff <blp>
Group administrator
Sat 08 Mar 2008 07:24:05 AM UTC, comment #20: 


>I suggest that the sensitivity of the GtkEntry follow the state of
>the checkbox. The same should probably apply to the "Quote >separator characters with" checkbox and its ComboBox.


OK.

Here's the revised patch.

(file #15207)

Ben Pfaff <blp>
Group administrator
Fri 07 Mar 2008 11:34:25 PM UTC, comment #19: 

One other small problem I noticed:

On the "Choose separators" page, the "Custom" checkbutton doesn't interact properly with its associated GtkEntry, which can lead to the situation where what's displayed doesn't reflect the internal state of the importer.

I suggest that the sensitivity of the GtkEntry follow the state of the checkbox.  The same should probably apply to the "Quote separator characters with" checkbox and its ComboBox.

John Darrington <jmd>
Group administrator
Fri 07 Mar 2008 06:06:33 AM UTC, comment #18: 

Jason Stover writes:

> psppire: src/data/variable.c:179: var_set_name: Assertion `var_is_plausible_name (name, 0)' failed.


Very clever: you picked a data file that used the PSPP reserved word "and" as a variable name.  The function for generating unique names wasn't smart enough to notice this, but the dictionary refused to allow adding the variable, hence the assertion failure.  Good catch!

I checked in a change that fixes the problem.  After that, I was able to import your data file without trouble, although the actual import operation could certainly be a little faster.

Ben Pfaff <blp>
Group administrator
Fri 07 Mar 2008 05:32:12 AM UTC, comment #17: 


>I think what I'll do here is disable recognition of a roman
>numeral on its own as a month, only supporting them when they are
>part of a larger date format, e.g. "1 x 1978" in EDATE format (not
>that I've seen anyone do that either).


I checked in a change that did just that.

Ben Pfaff <blp>
Group administrator
Thu 06 Mar 2008 11:03:38 PM UTC, comment #16: 

I tried importing this file:

http://math.gcsu.edu/~jhs/sentence_data.txt.bz2

It has about 1.2*10^6 rows and 15 or so columns. About 62M
uncompressed.

In the window marked "Select Data to Import," the check-box that lets the user select the top row as containing variable names is a little hard to see. It might be easier at the top of the window, above the data, just under the sentence that asks the user to select the first row that contains the data.

I checked this box, hit the "Forward" key and got the following:

 psppire: src/data/variable.c:179: var_set_name: Assertion `var_is_plausible_name (name, 0)' failed.

Jason H Stover <jstover>
Group Member
Thu 06 Mar 2008 04:34:08 PM UTC, comment #15: 


>...when I import my /etc/passwd file, it assigns the password
>field with format MONTH3. Presumably this is because it's
>classifying "x" as the roman numeral ten. The year 10 AD is of
>course not representable in PSPP, so it produces funny results.
>
>Perhaps it should only treat things which could be roman numerals
>as dates if they're in recent history?


PSPP only allows months to be written as roman numerals, not years.  The MONTH format represents a month but not a date; that is, a normal field of type MONTH would contain something like "JANUARY", and your field that contains "x" is interpreted as October.  So there's no year or day to make it recent or long ago.

I've never actually encountered a data file that represents months with roman numerals, even though SPSS supports it, so I think what I'll do here is disable recognition of a roman numeral on its own as a month, only supporting them when they are part of a larger date format, e.g. "1 x 1978" in EDATE format (not that I've seen anyone do that either).

>If I override the allocated format to string, then on import, I
>get an error:


Oops.  Will fix.

Ben Pfaff <blp>
Group administrator
Thu 06 Mar 2008 10:39:39 AM UTC, comment #14: 

It's interesting that when I import my /etc/passwd file, it assigns the password field with format MONTH3.  Presumably this is because it's classifying "x" as the roman numeral ten.  The year 10 AD is of course not representable in PSPP, so it produces funny results.

Perhaps it should only treat things which could be roman numerals  as dates if they're in recent history?


If I override the allocated format to string, then on import, I get an error:
 syntax error: Syntax error expecting `(' at `8'.
 syntax error: Syntax error expecting variable name at `8'.

the journal file shows that it's doing:

> VARIABLE LEVEL VAR002 (SCALE).
> VARIABLE ALIGNMENT VAR002 (RIGHT).
> VARIABLE WIDTH VAR002 8.



On the other hand, if I tell it that the password field is a numeric format, then it gets SYSMIS values.


With regard to the window size, it may be most appropriate just to call gtk_window_maximize.


John Darrington <jmd>
Group administrator
Thu 06 Mar 2008 06:38:41 AM UTC, comment #13: 

Updated this patch against current CVS.  This includes number of fixes versus previous snapshots.

Known remaining bugs:
- The assistant window seems to be too small, especially for the last page. I always find myself making it larger with the mouse. I'm not sure how to tell Gtk that it should be bigger.
- Does not preserve dictionary information if you click Prev from the final page of the assistant and then click Next again.
- Slow response when you change dictionary information and there are many columns.
- Ugly code.

Features that I want to include eventually, but will probably not make it into the initial version I check in:
- Support for columnar data (as opposed to delimited data).
- Template load and save.


(file #15189)

Ben Pfaff <blp>
Group administrator
Tue 19 Feb 2008 12:35:17 AM UTC, comment #12: 


>If the text importer sees a field with some confusing data
>in it, it asks the user to choose the format manually. I think
>this is good, but it should make the default format 'String'.


I agree.  This is one of just a few features that I need to implement before final review.

If I have time, I will also try to make it recognize common date and time formats automatically.

Ben Pfaff <blp>
Group administrator
Mon 18 Feb 2008 09:56:48 PM UTC, comment #11: 

If the text importer sees a field with some confusing data
in it, it asks the user to choose the format manually. I think this is good, but it should make the default format 'String'. That way,
the user won't have to change as many of the formats manually: 'String' is likely to be the most common non-numeric format, and by  the time the user sees that format-changing window, the variables in question will not have numeric format anyway.

Jason H Stover <jstover>
Group Member
Thu 14 Feb 2008 02:18:52 AM UTC, comment #10: 

Impressive. I loaded some comma-delimited data more easily with this patch than with the other software.

Jason H Stover <jstover>
Group Member
Wed 13 Feb 2008 06:08:00 AM UTC, comment #9: 

Other problems:
- Dictionary information other than format is not propagated to the final dictionary via syntax.
- Changes to variable names in the dictionary pane are not propagated to the data preview pane (noticed by JMD).

Ben Pfaff <blp>
Group administrator
Mon 11 Feb 2008 06:02:24 PM UTC, comment #8: 

I'm getting a strange error:

src/ui/gui/text-data-import-dialog.c: In function 'get_tooltip_location':
src/ui/gui/text-data-import-dialog.c:666: warning: implicit declaration of function 'gtk_tree_view_convert_widget_to_bin_window_coords'
src/ui/gui/text-data-import-dialog.c: At top level:
src/ui/gui/text-data-import-dialog.c:726: error: expected declaration specifiers or '...' before 'GtkTooltip'
src/ui/gui/text-data-import-dialog.c: In function 'on_query_input_tooltip':
src/ui/gui/text-data-import-dialog.c:738: warning: implicit declaration of function 'gtk_tooltip_set_text'
src/ui/gui/text-data-import-dialog.c:738: error: 'tooltip' undeclared (first use in this function)
src/ui/gui/text-data-import-dialog.c:738: error: (Each undeclared identifier is reported only once
src/ui/gui/text-data-import-dialog.c:738: error: for each function it appears in.)
src/ui/gui/text-data-import-dialog.c: At top level:
src/ui/gui/text-data-import-dialog.c:838: error: expected declaration specifiers or '...' before 'GtkTooltip'
src/ui/gui/text-data-import-dialog.c: In function 'on_query_output_tooltip':
src/ui/gui/text-data-import-dialog.c:849: error: 'tooltip' undeclared (first use in this function)
make[2]: * [src/ui/gui/src_ui_gui_psppire-text-data-import-dialog.o] Error 1

The reason this is strange is that gtk/gtk.h has GtkTooltip in it.

Jason H Stover <jstover>
Group Member
Mon 11 Feb 2008 03:06:06 PM UTC, comment #7: 

And here's the patch (oops).

(file #15022)

Ben Pfaff <blp>
Group administrator
Mon 11 Feb 2008 07:11:30 AM UTC, comment #6: 

Here's the newest rev of the text-import patch.  This one is almost ready for review.

I know about the following bugs:
- Sometimes there's a tooltip-related segfault after clicking Apply on the final page.  Have not tracked this down yet.
- The assistant window seems to be too small, especially for the last page.  I always find myself making it larger with the mouse.  I'm not sure how to tell Gtk that it should be bigger.
- Some others I don't recall at the moment :-)

Stuff I'm going to do before asking for review:
- Make the code attempt to guess column types from the data (currently all columns are assumed to be in F format).
- Code cleanup

Features that I want to include eventually, but will probably not make it into the initial versin I check in:
- Support for columnar data (as opposed to delimited data).
- Automatic guessing of the correct delimiters
- Preserving dictionary information if you click Prev from the final page of the assistant and then click Next again
- Template load and save.

This patch adds a menu item to the File menu.  I don't know if that's the way to go; perhaps it should hook into File | Open | Data somehow.

Ben Pfaff <blp>
Group administrator
Sat 26 Jan 2008 10:57:44 PM UTC, comment #5: 


>The changes you made to glade-register.c are incorrect.


Yes, that represents a failed experiment.  The goal was to teach glade how to construct a GtkAssistant, which it doesn't know how to do.  Those changes will get dropped entirely.

>Nothing appears to use psppliststore.c (for which I'm grateful).


Heh.  Yes, that will be dropped too.

>Some of the copyright notices have the year 2007. This should be
>changed to 2008.


Thanks, will do.

>The variable sheet has it's row numbers starting from zero. For
>consistancy, perhaps they should start from unity.


OK, will do.

Ben Pfaff <blp>
Group administrator
Sat 26 Jan 2008 09:30:48 PM UTC, comment #4: 

It's looking promising.  There's a whole lot of small issues that I noticed, but since you acknoledge that it's awaiting cleanup, I'll only mention those of which you might be unaware:

The changes you made to glade-register.c are incorrect.  Registering GtkTypeAssistant is redundant, since it's already registered through the gtk+ library.  Furthermore, you're not actually using libglade to load GtkAssistant widgets.

Nothing appears to use psppliststore.c (for which I'm grateful).

Some of the copyright notices have the year 2007.  This should be changed to 2008.

The variable sheet has it's row numbers starting from zero.  For consistancy, perhaps they should start from unity.  Or even, perhaps the row title column should be switched off (there's not currently a way to do this, but it could probably be added to GtkSheet fairly easily).

John Darrington <jmd>
Group administrator
Sat 26 Jan 2008 07:40:07 AM UTC, comment #3: 

Everyone is (quite justifiably) probably getting bored with waiting for me to get the text data import stuff done, so I'm presenting another pre-review for this stuff.  In this version, most of the interface works, but the code is crappy and not cleaned up yet, and you can't actually close the dialog and read in data yet, and it doesn't really do the guessing of data formats that it should.  But it's much closer than before.

John, regarding your comment #2: I think that I made some adjustments that helped with the funny-looking layouts.  Maybe I'm still missing some things.  But I couldn't figure out how to use more than one font in a single text string.  I tried making the example characters bold, but it just looked funny to me.

(file #14873)

Ben Pfaff <blp>
Group administrator
Thu 03 Jan 2008 04:02:54 AM UTC, comment #2: 

It looks fine to me so far.

Jason H Stover <jstover>
Group Member
Sun 30 Dec 2007 09:04:14 AM UTC, comment #1: 


Some of the text is squashed up towards the top of the panes.  I managed to improve things by setting the Expand packing property on a number of label widgets, and by adding a bit of padding here and there.

On the "Choose seperators" page, you might want to try using a different font for your example characters.

Other than that, I can't think of any improvements.  I think it looks good.




John Darrington <jmd>
Group administrator
Sun 30 Dec 2007 07:38:56 AM UTC, original submission:  

I'm trying to figure out the GUI design for the text data importer.  Here's what I have right now.  It's text-data-import.glade from the patch that's the interesting part.  The numbered windows are each a page from the import assistant.  They should be mostly self-explanatory, except:

On page 3, the tree view will contain lines from the file with a radio button to select to first line of data.

On page 4, the tree view will show how lines from the file are divided into columns.

On page 5, the Variables tree view will show the Variables View from the data editor and the Data Preview will show how the imported data will look with the current settings.

I don't yet have even a draft for how to deal with columnar files.

This seems kind of ugly, but it's also the best I've come up with so far.  If any of you have a little more design talent, I'd really appreciate some help or some tips.

Ben Pfaff <blp>
Group administrator

 

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

Attached Files
file #15273:  text-import.patch added by blp (123KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jstover (Posted a comment)
  • -email is unavailable- added by jmd (Posted a comment)
  • -email is unavailable- added by blp (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 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-03-16 blp StatusNone Done
        Open/ClosedOpen Closed
    2008-03-16 blp Attached File#15207 Removed
    2008-03-16 blp Attached File- Added text-import.patch, #15273
        SummaryText import patch pre-review take 4 Text import
    2008-03-08 blp Attached File#15189 Removed
    2008-03-08 blp Attached File- Added text-import.patch, #15207
    2008-03-06 blp Attached File#15022 Removed
    2008-03-06 blp Attached File- Added text-import.patch, #15189
        SummaryText import patch pre-review take 3 Text import patch pre-review take 4
    2008-02-11 blp Attached File- Added text-import.patch, #15022
    2008-02-11 blp Attached File#14722 Removed
    2008-02-11 blp Attached File#14873 Removed
    2008-02-11 blp SummaryText import patch pre-review take 2 Text import patch pre-review take 3
    2008-01-26 blp SummaryPreliminary review for visual design Text import patch pre-review take 2
    2008-01-26 blp Attached File- Added text-import.patch, #14873
    2007-12-30 blp Attached File- Added text-data-import-dialog.patch, #14722

    Back to the top

    Powered by Savane 3.15-e6e5.
    Corresponding source code