unoconv / unoserver

MIT License
496 stars 69 forks source link

New feature: unocompare command for comparing documents #51

Closed bvarga91 closed 1 year ago

bvarga91 commented 1 year ago

Compare two documents and export the result to any format. Can be used likewise the unoserver or unoconvert. More details in README.

regebro commented 1 year ago

Interesting. Could I get some tests for this? (And the code should pass black)

bvarga91 commented 1 year ago

Interesting. Could I get some tests for this? (And the code should pass black)

Hi, Unfortunately, I won't have time to write a test for it in the near future. To test it, just enough to use two different .odt file and compare them with the new unocompare command and open the exported .pdf file. (The code should pass black with the update. :) )

bvarga91 commented 1 year ago

Dear Regebro,

Can we run a black again on this pull request? And if it is pass is it possible to merge or the tests are necessary, by all means?

regebro commented 1 year ago

I tested it out a bit more, and I think it only works on writer documents, right? It should probably raise an error message for document types that can't actually be compared.

bvarga91 commented 1 year ago

Thank you very much for the testing, Regebro.

The comparing feature only works with writer and calc documents (just like in LibreOffice). In case of Calc documents in LibreOffice, there is no diff, like in case of writer documents, but differences can be rejected and accapted and then can be exported to *.pdf. So technically the comparing works both writer and calc documents.

In case of other documents type we already have a raise with an error message. https://github.com/unoconv/unoserver/pull/51/commits/e7c185744e157d1b9ac957a22ef83dc0cdd03c15#diff-6c677a78f83a01327881ee7d7149546d18797f88fb0c99e271207375cd6ab6b8R39

regebro commented 1 year ago

I don't think I understand how it's supposed to work with calc documents. All I get is the original. When I do "compare" through the UI I can accept and reject changes, sure, but this is a command line program, so nothing happens.

bvarga91 commented 1 year ago

"When I do "compare" through the UI I can accept and reject changes," --> Yes, thats true and also you can click just 'cancel' without accept and reject. After clicking on the 'cancel' we can see all the diffs/tracked changes in case of writer documents (then we export it to .pdf in this command line app), but in case of calc documents we cannot see any diffs/tracked changes after just clicking on the 'cancel', however in writer we have we have the usefull diff what we export to .pdf.

In case of Calc we just export the untracked document to *.pdf after clicing on the cancel. (Both in LibreOffice and this command line app) So its not useful in case of Calc documents, I would say, but at least this compare feature is supported by LibreOffice. Thats why I left "com.sun.star.sheet.SpreadsheetDocument" in the supported list. In case of Impress, Draw, etc, it is not supported at all.

Anyway, since in case of Calc it is not very useful, I can remove it from the list, or add a different raise message in case of Calc documents, if you deem it necessary. :)

What would you prefer then? :)

regebro commented 1 year ago

Yeah, I think we should raise an error message for Calc documents, the current behavior is confusing.

bvarga91 commented 1 year ago

All right. Thanks for the feedback. In that case I will remove the "com.sun.star.sheet.SpreadsheetDocument" from the doc list.

bvarga91 commented 1 year ago

Is this error (1 failing check: note: This error originates from a subprocess, and is likely not a problem with pip.) related to the feature code or is it independent of it? Should I modify something?

bvarga91 commented 1 year ago

Hi Regebro,

Is it possible to merge this new feature in the future, or is there still missing something and should I complete?

Thank you very much for your help. :)

Best, Balázs

regebro commented 1 year ago

If you could fix that black formatting error that would be fantastic, then I can just push the button.

bvarga91 commented 1 year ago

Thank you very much for the reply Regebro. :) I am trying, but somehow it looks like on my local Ubuntu the black check is green.

Some command what I tried in the source folder of unoconv/unoserver:

bvarga@bvarga-VirtualBox:~/unoserver/unoserver/src/unoserver$ black --version black, 22.10.0 (compiled: yes) Python (CPython) 3.10.6 bvarga@bvarga-VirtualBox:~/unoserver/unoserver/src/unoserver$ black . --check All done! ✨ 🍰 ✨ 4 files would be left unchanged. bvarga@bvarga-VirtualBox:~/unoserver/unoserver/src/unoserver$ black comparer.py All done! ✨ 🍰 ✨ 1 file left unchanged. bvarga@bvarga-VirtualBox:~/unoserver/unoserver/src/unoserver$

Any suggestion maybe? :) Thanks in advance.

regebro commented 1 year ago

Probably a version issue.

Edit: Nope, that version of black will definitely make changes to that file, so that's strange.

regebro commented 1 year ago

OK, merged

bvarga91 commented 1 year ago

Thanks a lot Regebro, for the reviews, suggestions and then the merge. :)