Bug 118884 - Unary/Binary Operators glyph rendering initially misplaced
Summary: Unary/Binary Operators glyph rendering initially misplaced
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Formula Editor (show other bugs)
Version:
(earliest affected)
6.1.0.1 rc
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: CommonSalLayout-refactoring-regressions
  Show dependency treegraph
 
Reported: 2018-07-22 14:31 UTC by Telesto
Modified: 2019-04-25 15:14 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments
Screenshot (27.46 KB, image/png)
2018-07-22 14:31 UTC, Telesto
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Telesto 2018-07-22 14:31:28 UTC
Description:
Urnary/Binary Operators glyph rendering initially misplaced. Follow up of bug 117534

Steps to Reproduce:
1. Launch LibreOffice Start Center
2. Open Math Formula. Notice that the glyph aren't placed correctly. 
3. Switch to relations & back to Urnary/Binary Operators (everything is fine)


Actual Results:
Glyph are not placed correctly. 


Expected Results:
Should be OK when opening


Reproducible: Always


User Profile Reset: No



Additional Info:
Found in
Version: 6.2.0.0.alpha0+
Build ID: fa881095bc62c3646406c82a98d8503377288a54
CPU threads: 4; OS: Windows 6.3; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-07-22_03:27:00
Locale: nl-NL (nl_NL); Calc: CL
Comment 1 Telesto 2018-07-22 14:31:51 UTC
Created attachment 143686 [details]
Screenshot
Comment 2 Roman Kuznetsov 2018-07-22 16:23:51 UTC
confirmed

Version: 6.2.0.0.alpha0+ (x64)
Build ID: fa881095bc62c3646406c82a98d8503377288a54
CPU threads: 4; OS: Windows 10.0; UI render: GL; 
TinderBox: Win-x86_64@42, Branch:master, Time: 2018-07-22_04:57:13
Locale: ru-RU (ru_RU); Calc: CL
Comment 3 Telesto 2018-07-23 09:03:11 UTC
Looks like a CommonSalLayout-refactoring-regressions to me
Comment 4 Jacques Guilleron 2018-07-23 15:12:16 UTC
Hi Telesto, Kompilainenn,

I confirm for Unary/Binary Operators.
Operator Brackets are also perturmed in Elements with
LO 6.2.0.0.alpha0+ Build ID: fa881095bc62c3646406c82a98d8503377288a54
CPU threads: 2; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-07-22_03:27:00
Locale: fr-FR (fr_FR); Calc: CL
By opening and closing it three times, the issue seems to be fixed, but the sixths
item remains misplaced.
I also reproduce with
LO 6.1.0.0.beta1 Build ID: 8c76dfe1284e211954c30f219b3a38dcdd82f8a0
Threads CPU : 2; OS : Windows 6.1; UI Render : par défaut; 
Locale : fr-FR (fr_FR); Calc: CL
but not with
LO 6.1.0.0.alpha1 Build ID: cb47f0d320994e001bc38dc2ee9b7d957b15e6ab
Threads CPU : 2; OS : Windows 6.1; UI Render : par défaut; 
Locale : fr-FR (fr_FR); Calc: CL
Comment 5 V Stuart Foote 2018-07-23 15:32:00 UTC
IIUC, this has nothing to do with the CommonSalLayout-refactoring-regression. The correct font is selected and rendered.

Rather, this is an issue with sm node layout for the element previews. Seems that the whole node is not being generated on creation--requiring a second, or third parsing of the element nodes for the preview.

For example--

1. relaucnh of LO
2. open the sm Formula editor
3. select the elements drop list

-- nodes of Unary/Binary Operators are malformed

4. advance DL to Relations and back to Unary/Binary Operators

-- nodes of Unary/Binary Operators are fully formed

5. advance DL to Brackets

-- nodes for the Scalable brackets are malformed
a.) Round Brackets (scalable) -- left is too tall, right extends out of section, and incorrect spacing to other node elements

b.) Braces Top (scalable), Braces Bottom (scalable) both have incorrect spacing to other node elements

6. advance DL to Formats and back to Brackets

-- nodes for scalable Braces Top & Braces Bottom now correctly formatted; but node for Round Brackets (scalable) is not fully correct. I.e. while left and right are correctly scaled, the position of elements including placement of right remains wrong for the node preview.

7. advance DL to Formats and back to Brackets

