New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Negative image offset leads to corrupt xlsx file when re-saved #273

Closed
kjosib opened this Issue Jul 9, 2015 · 10 comments

Comments

2 participants
@kjosib

kjosib commented Jul 9, 2015

https://gist.github.com/kjosib/97564bb295dd6e7f3b7a

That links to a 5-line sample program for causing this issue. Comments describe exactly how to provoke the bug. You can use this sample png if you like:
test

The program inserts a small image (208x49) with an x_offset more negative than the width of the image. (This is done to position the image relative to the right edge of the printable area, without knowing the width of all columns.) You can generate, open, and read such a file just fine. However, such a file will often corrupt if Excel then saves out modifications. It doesn't happen if the image is big enough or the offset greater than 0-imagewidth. (I'm running 0.7.3, just upgraded via PIP.)

Is this something that is even possible to address?

Another solution (for me, not for this bug) would be adding support for images in print headers.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 9, 2015

Owner

Hi,

Another solution (for me, not for this bug) would be adding support for images in print headers.

Images in headers are supported in set_header() (and 'set_footer()'). See the following example.

Regards,

John

Owner

jmcnamara commented Jul 9, 2015

Hi,

Another solution (for me, not for this bug) would be adding support for images in print headers.

Images in headers are supported in set_header() (and 'set_footer()'). See the following example.

Regards,

John

@jmcnamara jmcnamara self-assigned this Jul 9, 2015

@jmcnamara jmcnamara closed this Jul 22, 2015

@kjosib

This comment has been minimized.

Show comment
Hide comment
@kjosib

kjosib Jul 23, 2015

Confused. Is there some code in the next version or do we consider the work-around as the best solution? Is this bug even fixable within the xlsx file format? Thank you for your time.

kjosib commented Jul 23, 2015

Confused. Is there some code in the next version or do we consider the work-around as the best solution? Is this bug even fixable within the xlsx file format? Thank you for your time.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 23, 2015

Owner

I replied to the part where you said that a solution would be images in the print headers. That is supported.

You didn't respond to that so I presumed that solution was okay. Is it not?

Owner

jmcnamara commented Jul 23, 2015

I replied to the part where you said that a solution would be images in the print headers. That is supported.

You didn't respond to that so I presumed that solution was okay. Is it not?

@kjosib

This comment has been minimized.

Show comment
Hide comment
@kjosib

kjosib Jul 24, 2015

I appreciated the tip that such images were (now) supported. It's wonderful
that you got my business going again. I'm sure I only tripped over this bug
as a consequence of starting with XLSXwriter before the support existed. So
now can you reliably open, modify, and save workbooks constructed with
small images with large negative x-offsets without resulting in a corrupted
XLSX file? If so, by all means close the bug. If not, does GitHub have a
"won't-fix" or "not-really-a-bug" category?

On Thu, Jul 23, 2015 at 4:08 AM, John McNamara notifications@github.com
wrote:

I replied to the part where you said that a solution would be images in
the print head headers. That is supported.

You didn't respond to that so I presumed that solution was okay is it not?


Reply to this email directly or view it on GitHub
#273 (comment)
.

kjosib commented Jul 24, 2015

I appreciated the tip that such images were (now) supported. It's wonderful
that you got my business going again. I'm sure I only tripped over this bug
as a consequence of starting with XLSXwriter before the support existed. So
now can you reliably open, modify, and save workbooks constructed with
small images with large negative x-offsets without resulting in a corrupted
XLSX file? If so, by all means close the bug. If not, does GitHub have a
"won't-fix" or "not-really-a-bug" category?

On Thu, Jul 23, 2015 at 4:08 AM, John McNamara notifications@github.com
wrote:

I replied to the part where you said that a solution would be images in
the print head headers. That is supported.

You didn't respond to that so I presumed that solution was okay is it not?


Reply to this email directly or view it on GitHub
#273 (comment)
.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Oct 4, 2015

Owner

Apologies. I didn't look at this closely enough when you reported it and assumed that you were using the negative offset to force the image into the header.

I can see now that isn't what you were reporting.

It is in fact a bug. Basically, negative offsets aren't supported and will produce undefined behaviour.

I'll reopen this issue and have a look at fixing it.

Owner

jmcnamara commented Oct 4, 2015

Apologies. I didn't look at this closely enough when you reported it and assumed that you were using the negative offset to force the image into the header.

I can see now that isn't what you were reporting.

It is in fact a bug. Basically, negative offsets aren't supported and will produce undefined behaviour.

I'll reopen this issue and have a look at fixing it.

@jmcnamara jmcnamara reopened this Oct 4, 2015

@jmcnamara jmcnamara added bug and removed ready to close labels Oct 4, 2015

@jmcnamara jmcnamara changed the title from (very) Subtle Bug in Image Insert; leads to corrupt xlsx file when excel saves modified file to Negative image offset leads to corrupt xlsx file when re-saved Oct 4, 2015

jmcnamara added a commit that referenced this issue Oct 5, 2015

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Oct 5, 2015

Owner

I've pushed a partial fix for this issue to the master branch.

There are still some issue for negative offsets less that the size of a row/column but I'm working on that.

It is still a work in progress but for your case it should work.

Owner

jmcnamara commented Oct 5, 2015

I've pushed a partial fix for this issue to the master branch.

There are still some issue for negative offsets less that the size of a row/column but I'm working on that.

It is still a work in progress but for your case it should work.

jmcnamara added a commit that referenced this issue Oct 5, 2015

@jmcnamara jmcnamara added the short term label Oct 5, 2015

jmcnamara added a commit that referenced this issue Oct 7, 2015

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Oct 7, 2015

Owner

Fixed in version 0.7.6 of XlsxWriter on PyPI. Thanks for the report and sorry for missing the issue the first time around.

Owner

jmcnamara commented Oct 7, 2015

Fixed in version 0.7.6 of XlsxWriter on PyPI. Thanks for the report and sorry for missing the issue the first time around.

@jmcnamara jmcnamara closed this Oct 7, 2015

@kjosib

This comment has been minimized.

Show comment
Hide comment
@kjosib

kjosib Oct 9, 2015

Cool. I'm looking over the entire function that changed, and notice another way to break things. Try inserting an image at cell B2 with a large negative offset. It will skip the clamping at worksheet.py line 3971. The call self._size_col(-1) looks like it will return the default column width, and then we'll have an image homed at a negative column. I'll bet that's not supported either...

kjosib commented Oct 9, 2015

Cool. I'm looking over the entire function that changed, and notice another way to break things. Try inserting an image at cell B2 with a large negative offset. It will skip the clamping at worksheet.py line 3971. The call self._size_col(-1) looks like it will return the default column width, and then we'll have an image homed at a negative column. I'll bet that's not supported either...

@kjosib

This comment has been minimized.

Show comment
Hide comment
@kjosib

kjosib Oct 9, 2015

Thanks, by the way :)

kjosib commented Oct 9, 2015

Thanks, by the way :)

jmcnamara added a commit that referenced this issue Oct 11, 2015

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Oct 11, 2015

Owner

I'm looking over the entire function that changed, and notice another way to break things. Try inserting an image at cell B2 with a large negative offset.

Well spotted. I've change one of the test cases to reflect that and pushed a fix.

Thanks.

Owner

jmcnamara commented Oct 11, 2015

I'm looking over the entire function that changed, and notice another way to break things. Try inserting an image at cell B2 with a large negative offset.

Well spotted. I've change one of the test cases to reflect that and pushed a fix.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment