Bug 61203 - XSSFDrawing.getAnchorFromParent handles CTOneCellAnchor incorrectly, ignores CTAbsoluteAnchor
Summary: XSSFDrawing.getAnchorFromParent handles CTOneCellAnchor incorrectly, ignores ...
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.17-dev
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Reported: 2017-06-21 00:22 UTC by Greg Woolsey
Modified: 2017-06-23 08:38 UTC (History)
0 users


Note You need to log in before you can comment on or make changes to this bug.
Description Greg Woolsey 2017-06-21 00:22:35 UTC
It currently creates an invalid XSSFClientAnchor for a CTOneCellAnchor instance, and returns null for CTAbsoluteAnchor instances.

These two should return XSSFAnchor instances that reflecs the specifics of their cases.

The current XSSFClientAnchor is for a CTTwoCellAnchor that uses "from" and "to" row/column/offset values to define the bounding rectangle.

CTOneCellAnchor uses a "from" row/column/offset to define the top/left corner of the bounds, and a CTPositiveSize2D (xdr:ext element) to specify the shape length and width.

CTAbsoluteAnchor uses a CTPoint2D to specify the absolute top/left offset, and a CTPositiveSize2D to define the shape width and height.  

This is essentially what XSSFChildAnchor does currently, but it is not actually used except by a unit test, where it looks like it is used incorrectly and the test results are never checked for accuracy.

POI needs a new sub-class of XSSFAnchor to handle CTOneCellAnchor, with appropriate constructors.  This is somewhat of a hybrid of the two existing classes.

Since XSSFChildAnchor is not actually created outside unit tests, all the places POI builds an XSSFClientAnchor need to be checked to see where we need to add logic to decicde which instance needs to be created.

A factory is likely the best option here, to centralize the type based logic.
Comment 1 Greg Woolsey 2017-06-21 00:30:37 UTC
I think the Excel chart/shape properties map like this:

- Move and size with cells == CTTwoCellAnchor
- Move but don't size with cells == CTOneCellAnchor
- Don't move or size with cells == CTAbsoluteAnchor
Comment 2 Greg Woolsey 2017-06-21 17:38:58 UTC
I found that the ClientAnchor SS interface is actually the widely used variation.  It looks like members calling constructors of XSSFClientAnchor just need to be taught to properly fill it in based on the cases outlined above, and the JavaDoc needs updating to mention that row/column references < 0 mean there is no row/column anchor for that position (top-left or bottom-right) and the absolute values must be used instead.

These -1 R/C values would align with the AnchorType enum value as well, per the above mapping and updated JavaDocs.  

Callers just looking for current cached position and size info can use the X/Y values stored, regardless of anchor type.  If rows/columns may have been resized and/or inserted/deleted, and an updated position is desired, callers would need to calculate that themselves based on anchor type and possible R/C stored values, if any, in combination with current sheet values for row heights and column widths.  This could be complex if there is a need to factor in offsets within the anchor cells as well, since that would require knowledge of the starting cell absolute position.
Comment 3 Greg Woolsey 2017-06-21 20:58:57 UTC
Not sure how possible or helpful a unified API can be.  Variations that involve row/column references use the X/Y offsets as "distance from the corner of the cell" while absolute references use it as "distance from the corner of the worksheet".

This overloading with conflicting interpretations is confusing but also the way the ECMA spec does things.
Comment 4 Greg Woolsey 2017-06-21 23:10:51 UTC
Even more confusing.  A CTTwoCellAnchor can have an attribute "editAs" with values:

* oneCell
* twoCell
* absolute

where the default if not specified is twoCell.  This is also a way Excel saves these sometimes.

Given this, I think XSSFClientAnchor needs to be extended to know about all the different source combinations and just do the math necessary to adjust everything when a setter is called.
Comment 5 Greg Woolsey 2017-06-23 05:18:41 UTC
Added basic support in r1799643.  Resolving this for now, can be re-opened if the implementation is incomplete or buggy.  See the revision log for more details.
Comment 6 Andreas Beeker 2017-06-23 08:38:22 UTC
Please merge EMUUtils into org.apache.poi.util.Units - its nicer to have it in one place