Skip to content

LT-22351: Offer Graphite features in Styles and refresh after font changes #1165

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.

Sign up for GitHub

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

Merged
jasonleenaylor merged 3 commits into from
Sep 30, 2026
+222 −10
Merged

LT-22351: Offer Graphite features in Styles and refresh after font changes #1165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3ac5e3
LT-22351: Offer Graphite features in the Styles dialog font tab
jasonleenaylor Sep 29, 2026
a145300
LT-22351: Refresh views when a writing system's font settings change
jasonleenaylor Sep 29, 2026
a0070f8
LT-22351: Test the refresh when font features change in place
jasonleenaylor Sep 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Src/FwCoreDlgs/FwCoreDlgControls/DefaultFontsControl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,8 @@ protected void SetSelectedFonts()
bool isGraphiteFont = m_defaultFontFeaturesButton.IsGraphiteFont;
m_graphiteGroupBox.Enabled = isGraphiteFont || m_defaultFontFeaturesButton.HasFontFeatures;
m_enableGraphiteCheckBox.Enabled = isGraphiteFont;
if (!isGraphiteFont)
m_ws.IsGraphiteEnabled = false;
m_enableGraphiteCheckBox.Checked = m_ws.IsGraphiteEnabled;
// Display only; writing the flag here would mark an untouched writing system changed.
m_enableGraphiteCheckBox.Checked = isGraphiteFont && m_ws.IsGraphiteEnabled;
}

/// <summary>
Expand Down
12 changes: 12 additions & 0 deletions Src/FwCoreDlgs/FwCoreDlgControls/FwFontAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,18 @@ public string FontName
set { m_btnFontFeatures.FontName = value; }
}

/// ------------------------------------------------------------------------------------
/// <summary>
/// Gets or sets whether the Font Features button offers Graphite features ahead of
/// OpenType features when the current font supports both.
/// </summary>
/// ------------------------------------------------------------------------------------
public bool UseGraphiteFeatures
{
get { CheckDisposed(); return m_btnFontFeatures.UseGraphiteFeatures; }
set { CheckDisposed(); m_btnFontFeatures.UseGraphiteFeatures = value; }
}

/// ------------------------------------------------------------------------------------
/// <summary>
/// Gets or sets a value indicating whether the controls for super/subscript are enabled or not.
Expand Down
22 changes: 21 additions & 1 deletion Src/FwCoreDlgs/FwCoreDlgControls/FwFontTab.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public partial class FwFontTab : UserControl, IStylesTab
private bool m_fIgnoreWsSelectedIndexChange = false;
private StyleInfo m_currentStyleInfo;
private int m_currentWs = -1;
private ILgWritingSystemFactory m_wsf;

private bool m_fFontListIncludesRealNames = false;

Expand Down Expand Up @@ -81,7 +82,12 @@ public void CheckDisposed()
/// ------------------------------------------------------------------------------------
public ILgWritingSystemFactory WritingSystemFactory
{
set { CheckDisposed(); m_FontAttributes.WritingSystemFactory = value; }
set
{
CheckDisposed();
m_wsf = value;
m_FontAttributes.WritingSystemFactory = value;
}
}
#endregion

Expand Down Expand Up @@ -302,6 +308,8 @@ public void UpdateForStyle(StyleInfo styleInfo, int ws)
// only include the magic font names, not the real ones.
FillFontNames(ws > -1);

m_FontAttributes.UseGraphiteFeatures = IsGraphiteEnabled(ws);

@mark-sil mark-sil Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason the Styles-dialog half of this landed without tests? FwFontTabTests already has a cache-backed fixture that calls FillFontInfo(Cache), and FwFontTab already exposes internal ComboBox FontNamesComboBox, so an internal accessor for the attributes control would be in keeping with what's there.

Two tests — one that sets a writing system's IsGraphiteEnabled and asserts the flag reaches the button after UpdateForStyle(styleInfo, ws.Handle), and one for the -1 row — would pin both this line and the default-row rule below. Happy to be told the accessor isn't worth it.

@jasonleenaylor jasonleenaylor Sep 30, 2026

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.

That test existed and was dropped before the PR went up (UpdateForStyle_GraphiteWritingSystem_PrefersGraphiteFeatures): it set IsGraphiteEnabled on the vernacular writing system, called UpdateForStyle for that row and for -1, and asserted the flag on the attributes control. The only observable the fixture can reach is that flag, so the test restated IsGraphiteEnabled(ws) in test form; the thing that matters, which list the button shows, needs a Graphite font and a device context the fixture does not have. Not worth the accessor for that reason, though I am open to it if you see something it would catch.


m_FontAttributes.ShowingInheritedProperties = true; // Always allow re-setting to unspecified for font attributes

// Initialize controls based on whether or not this style inherits from another style.
Expand Down Expand Up @@ -418,6 +426,18 @@ internal ComboBox FontNamesComboBox
}

#region private methods
/// <summary>
/// Returns whether Graphite rendering is enabled for the given writing system. The
/// default row (-1) covers every writing system, so it reports false.
/// </summary>
private bool IsGraphiteEnabled(int ws)
{
if (ws <= 0 || m_wsf == null)

@mark-sil mark-sil Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With ws <= 0 returning false, the "Default settings" row keeps the OpenType list — and that row is the one selected when the Font tab first opens. Does the reporter's workflow reach the Font Features button through the per-writing-system row, or through Default settings? If it's the latter, they'd still see the OpenType list for Charis SIL 5.0, which is the symptom in LT-22351.

I follow why the default row can't assume Graphite (feature codes are font-specific, and that row spans writing systems that may use different fonts), so I'm not suggesting it should. I'm just trying to work out whether the reported case ends up covered.

@jasonleenaylor jasonleenaylor Sep 29, 2026

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.

Ken's report has the answer: he says the list "showed the open type list instead of the graphite list even though Charis SIL was selected in the style", and that to set features from the Font tab "you must also set the font and not rely on inheritance". Selecting a font in the Font tab is only possible on a writing-system row. The Default settings row lists only the magic names in a DropDownList, and the features button enables only for a real font, so on that row it stays disabled unless the style already stores a real font at the default level. Our repro copy of the project has Normal with no default-level font and Charis SIL in the seh override row, which is the row the fix reads IsGraphiteEnabled from. A style that does store a real default-level font would still get the OpenType list there, for the reason you gave.

return false;
var writingSystem = m_wsf.get_EngineOrNull(ws);
return writingSystem != null && writingSystem.IsGraphiteEnabled;
}

/// ------------------------------------------------------------------------------------
/// <summary>
/// Fills the font names.
Expand Down
154 changes: 154 additions & 0 deletions Src/FwCoreDlgs/FwCoreDlgsTests/FwWritingSystemSetupModelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,160 @@ public void Model_WritingSystemChanged_NotCalledOnIrrelevantChange()
Assert.That(writingSystemChanged, Is.False, "WritingSystemUpdated should not have been called after this change");
}

[Test]
public void Model_WritingSystemChanged_CalledOnDefaultFontFeaturesChange()
{
var writingSystemChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL");
testModel.Save();
testModel.WritingSystemUpdated += (sender, args) =>
{
writingSystemChanged = true;
};
// Changing only the font features must refresh views that cache rendered output.
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL") { Features = "cv43=1,smcp=1" };

@mark-sil mark-sil Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A question about what this pins down: this test (and the Graphite-toggle and reorder ones below) swaps in a whole new FontDefinition, but DefaultFontsControl mutates the existing one in place — m_ws.DefaultFont.Features = m_defaultFontFeaturesButton.FontFeatures in m_defaultFontFeaturesButton_FontFeatureSelected. That path only reaches RenderingChanged because WritingSystemDefinition.IsChanged folds in ChildrenIsChanged(_fonts).

Would one test that mutates DefaultFont.Features in place, rather than replacing the object, be worth adding? As written, if that child-level change tracking ever shifts upstream, workingWs.IsChanged goes false, RenderingChanged is never consulted, and LT-22351 reopens with every one of these tests still green.

@jasonleenaylor jasonleenaylor Sep 30, 2026

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.

Yes, worth adding, and it is in a0070f8: Model_WritingSystemChanged_CalledOnInPlaceFontFeaturesChange sets CurrentDefaultFont once, saves, then writes Features on that same object through CurrentDefaultFont, which is _currentWs.DefaultFont, so it is the statement the dialog makes in m_defaultFontFeaturesButton_FontFeatureSelected. It asserts WritingSystemUpdated, which Save only reaches through workingWs.IsChanged and then RenderingChanged, so if the child-level tracking in WritingSystemDefinition.IsChanged ever stopped folding in the fonts, this test goes red on the symptom Ken reported. The three existing tests stay, since replacing the definition is also a real path, through the font combo.

testModel.Save();
Assert.That(writingSystemChanged, Is.True, "WritingSystemUpdated should have been called after this change");
}

