Implement revisions from review

Author: johnml1135

Src/Common/FwUtils/FontFeatureSettings.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Globalization;
 using System.Linq;
+using System.Diagnostics;
 
 namespace SIL.FieldWorks.Common.FwUtils
 {
@@ -10,6 +11,14 @@ namespace SIL.FieldWorks.Common.FwUtils
 	/// </summary>
 	public static class FontFeatureSettings
 	{
+		private static readonly TraceSwitch s_traceSwitch =
+			new TraceSwitch("FwUtils_FontFeatureSettings", "Font feature parsing diagnostics");
+
+		internal static TraceSwitch DiagnosticsSwitch
+		{
+			get { return s_traceSwitch; }
+		}
+
 		/// <summary>
 		/// Parses a comma-separated font feature string into normalized feature settings.
 		/// Invalid entries are ignored so project data cannot crash render/UI paths.
@@ -28,16 +37,25 @@ public static IReadOnlyList<FontFeatureSetting> Parse(string features)
 
 				var equalsIndex = part.IndexOf('=');
 				if (equalsIndex <= 0 || equalsIndex == part.Length - 1)
+				{
+					TraceIgnoredEntry(part, "expected tag=value");
 					continue;
+				}
 
 				var tag = part.Substring(0, equalsIndex).Trim();
 				var valueText = part.Substring(equalsIndex + 1).Trim();
 				if (!IsValidOpenTypeTag(tag))
+				{
+					TraceIgnoredEntry(part, "tag must contain exactly four printable ASCII characters");
 					continue;
+				}
 
 				int value;
 				if (!int.TryParse(valueText, NumberStyles.Integer, CultureInfo.InvariantCulture, out value) || value < 0)
+				{
+					TraceIgnoredEntry(part, "value must be a non-negative integer");
 					continue;
+				}
 
 				settingsByTag[tag] = new FontFeatureSetting(tag, value);
 			}
@@ -63,7 +81,21 @@ public static string NormalizePreservingLegacy(string features)
 				return string.Empty;
 
 			var trimmed = features.Trim();
-			return char.IsLetter(trimmed[0]) ? Normalize(trimmed) : trimmed;
+			return LooksLikeLegacyGraphiteFeatureString(trimmed) ? trimmed : Normalize(trimmed);
+		}
+
+		private static bool LooksLikeLegacyGraphiteFeatureString(string features)
+		{
+			var firstPart = features.Split(',').FirstOrDefault();
+			if (string.IsNullOrWhiteSpace(firstPart))
+				return false;
+
+			var equalsIndex = firstPart.IndexOf('=');
+			if (equalsIndex <= 0)
+				return false;
+
+			var featureId = firstPart.Substring(0, equalsIndex).Trim();
+			return featureId.Length > 0 && featureId.All(char.IsDigit);
 		}
 
 		/// <summary>
@@ -73,6 +105,16 @@ public static bool IsValidOpenTypeTag(string tag)
 		{
 			return tag != null && tag.Length == 4 && tag.All(character => character >= 0x20 && character <= 0x7e);
 		}
+
+		private static void TraceIgnoredEntry(string part, string reason)
+		{
+			Trace.WriteLineIf(s_traceSwitch.TraceWarning,
+				string.Format(CultureInfo.InvariantCulture,
+					"Ignored invalid font feature entry '{0}': {1}.",
+					part,
+					reason),
+				s_traceSwitch.DisplayName);
+		}
 	}
 
 	/// <summary>

Src/Common/FwUtils/FwUtilsTests/FontFeatureSettingsTests.cs

@@ -1,4 +1,6 @@
 using System.Linq;
+using System.Diagnostics;
+using System.IO;
 using NUnit.Framework;
 
 namespace SIL.FieldWorks.Common.FwUtils
@@ -33,6 +35,43 @@ public void Parse_IgnoresInvalidEntries()
 				Is.EqualTo(new[] { "liga=0", "smcp=1" }));
 		}
 
