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]

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.

[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.

[xuri]

Any update for other remaining code review issues?

[R3dByt3]

Im working on it hoping to complete it today, from what I have noticed the anchoring wasnt fully correct and confused me a bit.

[R3dByt3]

Hopefully this covers all your questions or issues: If we are talking about oneCellAnchors, Excel starts overlapping images from last to first if the images do not fit into the column. This can be observed by resizing the column. Implementing an overflow by hand creates other issues like I have mentioned earlier (image separation; incorrect anchor cell, which causes unexpected movements on column resizing/hiding). At least currently I do not know about a property or similar to control or override this behavior of excel regarding too many images for column size X.

Does this answer your question or did I miss something?

[xuri]

The positionObjectPixels function has too many parameters, I pushed a commit on your branch, combine some parameters to made it shorten and simplicity.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.23%. Comparing base (3650e5c) to head (7178a11).
Report is 10 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2123    +/-   ##
========================================
  Coverage   99.22%   99.23%            
========================================
  Files          32       32            
  Lines       30114    30222   +108     
========================================
+ Hits        29882    29991   +109     
+ Misses        154      153     -1     
  Partials       78       78            

Flag	Coverage Δ
unittests	99.23% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[xuri]

LGTM, thanks for your contribution.

[xuri]

Hi @R3dByt3, this changes will cause generated workbook corrupted on Office 2007. The EditAs should be empty if Positioning is oneCell. We need fix it like this:

cellAnchor := xdrCellAnchor{}
-cellAnchor.EditAs = opts.Positioning
from := xlsxFrom{}
from.Col = colStart
from.ColOff = x1 * EMU
from.Row = rowStart
from.RowOff = y1 * EMU
cellAnchor.From = &from

if opts.Positioning != "oneCell" {
    to := xlsxTo{}
    to.Col = colEnd
    to.ColOff = x2 * EMU
    to.Row = rowEnd
    to.RowOff = y2 * EMU
    cellAnchor.To = &to
+   cellAnchor.EditAs = opts.Positioning
}

[Now-Shimmer]

Hi @xuri, @R3dByt3, I created a PR #2128 to fix this. Please take a review for that, thanks!

[xuri]

I found new issue about PR #2123. If open a workbook which contain shape with one cell anchor, and which has following element:

<xdr:ext cx="4495333" cy="937629"/>
<xdr:sp macro="" textlink="">

After this changes, open the workbook and insert a column, the generated workbook corrupted, some attribute of one cell anchor shape is missing:

<xdr:ext></xdr:ext>
<xdr:sp>

The next version 2.9.1 will release on May 13th, 2025. I will revert this PR #2123 if we not resolve this issue.

[xuri]

Issue 3: if adding drawing objects such as pictures, charts, shapes, and form controls with the Positioning value as absolute, the drawing object size will fit the cell. Just like enable LockAspectRatio.

[R3dByt3]

I will take a look at these issues

[crush-wu]

In this PR:

• Change default column width from 9.140625 to 10.5
• Change default column width in pixels from 64 to 84
• Change default row height from 15 to 15.6
• Change default row height in pixels from 20 to 20.8
• Change column width and row height to pixels conversion calculation logic

That's a really a huge breaking change, all drawing objects such as pictures, charts, shapes, and form controls size which depends on width and height would be impacted. Users need to adjust all sizes for their code. Can we keep the origin default value?

[R3dByt3]

@crush-wu if I remember correctly, during implementation and testing these changes caused the images of AddPictureTests.xlsx to keep their aspect ratios (better), if a column has not been modified / has default dimensions. Partially as mentioned earlier during this PR some of the units used by excel would actually require calculations based on font and font size, which makes any hard coded values incorrect. On top of that I can not test this for various excel versions.

If possible I will try to roll back this change too, if not - im unsure. Best case would be another PR with font calculations or hardcoded factors?

[Now-Shimmer]

I found new issue about PR #2123. If open a workbook which contain shape with one cell anchor, and which has following element:

<xdr:ext cx="4495333" cy="937629"/>
<xdr:sp macro="" textlink="">

After this changes, open the workbook and insert a column, the generated workbook corrupted, some attribute of one cell anchor shape is missing:

<xdr:ext></xdr:ext>
<xdr:sp>

The next version 2.9.1 will release on May 13th, 2025. I will revert this PR #2123 if we not resolve this issue.

Hi @xuri, @R3dByt3, I fixed this issue on this commit 1b9e19b

[Now-Shimmer]

Issue 3: if adding drawing objects such as pictures, charts, shapes, and form controls with the Positioning value as absolute, the drawing object size will fit the cell. Just like enable LockAspectRatio.

Hi @R3dByt3, @xuri, this issue fixed by this commit cbcf7f9. All issues has been fixed, any suggestion about this?

[R3dByt3]

Issue 3: if adding drawing objects such as pictures, charts, shapes, and form controls with the Positioning value as absolute, the drawing object size will fit the cell. Just like enable LockAspectRatio.

Hi @R3dByt3, @xuri, this issue fixed by this commit cbcf7f9. All issues has been fixed, any suggestion about this?

LGTM - thank you very much for fixing these issues so fast

[R3dByt3]

@Now-Shimmer @xuri @crush-wu

Based on the current code (incl. PR) I came to the following conclusion and suggestion for #2128

Currently I see 2 problems which would introduce a bigger change:

1. Pixels are passed as ints (I know that is correct, but on calculations including multiple rows and columns this may aggregate to bigger rounding errors. So any pixel calculation rounding should only be applied once as the last step)
2. Like mentioned multiple times already column width to px would require the amount of pixels a '0' takes respecting the font size, if I remember correctly.

Regarding the current updated constants I have to admit some kind of error:

Looking at a new Excel file I see:

Which means updating col.go constants (l.23) again to:

// Define the default cell size and EMU unit of measurement.
const (
	defaultColWidth        float64 = 10.38
	defaultColWidthPixels  float64 = 88.0
	defaultRowHeight       float64 = 14.25
	defaultRowHeightPixels float64 = 19
	EMU                    int     = 9525
)

Also please consider removing rows.go (l.1015+):

	if width < 1 {
		pixels = (width * 12) + 0.5
		return float64(int(pixels))
	}

After adding these changes as well the pictures (without offset, Excel includes these for width, height percentage calculation) in /test/TestAddPicture1.xlsx seems to be sized correctly (+/-1% rounding error?).

Using the old constants it can be observed that the images without offsets are being stretched at least to my research the are no case switches or borders within these calculations https://github.com/closedxml/closedxml/wiki/Cell-Dimensions (ClosedXML builds on top of the offical MS .NET SDK for Office Files and works flawless, just for way to much memory...)

@Now-Shimmer Would you be so kind to add these 2 changes to ur PR to fix my PR?

[Now-Shimmer]

I tested across multiple Excel application, the default column width and row height depends on language settings, so I think we can't using fixed value. I reverted those value to keep size of drawing object same as previous version, for example, create a chart with the code in README, after #2123 checked in, the deafult size of chart has been changed.

[R3dByt3]

I guess that might have been a pitfall I have fallen into... Good job @Now-Shimmer and good research.

@Now-Shimmer @xuri What do you think about making these values configurable / overrideable on opening a file?

[R3dByt3]

@Now-Shimmer I could not help but to wonder a bit more so I researched further:

Can you verify this with ur tests?
https://answers.microsoft.com/en-us/msoffice/forum/all/excel-new-blank-book-default-row-height-problem/9df7c580-9020-4b51-ae66-34e86a01dae3

Maybe it would be possible to fix this by a precomputed table, since even this topic seems to depend on font settings.
https://github.com/closedxml/closedxml/wiki/Cell-Dimensions#skiasharp-code

[xuri]

Good job! I will take look for the PR #2128. Yeah, I agree with made it configurable by add related fields in the Options data type in future version.