qax-os / excelize

Go language library for reading and writing Microsoft Excel™ (XLAM / XLSM / XLSX / XLTM / XLTX) spreadsheets
https://xuri.me/excelize
BSD 3-Clause "New" or "Revised" License
17.64k stars 1.69k forks source link

Add support for getting cell-embedded pictures/images #1857

Closed msackman closed 3 months ago

msackman commented 3 months ago

GetCellPicture gets a picture that is embedded within a cell. Excel itself (both standalone, and as part of Office 365) supports images being embedded within a cell, as well as images floating over one or more cells (supported by GetPictures and GetPictureCells).

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.21%. Comparing base (703b737) to head (17f2e87).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1857 +/- ## ======================================= Coverage 99.21% 99.21% ======================================= Files 32 32 Lines 23973 24031 +58 ======================================= + Hits 23784 23842 +58 Misses 101 101 Partials 88 88 ``` | [Flag](https://app.codecov.io/gh/qax-os/excelize/pull/1857/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qax-os) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qax-os/excelize/pull/1857/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qax-os) | `99.21% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qax-os#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

msackman commented 3 months ago

Thank you for looking at this. I'm not a massive fan of this API though: as a consumer of this API, I think it's important to know what type of image I'm accessing - I want to take different actions depending on whether the image is embedded within the cell, or whether it's floating. By combining all these APIs together, I'm not able to do that.

msackman commented 3 months ago

I suggest that add a new field in the Picture data structure to indicate the type of the picture.

Thank you - that would meet my use case well.

xuri commented 3 months ago

There is another way to detect if the picture is an embedded cell image: get the picture cells by the GetPictureCells function at first, then get the cell's formula function by GetCellFormula, if you got the formula error #VALUE!, which represents the image was a cell image. We can avoid introducing new fields in this way to keep the library simple and core. What do you think about it?

msackman commented 3 months ago

What do you think about it?

In general, I believe code should be as explicit, obvious, and hard-to-misinterpret as possible. Using the magical #VALUE! seems non-obvious to me -- it requires plenty of prior-knowledge and is not particularly discoverable. So if it were me, I would introduce some sort of enum-like-thing to indicate the source of the image, and add a suitable field to the Picture struct. I think that would be more obvious and easier to work with.

xuri commented 3 months ago

Thanks for your feedback. I will add support for that recently.

xuri commented 3 months ago

The new field InsertType with PictureInsertType data type has been added in the Picture structure by pull request #1864, Please upgrade to the master branch code by go get -u github.com/xuri/excelize/v2@master, this feature will be released in the next version. In addition, the GetPictures and GetPictureCells functions still don't support getting cell images inserted by the IMAGE formula function currently, I'll consider adding support for that later.

xuri commented 3 months ago

The Excelize library now support get the cell images inserted by IMAGE formula function.

msackman commented 3 months ago

The Excelize library now support get the cell images inserted by IMAGE formula function.

Awesome, thank you - yep, already using that feature. All working well.