docx writer does not support tracked deletions anymore

[tolot27]

If I use the provided test files (track_changes_deletion_all.native or track_changes_deletion.docx in case of round-tripping) like with pandoc -s -t docx --track-changes=all track_changes_deletion_all.native -o track_changes_deletion_test.docx, the output docx contains the deletion as it was rejected and it is not marked as a tracked change anymore.

Insertions and comments were converted sucessfully.

pandoc version is 2.1.1

Maybe the test files should be added to the tests in https://github.com/jgm/pandoc/blob/master/test/Tests?

[jkr]

We discussed this yesterday in issue #4301. The --track-changes options are for the docx reader. You are using the markdown reader here. The output of --track-changes=accept and --track-changes=reject will have no track changes information, and that is what this test file tests.

If you want to put track changes information into a docx file, you have to use the output of --track-changes=all.

[jkr]

Sorry -- I misunderstood the issue. Let me check this and get back to you.

[jkr]

Confirmed -- this is indeed an issue. Note, though, that --track-changes= options only affect the docx reader. So, since this is reading native, they will have no effect here. Nevertheless, the track-changes info isn't being written to the docx, and it should be.

[jkr]

Okay -- fixed. Sorry again about the misunderstanding, and thanks for reporting the issue!

[tolot27]

Okay, thanks for the fix. What about adding a test case for it.

[jkr]

our test framework for the docx writer is a bit lacking at the moment. There's been some discussion about how to fix that.

@jgm -- this would certainly be a good argument for adding golden tests to docx, after the fashion of the pptx writer. I think this issue was introduced in 5441e11b06 (almost a year ago) and no one noticed it till now. Should I work on abstracting those tests to Test.Writers.OOXML?

[tolot27]

Note, though, that --track-changes= options only affect the docx reader.

The reason for that is not obvious to me. Since you closed #4301, is it worth to discuss this anywhere else, i. e. at pandoc-discuss?

BTW: I'm willing to support pandoc and I would like to spend time on pandoc itself rather than on lua filters, if it makes sense. As track changes were introduced some years ago and are supported by a number of formats (docx, odt, html, and even criticmarkup, even if it is not supported here) and are partialy implemented in pandoc (docx, for html see i.e. #1273 (comment)) I think it is worth to support it pandoc for these formats and support accept/reject for all the other formats (IMHO).

[jkr]

I closed #4301 because the issue there focused heavily on the documentation, which was accurate. This would probably be a better topic for pandoc-discuss, since that is not a bug (as this was), but rather a choice (that might or might not have been the right choice).

The reason that this currently requires more of an open discussion, I think, is that the current spans and divs output by --track-changes=all is quite ugly, and I don't think it is what anyone would want to be the actual way of recording changes in pandoc markdown or the AST. It's a hack that means not that "changes were inserted here," but rather "changes were inserted in a docx document here." That's why it only affects the docx reader. (That being said, the info is there if you want to write a filter around it, or around critic markup, or whatever.)

It seems that before we talk about officially having all writer write track-changes info, we'd have to decide what form we want that to take in pandoc. It's not an issue to be fixed, so much as a question of how to proceed in the future. So I think pandoc-discuss is more appropriate.

[tolot27]

It seems that before we talk about officially having all writer write track-changes info

That was not my general aim with #4301. I just wanted that the track-changes info is removed per default in all writers, which is possible with the roundtrip md -> docx -md with --track-changes=accept.

[jgm]

+++ Jesse Rosenthal [Jan 25 18 07:52 ]:

***@***.*** -- this would certainly be a good argument for adding golden tests to docx, after the fashion of the pptx writer. I think this issue was introduced in [2]5441e11 (almost a year ago) and no one noticed it till now. Should I work on abstracting those tests to Test.Writers.OOXML?

That would be great!

[jkr]

John MacFarlane <notifications@github.com> writes:

+++ Jesse Rosenthal [Jan 25 18 07:52 ]: > Should I work on abstracting those tests to > Test.Writers.OOXML? That would be great!

Okay -- just merged it as Tests.Writers.OOXML. `pptxTest` in Tests.Writers.Powerpoint is now just `ooxmlTest writePowerpoint`. As far as the docx tests go, I guess we can just use the natives in the reader test directory, though we'll want to define different names for the golden tests so we don't get them mixed up with ms-generated docx files the reader tests: ~~~ ... , docxTest "Inline formatting" def "docx/inline_formatting.native" "docx/golden/inline_formatting.docx" ... ~~~ One more issue that will have to be dealt with. It looks like the docx writer uses uniqueIds. That will make the outcome non-deterministic, which will make comparing harder. I'll take a look to see if there's any way to make it more deterministic (keeping a state list of ids) -- I didn't have to do it in pptx, but I'm not sure if that's down to a format difference. If there isn't we'll either have to look for some safe ways to give the comparison a pass, as we do with the time values in the docprops, or think about an option for passing ersatz values. Then we can just run it, and someone (who probably won't be me for the next week or so) will just have to go through the golden files one by one in office to ensure that they (a) are not corrupt, and (b) exhibit the correct behavior.

[jkr]

@jgm, yeah we can actually take care of that problem by just keeping something in state as say, stCurId :: Int, and then just redefining getUniquedId to something like

getUniqueId :: (PandocMonad m) => m String
getUniqueId = do
  n <- gets stateCurId
  modify $ \st -> st{stateCurId = stateCurId + 1}
  return n

At the moment you avoid collisions by adding 20 to the output of P.uniqueHash, so we could just start this by initializing stCurId=20. That should only leave utcTime as non-deterministic (I mean, given the input files), and we can safely ignore those in the comparisons.

[jkr]

Oh, there's also a random in mkAbstractNum

[jkr]

OK, see #4308

[jgm]

+++ Jesse Rosenthal [Jan 25 18 12:46 ]:

One more issue that will have to be dealt with. It looks like the docx writer uses uniqueIds. That will make the outcome non-deterministic, which will make comparing harder. I'll take a look to see if there's any way to make it more deterministic (keeping a state list of ids) -- I didn't have to do it in pptx, but I'm not sure if that's down to a format difference. If there isn't we'll either have to look for some safe ways to give the comparison a pass, as we do with the time values in the docprops, or think about an option for passing ersatz values.

Running the writer in PandocPure would be one way to go, I suppose, though itt requires some setup.