Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIN32SS][FONT] Fix the system logical stock font data #709

Merged
merged 17 commits into from Aug 10, 2018
Merged

[WIN32SS][FONT] Fix the system logical stock font data #709

merged 17 commits into from Aug 10, 2018

Conversation

katahiromz
Copy link
Contributor

@katahiromz katahiromz commented Jul 28, 2018

Purpose

JIRA issue: CORE-14885
This PR will fix the stock font data to improve font selection.

@katahiromz katahiromz changed the title [WIN32SS][FONT] Fix stock fonts [WIN32SS][FONT] Fix the stock fonts Jul 28, 2018
@katahiromz katahiromz changed the title [WIN32SS][FONT] Fix the stock fonts [WIN32SS][FONT] Fix the system stock font datas Jul 28, 2018
@katahiromz katahiromz changed the title [WIN32SS][FONT] Fix the system stock font datas [WIN32SS][FONT] Fix the system stock fonts Jul 28, 2018
@JoachimHenze
Copy link
Contributor

JoachimHenze commented Jul 28, 2018

Some observations when applying PR-709 as is on top of 0.4.10-dev-370-g8f44035:
-hell does not come down to earth, even when applying this on its own right here and right now.

patch has no visible effects for me on:
-the fi/fli issue CORE-12091 can not longer be observed with Tahoma 12 in Wordpad (but it's still there, we just have to use smaller font sizes in Wordpad to make it happen atm)
-msi dialogs are still too large CORE-13161 and Total Commander fonts CORE-11620 are still too large, which means the freetype.c changes within https://jira.reactos.org/secure/attachment/47727/47727_CORE-11620-totalCommanderFontSizePatch.patch is what actually fixes those.
-CORE-13211 (the right arrows) are still present
-Abiword 2.6.8 font-size issues, text overflow in about tab 'others', font size in about tab is suspiciously small, is still present
-CORE-14778 (Python idle fonts) is still present

patch will influence:
-the freetype.c changes applied on its own do lead to regressions (in rapps) but 'fix system stock fonts' commit1-changes will mitigate those. that's what I consider the beneficial part of PR-709 from testers pov

If others think PR-709 is correct from technical POV (and more like 2k3), then I think it's reasonable to commit it. I did not check that part myself!

@katahiromz
Copy link
Contributor Author

My test program I made:
https://jira.reactos.org/secure/attachment/47775/StockFontsTest.zip

THE TEST RESULTS:
Windows 2003 English:
w2k3-english

Windows 10 Japanese:
w10-japanese

@katahiromz
Copy link
Contributor Author

Windows XP Japanese:
winxp-japaneses

Windows 7 Japanese:
win7-japanese

@katahiromz katahiromz changed the title [WIN32SS][FONT] Fix the system stock fonts [WIN32SS][FONT] Fix the system logical stock font data Jul 29, 2018
@katahiromz
Copy link
Contributor Author

Could you try this test program in Russian Windows 2003 or XP?

@JoachimHenze
Copy link
Contributor

I have no russian OS available, sorry.

@katahiromz
Copy link
Contributor Author

Thanks.

Win2007 Home Server Russian:
win2007-home-server-russian

@katahiromz
Copy link
Contributor Author

Thanks!

Windows XP x64 SP2 English with integrated Russian MUI:
ada

@katahiromz
Copy link
Contributor Author

katahiromz commented Aug 8, 2018

OEM_FIXED_FONT is SHIFTJIS_CHARSET or OEM_CHARSET.
ANSI_FIXED_FONT and ANSI_VAR_FONT are ANSI_CHARSET.
SYSTEM_FONT, SYSTEM_FIXED_FONT and DEFAULT_GUI_FONT are the charset of the user language.

@katahiromz
Copy link
Contributor Author

It seems like these data are changeable by the user language. Not fixed data.

@katahiromz
Copy link
Contributor Author

@HBelusca: What's your opinion about these screenshots?

@HBelusca
Copy link
Contributor

HBelusca commented Aug 9, 2018

@katahiromz: They look good; I suppose the "discrepancies" between MS and ROS concerning lfPitchAndFamily values come from the fact we do not use the very same font files as Windows?
Also I notice that in ROS Russian the DEVICE_DEFAULT_FONT settings don't completely look the same as SYSTEM_FONT ? (lfCharSet is different).

if (uCodePage == 1) // CP_OEMCP
return OEM_CHARSET;

if (uCodePage == 2) // CP_MACCP
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it possible to use the defines names directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot #include <winnls.h> at here, and if included <windows.h> then compilation will be slow...

@@ -160,6 +160,28 @@ static const CHARSETINFO g_FontTci[MAXTCIINDEX] =
{ SYMBOL_CHARSET, CP_SYMBOL, {{0,0,0,0},{FS_SYMBOL,0}} }
};

BYTE FASTCALL IntCharSetFromCodePage(UINT uCodePage)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to add a comment that refers to gdi/gdi32/objects/font.c!TranslateCharsetInfo() function, in the TCI_SRCCODEPAGE case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@@ -1978,6 +1978,7 @@ TranslateCharsetInfo(
while (index < MAXTCIINDEX && !(*lpSrc>>index & 0x0001)) index++;
break;
case TCI_SRCCODEPAGE:
/* ciACP is also referred by IntCharSetFromCodePage() function */
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I was thinking about adding the comment in IntCharSetFromCodePage(), not here.

@katahiromz
Copy link
Contributor Author

English AFTER:
ros-english-after

Japanese AFTER:
ros-japanese-after

Russian AFTER:
ros-russian-after

It's perfect!

Copy link
Member

@learn-more learn-more left a comment

Choose a reason for hiding this comment

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

One comment,
other than that, lgtm.

Amazing work!

BOOL bIsCJK;
static const WCHAR SimSun[] = { 0x5B8B, 0x4F53, 0 };
static const WCHAR MingLiU[] = { 0x7D30, 0x660E, 0x9AD4 };
static const WCHAR Batang[] = { 0xBC14, 0xD0D5 };
Copy link
Member

Choose a reason for hiding this comment

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

Both MingLiU and Batang need a null-terminator, if they are used with wcscpy.

@HBelusca
Copy link
Contributor

Can we merge this PR even if PR #713 hasn't been merged yet?

@katahiromz
Copy link
Contributor Author

katahiromz commented Aug 10, 2018

Yeah, OK. There's no significant change in looking.

@HBelusca
Copy link
Contributor

Thanks, let's go!

@HBelusca HBelusca merged commit cbfe4d0 into reactos:master Aug 10, 2018
@katahiromz katahiromz deleted the stock-font-fix branch August 10, 2018 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants