microsoft / Font-Validator

Font Validator is a tool for testing fonts prior to release. This testing ensures that fonts meet Microsoft's high quality standards and perform exceptionally well on Microsoft's platform.
Other
117 stars 41 forks source link

Some feedbacks from Paul Wise #16

Open HinTak opened 8 years ago

HinTak commented 8 years ago

Feedback from @pabs3 via e-mail, just the technical part:

A -files option for FontValidator.exe would be nice so you don't have
to type -file over and over again, it should interpret the rest of the
command-line as individual files to check.

A -quiet option for FontValidator.exe that would disable the progress
information and only print errors/warnings to stdout.

An option for FontValidator.exe to print a human-readable report to
stdout instead of printing an XML report to a temporary file.

Please add per-file copyright information:

http://lu.is/blog/2012/03/17/on-the-importance-of-per-file-license-information/

Please remove or decompile Windows-specific binary files like these:

OTFontFileVal/OTFontFileVal.ValStrings.resources
Glyph/NS_Glyph.GErrStrings.resources
FontVal/FontVal.ResultsForm.resources
FontVal/FontVal.FormTransform.resources
FontVal/FontVal.FormReportOptions.resources
FontVal/FontVal.FormAbout.resources
FontVal/FontVal.Form1.resources

Please remove auto-generated files and build them in the Makefile:

*.cd (I think?)
bin/FontValidatorHelp.chm

A new app icon would be nice, the current one is pretty ugly and
doesn't have a glyph in it to indicate it is about fonts.

Please use split-out PNG icons as the base for the icons and create the
.ico files at build time. ICO is a Windows-specific format.

Please add a .gitignore file:

/bin/
/GenerateFValData/bin/
*.chm
*.resources
*.ico
*.cd

README-extra.txt should be renamed to TODO or similar and the TODO
items from README.txt moved there.

These files are duplicates but they got out of sync:

FontVal/fval.xsl
bin/fval.xsl

There are some warnings from mono:

Table_EBLC.cs(1217,39): warning CS0219: The variable `ic' is assigned but its value is never used
Table_OS2.cs(943,28): warning CS0219: The variable `fourByte' is assigned but its value is never used
TAble_Zapf.cs(340,18): warning CS0414: The private field `OTFontFile.Table_Zapf.FeatureInfo.m_offsetFeatureInfo' is assigned but its value is never used
TAble_Zapf.cs(341,23): warning CS0414: The private field `OTFontFile.Table_Zapf.FeatureInfo.m_bufTable' is assigned but its value is never used
DICondBuilder.cs(39,47): warning CS0219: The variable `infoPars' is assigned but its value is never used
Glyph.cs(1093,36): warning CS0219: The variable `linters' is assigned but its value is never used
Glyph.cs(1251,17): warning CS0219: The variable `ind' is assigned but its value is never used
ValDriver.cs(269,20): warning CS0219: The variable `sTable' is assigned but its value is never used
val_VDMX.cs(133,32): warning CS0219: The variable `ratio' is assigned but its value is never used
val_cmap.cs(645,32): warning CS0219: The variable `charbuf' is assigned but its value is never used
val_hhea.cs(394,28): warning CS0219: The variable `dItalicAngle' is assigned but its value is never used
val_post.cs(374,29): warning CS0219: The variable `buf' is assigned but its value is never used
FormReportOptions.cs(232,32): warning CS0168: The variable `what' is declared but never used
Program.cs(147,36): warning CS0168: The variable `e' is declared but never used
ResultsForm.cs(69,37): warning CS0219: The variable `resources' is assigned but its value is never used
ResultsForm.cs(171,16): warning CS0649: Field `FontVal.ResultsForm.m_sFilename' is never assigned to, and will always have its default value `null'
ResultsForm.cs(172,14): warning CS0649: Field `FontVal.ResultsForm.m_bDeleteOnClose' is never assigned to, and will always have its default value `false'
CmdLineInterface.cs(19,31): warning CS0414: The private field `FontValidator.CmdLineInterface.m_bOpenReportFiles' is assigned but its value is never used
CmdLineInterface.cs(22,31): warning CS0414: The private field `FontValidator.CmdLineInterface.m_curOTFileVal' is assigned but its value is never used

There are some spelling errors:

$ codespell --quiet-level=3
./OTFontFile/Table_kern.cs:331: Cant  ==> Can't
./GenerateFValData/OurData.xml:8616: boundry  ==> boundary
./FontVal/FontVal.FormReportOptions.resx:367: IwTh  ==> with
./FontVal/FontVal.FormReportOptions.resx:377: IwTh  ==> with
./FontVal/FontVal.FormReportOptions.resx:486: IwTh  ==> with
./FontVal/FontVal.FormTransform.resx:367: IwTh  ==> with
./FontVal/FontVal.FormTransform.resx:377: IwTh  ==> with
./FontVal/FontVal.FormTransform.resx:486: IwTh  ==> with
./GMath/Equation.cs:120: asign  ==> assign
./GMath/Equation.cs:359: asign  ==> assign
./OTFontFileVal/val_cmap.cs:413: cant  ==> can't
./OTFontFileVal/val_cmap.cs:418: cant  ==> can't
./OTFontFileVal/val_glyf.cs:120: AUXILLIARY  ==> AUXILIARY
./Compat/Compat.cs:67: seperately  ==> separately

$ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname .svn -o -iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o -iname _darcs -o -iname .pc -o -iname .cabal-sandb
ox -o -iname .cdv -o -iname .metadata -o -iname CMakeFiles -o -iname _build -o -iname _sgbak -o -iname autom4te.cache -o -iname blib -o -iname cover_db -o -iname node_modules -o -iname 
'~.dep' -o -iname '~.dot' -o -iname '~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( -iname '*.bak' -o -iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 'core.*' -o -iname '*
~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' -o -iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o -iname '*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o
 -iname '*.css.min' \) -exec spellintian --picky {} +
./OTFontFile/Table_name.cs: english -> English
./OTFontFile/Table_kern.cs: suppported -> supported
./GenerateFValData/OurData.xml: boundry -> boundary
./GMath/Contour.cs: Returnes -> Returns
./Glyph/FManager.cs: shoud -> should
./OTFontFileVal/val_name.cs: english -> English
./OTFontFileVal/val_glyf.cs: AUXILLIARY -> AUXILIARY
./OTFontFileVal/val_OS2.cs: latin -> Latin
./Compat/Compat.cs: seperately -> separately

There are some duplicate files:

$ fdupes -q -r .
./FontVal/Btn2.ico
./FontVal/Btn4.ico

./GenerateFValData/toc-error-postamble.txt
./GenerateFValData/toc-warning-postamble.txt
./GenerateFValData/toc-info-postamble.txt

./OTFontFile/AssemblyInfo.cs
./GenerateFValData/Properties/AssemblyInfo.cs
./ValCommon/AssemblyInfo.cs
./GMath/AssemblyInfo.cs
./Glyph/AssemblyInfo.cs
./OTFontFileVal/AssemblyInfo.cs

./OTFontFile/ClassDiagram2.cd
./OTFontFile/ClassDiagram1.cd
./OTFontFileVal/ClassDiagram2.cd

Check if these can be switched to https://

$ grep -rF http: .
<lots>

There are a lot more TODO items:

$ grep -riE 'fixme|todo|hack|xxx|broken' .
<lots>

and part of my response, edited:

There are many binary blobs in the repo at the moment because they are difficult to make, or requires windows-only tools to do so; the resources files are one (mono's resgen is buggy), CHM is another (requires MS HTML Help compiler).

There is a -quiet option to FontValidator - it does not do what you do. I think you want a '-stdout' option, where report is write to stdout instead of to a xml file? I have been thinking about that too.

Argh, I see you are looking at upstream, possibly, about bin/fval, FontVal/fval out of sync? That's my issue about Microsoft not reviewing that change and asked me to...
HinTak commented 8 years ago

Captured here so that it is not forgotten, and others can work on them too.

HinTak commented 8 years ago

@pabs3 : have a hell of trouble installing spellintian (on non-Debian system). After installing Class::Accessor and apt-devel from Fedora, clone'ing lintian , Debian-Dpkg, libapt-pkg-perl from debian, I am with :

...
AptPkg.xs: In function ‘void XS_AptPkg___src_records_Find(PerlInterpreter*, CV*)’:
AptPkg.xs:1391:106: error: ‘const struct pkgSrcRecords::File’ has no member named ‘MD5Hash’
AptPkg.xs:1391:125: error: ‘const struct pkgSrcRecords::File’ has no member named ‘MD5Hash’
AptPkg.c:3898:7: warning: unused variable ‘RETVAL’ [-Wunused-variable]
  SV * RETVAL;
       ^
Makefile:377: recipe for target 'AptPkg.o' failed

or

...
AptPkg.xs: In function ‘void boot_AptPkg(PerlInterpreter*, CV*)’:
AptPkg.xs:1484:168: error: ‘None’ is not a member of ‘pkgCache::Version’
AptPkg.xs:1485:167: error: ‘All’ is not a member of ‘pkgCache::Version’
AptPkg.xs:1486:171: error: ‘Foreign’ is not a member of ‘pkgCache::Version’
AptPkg.xs:1487:168: error: ‘Same’ is not a member of ‘pkgCache::Version’
AptPkg.xs:1488:171: error: ‘Allowed’ is not a member of ‘pkgCache::Version’
AptPkg.xs:1489:174: error: ‘AllForeign’ is not a member of ‘pkgCache::Version’
AptPkg.xs:1490:174: error: ‘AllAllowed’ is not a member of ‘pkgCache::Version’
Makefile:377: recipe for target 'AptPkg.o' failed
make: *** [AptPkg.o] Error 1

depends on whether I try to compile v0.1.29 or v0.1.27 . I am with apt-0.5.15lorg3.95-21.git522 (this appears to be a fork of libapt-pkg for rpm-based systems, from http://apt-rpm.org/ ). In any case, it is a bit stupid for a spell checker to be so tightly bound to a distro. Please forward above to relevant Debian people.

Anyway, I fixed what should be fixed, I think: https://github.com/HinTak/Font-Validator/commit/81620aab2f49fa1503c49d6fc514cc19d434d403 https://github.com/HinTak/Font-Validator/commit/1b50fa27b8e7c16ff54d21d976eaa7cfca02b77c