Bug 79599 - FILEOPEN FILESAVE RTF: Libreoffice produces incompatible rtf-code for text-highlighting (uses \chcbpatN instead of \highlightN)
Summary: FILEOPEN FILESAVE RTF: Libreoffice produces incompatible rtf-code for text-hi...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium normal
Assignee: Not Assigned
QA Contact:
URL:
Whiteboard: target:4.4.0 target:4.3.2
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-03 17:21 UTC by Norbert X
Modified: 2014-08-08 22:57 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments
ODT and RTF files produced by LibO and MSO 2003,2007 (16.59 KB, application/zip)
2014-06-03 17:24 UTC, Norbert X
Details
modified test file for fdo#61909 (1.30 KB, application/rtf)
2014-06-08 21:16 UTC, Norbert X
Details
patch for fdo#79599 (2.08 KB, patch)
2014-06-10 09:16 UTC, Norbert X
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert X 2014-06-03 17:21:26 UTC
It seems that this bug has very long history. 
It is related to bug 37540 and bug 64490.

In this bugreport I summarize Romano Giannetti's comments.

Part 1. From ODT to RTF (LibO as writer, MSO as reader)

  Let's start with creating simple ODT document.
  The document will contain three letters 'T', 'x' and 'T' (without spaces, tabs and delimeters at one paragraph). Letter 'x' is highlighted by Highligting button (default yellow color).

  I use LibreOffice from Ubuntu Precise PPA (package 4.2.4~rc2-0ubuntu1~precise3 ), MSO 2003 SP0, MSO 2007 SP3.

  The original ODT document is named 'highlight_lo.odt'.
  Then I opened ODT document and saved it as RTF file - 'highlight_lo.rtf'.
  If I open 'highlight_lo.rtf' file with MSO Word 2003 or 2007 I can't remove yellow highlight.

  If I open 'highlight_lo.rtf' with 'gedit' I see the following code sequence about yellow 'x': {\chcbpat2\rtlch \ltrch\loch x}
  According to RTF-file specification [1] \chcbpat is a part of <chshading> control word (controls character borders and shading, see p.142).
  But (as Romano Giannetti suggested earlier) the correct code for higlighting is \highlightN (see p.145).
  After manual replacing of 'chcbpat' to 'highlight' I get editable RTF document - it is named 'highlight_lo_fixed.rtf'. The resulting document became editable by MSO 2003 and 2007.
  So for comfortable document exchange LibO should use '\highlightN' instead of '\chcbpatN'.
  But there is a problem - LibO does not show 'x' with yellow background.


Part 2. Reading RTF in LibO (MSO 2003 and 2007 as writers, LibO as reader)

  2a. I created document with the same content and appearance in MSO 2003. It is named 'highlight_2k3.rtf'.
  I opened this document in LibO and the 'x' is not highlighted. If I open this document in 'gedit' the code sequence for 'x' is:
{\lang1033\langfe1049\highlight7\langnp1033\insrsid11681501\charrsid11681501 x}, so it contains '\highlight7', but not '\chcbpat7'.

  2b. I created document with the same content and appearance in MSO 2007. It is named 'highlight_2k7.rtf'.
  I opened this document in LibO and the 'x' is not highlighted. If I open this document in 'gedit' the code sequence for 'x' is:
{\rtlch\fcs1 \af31507 \ltrch\fcs0 \lang1033\langfe1033\highlight7\langnp1033\insrsid2260637\charrsid2260637 x}, so it contains '\highlight7', but not '\chcbpat7'.


Conclusion.
  For normal RTF-document exchange between MSO and LibO users LibO should use and support '\highlightN' control sequence instead of '\chcbpatN'.

1. http://www.microsoft.com/downloads/details.aspx?FamilyId=DD422B8D-FF06-4207-B476-6B5396A18A2B&displaylang=en
Comment 1 Norbert X 2014-06-03 17:24:21 UTC
Created attachment 100365 [details]
ODT and RTF files produced by LibO and MSO 2003,2007
Comment 2 Norbert X 2014-06-03 19:15:29 UTC
As a small note.
I performed the aforementioned tests with Abiword (version 2.9.2 in Ubuntu Precise).

The results are the following:
Part 1 - Abiword uses '\highlightN' for character background, so RTF-document exchange between Abiword and MSO 2003,2007 works as expected.
Part 2 - Abiword is able to remove highlight from documents which are produced in MSO 2003 and MSO 2007.

