Xlsx image background in comments #1547

[leo-bsv]

Add a picture to the background of the comment. Supports four image formats: png, jpeg, gif, bmp. A method for changing the size of a comment to the size of an image.

• a bugfix
• a new feature

Checklist:

• Changes are covered by unit tests
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change
• Documentation is updated as necessary

Why this change is needed?

[leo-bsv]

Two questions:

1. Need to update documentation for this functionality - how i can do this?
2. Can't fix some checks - is someone can help me with that or PR can be merged in this state?
   Thanks.

[oleibman]

For the php-cs-fixer problems, you have redundant declarations for return type in hasBackgroundImage and getBackgroundImage, and for the parameter in setBackgroundImage. Remove the docBlock declarations, and keep the declarations in the function declaration.

For the documentation updates, the docs are in directory /docs. They are in markdown format. You just need to edit them as plaintext.

[oleibman]

When you add the new element backgroundImage to the Comment class, you need to make sure that it takes part in the calculation for getHashCode.

[leo-bsv]

Thanks for answers. I'll try to fix it.

[oleibman]

This fixes #1547.

[leo-bsv]

Ok, all checks passed. How i can link this PR and issue #1547? Or I can't do this?

[oleibman]

To link your PR to the issue, edit your commit message to include fix #1547.

You do not need to keep merging master into your PR.

Your change will require some time to review. It is complex, and, unfortunately, hits some of the hardest to understand code in the project, especially in Reader/Xlsx. There are 2 much simpler changes (#2416 and #2423) which already deal with problems with reading and writing image files; these may be applicable to how you are approaching this problem. I will be merging those in the near future, after which I will start on yours in earnest.

[leo-bsv]

Ok, thank you.

[oleibman]

For your tests, it is not true that "The only thing we need is to ensure the file is loaded without exception." There are many assertions you can and should be making - do the expected cells have comments, do the comments have images, are the images the right type and size, etc. FWIW, my preliminary testing indicates that your testBuildWithDifferentImageFormats is probably behaving as expected, but I'm not so sure about your testSaveLoadWithDrawingInComment. When I use your code to load your test file and save it under a new name, I see a comment with an image, but the image is just a black background - there doesn't seem to be a media directory, which would hold the image, in the output file.

[leo-bsv]

Ok, I will fix it and add test soon.

[leo-bsv]

@oleibman please check it again, i added tests and fix.

[oleibman]

Getting close. The result for testSaveLoadWithDrawingInComment now looks correct.

In testBuildWithDifferentImageFormats, there is no "tiff" image in the output file by design. Since you are expecting it to fail when you try to add a tiff image, your try block should add a self::fail statement:

try {
    $comment->set...
    self::fail('Should throw exception when attempting to add tiff');
} catch (...

I need you to add tests after the write/reload sequences as well, essentially the same tests as you made before the write. This will help confirm that read and write work correctly.

[leo-bsv]

Is it ok now? )

[oleibman]

Yes, I believe it is okay now. I do not have time to merge it at the moment, but some time in the next day or two.

Scrutinizer reports some problems. For documentation purposes, here is why I am ignoring them:

• It thinks, incorrectly, that getimagesize always returns an array (in 2 places). However, the documentation is clear - false may also be returned.
• Ditto for image_type_to_extension.
• The addition of 2 functions to BaseDrawing, each with complexity rating A, dropped the overall complexity rating from B to C. Attempting to avoid this would make things less maintainable, not more.

[oleibman]

Thank you for your contribution.