[Test]
public void Model_WritingSystemChanged_CalledOnInPlaceFontFeaturesChange()
{
var writingSystemChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL");
testModel.Save();
testModel.WritingSystemUpdated += (sender, args) =>
{
writingSystemChanged = true;
};
// The font dialog edits the existing definition rather than replacing it.
testModel.CurrentDefaultFont.Features = "cv43=1";
testModel.Save();
Assert.That(writingSystemChanged, Is.True, "WritingSystemUpdated should have been called after this change");
}

[Test]
public void Model_WritingSystemChanged_CalledOnGraphiteToggle()
{
var writingSystemChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL");
testModel.IsGraphiteEnabled = false;
testModel.Save();
testModel.WritingSystemUpdated += (sender, args) =>
{
writingSystemChanged = true;
};
testModel.IsGraphiteEnabled = true;
testModel.Save();
Assert.That(writingSystemChanged, Is.True, "WritingSystemUpdated should have been called after this change");
}

[Test]
public void Model_WritingSystemChanged_CalledOnRightToLeftChange()
{
var writingSystemChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.WritingSystemUpdated += (sender, args) =>
{
writingSystemChanged = true;
};
testModel.CurrentWsSetupModel.CurrentRightToLeftScript = true;
testModel.Save();
Assert.That(writingSystemChanged, Is.True, "WritingSystemUpdated should have been called after this change");
}

[Test]
public void Model_WritingSystemChanged_CalledOnNumberingSystemChange()
{
var writingSystemChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.WritingSystemUpdated += (sender, args) =>
{
writingSystemChanged = true;
};
testModel.CurrentWsSetupModel.CurrentNumberingSystemDefinition = NumberingSystemDefinition.CreateCustomSystem("abcdefghij");
testModel.Save();
Assert.That(writingSystemChanged, Is.True, "WritingSystemUpdated should have been called after this change");
}

[Test]
public void Model_WritingSystemChanged_NotCalledOnReorderedFontFeatures()
{
var writingSystemChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL") { Features = "smcp=1,cv43=1" };
testModel.Save();
testModel.WritingSystemUpdated += (sender, args) =>
{
writingSystemChanged = true;
};
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL") { Features = "cv43=1,smcp=1" };
testModel.Save();
Assert.That(writingSystemChanged, Is.False, "Reordering the same features is not a rendering change");
}

[Test]
public void Model_WritingSystemChanged_NotCalledWhenListAlsoChanged()
{
var writingSystemChanged = false;
var listChanged = false;
var mockWsManager = new Mock<IWritingSystemManager>();

var container = new TestWSContainer(new[] { "fr", "en" });
var testModel = new FwWritingSystemSetupModel(container,
FwWritingSystemSetupModel.ListType.Vernacular, mockWsManager.Object);
testModel.WritingSystemUpdated += (sender, args) => { writingSystemChanged = true; };
testModel.WritingSystemListUpdated += (sender, args) => { listChanged = true; };
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL");
testModel.MoveDown();
testModel.Save();
Assert.That(listChanged, Is.True, "WritingSystemListUpdated should have been called after this change");
Assert.That(writingSystemChanged, Is.False, "One refresh is enough when the list update already refreshes");
}