Also I tested Caligra Words 2.4.0:
  it does not render background in RTF file ('highlight_lo.rtf', produced by LibO), 
  it renders background normally in RTF files ('highlight_2k3.rtf', 'highlight_2k7.rtf', produced by MSO 2003 and 2007) and is able to remove highlight
Comment 3 Yousuf Philips (jay) 2014-06-04 22:02:30 UTC
Confirmed in Linux Mint in 3.3.0, 4.2.4 and 4.3 beta. Confirmed against Kingsoft Writer and Word 2013.
Comment 4 Norbert X 2014-06-05 19:03:32 UTC
I did checkout of sources from git,
found that RTF-export may be fixed
by changing line 2237 of file sw/source/filter/ww8/rtfattributeoutput.cxx (http://cgit.freedesktop.org/libreoffice/core/tree/sw/source/filter/ww8/rtfattributeoutput.cxx?id=655377e90c57bb68e5000f06f76531baf9eeaa37#n2237).

If I replace 

  m_aStyles.append(OOO_STRING_SVTOOLS_RTF_CHCBPAT);

with

  m_aStyles.append(OOO_STRING_SVTOOLS_RTF_HIGHLIGHT);

and recompile, I get RTF-file  which may be normally edited by MSO (and possible others, because of \highlight controlword instead of \chcbpat).

But if I open this RTF-file with patched LibreOffice it does not render highlight.

Should I change something in editeng/source/rtf/rtfitem.cxx and/or writerfilter/source/rtftok/rtfdocumentimpl.cxx ? Any ideas ?
Comment 5 Michael Meeks 2014-06-06 10:01:21 UTC
Miklos any mentoring thoughts ? =) [ rather than fixing it yourself ]. I guess he'd like some code pointers.
Comment 6 Miklos Vajna 2014-06-06 10:07:44 UTC
editeng is not related to the Writer RTF import/export, as you already found, the relevant import part is writerfilter/source/rtftok/rtfdocumentimpl.cxx, and the relevant export part is sw/source/filter/ww8/rtfattributeoutput.cxx.

It's kind of sad that Word has this stupid duplication of character background and highlighting, but sure, if that helps the life of Word users, then we can map our character background to Word's highlight, and not its character background.

\highlight is already handled here:

http://opengrok.libreoffice.org/xref/core/writerfilter/source/rtftok/rtfdocumentimpl.cxx#3646

If you could check why it's not working the way you would expect it, that would be great. :-)
Comment 7 Norbert X 2014-06-08 21:16:18 UTC
Created attachment 100688 [details]
modified test file for fdo#61909

Thank you Michael and Miklos!

I'm playing with RTF-import part and it seems there is something wrong in DomainMaper (writerfilter/source/dmapper/DomainMapper.cxx), it does not show highlighting in document - see NS_ooxml::LN_EG_RPrBase_highlight at http://opengrok.libreoffice.org/xref/core/writerfilter/source/dmapper/DomainMapper.cxx#1336 .

I installed KDevelop, configured source code for debugging but I have no idea how to debug this. It looks so complicated.

