GNOME Bugzilla – Bug 60468
Oleo import fails
Last modified: 2004-12-22 21:47:04 UTC
When trying to open an Oleo spreadsheet in gnumeric, I get % gnumeric s16w3301.oleo ** CRITICAL **: file cell-draw.c: line 180 (cell_draw): assertion `cell->rendered_value' failed. ** CRITICAL **: file cell-draw.c: line 180 (cell_draw): assertion `cell->rendered_value' failed. [ message repeats ] No data is visible in the normal spreadsheet window, but it does look like *something* has been read, based on selection of individual cells. If I save as gnumeric file, then re-open, data shows up, but is far from correct. Example files will be attached...
Created attachment 5597 [details] Oleo spreadsheet file
Created attachment 5598 [details] Gnumeric version of the same file (incorrectly converted)
Created attachment 5599 [details] Oleo screenshot
Oh, and I was wondering if the correct converter was being used, but Plugin list marks "Gnu Oleo" with "[in memory]" after the file is opened. That does mean that the plugin has been used, right?
The warnings about unrendered cells were fixed several versions ago (the current release is 0.70). However, the quality (or lack thereover) of the oleo importer has not changed. It has not been maintained and could use some serious work.
No, I definitely get these error messages with the version I checked out from CVS yesterday. As for the import code, I think someone here will try to update it.
Created attachment 5604 [details] [review] Proposed fix
The way I see it, there are two major problems with the current oleo plugins: 1. Formatting commands are ignored completely. That's a bad idea even if we aren't interested in the formating as such, since these lines may update the "active" row or column no (i.e. the row/column subsequent lines that don't specify it themselves apply to.) 2. No attempt is made to parse formulas. The patch I just attached addresses both these issues. It adds code that will pick up row or column no from formatting lines, and also introduces a formula parser. The routine doesn't fully interprete the expressions, though, it just replaces Oleo cell specs with Gnumeric equivalents (relative cell ids are currently converted to absolute ones, but that could probably be changed quite eaily), then calls Gnumeric's own parser. This approach works for simple cases at least.
I've now also trying to handle the actual format strings properly, but I haven't been very succesful so far. What I'm doing is char fmt_string[100]; Range range; MStyle *mstyle = mstyle_new_default (); mstyle_ref(mstyle); range_init_full_sheet(&range); if(col>=0) range.start.col=range.end.col=OLEO_TO_GNUMERIC (col); if(row>=0) range.start.row=range.end.row=OLEO_TO_GNUMERIC (row); <build fmt_string> mstyle_set_align_h(mstyle, <correct alignment>); if (fmt_string[0]) { mstyle_set_format_text (mstyle, fmt_string); } sheet_style_set_range(sheet, &range, mstyle); mstyle_unref(mstyle); is that the correct approach? What I want is to build an mstyle structure (I think) then somehow apply it to a cell _or_ an entire row or column; row or col is negative if the appropriate value was left out from the file (I'm assuming that means the formatting applies to the entire column or row.)
The cell->rendered_value assertion message goes away if I call cell_render_value(), or cell_get_rendered_text() etc., after inserting data with cell_set_value()/cell_set_expr().
You should not have to call the render routines at that level. I'll have a look at your patch and see if I can replicate your render problem shortly. In the mean time, this conversation would probably go faster if you could drop by #gnumeric @ irc.gnome.org. thanks for looking at this.
Hmmm. I'm not really an IRC kind of person, but we'll see... In any case, I've now prepared a new version of my patch. Changes since the last time: 1. Started adding support for "formatting" commands. None of it works yet, unfortunately. 2. Better code for references in formulas. CellRef type taken into use, which made the code both simpler and more general, and allowed the distinction between relative and absolute references to be made. (Actually, all references were relative earlier, and not absolute like I said.) 3. cell_set_expr_and_value() called when there is an expression. cell_set_expr() after cell_set_value() like the previous version did seemed to clear the value. 4. Workaround for the "rendered_value" assertion problem, see above. 5. cell_queue_recalc() called after inserting value and/or expression. Whether or not we want this is a matter of taste; the main intention is to make sure error value is displayed right away when a formula couldn't be parsed. It may also mean that inserting expression _and_ value, see 3, is meaningless.
Created attachment 5617 [details] [review] Proposed fix, take 2 (replaces old patch)
There is no need to call cell_render or to handle recalcs at this level. Adding a call to sheet_flag_recompute_spans (sheet); after the sheet is created is sufficent. The rest of the patch certainly improves things, although it needs to be reformatted to match the rest of Gnumeric. I'll apply it and make the necessary tweaks, but I'll leave the bug open until the formatting works more smoothly.
Seems like I won't have time to look at the formatting stuff today, either. I will get around to it eventually, though, so if in the meantime anyone could add a brief outline of what calls I should use to set the value type and font, justification etc. of a cell, row or the sheet, I would be very grateful.
Created attachment 5714 [details] [review] Formatting updates
It turns out I was using the correct calls all along, they just didn't have any effect because the range was incorrectly initialised. Changing types of row/col arguments to oleo_set_format() was enought to get some of the formatting right. However, due to Oleo's "incremental" approach to cell specification, setting format for ranges of cells just won't do, we need to keep track of the current format, and apply it to each cell as values are inserted. The latest patch will make the appropriate modifications to the code. There is no proper support for "default" entries yet, though; we may want to update style of a range when parsing these, and merge them somehow with "cell" style set up later. Also, please revise calls to mstyle_ref() and mstyle_unref() as I'm not eentirely sure how these are meant to be used.
Patch has finally been applied.
Does this mean that this bug should now be closed ?
Good point. I suppose we can reopen if more development is done.