Fix LT-22351: load default font features in BaseStyleInfo.ProcessStyleRules

CHANGELOG.md

@@ -28,6 +28,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- [SIL.LCModel] BaseStyleInfo now loads default font features (ktptFontVariations) from style rules, fixing font features set as style defaults being dropped on reload (LT-22351)
+- [SIL.LCModel] BaseStyleInfo no longer clobbers ws-specific font overrides inherited from a based-on style with the default font info's merely-inherited values; only explicitly-set default properties are propagated to ws overrides (LT-22351)
+- [SIL.LCModel] BulletInfo now decodes all properties of an encoded bullet font info instead of stopping at the first string property, fixing bullet font features (ktptFontVariations) being lost when a bullet font name was also set (LT-22351)
 - [SIL.LCModel] Deleting a duplicated phonological rule no longer deletes pooled feature constraints and environment contexts the original rule still references (LT-22575)
 - [SIL.LCModel] Fixed crash when bulk deleting entries involved in a lexical relation (LT-21598)
 - [SIL.LCModel.FixData] Find and Fix removes duplicate Targets from a LexReference (and deletes it if fewer than two distinct Targets remain) (LT-21598)

src/SIL.LCModel/DomainServices/BaseStyleInfo.cs

@@ -935,6 +935,10 @@ private void ProcessStyleRules(ITsTextProps styleProps,
 						m_defaultFontInfo.m_fontName.ExplicitValue = sProp;
 						break;
 
+					case (int)FwTextPropType.ktptFontVariations:
+						SetFontStringProp(tpt, m_defaultFontInfo, sProp);
+						break;
+
 					case (int)FwTextPropType.ktptBulNumTxtBef:
 						{
 							m_bulletInfo.IsInherited = false;
@@ -1436,7 +1440,11 @@ private void SetNonExplicitPropertiesToInherited()
 				fontInfoOverride.Value.InheritAllProperties(inheritPropsFrom);
 				if (m_defaultFontInfo.IsAnyExplicit)
 				{
-					fontInfoOverride.Value.InheritAllProperties(m_defaultFontInfo);
+					// Propagate only the explicitly-set default (style-level) properties into the
+					// override; the default's other values were merely inherited from the based-on
+					// style's default and must not clobber what was just inherited from the
+					// based-on style's ws override.
+					fontInfoOverride.Value.InheritExplicitProperties(m_defaultFontInfo);
 				}
 			}
 			m_rtl.InheritValue(basedOn.m_rtl);

src/SIL.LCModel/DomainServices/BulletInfo.cs

@@ -210,13 +210,13 @@ private static FontInfo DecodeFontInfo(string blob)
 					{
 						fontInfo.m_fontName.ExplicitValue = blob.Substring(i, iPropLim - i);
 						i += (iPropLim - i + 1);
-						break;
+						continue;
 					}
 					else if (tpt == FwTextPropType.ktptFontVariations)
 					{
 						fontInfo.m_features.ExplicitValue = blob.Substring(i, iPropLim - i);
 						i += (iPropLim - i + 1);
-						break;
+						continue;
 					}
 
 					int nVal = (int)blob[i] + (int)(blob[i + 1] << 16);

src/SIL.LCModel/DomainServices/FontInfo.cs

@@ -273,5 +273,40 @@ internal void InheritAllProperties(FontInfo basedOnFontInfo)
 			m_offset.InheritValue(basedOnFontInfo.m_offset);
 			m_features.InheritValue(basedOnFontInfo.m_features);
 		}
+
+		/// ------------------------------------------------------------------------------------
+		/// <summary>
+		/// Inherit the values of only those properties that are explicitly set on the specified
+		/// font info, but don't force a change in the inherit value.
+		/// </summary>
+		/// <remarks>Explicitly set values on this font info will not be affected</remarks>
+		/// <param name="basedOnFontInfo">The font info from which to get the explicitly set
+		/// values.</param>
+		/// ------------------------------------------------------------------------------------
+		internal void InheritExplicitProperties(FontInfo basedOnFontInfo)
+		{
+			if (basedOnFontInfo.m_fontName.IsExplicit)
+				m_fontName.InheritValue(basedOnFontInfo.m_fontName);
+			if (basedOnFontInfo.m_fontSize.IsExplicit)
+				m_fontSize.InheritValue(basedOnFontInfo.m_fontSize);
+			if (basedOnFontInfo.m_fontColor.IsExplicit)
+				m_fontColor.InheritValue(basedOnFontInfo.m_fontColor);
+			if (basedOnFontInfo.m_backColor.IsExplicit)
+				m_backColor.InheritValue(basedOnFontInfo.m_backColor);
+			if (basedOnFontInfo.m_bold.IsExplicit)
+				m_bold.InheritValue(basedOnFontInfo.m_bold);
+			if (basedOnFontInfo.m_italic.IsExplicit)
+				m_italic.InheritValue(basedOnFontInfo.m_italic);
+			if (basedOnFontInfo.m_superSub.IsExplicit)
+				m_superSub.InheritValue(basedOnFontInfo.m_superSub);
+			if (basedOnFontInfo.m_underline.IsExplicit)
+				m_underline.InheritValue(basedOnFontInfo.m_underline);
+			if (basedOnFontInfo.m_underlineColor.IsExplicit)
+				m_underlineColor.InheritValue(basedOnFontInfo.m_underlineColor);
+			if (basedOnFontInfo.m_offset.IsExplicit)
+				m_offset.InheritValue(basedOnFontInfo.m_offset);
+			if (basedOnFontInfo.m_features.IsExplicit)
+				m_features.InheritValue(basedOnFontInfo.m_features);
+		}
 	}
 }

