After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 506017 - importing of merged cell with cell comment set incorrect cellbound
importing of merged cell with cell comment set incorrect cellbound
Status: RESOLVED FIXED
Product: Gnumeric
Classification: Applications
Component: import/export MS Excel (tm)
git master
Other All
: Normal normal
: ---
Assigned To: Jody Goldberg
Jody Goldberg
Depends on:
Blocks:
 
 
Reported: 2007-12-28 06:40 UTC by Reggie Chan
Modified: 2008-01-24 23:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch as per previous comment (2.48 KB, patch)
2007-12-28 07:07 UTC, Reggie Chan
none Details | Review
Updated patch as per comment #4 (2.62 KB, patch)
2007-12-31 04:51 UTC, Reggie Chan
rejected Details | Review
scrub things in comment importers (6.98 KB, patch)
2008-01-23 12:07 UTC, Jody Goldberg
rejected Details | Review
actual patch. (4.27 KB, patch)
2008-01-24 14:41 UTC, Jody Goldberg
committed Details | Review

Description Reggie Chan 2007-12-28 06:40:43 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
Comment 1 Reggie Chan 2007-12-28 07:06:17 UTC
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?
Comment 2 Reggie Chan 2007-12-28 07:07:53 UTC
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)
Comment 3 Morten Welinder 2007-12-28 15:07:10 UTC
+	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.
Comment 4 Reggie Chan 2007-12-31 04:26:43 UTC
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?
Comment 5 Reggie Chan 2007-12-31 04:51:31 UTC
Created attachment 101883 [details] [review]
Updated patch as per comment #4
Comment 6 Jody Goldberg 2008-01-08 01:01:40 UTC
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.
Comment 7 Jody Goldberg 2008-01-23 12:07:19 UTC
Created attachment 103541 [details] [review]
scrub things in comment importers

Should be safe for 1.8 and HEAD.
Comment 8 Jody Goldberg 2008-01-24 14:37:38 UTC
wrong patch file.
Comment 9 Jody Goldberg 2008-01-24 14:41:43 UTC
Created attachment 103637 [details] [review]
actual patch.

This is what went in.
Comment 10 Morten Welinder 2008-01-24 23:50:23 UTC
Fixed, it seems.