+		[Test]
+		public void Parse_LogsIgnoredInvalidEntries()
+		{
+			var writer = new StringWriter();
+			var listener = new TextWriterTraceListener(writer);
+			var previousLevel = FontFeatureSettings.DiagnosticsSwitch.Level;
+
+			try
+			{
+				FontFeatureSettings.DiagnosticsSwitch.Level = TraceLevel.Warning;
+				Trace.Listeners.Add(listener);
+
+				FontFeatureSettings.Parse("smcp=1,bad=2,kern=x,broken");
+
+				listener.Flush();
+				var output = writer.ToString();
+				Assert.That(output, Does.Contain("Ignored invalid font feature entry 'bad=2'"));
+				Assert.That(output, Does.Contain("Ignored invalid font feature entry 'kern=x'"));
+				Assert.That(output, Does.Contain("Ignored invalid font feature entry 'broken'"));
+			}
+			finally
+			{
+				Trace.Listeners.Remove(listener);
+				listener.Dispose();
+				FontFeatureSettings.DiagnosticsSwitch.Level = previousLevel;
+			}
+		}
+
+		[Test]
+		public void Parse_AcceptsCustomPrintableAsciiTags()
+		{
+			var settings = FontFeatureSettings.Parse("!abc=1,a\"b\\=2").ToArray();
+
+			Assert.That(settings.Select(setting => setting.ToString()),
+				Is.EqualTo(new[] { "!abc=1", "a\"b\\=2" }));
+		}
+
 		[Test]
 		public void Normalize_ReturnsDeterministicRendererNeutralString()
 		{
@@ -50,5 +89,12 @@ public void NormalizePreservingLegacy_NormalizesOpenTypeFeatures()
 		{
 			Assert.That(FontFeatureSettings.NormalizePreservingLegacy(" smcp = 1, kern=0 "), Is.EqualTo("kern=0,smcp=1"));
 		}
+
+		[Test]
+		public void NormalizePreservingLegacy_NormalizesOpenTypeFeaturesThatStartWithPunctuation()
+		{
+			Assert.That(FontFeatureSettings.NormalizePreservingLegacy(" !abc = 1, kern=0 "),
+				Is.EqualTo("!abc=1,kern=0"));
+		}
 	}
 }

Src/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.cs

@@ -40,12 +40,14 @@ public class FontFeaturesButton : Button
 		private string m_fontName; // The font for which we are editing the features.
 		private string m_fontFeatures; // The font feature string stored in the writing system.
 		private IFontFeatureProvider m_featureProvider;
+		private static readonly TraceSwitch s_openTypeTraceSwitch =
+			new TraceSwitch("FontFeatures.OpenType", "OpenType font feature discovery and provider selection", "Off");
 		private ILgWritingSystemFactory m_wsf;
 		private int[] m_values;	// The actual list of values we're editing.
 		private int[] m_ids;		// The corresponding ids.
 		private bool m_isGraphiteFont;
 		private bool m_hasFontFeatures;
-		private bool m_useGraphiteFeatures = true;
+		private bool m_useGraphiteFeatures;
 		#endregion
 
 		#region Constructor and dispose stuff
@@ -376,28 +378,42 @@ public void SetupFontFeatures()
 			{
 				var graphiteProvider = CreateGraphiteProvider(hdg);
 				m_isGraphiteFont = graphiteProvider != null;
+				var openTypeProvider = OpenTypeFontFeatureProvider.Create(hdg.Hdc);
 
-				if (m_useGraphiteFeatures && graphiteProvider != null && graphiteProvider.HasFeatures)
-				{
-					m_featureProvider = graphiteProvider;
-					m_hasFontFeatures = true;
-					Enabled = true;
+				var primaryProvider = m_useGraphiteFeatures
+					? (IFontFeatureProvider)graphiteProvider
+					: openTypeProvider;
+				var secondaryProvider = m_useGraphiteFeatures
+					? openTypeProvider
+					: (IFontFeatureProvider)graphiteProvider;
+
+				if (TrySelectFeatureProvider(primaryProvider))
 					return;
-				}
 
-				var openTypeProvider = OpenTypeFontFeatureProvider.Create(hdg.Hdc);
-				if (openTypeProvider != null && openTypeProvider.HasFeatures)
-				{
-					m_featureProvider = openTypeProvider;
-					m_hasFontFeatures = true;
-					Enabled = true;
+				if (TrySelectFeatureProvider(secondaryProvider))
 					return;
-				}
 
 				Enabled = false;
 			}
 		}
 
