Compact encoding reform

[frank-trampe]

I added a new unencoded glyph to rmo-triangle2.sfd from debugfonts and then tried to rename it.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7a95c1c in WordlistEscapedInputStringToRealString (sf=0xcf02d90, 
    input_const=<optimized out>, selected_out=<optimized out>, 
    getUnicodeFunc=0x7ffff7a95470 <WordlistEscapedInputStringToRealString_getFakeUnicodeAsScUnicodeEnc>, udata=0x0) at wordlistparser.c:288
288             n = sf->map->backmap[sc->orig_pos];
(gdb) where
#0  0x00007ffff7a95c1c in WordlistEscapedInputStringToRealString (
    sf=0xcf02d90, input_const=<optimized out>, selected_out=<optimized out>, 
    getUnicodeFunc=0x7ffff7a95470 <WordlistEscapedInputStringToRealString_getFakeUnicodeAsScUnicodeEnc>, udata=0x0) at wordlistparser.c:288
#1  0x00007ffff795ce5d in CV_OnCharSelectorTextChanged (g=<optimized out>, 
    e=<optimized out>) at charview.c:12636
#2  0x00007ffff69eaa53 in GTextFieldChanged (gt=<optimized out>, 
    src=<optimized out>) at gtextfield.c:209
#3  0x00007ffff69eed3d in gtextfield_mouse (event=0x7fffffffbbc0, g=0xb9b9540)
    at gtextfield.c:1789
#4  gtextfield_mouse (g=0xb9b9540, event=0x7fffffffbbc0) at gtextfield.c:1654
#5  0x00007ffff69b1780 in _GWidget_Container_eh (gw=0xcee7a30, 
    event=0x7fffffffbbc0) at gcontainer.c:302
#6  0x00007ffff69b1c9b in _GWidget_TopLevel_eh (event=0x7fffffffbbc0, 
    gw=0xcee7a30) at gcontainer.c:745
#7  _GWidget_TopLevel_eh (gw=0xcee7a30, event=0x7fffffffbbc0)
    at gcontainer.c:616
#8  0x00007ffff69fce1d in dispatchEvent (gdisp=<optimized out>, 
    event=0x7fffffffbf30) at gxdraw.c:3478
#9  0x00007ffff69fee4e in GXDrawEventLoop (gd=0x681960) at gxdraw.c:3586
#10 0x00007ffff7a7e915 in fontforge_main (argc=<optimized out>, 
    argv=<optimized out>) at startui.c:1354
#11 0x00007ffff730178d in __libc_start_main (main=0x400710 <main>, argc=1, 

[davelab6]

Please fix :)

[adrientetar]

Looks like the code I added to parse unencoded glyphs, this is the code path that parses the input string in CharView.

EDIT: rmo-triangle2.sfd indeed uses Compact encoding, edited the title to reflect the issue.

[adrientetar]

Are you using Compact encoding? Because it's something I noticed did not work with my patch but I don't think there's a way to fix it, see my comment about it (cc #1550).

[frank-trampe]

There's always a way to fix it.

[adrientetar]

What I meant is that I think it would warrant a refactoring of the Compact encoding functionality – I htink so at least, I don't know the APIs very well.

[frank-trampe]