While working on this bug I found previous bug 61909 and what is interesting - the test file ( http://cgit.freedesktop.org/libreoffice/core/plain/sw/qa/extras/rtfimport/data/fdo61909.rtf ) have white highlight in RTF-markup, so highlight is invisible ( \highlight11 is white=rgb(255,255,255) ).
I modified it (used all colors from color table, see fdo61909_mod.rtf) - all paragraphs have no highlight.
I also compliled old libreoffice (commit http://cgit.freedesktop.org/libreoffice/core/commit/?id=65f42760736d656b4999aa8830cb4f44f6d4e718&h=libreoffice-4-0 , which is related to bug 61909) and there is no higlight too.
Comment 8 Norbert X 2014-06-10 09:16:43 UTC
Created attachment 100793 [details]
patch for fdo#79599

It seems that I solved the problem.

For now LibO Writer do not produce \chcbpat (uses \highlight instead of it).
And what is important - MS Word 2003 and 2007 can remove produced highlight.

Some notes:
  DomainMapper had wrong logic before.
  nIntValue contains full color (0xRRGGBB), so getColorFromIndex is not needed.
  For now PROP_CHAR_HIGHLIGHT is not used.
Comment 9 Norbert X 2014-06-12 14:37:43 UTC
Comment on attachment 100793 [details]
patch for fdo#79599

This patch breaks some unit-tests, so I mark it as obsolete.
Comment 10 Norbert X 2014-06-13 17:11:07 UTC
Changes pushed to gerrit:
https://gerrit.libreoffice.org/9776 , https://gerrit.libreoffice.org/9777 ,  https://gerrit.libreoffice.org/9778 .

With these three commits RTF higlighting works as expected and allows users to exchange RTF-documents between MS Word and LibreOffice.
Comment 11 Yousuf Philips (jay) 2014-06-13 20:21:03 UTC
Hi nrbrtx,

Look forward to testing your patches once they are merged into the daily builds. Do you plan to stick around and help out more with LibreOffice bugs?
Comment 12 Norbert X 2014-06-14 08:14:49 UTC
Hi, Jay!

I hope I can help out more. 
Now I'm trying to understand and fix the most annoying bug 64490.
Comment 13 Yousuf Philips (jay) 2014-06-14 15:02:42 UTC
Hi nrbrtx,

Seems this issue has been bugging you for the last year, as the other bug is a duplicate of this one for doc and docx. Hope it turns out as fruitful as this one :)

Note: it would be nice if you added your name or nickname, so i dont have to refer to you by your email user id. You can change it by visiting < https://bugs.freedesktop.org/userprefs.cgi?tab=account > and adding something to the 'Your real name' field. :)
Comment 14 Commit Notification 2014-06-19 19:52:06 UTC
nrbrtx@gmail.com committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7eafd8ccac56d7503b4287dfa3acac2cf0560b20

fdo#79599: use \highlightN instead of \chcbpatN in RTF import and export



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 15 Norbert X 2014-06-20 10:09:09 UTC
Tested on Windows  (
Version: 4.4.0.0.alpha0+
Build ID: 67f64c266cd5a0368eff0be727228bc6a9c67cb2
TinderBox: Win-x86@42, Branch:master, Time: 2014-06-20_01:28:13
) - the bug is fixed.

Is it possible to push this changes to all not-EOL versions of LibO (4.2, 4.3)? Who can perform this push?
Comment 16 tommy27 2014-07-08 00:47:59 UTC
Thanks 4 your patch. I set status to FIXED.
regarding 4.2.x and 4.3.x backport you should ask in the developer mailing list how to ask a patch review.
Comment 17 Yousuf Philips (jay) 2014-08-06 02:40:52 UTC
Hey Norbert,

Any chances for 4.3 backport as i just came across an RTF today which wouldnt load the highlight. :)
Comment 18 Adolfo Jayme 2014-08-06 18:09:31 UTC
You can use the Cherry Pick button in Gerrit to apply a patch to a different branch.
Comment 19 Miklos Vajna 2014-08-07 14:14:43 UTC
(In reply to comment #18)
> You can use the Cherry Pick button in Gerrit to apply a patch to a different
> branch.

Please never do that or suggest others to do so. If you use the cherry-pick button, then you'll create a review that you haven't even build-tested, if something breaks people will learn that your patches are under-tested and it'll just commit to a bad reputation...

Thanks! :-)
Comment 20 Norbert X 2014-08-07 18:21:26 UTC
Thank you, Adolfo Jayme! 
Thank you, Miklos Vajna and sberg from IRC!

Cherry-picked locally, tested and pushed to LibO 4.3 branch,
Gerrit - https://gerrit.libreoffice.org/#/c/10813/ (previously reviewed https://gerrit.libreoffice.org/#/c/9776/).
Comment 21 Commit Notification 2014-08-08 19:31:57 UTC
nrbrtx@gmail.com committed a patch related to this issue.
It has been pushed to "libreoffice-4-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=22731ba9e15978ab1d1ed98a29d88431cf674257&h=libreoffice-4-3

fdo#79599: use \highlightN instead of \chcbpatN in RTF import and export


It will be available in LibreOffice 4.3.2.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 22 Adolfo Jayme 2014-08-08 22:57:46 UTC
(In reply to comment #19)
> Please never do that or suggest others to do so. If you use the cherry-pick
> button, then you'll create a review that you haven't even build-tested

Fine, then that button is only for obvious, tiny changes :-) otherwise I do not understand why it was added in the first place...