tests/SIL.LCModel.Tests/DomainServices/BaseStyleInfoTests.cs

@@ -274,6 +274,35 @@ public void CreateCopy()
 			Assert.AreEqual(false, newInfo.IsBuiltIn, "Copies of styles should not be considered built in");
 		}
 
+		/// ------------------------------------------------------------------------------------
+		/// <summary>
+		/// Tests that font features (ktptFontVariations) set as a style-level default (not a
+		/// writing-system override) are loaded into the default font info. See LT-22351: such
+		/// features were saved to the database but silently dropped when the style was reloaded,
+		/// because ProcessStyleRules had no case for ktptFontVariations.
+		/// </summary>
+		/// ------------------------------------------------------------------------------------
+		[Test]
+		public void ConstructBasedOnStyle_FontFeatures()
+		{
+			ITsPropsBldr props;
+
+			IStStyle mainTitleStyle = AddTestStyle("Title Main", ContextValues.Title,
+				StructureValues.Body, FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			props = mainTitleStyle.Rules.GetBldr();
+			props.SetStrPropValue((int)FwTextPropType.ktptFontFamily, "Arial");
+			props.SetStrPropValue((int)FwTextPropType.ktptFontVariations, "smcp=1,ss01=2");
+			mainTitleStyle.Rules = props.GetTextProps();
+
+			DummyStyleInfo entry = new DummyStyleInfo(mainTitleStyle);
+			FontInfo fontInfo = entry.FontInfoForWs(-1);
+			Assert.IsNotNull(fontInfo);
+			Assert.IsTrue(fontInfo.m_features.IsExplicit,
+				"Default font features set on the style should be explicit, not inherited");
+			Assert.AreEqual("smcp=1,ss01=2", fontInfo.m_features.Value);
+			Assert.AreEqual("Arial", (string)fontInfo.m_fontName.Value);
+		}
+
 		/// <summary>
 		/// Minimal test of an alternate constructor to verify that it records the cache from the style.
 		/// </summary>
@@ -402,6 +431,178 @@ public void SetBasedOnStyleAndInheritValues_WsOverrideInChildInheritsParentValue
 				"The child entry in the StyleInfoCollection did not inherit the fontName properly");
 		}
 
