GNOME Bugzilla – Bug 506017
importing of merged cell with cell comment set incorrect cellbound
Last modified: 2008-01-24 23:50:23 UTC
Steps to reproduce: 1) Create an xls file with merge cell at B2:C4 and set cell comment at the merged cell 2) Import the xls file to gnumeric 3) Export the xls file as .gnumeric 4) Read the .gnumeric file's xml, look for "CellComment" Expected result: 5) The SheetObject has ObjectBound of B2:C4 Actual result: 5) The SheetObject has ObjectBound of B2
It seems cell_comment_set_pos does not handle merge cell, actually it could not since object Sheet is not provided, and it seems incorrect to assume the passed CellComment containing Sheet already. Shouldn't we change the signature cell_comment_set_pos (GnmComment *cc, GnmCellPos const *pos) to cell_comment_set_pos (GnmComment *cc, Sheet *sheet, GnmCellPos const *pos) and set correct anchor with merge cell considered? Then change plugins/excel/ms-excel-read to pass in sheet together in the function call. I'll attach patch for the above proposed change in the next comment. Will it break anything?
Created attachment 101719 [details] [review] Patch as per previous comment changed void cell_comment_set_pos (GnmComment *cc, GnmCellPos const *pos) to void cell_comment_set_pos (GnmComment *cc, Sheet *sheet, GnmCellPos const *pos)
+ merge = gnm_sheet_merge_contains_pos (sheet, pos); + if (merge) + range_init (&r, merge->start.col, merge->start.row, + merge->end.col, merge->end.row); + else + r.start = r.end = *pos; Why not simply + merge = gnm_sheet_merge_contains_pos (sheet, pos); + if (merge) + r = *merge + else + range_init_cellpos (&r, pos); ??? Other than that, looks good to be. No commits before 1.8 is out, though.
Thanks Morten, the r = *merge is indeed neater, i still need more hardwork on better using the C syntax =( After thinking for another while, i would like to change the patch to handle sheet_object_set_sheet as well inside the function cell_comment_set_pos. From user of the function's point of view, the purpose is to change an existing cell comment object's position, so in the case of the new position's sheet being different from the original one, sheet_object_set_sheet is expected to be called. I'll update the patch with "r = *merge" and handle sheet_object_set_sheet as well inside the function. Do you see any problem with that change?
Created attachment 101883 [details] [review] Updated patch as per comment #4
This is solving the wrong problem. Comments should have anchors == top left corner. The view takes care of the merge. I'll need to research why the xls import gets something else, and whether the xls export needs to generate that too.
Created attachment 103541 [details] [review] scrub things in comment importers Should be safe for 1.8 and HEAD.
wrong patch file.
Created attachment 103637 [details] [review] actual patch. This is what went in.
Fixed, it seems.