diff --git a/include/xlsxwriter/worksheet.h b/include/xlsxwriter/worksheet.h index 2264f27d..5db18041 100644 --- a/include/xlsxwriter/worksheet.h +++ b/include/xlsxwriter/worksheet.h @@ -227,6 +227,7 @@ enum cell_types { ARRAY_FORMULA_CELL, BLANK_CELL, BOOLEAN_CELL, + COMMENT, HYPERLINK_URL, HYPERLINK_INTERNAL, HYPERLINK_EXTERNAL @@ -898,6 +899,7 @@ typedef struct lxw_worksheet { FILE *optimize_tmpfile; struct lxw_table_rows *table; struct lxw_table_rows *hyperlinks; + struct lxw_table_rows *comments; struct lxw_cell **array; struct lxw_merged_ranges *merged_ranges; struct lxw_selections *selections; @@ -1061,7 +1063,6 @@ typedef struct lxw_row { uint8_t row_changed; uint8_t data_changed; uint8_t height_changed; - uint8_t has_comments; struct lxw_table_cells *cells; diff --git a/src/worksheet.c b/src/worksheet.c index f45d2e53..ac0deb65 100644 --- a/src/worksheet.c +++ b/src/worksheet.c @@ -87,9 +87,14 @@ lxw_worksheet_new(lxw_worksheet_init_data *init_data) GOTO_LABEL_ON_MEM_ERROR(worksheet->hyperlinks, mem_error); RB_INIT(worksheet->hyperlinks); + worksheet->comments = calloc(1, sizeof(struct lxw_table_rows)); + GOTO_LABEL_ON_MEM_ERROR(worksheet->comments, mem_error); + RB_INIT(worksheet->comments); + /* Initialize the cached rows. */ worksheet->table->cached_row_num = LXW_ROW_MAX + 1; worksheet->hyperlinks->cached_row_num = LXW_ROW_MAX + 1; + worksheet->comments->cached_row_num = LXW_ROW_MAX + 1; if (init_data && init_data->optimize) { worksheet->array = calloc(LXW_COL_MAX, sizeof(struct lxw_cell *)); @@ -402,6 +407,18 @@ lxw_worksheet_free(lxw_worksheet *worksheet) free(worksheet->hyperlinks); } + if (worksheet->comments) { + for (row = RB_MIN(lxw_table_rows, worksheet->comments); row; + row = next_row) { + + next_row = RB_NEXT(lxw_table_rows, worksheet->comments, row); + RB_REMOVE(lxw_table_rows, worksheet->comments, row); + _free_row(row); + } + + free(worksheet->comments); + } + if (worksheet->merged_ranges) { while (!STAILQ_EMPTY(worksheet->merged_ranges)) { merged_range = STAILQ_FIRST(worksheet->merged_ranges); @@ -432,7 +449,7 @@ lxw_worksheet_free(lxw_worksheet *worksheet) free(worksheet->chart_data); } - /* Just free the list. The list objects are free elsewhere. */ + /* Just free the list. The list objects are freed from the RB tree. */ free(worksheet->comment_objs); if (worksheet->selections) { @@ -700,6 +717,24 @@ _new_boolean_cell(lxw_row_t row_num, lxw_col_t col_num, int value, return cell; } +/* + * Create a new comment cell object. + */ +STATIC lxw_cell * +_new_comment_cell(lxw_row_t row_num, lxw_col_t col_num, + lxw_vml_obj *comment_obj) +{ + lxw_cell *cell = calloc(1, sizeof(lxw_cell)); + RETURN_ON_MEM_ERROR(cell, cell); + + cell->row_num = row_num; + cell->col_num = col_num; + cell->type = COMMENT; + cell->comment = comment_obj; + + return cell; +} + /* * Create a new worksheet hyperlink cell object. */ @@ -795,7 +830,6 @@ _insert_cell_list(struct lxw_table_cells *cell_list, /* If existing_cell is not NULL, then that cell already existed. */ /* Remove existing_cell and add new one in again. */ if (existing_cell) { - existing_cell->comment = NULL; RB_REMOVE(lxw_table_cells, cell_list, existing_cell); /* Add it in again. */ @@ -814,7 +848,6 @@ _insert_cell(lxw_worksheet *self, lxw_row_t row_num, lxw_col_t col_num, lxw_cell *cell) { lxw_row *row = _get_row(self, row_num); - lxw_vml_obj *existing_comment = NULL; if (!self->optimize) { row->data_changed = LXW_TRUE; @@ -825,20 +858,47 @@ _insert_cell(lxw_worksheet *self, lxw_row_t row_num, lxw_col_t col_num, row->data_changed = LXW_TRUE; /* Overwrite an existing cell if necessary. */ - if (self->array[col_num]) { - existing_comment = self->array[col_num]->comment; - self->array[col_num]->comment = NULL; + if (self->array[col_num]) _free_cell(self->array[col_num]); - } self->array[col_num] = cell; - self->array[col_num]->comment = existing_comment; } } } /* - * Insert a hyperlink object into the hyperlink list. + * Insert a blank placeholder cell in the cells RB tree in the same position + * as a comment so that the rows "spans" calculation is correct. Since the + * blank cell doesn't have a format it is ignored when writing. If there is + * already a cell in the required position we don't have add a new cell. + */ +STATIC void +_insert_cell_placeholder(lxw_worksheet *self, lxw_row_t row_num, + lxw_col_t col_num) +{ + lxw_row *row; + lxw_cell *cell; + + /* The spans calculation isn't required in constant_memory mode. */ + if (self->optimize) + return; + + cell = _new_blank_cell(row_num, col_num, NULL); + if (!cell) + return; + + /* Only add a cell if one doesn't already exist. */ + row = _get_row(self, row_num); + if (!RB_FIND(lxw_table_cells, row->cells, cell)) { + _insert_cell_list(row->cells, cell, col_num); + } + else { + _free_cell(cell); + } +} + +/* + * Insert a hyperlink object into the hyperlink RB tree. */ STATIC void _insert_hyperlink(lxw_worksheet *self, lxw_row_t row_num, lxw_col_t col_num, @@ -849,6 +909,18 @@ _insert_hyperlink(lxw_worksheet *self, lxw_row_t row_num, lxw_col_t col_num, _insert_cell_list(row->cells, link, col_num); } +/* + * Insert a comment into the comment RB tree. + */ +STATIC void +_insert_comment(lxw_worksheet *self, lxw_row_t row_num, lxw_col_t col_num, + lxw_cell *link) +{ + lxw_row *row = _get_row_list(self->comments, row_num); + + _insert_cell_list(row->cells, link, col_num); +} + /* * Next power of two for column reallocs. Taken from bithacks in the public * domain. @@ -2155,7 +2227,7 @@ _get_comment_params(lxw_vml_obj *comment, lxw_comment_options *options) /* Set the default start cell and offsets for the comment. These are * generally fixed in relation to the parent cell. However there are some - * edge cases for cells at the, er, edges. */ + * edge cases for cells at the, well yes, edges. */ if (row == 0) y_offset = 2; else if (row == LXW_ROW_MAX - 3) @@ -2559,20 +2631,16 @@ lxw_worksheet_prepare_vml_objects(lxw_worksheet *self, size_t used = 0; char *vml_data_id_str; - RB_FOREACH(row, lxw_table_rows, self->table) { + RB_FOREACH(row, lxw_table_rows, self->comments) { - if (row->has_comments) { - RB_FOREACH(cell, lxw_table_cells, row->cells) { - if (cell->comment) { - /* Calculate the worksheet position of the comment. */ - _worksheet_position_vml_object(self, cell->comment); + RB_FOREACH(cell, lxw_table_cells, row->cells) { + /* Calculate the worksheet position of the comment. */ + _worksheet_position_vml_object(self, cell->comment); - /* Store comment in a simple list for use by packager. */ - STAILQ_INSERT_TAIL(self->comment_objs, cell->comment, - list_pointers); - comment_count++; - } - } + /* Store comment in a simple list for use by packager. */ + STAILQ_INSERT_TAIL(self->comment_objs, cell->comment, + list_pointers); + comment_count++; } } @@ -5080,18 +5148,9 @@ worksheet_write_comment_opt(lxw_worksheet *self, lxw_row_t row_num, lxw_col_t col_num, char *text, lxw_comment_options *options) { - lxw_row *row; lxw_cell *cell; lxw_error err; lxw_vml_obj *comment; - uint8_t data_changed = LXW_FALSE; - - if (self->optimize) { - LXW_WARN("worksheet_write_comment/opt(): " - "Not supported in 'constant_memory' mode."); - - return LXW_ERROR_FEATURE_NOT_SUPPORTED; - } err = _check_dimensions(self, row_num, col_num, LXW_FALSE, LXW_FALSE); if (err) @@ -5109,40 +5168,24 @@ worksheet_write_comment_opt(lxw_worksheet *self, comment->text = lxw_strdup(text); GOTO_LABEL_ON_MEM_ERROR(comment->text, mem_error); - row = lxw_worksheet_find_row(self, row_num); - if (row) - data_changed = row->data_changed; - - cell = lxw_worksheet_find_cell_in_row(row, col_num); - if (cell) { - free(cell->comment); - } - else { - /* If there isn't an existing cell we use a new blank cell. */ - cell = _new_blank_cell(row_num, col_num, NULL); - _insert_cell(self, row_num, col_num, cell); - } - comment->row = row_num; comment->col = col_num; + cell = _new_comment_cell(row_num, col_num, comment); + GOTO_LABEL_ON_MEM_ERROR(cell, mem_error); + + _insert_comment(self, row_num, col_num, cell); + /* Set user and default parameters for the comment. */ _get_comment_params(comment, options); - cell->comment = comment; - - if (!row) { - row = lxw_worksheet_find_row(self, row_num); - row->data_changed = LXW_FALSE; - } - else { - row->data_changed = data_changed; - } - - row->has_comments = LXW_TRUE; self->has_vml = LXW_TRUE; self->has_comments = LXW_TRUE; + /* Insert a placeholder in the cell RB table in the same position so + * that the worksheet row "spans" calculations are correct. */ + _insert_cell_placeholder(self, row_num, col_num); + return LXW_NO_ERROR; mem_error: diff --git a/test/functional/src/test_comment16.c b/test/functional/src/test_comment16.c new file mode 100644 index 00000000..ed21ca78 --- /dev/null +++ b/test/functional/src/test_comment16.c @@ -0,0 +1,30 @@ +/***************************************************************************** + * Test cases for libxlsxwriter. + * + * Test to compare output against Excel files. + * + * Copyright 2014-2019, John McNamara, jmcnamara@cpan.org + * + */ + +#include "xlsxwriter.h" + +int main() { + + lxw_workbook *workbook = workbook_new("test_comment16.xlsx"); + lxw_worksheet *worksheet = workbook_add_worksheet(workbook, NULL); + + worksheet_write_string(worksheet, CELL("A1"), "Foo", NULL); + worksheet_write_string(worksheet, CELL("C7"), "Bar", NULL); + worksheet_write_string(worksheet, CELL("G14"), "Baz", NULL); + + worksheet_write_comment(worksheet, CELL("A1"), "Some text"); + worksheet_write_comment(worksheet, CELL("D1"), "Some text"); + worksheet_write_comment(worksheet, CELL("C7"), "Some text"); + worksheet_write_comment(worksheet, CELL("E10"), "Some text"); + worksheet_write_comment(worksheet, CELL("G14"), "Some text"); + + worksheet_set_comments_author(worksheet, "John"); + + return workbook_close(workbook); +} diff --git a/test/functional/src/test_comment56.c b/test/functional/src/test_comment56.c new file mode 100644 index 00000000..a7294f31 --- /dev/null +++ b/test/functional/src/test_comment56.c @@ -0,0 +1,41 @@ +/***************************************************************************** + * Test cases for libxlsxwriter. + * + * Test to compare output against Excel files. + * + * Copyright 2014-2019, John McNamara, jmcnamara@cpan.org + * + */ + +#include "xlsxwriter.h" + +int main() { + + lxw_workbook *workbook = workbook_new("test_comment56.xlsx"); + lxw_worksheet *worksheet = workbook_add_worksheet(workbook, NULL); + + worksheet_write_number(worksheet, CELL("A1"), 1234, NULL); + worksheet_write_number(worksheet, CELL("C7"), 1234, NULL); + worksheet_write_number(worksheet, CELL("G14"), 1234, NULL); + + worksheet_write_comment(worksheet, CELL("A1"), "Some text"); + worksheet_write_comment(worksheet, CELL("D1"), "Some text"); + worksheet_write_comment(worksheet, CELL("C7"), "Some text"); + worksheet_write_comment(worksheet, CELL("E10"), "Some text"); + worksheet_write_comment(worksheet, CELL("G14"), "Some text"); + + /* Repeat above to check for overwrite leaks. */ + worksheet_write_string(worksheet, CELL("A1"), "Foo", NULL); + worksheet_write_string(worksheet, CELL("C7"), "Bar", NULL); + worksheet_write_string(worksheet, CELL("G14"), "Baz", NULL); + + worksheet_write_comment(worksheet, CELL("A1"), "Some text"); + worksheet_write_comment(worksheet, CELL("D1"), "Some text"); + worksheet_write_comment(worksheet, CELL("C7"), "Some text"); + worksheet_write_comment(worksheet, CELL("E10"), "Some text"); + worksheet_write_comment(worksheet, CELL("G14"), "Some text"); + + worksheet_set_comments_author(worksheet, "John"); + + return workbook_close(workbook); +} diff --git a/test/functional/src/test_optimize13.c b/test/functional/src/test_optimize13.c new file mode 100644 index 00000000..67b1f578 --- /dev/null +++ b/test/functional/src/test_optimize13.c @@ -0,0 +1,25 @@ +/***************************************************************************** + * Test cases for libxlsxwriter. + * + * Test to compare output against Excel files. + * + * Copyright 2014-2019, John McNamara, jmcnamara@cpan.org + * + */ + +#include "xlsxwriter.h" + +int main() { + + lxw_workbook_options options = {.constant_memory = LXW_TRUE}; + + lxw_workbook *workbook = workbook_new_opt("test_optimize13.xlsx", &options); + lxw_worksheet *worksheet = workbook_add_worksheet(workbook, NULL); + + worksheet_write_string(worksheet, CELL("A1"), "Foo", NULL); + worksheet_write_comment(worksheet, CELL("B2"), "Some text"); + + worksheet_set_comments_author(worksheet, "John"); + + return workbook_close(workbook); +} diff --git a/test/functional/src/test_optimize14.c b/test/functional/src/test_optimize14.c new file mode 100644 index 00000000..3a24eec3 --- /dev/null +++ b/test/functional/src/test_optimize14.c @@ -0,0 +1,32 @@ +/***************************************************************************** + * Test cases for libxlsxwriter. + * + * Test to compare output against Excel files. + * + * Copyright 2014-2019, John McNamara, jmcnamara@cpan.org + * + */ + +#include "xlsxwriter.h" + +int main() { + + lxw_workbook_options options = {.constant_memory = LXW_TRUE}; + + lxw_workbook *workbook = workbook_new_opt("test_optimize14.xlsx", &options); + lxw_worksheet *worksheet = workbook_add_worksheet(workbook, NULL); + + worksheet_write_string(worksheet, CELL("A1"), "Foo", NULL); + worksheet_write_string(worksheet, CELL("C7"), "Bar", NULL); + worksheet_write_string(worksheet, CELL("G14"), "Baz", NULL); + + worksheet_write_comment(worksheet, CELL("A1"), "Some text"); + worksheet_write_comment(worksheet, CELL("D1"), "Some text"); + worksheet_write_comment(worksheet, CELL("C7"), "Some text"); + worksheet_write_comment(worksheet, CELL("E10"), "Some text"); + worksheet_write_comment(worksheet, CELL("G14"), "Some text"); + + worksheet_set_comments_author(worksheet, "John"); + + return workbook_close(workbook); +} diff --git a/test/functional/test_comment.py b/test/functional/test_comment.py index b7401c74..e5cc17a3 100644 --- a/test/functional/test_comment.py +++ b/test/functional/test_comment.py @@ -59,3 +59,9 @@ class TestCompareXLSXFiles(base_test_class.XLSXBaseTest): def test_comment15(self): self.run_exe_test('test_comment15') + def test_comment16(self): + self.run_exe_test('test_comment16') + + # Memory leak test. + def test_comment56(self): + self.run_exe_test('test_comment56', 'comment16.xlsx') diff --git a/test/functional/test_optimize.py b/test/functional/test_optimize.py index 7fda5724..712241c5 100644 --- a/test/functional/test_optimize.py +++ b/test/functional/test_optimize.py @@ -33,6 +33,12 @@ class TestCompareXLSXFiles(base_test_class.XLSXBaseTest): # Skip some of the XlsxWriter tests until the required functionality is ported. + def test_optimize13(self): + self.run_exe_test('test_optimize13') + + def test_optimize14(self): + self.run_exe_test('test_optimize14') + def test_optimize21(self): self.run_exe_test('test_optimize21') diff --git a/test/functional/xlsx_files/comment16.xlsx b/test/functional/xlsx_files/comment16.xlsx new file mode 100644 index 00000000..7e0b5b63 Binary files /dev/null and b/test/functional/xlsx_files/comment16.xlsx differ diff --git a/test/functional/xlsx_files/optimize13.xlsx b/test/functional/xlsx_files/optimize13.xlsx new file mode 100644 index 00000000..fa4bf9c4 Binary files /dev/null and b/test/functional/xlsx_files/optimize13.xlsx differ diff --git a/test/functional/xlsx_files/optimize14.xlsx b/test/functional/xlsx_files/optimize14.xlsx new file mode 100644 index 00000000..68786401 Binary files /dev/null and b/test/functional/xlsx_files/optimize14.xlsx differ