[Test]
public void Model_WritingSystemChanged_RaisedAfterUnitOfWorkCloses()
{
Cache.ActionHandlerAccessor.EndUndoTask();
var testModel = new FwWritingSystemSetupModel(Cache.LangProject, FwWritingSystemSetupModel.ListType.Vernacular,
Cache.ServiceLocator.WritingSystemManager, Cache);
var depthWhenRaised = -1;
testModel.WritingSystemUpdated += (sender, args) =>
{
depthWhenRaised = Cache.ActionHandlerAccessor.CurrentDepth;
};
testModel.CurrentDefaultFont = new FontDefinition("Charis SIL") { Features = "cv43=1,smcp=1" };
testModel.Save();
Assert.That(depthWhenRaised, Is.EqualTo(0), "Listeners refresh views, which must not run inside the save's unit of work");
}

[TestCase(FwWritingSystemSetupModel.ListType.Vernacular)]
[TestCase(FwWritingSystemSetupModel.ListType.Analysis)]
public void WritingSystemTitle_ChangesByType(FwWritingSystemSetupModel.ListType type)
Expand Down
39 changes: 33 additions & 6 deletions Src/FwCoreDlgs/FwWritingSystemSetupModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public List<WSListItemModel> WorkingList
public event EventHandler WritingSystemListUpdated;

/// <summary>
/// This event is fired only when the id or abbreviation for a writing system changes
/// Raised once after a save when an existing writing system's id, abbreviation, or
/// rendering settings changed. Not raised when WritingSystemListUpdated is.
/// </summary>
public event EventHandler WritingSystemUpdated;

Expand Down Expand Up @@ -663,6 +664,9 @@ public void Save()
{
uowHelper = new NonUndoableUnitOfWorkHelper(Cache.ActionHandlerAccessor);
}
// Listeners refresh every window, so notify once, after the unit of work has closed.
var needsRefresh = false;
var listChanged = false;

try
{
Expand Down Expand Up @@ -715,6 +719,7 @@ public void Save()
else if (workingWs.IsChanged)
{
var didAbbrevOrIdChange = origWs.Abbreviation != workingWs.Abbreviation;
var didRenderingChange = RenderingChanged(origWs, workingWs);
var oldId = origWs.Id;
var oldHandle = origWs.Handle;
// copy the working writing system content into the original writing system
Expand All @@ -729,9 +734,9 @@ public void Save()
}
didAbbrevOrIdChange = true;
}
if (didAbbrevOrIdChange)
if (didAbbrevOrIdChange || didRenderingChange)
{
WritingSystemUpdated?.Invoke(this, EventArgs.Empty);
needsRefresh = true;
}
}

Expand Down Expand Up @@ -767,13 +772,35 @@ public void Save()
}
finally
{
if (CurrentWsListChanged)
listChanged = CurrentWsListChanged;
if (uowHelper != null)
uowHelper.Dispose();
if (listChanged)
{
WritingSystemListUpdated?.Invoke(this, EventArgs.Empty);
}
if (uowHelper != null)
uowHelper.Dispose();
}
if (needsRefresh && !listChanged)
{
WritingSystemUpdated?.Invoke(this, EventArgs.Empty);
}
}

/// <summary>
/// Returns whether the two definitions would render text differently: another default
/// font, other font features once normalized, or a different Graphite setting, text
/// direction, or numbering system.
/// </summary>
private static bool RenderingChanged(CoreWritingSystemDefinition origWs, CoreWritingSystemDefinition workingWs)
{
if (origWs.DefaultFontName != workingWs.DefaultFontName)
return true;
if (FontFeatureSettings.NormalizePreservingLegacy(origWs.DefaultFontFeatures)
!= FontFeatureSettings.NormalizePreservingLegacy(workingWs.DefaultFontFeatures))
return true;
return origWs.IsGraphiteEnabled != workingWs.IsGraphiteEnabled
|| origWs.RightToLeftScript != workingWs.RightToLeftScript
|| !Equals(origWs.NumberingSystem, workingWs.NumberingSystem);
}

private static void AddOrMoveInList(ICollection<CoreWritingSystemDefinition> allWritingSystems, int desiredIndex, CoreWritingSystemDefinition workingWs)
Expand Down
Loading