+		private bool TrySelectFeatureProvider(IFontFeatureProvider provider)
+		{
+			if (provider == null || !provider.HasFeatures)
+				return false;
+
+			m_featureProvider = provider;
+			m_hasFontFeatures = true;
+			Enabled = true;
+			Trace.WriteLineIf(s_openTypeTraceSwitch.TraceInfo,
+				string.Format(CultureInfo.InvariantCulture,
+					"FontFeaturesButton selected {0} provider for '{1}'.",
+					provider.ProviderName,
+					m_fontName ?? string.Empty),
+				s_openTypeTraceSwitch.DisplayName);
+			return true;
+		}
+
 		private IFontFeatureProvider CreateGraphiteProvider(HoldDummyGraphics hdg)
 		{
 			IRenderEngine renderer = GraphiteEngineClass.Create();
@@ -849,6 +865,7 @@ private void ItemClickHandler(Object sender, EventArgs e)
 
 		private interface IFontFeatureProvider
 		{
+			string ProviderName { get; }
 			bool HasFeatures { get; }
 			int[] GetFeatureIds();
 			string GetFeatureTag(int featureId);
@@ -885,6 +902,11 @@ public bool HasFeatures
 				get { return m_featureIds.Length > 0; }
 			}
 
+			public string ProviderName
+			{
+				get { return "Graphite"; }
+			}
+
 			public int[] GetFeatureIds()
 			{
 				return m_featureIds.ToArray();
@@ -964,6 +986,11 @@ public bool HasFeatures
 				get { return m_featureIds.Length > 0; }
 			}
 
+			public string ProviderName
+			{
+				get { return "OpenType"; }
+			}
+
 			public int[] GetFeatureIds()
 			{
 				return m_featureIds.ToArray();
@@ -1022,6 +1049,14 @@ internal static class OpenTypeFontFeatureReader
 			private const uint GdiError = 0xFFFFFFFF;
 			private const int MaxCacheEntries = 32;
 			private const int ObjFont = 6;
+			private static readonly HashSet<string> s_nonUserConfigurableTags =
+				new HashSet<string>(StringComparer.Ordinal)
+				{
+					"abvf", "abvm", "abvs", "akhn", "blwf", "blwm", "blws", "ccmp",
+					"cjct", "curs", "dist", "fina", "haln", "half", "init", "isol",
+					"ljmo", "locl", "mark", "medi", "mkmk", "nukt", "pref", "pres",
+					"pstf", "psts", "rclt", "rkrf", "rlig", "tjmo", "vjmo"
+				};
 			private static readonly uint[] s_layoutTables = { MakeTableTag("GSUB"), MakeTableTag("GPOS") };
 			private static readonly object s_cacheLock = new object();
 			private static readonly Dictionary<FontFeatureCacheKey, string[]> s_featureTagCache =
@@ -1130,11 +1165,24 @@ private static void ReadFeatureList(byte[] tableData, ISet<string> tags)
 						return;
 
 					var tag = System.Text.Encoding.ASCII.GetString(tableData, recordOffset, 4);
-					if (FontFeatureSettings.IsValidOpenTypeTag(tag))
+					if (FontFeatureSettings.IsValidOpenTypeTag(tag) && IsUserConfigurableTag(tag))
 						tags.Add(tag);
 				}
 			}
 
+			private static bool IsUserConfigurableTag(string tag)
+			{
+				if (!s_nonUserConfigurableTags.Contains(tag))
+					return true;
+
+				Trace.WriteLineIf(s_openTypeTraceSwitch.TraceInfo,
+					string.Format(CultureInfo.InvariantCulture,
+						"FontFeaturesButton filtered non-user OpenType feature '{0}'.",
+						tag),
+					s_openTypeTraceSwitch.DisplayName);
+				return false;
+			}
+
 			private static ushort ReadUInt16(byte[] data, int offset)
 			{
 				return (ushort)((data[offset] << 8) | data[offset + 1]);

Src/FwCoreDlgs/FwCoreDlgControls/FwCoreDlgControls.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!-- 
-    Microsoft ResX Schema 
-    
+  <!--
+    Microsoft ResX Schema
+
     Version 2.0
-    
-    The primary goals of this format is to allow a simple XML format 
-    that is mostly human readable. The generation and parsing of the 
-    various data types are done through the TypeConverter classes 
+
+    The primary goals of this format is to allow a simple XML format
+    that is mostly human readable. The generation and parsing of the
+    various data types are done through the TypeConverter classes
     associated with the data types.
-    
+
     Example:
-    
+
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-                
-    There are any number of "resheader" rows that contain simple 
+
+    There are any number of "resheader" rows that contain simple
     name/value pairs.
-    
-    Each data row contains a name, and value. The row also contains a 
-    type or mimetype. Type corresponds to a .NET class that support 
-    text/value conversion through the TypeConverter architecture. 
-    Classes that don't support this are serialized and stored with the 
+
+    Each data row contains a name, and value. The row also contains a
+    type or mimetype. Type corresponds to a .NET class that support
+    text/value conversion through the TypeConverter architecture.
+    Classes that don't support this are serialized and stored with the
     mimetype set.
-    
-    The mimetype is used for serialized objects, and tells the 
-    ResXResourceReader how to depersist the object. This is currently not 
+
+    The mimetype is used for serialized objects, and tells the
+    ResXResourceReader how to depersist the object. This is currently not
     extensible. For a given mimetype the value must be set accordingly:
-    
-    Note - application/x-microsoft.net.object.binary.base64 is the format 
-    that the ResXResourceWriter will generate, however the reader can 
+
+    Note - application/x-microsoft.net.object.binary.base64 is the format
+    that the ResXResourceWriter will generate, however the reader can
     read any of the formats listed below.
-    
+
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-    
+
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array 
+    value   : The object must be serialized into a byte array
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -566,4 +566,4 @@
   <data name="ksIllegalXmlChars" xml:space="preserve">
     <value>Characters with hexadecimal values 0x0 through 0x1f are illegal in XML documents and have been removed.</value>
   </data>
-</root>
+</root>