+		/// <summary>
+		/// Tests that a child style whose only explicit style-level default is font features
+		/// (ktptFontVariations) does not clobber the ws-specific overrides it inherits from its
+		/// based-on style. The explicitly-set default properties should be propagated to the ws
+		/// overrides, but the default's merely-inherited values must not overwrite what was just
+		/// inherited from the based-on style's ws override. See LT-22351.
+		/// </summary>
+		[Test]
+		public void SetBasedOnStyleAndInheritValues_ExplicitDefaultFeaturesDoNotClobberWsOverride()
+		{
+			// Font family override for the parent style
+			byte[] buffer = new byte[] {
+				0xC1, 0x87, 0x8B, 0x3B, // WS for english (little endian)
+				0x09, 0x00, // FF length
+				(byte)'F', 0,
+				(byte)'o', 0,
+				(byte)'n', 0,
+				(byte)'t', 0,
+				(byte)'a', 0,
+				(byte)'s', 0,
+				(byte)'t', 0,
+				(byte)'i', 0,
+				(byte)'c', 0,
+
+				0x00, 0x00, // Count of int props
+			};
+
+			ITsPropsBldr props;
+
+			var mainTitleStyle = AddTestStyle("Title Main", ContextValues.Title,
+				StructureValues.Body, FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			var inheritFromMain = AddTestStyle("Inherit Title", ContextValues.Title, StructureValues.Body,
+				FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			inheritFromMain.BasedOnRA = mainTitleStyle;
+			// The parent style has a ws-specific font override and a different default font
+			props = mainTitleStyle.Rules.GetBldr();
+			props.SetStrPropValue((int)FwTextPropType.ktptFontFamily, "Times New Roman");
+			props.SetStrPropValue((int)FwTextPropType.ktptWsStyle, DummyStyleInfo.MakeStringFromBuffer(buffer));
+			mainTitleStyle.Rules = props.GetTextProps();
+			// The child style's only explicit style-level default is font features
+			props = inheritFromMain.Rules.GetBldr();
+			props.SetStrPropValue((int)FwTextPropType.ktptFontVariations, "smcp=1");
+			inheritFromMain.Rules = props.GetTextProps();
+
+			var entry = new DummyStyleInfo(mainTitleStyle);
+			var childEntry = new DummyStyleInfo(inheritFromMain);
+			var styleInfos = new LcmStyleSheet.StyleInfoCollection();
+			styleInfos.Add(entry);
+			styleInfos.Add(childEntry);
+			// SUT
+			childEntry.SetBasedOnStyleAndInheritValues(styleInfos);
+			var childOverrides = childEntry.FontInfoForWs(Cache.DefaultAnalWs);
+			Assert.True(childOverrides.m_fontName.IsInherited,
+				"The child entry's ws override should still inherit the fontName");
+			Assert.AreEqual("Fontastic", childOverrides.m_fontName.Value,
+				"The fontName inherited from the parent's ws override should not be clobbered by the child's default font info");
+			Assert.AreEqual("smcp=1", childOverrides.m_features.Value,
+				"The child's explicit default font features should be propagated to its ws override");
+		}
+
+		/// <summary>
+		/// Tests that a child style's explicit style-level default properties are still
+		/// propagated into its ws-specific overrides (the intended behavior of LT-18109),
+		/// while properties the child does not set explicitly keep the values inherited from
+		/// the based-on style's ws override. See LT-22351.
+		/// </summary>
+		[Test]
+		public void SetBasedOnStyleAndInheritValues_ExplicitDefaultPropsPropagateIntoWsOverride()
+		{
+			// Font family override for the parent style
+			byte[] buffer = new byte[] {
+				0xC1, 0x87, 0x8B, 0x3B, // WS for english (little endian)
+				0x09, 0x00, // FF length
+				(byte)'F', 0,
+				(byte)'o', 0,
+				(byte)'n', 0,
+				(byte)'t', 0,
+				(byte)'a', 0,
+				(byte)'s', 0,
+				(byte)'t', 0,
+				(byte)'i', 0,
+				(byte)'c', 0,
+
+				0x00, 0x00, // Count of int props
+			};
+
+			ITsPropsBldr props;
+
+			var mainTitleStyle = AddTestStyle("Title Main", ContextValues.Title,
+				StructureValues.Body, FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			var inheritFromMain = AddTestStyle("Inherit Title", ContextValues.Title, StructureValues.Body,
+				FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			inheritFromMain.BasedOnRA = mainTitleStyle;
+			// The parent style has a ws-specific font override and a different default font
+			props = mainTitleStyle.Rules.GetBldr();
+			props.SetStrPropValue((int)FwTextPropType.ktptFontFamily, "Times New Roman");
+			props.SetStrPropValue((int)FwTextPropType.ktptWsStyle, DummyStyleInfo.MakeStringFromBuffer(buffer));
+			mainTitleStyle.Rules = props.GetTextProps();
+			// The child style's only explicit style-level default is bold, which the parent's
+			// ws override does not set
+			props = inheritFromMain.Rules.GetBldr();
+			props.SetIntPropValues((int)FwTextPropType.ktptBold,
+				(int)FwTextPropVar.ktpvDefault, 1);
+			inheritFromMain.Rules = props.GetTextProps();
+
+			var entry = new DummyStyleInfo(mainTitleStyle);
+			var childEntry = new DummyStyleInfo(inheritFromMain);
+			var styleInfos = new LcmStyleSheet.StyleInfoCollection();
+			styleInfos.Add(entry);
+			styleInfos.Add(childEntry);
+			// SUT
+			childEntry.SetBasedOnStyleAndInheritValues(styleInfos);
+			var childOverrides = childEntry.FontInfoForWs(Cache.DefaultAnalWs);
+			Assert.AreEqual("Fontastic", childOverrides.m_fontName.Value,
+				"The fontName inherited from the parent's ws override should not be clobbered by the child's default font info");
+			Assert.IsTrue(childOverrides.m_bold.Value,
+				"The child's explicit default bold setting should be propagated to its ws override");
+		}
+
+		/// <summary>
+		/// Tests that a child style's explicit style-level default font name applies across
+		/// writing systems: it wins over the font name in the based-on style's ws override.
+		/// See LT-22351.
+		/// </summary>
+		[Test]
+		public void SetBasedOnStyleAndInheritValues_ExplicitDefaultFontNameBeatsParentWsOverride()
+		{
+			// Font family override for the parent style
+			byte[] buffer = new byte[] {
+				0xC1, 0x87, 0x8B, 0x3B, // WS for english (little endian)
+				0x09, 0x00, // FF length
+				(byte)'F', 0,
+				(byte)'o', 0,
+				(byte)'n', 0,
+				(byte)'t', 0,
+				(byte)'a', 0,
+				(byte)'s', 0,
+				(byte)'t', 0,
+				(byte)'i', 0,
+				(byte)'c', 0,
+
+				0x00, 0x00, // Count of int props
+			};
+
+			ITsPropsBldr props;
+
+			var mainTitleStyle = AddTestStyle("Title Main", ContextValues.Title,
+				StructureValues.Body, FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			var inheritFromMain = AddTestStyle("Inherit Title", ContextValues.Title, StructureValues.Body,
+				FunctionValues.Prose, false, Cache.LangProject.StylesOC);
+			inheritFromMain.BasedOnRA = mainTitleStyle;
+			// The parent style has a ws-specific font override
+			props = mainTitleStyle.Rules.GetBldr();
+			props.SetStrPropValue((int)FwTextPropType.ktptWsStyle, DummyStyleInfo.MakeStringFromBuffer(buffer));
+			mainTitleStyle.Rules = props.GetTextProps();
+			// The child style has an explicit style-level default font name
+			props = inheritFromMain.Rules.GetBldr();
+			props.SetStrPropValue((int)FwTextPropType.ktptFontFamily, "Times New Roman");
+			inheritFromMain.Rules = props.GetTextProps();
+
+			var entry = new DummyStyleInfo(mainTitleStyle);
+			var childEntry = new DummyStyleInfo(inheritFromMain);
+			var styleInfos = new LcmStyleSheet.StyleInfoCollection();
+			styleInfos.Add(entry);
+			styleInfos.Add(childEntry);
+			// SUT
+			childEntry.SetBasedOnStyleAndInheritValues(styleInfos);
+			var childOverrides = childEntry.FontInfoForWs(Cache.DefaultAnalWs);
+			Assert.AreEqual("Times New Roman", childOverrides.m_fontName.Value,
+				"The child's explicit default font name should apply across writing systems, winning over the parent's ws override");
+		}
+
 		/// ------------------------------------------------------------------------------------
 		/// <summary>
 		/// Tests retrieving WS specific overrides from string

tests/SIL.LCModel.Tests/DomainServices/BulletInfoTests.cs

@@ -139,6 +139,37 @@ public void RoundTripEncodingAndDecodingOfFontInfo()
 
 			Assert.AreEqual(expectedFontInfo, bulletInfo2.FontInfo);
 		}
+
+		/// ------------------------------------------------------------------------------------
+		/// <summary>
+		/// Tests that we can save and restore a font info that has both a font name and font
+		/// features (ktptFontVariations) explicitly set. DecodeFontInfo used to stop parsing
+		/// after the first string property, so the font features (which are encoded after the
+		/// font name) were lost on every decode. See LT-22351.
+		/// </summary>
+		/// ------------------------------------------------------------------------------------
+		[Test]
+		public void RoundTripEncodingAndDecodingOfFontInfo_FontNameAndFontFeatures()
+		{
+			// Setup what we expect
+			FontInfo expectedFontInfo = m_infoTable["TestStyle"].FontInfoForWs(-1);
+			expectedFontInfo.m_fontName = new InheritableStyleProp<string>("Algerian");
+			expectedFontInfo.m_features = new InheritableStyleProp<string>("smcp=1,ss01=2");
+
+			BulletInfo bulletInfo1 = new BulletInfo();
+			bulletInfo1.FontInfo = expectedFontInfo;
+
+			BulletInfo bulletInfo2 = new BulletInfo();
+			bulletInfo2.EncodedFontInfo = bulletInfo1.EncodedFontInfo;
+
+			Assert.IsTrue(bulletInfo2.FontInfo.m_fontName.IsExplicit,
+				"The font name should survive the round trip");
+			Assert.AreEqual("Algerian", bulletInfo2.FontInfo.m_fontName.Value);
+			Assert.IsTrue(bulletInfo2.FontInfo.m_features.IsExplicit,
+				"The font features should survive the round trip");
+			Assert.AreEqual("smcp=1,ss01=2", bulletInfo2.FontInfo.m_features.Value);
+			Assert.AreEqual(expectedFontInfo, bulletInfo2.FontInfo);
+		}
 		#endregion
 	}
 }