Fixed drawingResize dimensions calculation

[R3dByt3]

PR Details

The current implementation does not seem to be in accordance to the specification:
https://github.com/closedxml/closedxml/wiki/Cell-Dimensions
https://github.com/closedxml/closedxml/wiki/Cell-Dimensions#mdw
https://github.com/closedxml/closedxml/wiki/Cell-Dimensions#skiasharp-code

Also there seem to be logical issues as well. This commit solves an identical issue as linked below.

Description

Looking at the C# ClosedXML Excel file Wiki:

• The conversion from pt to px was incorrect (row height).
• The default conversion from MDW to px were incorrect (column width - there is no margin) - also this calculation should depend on the used font; To be honest I have not checked if and how this could be achieved within the project.
• The aspect ratio calculation has been simplified
• The offset values were subtracted from the image dimensions which is simply not correct

Related Issue

#2001

Motivation and Context

In my use case I encountered the problems mentioned in the issue #2001 and these changes applied locally fixed my issues.

How Has This Been Tested

Try to add multiple images into a single cell using offsets as mentioned in #2001. Currently this will cause problems. Like mentioned earlier these changes solve these issues.

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[xuri]

Thanks for your pull request. I've left some comments.

[xuri]

After this change, the math import no longer required. Unit tests TestChartSize and TestColWidth should be update.

[R3dByt3]

@xuri Fixed - thanks for the notice, I just pasted my local changes to the browser, sorry about this one

[xuri]

The unit tests TestChartSize and TestColWidth test fail after this changes.

[R3dByt3]

Sorry somehow I missed that there are affected tests - will be fixed

[xuri]

The math import of this file also no longer required. Please run test locally by go test . after changes.

[R3dByt3]

I will do this from now on and this will be fixed

[xuri]

The image will overflow Sheet1!B30 cell after this changes, the image with AutoFit inserted by this unit test.

[xuri]

The Sheet2!I9 cell image has been affected (the ratio of width to height increases), the image is widened after this changes. The image inserted by this unit test.

[xuri]

This can be simplify with:

aspWidth := float64(cellWidth) / width
aspHeight := float64(cellHeight) / height
asp := min(aspWidth, aspHeight)
width, height = width*asp, height*asp

[R3dByt3]

Will be changed, thanks for the info

[R3dByt3]

The image will overflow Sheet1!B30 cell after this changes, the image with AutoFit inserted by this unit test.

I fear that this is a very complex subject which needs to be discussed, please hear me out.

First of all I just realized another issue of the current implementation and fork (at least from my point of view). If I understand you correctly you suggest, that AutoFit should add pictures into the target cell and resize the image in a way, that it is not rendered beyond the cell borders.

However the current implementation changes the from anchor if the offset leaves the area of the cell. This means that a cell insertion targeting A1 can end up starting in A2. While the initial visible result seems to be correct there is an issue: The image will be moved if Cell A1 is being enlarged. This is not correct and I think that I should fix this as well.

The starting anchor point can be outside of the cell dimensions which leads to correct anchoring and simply hides the image as long as the cell is not expanded enough. In the end this will still break your expectation, but just in another way.

AutoFit would resize the image respecting the aspect ratio to the current cell dimensions and if an offset is used which causes the image to overflow the cell, it would look cut off, but this could be changed by the user, by expading the row / col size.

However, from my understanding your expected behaviour comes along with various problematic edge cases and this is why I would like to suggest considering to review this feature:

1. If the same image is added twice with the same settings except the offset, it might be possible, that both images end up with different sizes. Should varying offsets really affect the image size?
2. Consider the case of adding multiple images or dynamically calculating cell sizes and images positions - Would it really be the expected behaviour, if there are only a few pixels left from the start anchor, that an image is added with for example 5x5, 1x1 pixels?
3. When adding multiple images dynamically, maybe even with varying native sizes, this may be not the desired behaviour like in my case and can only be fixed by calculating the required col / row size for all rows / cols in advance.

Otherwise another option like "ResizeToCell" would be required or even target size fields for the picture struct.

What are your thoughts about this?

[xuri]

Yes, I agree with you, offsets shouldn't affect the image size. Users using AutoFit with Offset will be affected after these changes, like user case in #1560, I accept this, we can make these changes as a breaking change and notice user in release notes.