So here's what's happening. FVAddUnencoded frees fv->normal (the cached copy of an old encoding), but sf->map also points there (since it hasn't been updated to point to fv->map). WordlistEscapedInputStringToRealString (which I think comes from @monkeyiq, who might have some insight here) tries to use sf->map, but it has been freed.

So we have a few options. We can set sf->map to fv->map only when its original target gets freed. We can stop freeing sf->normal. I don't like either of those options because it seems that anything referencing sf->map would expect a current, accurate encoding map. The other options are to make sure that sf->map always points to fv->map or to eliminate all use of sf->map outside of input and output by remapping requests to fv->map. The latter seems a more complete solution, but it's likely to be a lot more work than the former and may necessitate some large changes.

As such, I'm leaning towards making sure that sf->map always points to fv->map. The big question is whether this is always correct behavior, or, equivalently, where there are cases in which sf->map ought to point to something other than fv->map.

I'd like to have @mskala look at this thread as well.

[frank-trampe]

Ooh. It gets more interesting.

There can apparently be multiple font views open for the same splinefont, each with its own encoding maps, as evidenced by the fact that _FontViewBaseCreate will copy rather than reference the maps from the first fontview open for the given splinefont.

Does anybody know how this (having two divergent FontViews of the same SplineFont) would occur?

[ghost]

To be honest, I think the best thing is probably to refactor the handling of "Compact" encoding. As far as I can tell, its only real purpose is to change the behaviour of the GUI glyph list window. When there are slots without glyph data, turning on Compact causes the empty slots to be skipped over. This is useful to the point of necessity when one is working on a Unicode font, because there will always be long stretches of the encoding table with no glyphs. It was an unwelcome surprise for me when I found that the internal data structures change in a complicated way when Compact is turned on, far beyond the GUI. Compact-related data structure complexity was the root cause of #486 , one of my most hated bugs until it was fixed: when using "revert" to load a file with a new glyph added, the mapping that supports Compact has to change, and it was confusing the old and new mapping in a way that caused a crash. It sure would be nice if the main in-memory representation of the font could just not represent "Compact" at all, with the skipping of empty glyphs done only in the widget that displays the glyph view window. Then we wouldn't have to think about supporting Compact everywhere else.

As for divergent FontViews of the same SplineFont, I don't know what would cause that, but things to watch out for would be a sequence like creating two new fonts, renaming them to the same thing, and then doing "revert" to load them both from disk; or whether it may possibly occur in some case of loading a multi-font file like a TrueType collection.

[frank-trampe]

@mskala, I don't yet understand enough of what happens in encoding land or of the multi-view splinefont for a complete solution to the problem.

In #1663, I've tried to make sure that sf->map always points to fv->map. It's not entirely certain that this is in fact the best way to deal with the issue (even temporarily), so I'll await comments.

[davelab6]

Agree with Matthew that Compact should be a GUI only thing and the
functionality of this is flawed

[adrientetar]

Agree with mskala. (If someone works on doing it a GUI thing the same should be done to not display codepoints beyond Unicode which are invalid.)
Anyways, being the simplest thing conceptually it might not be the easiest to program but that's seemingly the clean way out that would permit recovering (does the current solution permit it?).

[ghost]

On Mon, 18 Aug 2014, Adrien Tétar wrote:

Agree with mskala. (If someone works on doing it a GUI thing the same should
be done to not display codepoints beyond Unicode which are invalid.)

I'm not sure what you're referring to there, but Unicode does go all the
way up to U+10FFFF, and it's important that FontForge should be able to
cover the whole range.

[frank-trampe]

I've spent some more time looking at the code. There are a few more bad things.

Sometimes fv->map is set to sf->map. Sometimes it is copied from sf->map. And sometimes it's completely different, due to re-encoding and addition and removal of glyphs.

fv->map and sf->map entries refer to glyphs in sf->glyphs, which are in turn used to refer back to the maps. Adding an unencoded glyph changes sf->glyphs but not sf->map (at least immediately) or the map entries for other fontviews.

This brings up another question. How did FontViewBase come to have its own divergent encoding map? Was this intentional?

This is my new proposal based upon my enhanced but still quite limited knowledge of the encoding functionality. We move the master encoding map into the SplineFont. We add an encoding flags value to FontViewBase. If no flags are set, fv->map points to fv->sf->map. If the compact flag is set, fv->map contains the output of running CompactEncMap on fv->sf->map. All adjustments happen to sf->map first. sf->map is never compact.

[adrientetar]

I'm not sure what you're referring to there, but Unicode does go all the way up to U+10FFFF

Scrolling down towards the bottom of FontView under UnicodeBmp encoding, a bunch of this error gets onto the console. So something must be wrong with that code there.

We move the master encoding map into the SplineFont.

This! What I thought was needed back when I did the debug that led to my unencoded glyphs parsing patch.

This proposal looks great and would fix a recovery hazard (#1550).

[davelab6]

I suggest @frank-trampe to postpone dealing with this until after the release... maybe next week ;)

Your plan sounds ok to me, but I suspect a more radical change is really needed...like discard everything about Compact that isn't GUI related. Then there is no compact flag to be set ;)

This might be a problem for some SFDs, but users can use the prior release to open such SFDs, uncompact them, re-save them, and move to a later build.

[frank-trampe]

We would need a compact flag in the FontViewBase, I think, in order to let it know how to update its internal encoding map when the master encoding map changes.