Bug 66283 - phantom and stylistic commands generate useless elements
Summary: phantom and stylistic commands generate useless elements
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Formula Editor (show other bugs)
Version:
(earliest affected)
4.2.0.0.alpha0+ Master
Hardware: All All
: medium normal
Assignee: Frédéric Wang
URL:
Whiteboard: target:4.2.0
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-27 21:43 UTC by Frédéric Wang
Modified: 2013-07-02 23:47 UTC (History)
1 user (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang 2013-06-27 21:43:22 UTC
Testcases: 

"phantom w" generates

  <mphantom>
   <mstyle>
    <mrow>
     <mi>x</mi>
    </mrow>
   </mstyle>
  </mphantom>

(mstyle and mrow are useless here)

"bold x" generates

  <mstyle mathvariant="bold">
   <mrow>
    <mi>x</mi>
   </mrow>
  </mstyle>

(mrow is useless here, same occurs with italic, size and font commands)

This is not a too serious issue since these elements won't have any effect on the rendering, but that would be good to avoid increasing the tree depth unnecessarily. I haven't looked into the code to see how easy it will be.
Comment 1 Frédéric Wang 2013-06-27 21:55:32 UTC
Mass changes to assign bugs to myself.
Comment 2 Frédéric Wang 2013-06-30 15:48:32 UTC
I've submitted a patch for review that should fix the issues given in comment 0:

https://gerrit.libreoffice.org/#/c/4634/

It seems that the old code tried to parse the two font properties of e.g. "phantom bold x" at once (i.e. with one call to ExportFont) but that didn't work and generated

   <mphantom>
    <mstyle>
     <mrow>
      <mstyle mathvariant="bold">
       <mrow>
        <mi>x</mi>
       </mrow>
      </mstyle>
     </mrow>
    </mstyle>
   </mphantom>

(two calls to ExportFont). With the new cold, I kept two calls to ExportFont but the useless mrow and mstyle elements are removed so you simply get

  <mphantom>
   <mstyle mathvariant="bold">
    <mi>x</mi>
   </mstyle>
  </mphantom>
Comment 3 ⁨خالد حسني⁩ 2013-06-30 18:08:21 UTC
I didn’t check the code (I trust you understanding it much better than me), but a thought just occurred to me; do this (and other recent changes) affect re-importing MathML back to StarMath syntax? The <semantics> tag would mask it, but just in case of third party tools stripping it.
Comment 4 Frédéric Wang 2013-06-30 19:59:17 UTC
(In reply to comment #3)
> I didn’t check the code (I trust you understanding it much better than me),
> but a thought just occurred to me; do this (and other recent changes) affect
> re-importing MathML back to StarMath syntax? The <semantics> tag would mask
> it, but just in case of third party tools stripping it.

I tested a bit the MathML import at the beginning and:

- It seemed quite bad. This was confirmed when I read the source later. So I suspect at the moment it is unlikely that anyone will be able to successfully use this feature for anything but simple markup. In particular probably not with MathML markup generated by other tools. There are many MathML constructions that can not be converted to the current StarMath syntax anyway.

- Math is even not able to import its own generated MathML code when the annotation is removed. Or it may import something that is not completely equivalent (I guess it won't be possible to regenerate the exact StarMath source anyway). For example the <mstyle> element used here are just ignored by the MathML import at the moment so you loose all colors and font styles.

In this particular case, the bug mentioned in the comment is about empty <mrow> used as parameters like "sum to {}{}" where the parameters can be lost if empty <mrow>'s are not exported to encode the "{}". Removing the useless elements mentioned in this bug won't change anything to this problem, since they don't correspond to anything in the StarMath source. Anyway, importing empty <mrow>'s just seems broken at the moment (although it no longer crashes) so I don't think we should worry about that. 

(note: I mean with a release build when I talk about the "current" situation)

In general, I added new elements/attributes that the MathML import don't know at all, so info are no longer lost at export but are certainly still lost at import. It would be worth checking if I haven't broken anything that used to be importable.

I'm not really willing to spend too much time on the MathML import since I don't think it is really used by people (I mean for MathML *without* the StarMath annotation) and it would be best to work directly on a MathML representation in the future.
Comment 5 ⁨خالد حسني⁩ 2013-06-30 20:17:31 UTC
That is pretty much my suspicion, so I don’t think we are making it any worse.
Comment 6 Commit Notification 2013-07-02 23:36:29 UTC
Frederic Wang committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6f86d931b4266d00fec75c0124ac9fc3026a4f1b

 fdo#66283 - MathML export: remove useless mrow/mstyle with font commands



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.