-- node for Round Brackets (scalable) is now fully correct

Believe this multi-pass composition of nodes for the element previews is burried in the way the sm module handles its node composition, and is not wrapped up in the font handling.
Comment 6 V Stuart Foote 2018-09-08 03:23:28 UTC
A notable improvement on Windows build 2018-09-08 vs 2018-09-07 - the sm nodes are no longer chaotic when moving the Elements drop list up and down its panels.

With the 9a9b81c7212fa6a6762246593acf3f1950677a22 build, all the nodes are now well proportioned and placed.

The see also bug 119302 for the scalable brackets is also better with the 9a9b81c7212fa6a6762246593acf3f1950677a22 build

Seems fixed... 

=-ref-=
Testing Windows 10 Pro 64-bit en-US with
Version: 6.2.0.0.alpha0+ (x64)
Build ID: 9a9b81c7212fa6a6762246593acf3f1950677a22
CPU threads: 8; OS: Windows 10.0; UI render: GL; 
TinderBox: Win-x86_64@42, Branch:master, Time: 2018-09-07_23:40:38
Locale: en-US (en_US); Calc: threaded

compared to same with
Version: 6.2.0.0.alpha0+ (x64)
Build ID: be5e7fed5f8dc4ba95b890d8b364a8be97fa2739
CPU threads: 8; OS: Windows 10.0; UI render: GL; 
TinderBox: Win-x86_64@42, Branch:master, Time: 2018-09-07_03:18:06
Locale: en-US (en_US); Calc: threaded

these two commits are in 9a9b81c7212fa6a6762246593acf3f1950677a22
   https://gerrit.libreoffice.org/#/c/60092/
   https://gerrit.libreoffice.org/#/c/60093/

This earlier commit was in be5e7fed5f8dc4ba95b890d8b364a8be97fa2739, but the sm nodes there were not fully stable
   https://gerrit.libreoffice.org/#/c/60091/
Comment 7 Jan-Marek Glogowski 2018-09-08 04:34:42 UTC
The primary fix is https://gerrit.libreoffice.org/#/c/60091/. I'm not sure why the old code was working at all.

The problem was the completely ignored nFallbackLevel of the glyphs.

For the rendering of the symbols, we do

1. set fallback 0 font OpenSymbol
2. set current scale factor to font 0
3. we render the glyphs to the cache

<something happens here - probably some normal glyphs are rendered which>

1. sets fallback 1 some normal font, as we can't render normal text with the Symbol font
2. set current scale factor to font 1

now we ask for the glyph bounding boxes of our symbols

The old code was wrong here, as it
1. used the wrong "current scale"
2. ignored the fallback level of the glyphs

This is why the glyph were rendered correct, but overlapping, because the non-Symbol fonts characters with the same glyph id are overall not so width.

Sometimes you were able to get the correct symbol rendered in the math window, but when you moved the cursor in the editor to the different parts of the formular, you could see the highlight frame was smaller then the glyph.
Took me a while to figure all this out...

This global "current scale factor" was always wrong. Probably before we were lucky and didn't get a fallback font. I don't know.

So the patch moves the scaling factor to the font object, where it belongs - no more global var.
And now, when we ask for the glyph bounding boxes, we check for the glyphs' fallback level, eventually change the font and use the correct scaling to get the correct bounding box.

https://gerrit.libreoffice.org/#/c/60093/ might also be a fix. Font lookup depends on the SalGraphics in strange way and not just the HDC (look at ImplDoSetFont). At least this moves the real lookup into the LogicalFontInstance, where it belongs.
Comment 8 Jan-Marek Glogowski 2018-09-08 04:49:00 UTC
And https://gerrit.libreoffice.org/#/c/60092/ is just cleanup patch to get rid of a now duplicated HFONT life-cycle management. The original version had a bug. I couldn't debug it, because of a memory corruption unrelated to my patch, with very "funny" failing assertions.

That resulted in https://gerrit.libreoffice.org/#/c/60162/. I just found two occurrences directly related to my failing unit test, but some grep found more and reviewing more code around the first hits revealed a lot more.

And this is just the result from a straight-forward grep over the code base. Who knows how many of these std:unique_ptr<...[]> array delete[] bugs are still there undetected. I'm just wondering why none of our static analysis tool-chains caught these earlier... build without online-updater?