-
Notifications
You must be signed in to change notification settings - Fork 806
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
Add support for reading threaded comments #1475
base: develop
Are you sure you want to change the base?
Conversation
Thanks! Am I right that it doesn't save threaded comments yet? |
Thats right. Threaded comments will not be touched. |
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. |
I'm sorry, but I currently don't have the resources (time) to help with the full implementation. |
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. 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 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.
|
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. |
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. 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. |
Hi @igitur
Let me hop in with some additional explanation:
Therefore, this pull request adds support for reading text from the note in both ways with and without the element. 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 |
if anyone in need of this it got pulled to https://github.com/stesee/ClosedXML a while ago |
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.