Add support for reading threaded comments

[bernd-herzog]

Fixes #1447

Enables reading the "legacy copy of threaded comments for versions of excel that does not support threaded comments".
Writing to threaded comments is still not supported.

[igitur]

Thanks! Am I right that it doesn't save threaded comments yet?

[bernd-herzog]

Thats right. Threaded comments will not be touched.

[igitur]

Do you have the energy to fully implement this? i.e. the loading (which you have done), full API support to add/edit/delete them, and saving them back to file. We'll have to discuss the API first before you make any changes.

[bernd-herzog]

I'm sorry, but I currently don't have the resources (time) to help with the full implementation.

[rominator1983]

Microsoft Excel places the backward-compatibility-entries for the new threaded comments feature in the older variant of comments (called Notes in Excel 365). That is fine.
Unfortunately at this point they decided to omit the r-element that they have put in before (which work fine with ClosedXml).
When you use a new Excel variant (365) you can still add older comments (now called Notes). And you can see that there is still an r-element.
Looking at the standard it is very clear, that either variants should be supported.
The XSD-Type CT_Rst (comment text) could have a child-element of type t or r.

So strictly speaking Bernds PR not really implements a part of the threaded comments for ClosedXML but fixes the issue where ClosedXml does not process contents of perfectly valid, standard conformant files.

Thus I would strongly suggest to include the PR anyway until someone really needs threaded comments to work as well and has the time to implement this.
I also need threaded comments to be visible via the API in any way and am now stuck to using a build of Bernds PR to achieve that.
This is of course quite cumbersome and leaves me hanging on an older ClosedXML-version.

[rominator1983]

I had some issues in referencing the right DLL. So no issue at all with Bernds fix on my side. I will delete my previous comment.

I have some round-tripping errors when opening a document with threaded comments and saving again:

`Message:
System.Collections.Generic.KeyNotFoundException : The given key was not present in the dictionary.

Stack Trace:
Dictionary``2.get_Item(TKey key)
XLColor.get_Color()
XLWorkbook.GenerateShape(XLCell c, String shapeTypeId)
XLWorkbook.GenerateVmlDrawingPartContent(VmlDrawingPart vmlDrawingPart, XLWorksheet xlWorksheet, SaveContext context)
XLWorkbook.CreateParts(SpreadsheetDocument document)
XLWorkbook.CreatePackage(Stream stream, Boolean newStream, SpreadsheetDocumentType spreadsheetDocumentType, Boolean validate)
XLWorkbook.Save(Boolean validate)
XLWorkbook.Save()`

I don't know yet, if that has to do with the PR or if this is an error in closed xml. I will check that out.

[igitur]

Sorry all, but I can't merge a PR that only partially supports a feature. It creates too many problems and later on where people raise issues about unexpected missing behaviour. I'll consider and merge this once full support is added.

[rominator1983]

I do understand. But this raises two questions:

First: How should threaded comments be implemented in you opinion? Is there already a concept on how to go forward on this?

And second: How would you like to treat the backward-compatibility-comments MS decided to put in there? At the moment they are just not read because MS did not put an r-element around the t. If they had ClosedXml would already have read those. Distinguishing them by the absence of the r-element (which is totally fine if you look at the standard as I already pointed out) would be a rather bad choice. And remember that nothing prevents MS to put the r there too in any next update of Office 365 causing threaded comments to be partially supported in existing builds of ClosedXml.
So there should ideally be a scenario to be able to read threaded comments separately while keeping the old Comment variant as is.

Here is a snippet of the two variants of notes as they appear in a comments-part right next to each other:

    <comment ref="A1" authorId="0" shapeId="0" xr:uid="{D04F30F8-9C34-4368-9FE6-7146EE2740CC}">
      <text>
        <t>[Threaded comment]
          Your version of Excel allows you to read this threaded comment; however, any edits to it will get removed if the file is opened in a newer version of Excel. Learn more: https://go.microsoft.com/fwlink/?linkid=870924
          Comment:
          comment</t>
      </text>
    </comment>
    <comment ref="A2" authorId="1" shapeId="0" xr:uid="{B6287F9B-6216-4C92-A0E9-D332FEFB8C30}">
      <text>
        <r>
          <rPr>
            <sz val="9"/>
            <color indexed="81"/>
            <rFont val="Tahoma"/>
            <charset val="1"/>
          </rPr>
          <t>note</t>
        </r>
      </text>
    </comment>

Another choice could be to adapt the LoadOptions so developers could opt-in to the behavior of Bernds PR.

As a side note: It would have been nice to know about this a little sooner instead of letting this sit here.

[ernstscheithauer]

Hi @igitur

Sorry all, but I can't merge a PR that only partially supports a feature. It creates too many problems and later on where people raise issues about unexpected missing behaviour. I'll consider and merge this once full support is added.

Let me hop in with some additional explanation:

• Newer Excel version support threaded comments, while older versions only support the older notes
• However: As usual, Microsoft tries to keep new features backwards compatible
• In order to do that, whenever a threaded comment is added in a newer Excel version then the same text content is added as a note on that same cell as well
• Therefore, older versions of Excel show the notes and therefore show the content of the threaded content to the user
• Now when newer versions of excel put the content into the notes, the xml markup is slightly different without the tag around the .

Therefore, this pull request adds support for reading text from the note in both ways with and without the element.
This does not break any further implementation of threaded comments, since it only fixes reading the notes.

This pull request therefore makes closedXML behave like other old Excel versions that do not support threaded comments, and I would +1 for merging it.

Could you please revisit your assessment of the impact of the pull request? If there are any further questions I would be happy to answer them.

Ernst

[rominator1983]

if anyone in need of this it got pulled to https://github.com/stesee/ClosedXML a while ago