Skip to content

GDraw and Charset cleanup#4664

Merged
jtanx merged 19 commits intofontforge:masterfrom
jtanx:gwc
Mar 31, 2021
Merged

GDraw and Charset cleanup#4664
jtanx merged 19 commits intofontforge:masterfrom
jtanx:gwc

Conversation

@jtanx
Copy link
Copy Markdown
Contributor

@jtanx jtanx commented Mar 13, 2021

This includes multiple changes, probably best viewed commit by commit. Primary focus is:

  • Removing unused code/functions from GDraw to make future porting easier
  • Working towards making GDraw only do paint commands in an expose event. Far from complete, not sure it ever will. But this starts with the removal of GDrawSetDifferenceMode and changes to how the cursor is rendered - previously would XOR the display where the cursor was on a timer, it now requests an expose/redraw for the cursor area
  • Getting rid of the custom charset conversions in favour if iconv (which is already a hard requirement). Shaves of ~2MB, for a rarely used feature(?) nowadays. I'm also fairly sure the conversion tables weren't always quite correct, from my ad-hoc testing.

There are a couple of gdraw related bug fixes too, related to keeping the palettes window visible when undocked.

Closes #4334

Type of change

  • Bug fix
  • New feature
  • Non-breaking change

jtanx added 17 commits March 7, 2021 16:00
It's completely dead code, the charview palettes are managed entirely
outside of these provided functions.
Use GDrawSetWindowTitles8/GDrawGetWindowTitle8 instead.
Makes undocked palettes windows transient to the main window so that they
stay on top even when unfocused. This used to work on Windows until a point
release change of GDK 3.
When a window has no child windows, it was returning the window's
exposable area, instead of intersecting that with the current
expose event's region.
Comment thread fontforge/parsettf.c
if ( enc==0 ) enc = -1;
} else if ( enc>0x100 )
enc = badencoding(info);
// Apart from these, cp932 is a strict superset of sjis
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section may be the most contentious part, using iconv for reading the cmap encoding. But at least for SJIS, I think this is actually better. There are some encoding differences for gb2312/big5/wansung/johab, but it's hard to say if it's for the better or worse.

From testing purely the conversion routines, our custom rolled conversion tables were spitting out garbage conversions for at least some slots, but I don't know if that had any effect in practice. Also I guess iconv support may not be equal everywhere. But I believe we're already using iconv for everything else as-is (particularly for re-encoding).

This was/is really hard to test, non-unicode encoded fonts are so rare today.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using iconv is better because at least font developers know what iconv's choices are and what to look out for. Using our own tables, based just on George's opinions, makes things more confusing. So I agree with this change.

Comment thread fontforgeexe/prefs.c
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Mar 13, 2021

This pull request introduces 6 alerts when merging df1f76b into 52e8432 - view on LGTM.com

new alerts:

  • 6 for Comparison result is always the same

Comment thread fontforge/parsettf.c Outdated
Comment thread fontforgeexe/prefs.c
@skef
Copy link
Copy Markdown
Contributor

skef commented Mar 26, 2021

Looks like we're having Windows build problems.

@jtanx
Copy link
Copy Markdown
Contributor Author

jtanx commented Mar 26, 2021

Yeah, I'm looking into it (msys2/MINGW-packages#8186 (comment))

Copy link
Copy Markdown
Contributor

@skef skef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thought I had approved this. Doing that now.

@jtanx
Copy link
Copy Markdown
Contributor Author

jtanx commented Mar 31, 2021

Looks like the cmake release is at least another week away so for now I've just pinned back to cmake 3.19 (fontforge/fontforgebuilds@c0067e1).

Will rerun the other builds now.

@jtanx jtanx merged commit 0c3685f into fontforge:master Mar 31, 2021
@jtanx jtanx deleted the gwc branch March 31, 2021 08:04
Omnikron13 pushed a commit to Omnikron13/fontforge that referenced this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing the character builder

3 participants