Bug 58365 - Add Sheet.getAllCellCommentRefs()
Summary: Add Sheet.getAllCellCommentRefs()
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: unspecified
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
Keywords: PatchAvailable
Depends on: 58637
  Show dependency tree
Reported: 2015-09-11 16:20 UTC by Hannes Erven
Modified: 2015-11-23 15:24 UTC (History)
1 user (show)

Suggested patch (5.66 KB, patch)
2015-09-11 16:21 UTC, Hannes Erven
Details | Diff
Suggested patch, version 2 (10.67 KB, patch)
2015-09-25 19:59 UTC, Hannes Erven
Details | Diff
Suggested patch, version 2, with CellAdress extended usage (28.45 KB, patch)
2015-09-25 20:14 UTC, Hannes Erven
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hannes Erven 2015-09-11 16:20:41 UTC
The current Sheet interface allows the retrieval of comments only by location (row,col). In XSLX files, it is easily possible to have a comment reference a cell that is not physically present in the file.
Iterating over all rows and columns as reported by get{First,Last}RowNum and get{First,Last}CellNum with hence miss comments that reference positions in "null" rows as well as positions to the left/right of physically present cells in a given row.

As discussed on poi-dev[1], I propose adding the following method to Sheet:

     * Returns all cell addresses of this sheet
     * having one or more comments associated.
     * @return A set of cell references (in no particular order).
    Set<String> getAllCellCommentRefs();

[1] http://mail-archives.apache.org/mod_mbox/poi-dev/201509.mbox/ajax/%3C55E84053.4080002%40erven.at%3E
Comment 1 Hannes Erven 2015-09-11 16:21:37 UTC
Created attachment 33098 [details]
Suggested patch

Suggested patch with implementation for HSSF, XSSF and SXSSF.
Comment 2 Javen O'Neal 2015-09-14 07:59:51 UTC
I think a Set<CellAddress> would be better than Set<String> since it eliminates the burden of string parsing in client code. Unfortunately, the class CellAddress doesn't currently exist in POI.

You don't want CellReference, since that has a concept of relative/absolute that doesn't apply here.
You don't want CellRangeAddress since that forms an area defined by the top left and lower left cells.

Taking a step back, can you get away with returning an array/list/iterator on the CellComment objects themselves instead of the anchors? I'm guessing the CellComment objects would be more useful to most people. If you need both, consider some kind of Map<CellAddress, CellComment>.

If you end up writing a CellAddress class, there are a quite a few classes that could be updated with this higher-level data structure.
Comment 3 Hannes Erven 2015-09-25 19:59:57 UTC
Created attachment 33143 [details]
Suggested patch, version 2

Javen, thank you for your suggestions.

I'll attach a new patch that includes a new class org.apache.poi.ss.util.CellAddress and now suggests a Sheet.getCellComments() with return type Map<CellAdress, Comment> method.

Internally, XSSF's CommentsTable still uses String cell references (addresses, in fact) to manage the comments; on first sight, changing that is possible, but will bloat the patch.

Also, I noticed that various XSSF implementation use CellReference(row,col) as a CellAddress replacement. I guess that's the places you meant that would also benefit from a switch to CellAddress ?
Comment 4 Hannes Erven 2015-09-25 20:14:47 UTC
Created attachment 33144 [details]
Suggested patch, version 2, with CellAdress extended usage

Here's another version of the patch.
XSSF's CommentsTable now also internally uses CellAddress objects to manage the comments, and hence a bunch of locations that previously used "String ref" were converted to "CellAdress ref".
Comment 5 Hannes Erven 2015-10-18 16:06:38 UTC

is there any feedback/thoughts on the patches?


Comment 6 Javen O'Neal 2015-11-23 09:40:00 UTC
Added in r1715743.

Docs updated r1715746.
Comment 7 Javen O'Neal 2015-11-23 09:40:50 UTC
Disregard comment 6.
Comment 8 Javen O'Neal 2015-11-23 15:24:25 UTC
Updated CommentsTable in r1715784.
Updated XSSFSheetXMLHandler in r1715794.
Updated remainder of patch in r1715839, plus a unit test for Sheet.getCellComments.

Docs and quick-guide updated in r1715840 and r1715842.

Hannes, thanks for your contribution to the POI project!
These changes should be available in the upcoming POI 3.14beta1 release.