Example apps — expose all 8 fonts + font-size slider#211
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a global font-size slider and expands the available math fonts (adding New Computer Modern, TeX Gyre Pagella, STIX Two, Fira Math, and Noto Sans Math) across the macOS, iOS, and SwiftMath examples. While these additions improve customization, several layout issues were identified. Specifically, in the iOS ViewController, the logic for finding the render panel constraint is too broad and could match the safe area layout guide, leading to layout conflicts. Additionally, across all platforms (macOS, iOS, and SwiftUI), changing the font size dynamically causes vertical clipping because the label heights and content view heights remain fixed at their startup values instead of scaling proportionally with the font size.
| for (NSLayoutConstraint* c in self.view.constraints) { | ||
| if (c.firstItem == self.scrollView && c.firstAttribute == NSLayoutAttributeTop) { | ||
| renderPanelRef = c.secondItem; | ||
| c.priority = UILayoutPriorityDefaultLow; | ||
| break; | ||
| } else if (c.secondItem == self.scrollView && c.secondAttribute == NSLayoutAttributeTop) { | ||
| renderPanelRef = c.firstItem; | ||
| c.priority = UILayoutPriorityDefaultLow; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current logic for finding the render panel constraint is too broad. If the scroll view's top constraint is anchored to the safe area layout guide (which is common in modern iOS layouts), renderPanelRef will be resolved to self.view.safeAreaLayoutGuide. Pinning the top of the size slider to the bottom of the safe area layout guide (which is at the bottom of the screen) will break the layout and cause constraint conflicts. Restrict the search to constraints where the second item is anchored via NSLayoutAttributeBottom to ensure we only match the actual render panel view.
for (NSLayoutConstraint* c in self.view.constraints) {
if (c.firstItem == self.scrollView && c.firstAttribute == NSLayoutAttributeTop) {
if (c.secondAttribute == NSLayoutAttributeBottom) {
renderPanelRef = c.secondItem;
c.priority = UILayoutPriorityDefaultLow;
break;
}
} else if (c.secondItem == self.scrollView && c.secondAttribute == NSLayoutAttributeTop) {
if (c.firstAttribute == NSLayoutAttributeBottom) {
renderPanelRef = c.firstItem;
c.priority = UILayoutPriorityDefaultLow;
break;
}
}
}There was a problem hiding this comment.
Done in 090db84. The search now also requires the other end of the constraint to be NSLayoutAttributeBottom, so it only matches scrollView.top == renderPanel.bottom and can no longer latch onto a scrollView.top-to-safe-area constraint.
| - (void)sizeChanged:(UISlider *)sender | ||
| { | ||
| CGFloat size = (CGFloat)sender.value; | ||
| for (MTMathUILabel* label in self.demoLabels) { label.fontSize = size; } | ||
| for (MTMathUILabel* label in self.labels) { label.fontSize = size; } | ||
| } |
There was a problem hiding this comment.
When the font size is changed dynamically via the slider, the labels' font sizes are updated, but their height constraints (which are fixed at startup) and the content view's total height are not updated. This causes the math formulas to be vertically clipped when the font size is increased, and the scrollable area will not adjust correctly. Consider dynamically scaling the height constraints of the labels and the content view's height constraint proportionally to the font size ratio (size / 15.0), or transition to a fully dynamic Auto Layout system without fixed height constraints.
There was a problem hiding this comment.
Fixed in 090db84. Each label now keeps a reference to its height constraint; sizeChanged: rescales it from the startup baseline font size (demo labels 15, test labels 20) and recomputes the content view height constraint, so formulas grow with the slider instead of clipping.
| - (void)changeSize:(NSSlider *)sender | ||
| { | ||
| CGFloat size = (CGFloat)sender.doubleValue; | ||
| for (MTMathUILabel* label in self.demoLabels) { label.fontSize = size; } | ||
| for (MTMathUILabel* label in self.labels) { label.fontSize = size; } | ||
| } |
There was a problem hiding this comment.
When the font size is changed dynamically via the slider, the labels' font sizes are updated, but their height constraints (which are fixed at startup) and the content view's frame height are not updated. This causes the math formulas to be vertically clipped when the font size is increased, and the scrollable area will not adjust correctly. Consider dynamically scaling the height constraints of the labels and the content view's frame height proportionally to the font size ratio (size / 15.0), or transition to a fully dynamic Auto Layout system without fixed height constraints.
There was a problem hiding this comment.
Fixed in 090db84. changeSize: now rescales each label height constraint from its baseline (demo 15, test 20) and grows the document view frame height to match.
| MathLabel(latex: formula.latex, fontSize: fontSize, mode: formula.mode, | ||
| font: font.font(size: fontSize)) | ||
| .frame(height: formula.height) |
There was a problem hiding this comment.
The height of the MathLabel frame is fixed to formula.height, which was designed for the default font size of 20. When the global fontSize is increased (up to 40), the formula will be vertically clipped. Scale the frame height proportionally to the font size ratio to prevent clipping.
| MathLabel(latex: formula.latex, fontSize: fontSize, mode: formula.mode, | |
| font: font.font(size: fontSize)) | |
| .frame(height: formula.height) | |
| MathLabel(latex: formula.latex, fontSize: fontSize, mode: formula.mode, | |
| font: font.font(size: fontSize)) | |
| .frame(height: formula.height * (fontSize / formula.fontSize)) |
There was a problem hiding this comment.
Fixed in 090db84 — the card height is now formula.height * (fontSize / formula.fontSize).
| font: font.font(size: testFontSize(at: i)) | ||
| font: font.font(size: fontSize) | ||
| ) | ||
| .frame(height: testHeight(at: i)) |
There was a problem hiding this comment.
The height of the MathLabel frame in the gallery is fixed to testHeight(at: i), which was designed for the original font sizes (15 for most, 30 for index 8, 10 for index 9). When the global fontSize is increased, the formulas will be vertically clipped. Scale the frame height proportionally to the font size ratio to prevent clipping.
| .frame(height: testHeight(at: i)) | |
| .frame(height: testHeight(at: i) * (fontSize / (i == 8 ? 30 : (i == 9 ? 10 : 15)))) |
There was a problem hiding this comment.
Fixed in 090db84. Restored the per-entry baseline (30 at index 8, 10 at index 9, else 15) as testBaselineFontSize(at:) and scale the row height by fontSize / that baseline.
kostub
force-pushed
the
feature/add-math-fonts-pr2
branch
from
6e6fb29 to
8b36f91
Compare
…match Addresses gemini-code-assist review on PR #211: - iOS: restrict the render-panel constraint search to scrollView.top == <panel>.bottom (checking the other end's attribute) so it can't match a scrollView.top-to-safe-area-top constraint and pin the slider off-screen. - iOS/macOS/SwiftUI: the size slider only changed fontSize, leaving the fixed startup label/content heights, which clipped formulas at larger sizes. Track each label's height constraint and rescale it (and the content/document height) from its startup baseline font size — demo labels 15, test labels 20 on iOS/macOS; per-entry baselines in SwiftUI. All three example apps build.
kostub
force-pushed
the
feature/add-math-fonts-pr3
branch
from
d2a99a6 to
090db84
Compare
|
/gemini review this PR |
There was a problem hiding this comment.
Code Review
This pull request introduces breaking API changes to MTFontManager by removing per-font convenience methods in favor of a generic fontWithName:size: accessor and exposing new public font name constants for eight bundled fonts. It also refactors the manager to enforce singleton usage and updates the iOS, macOS, and Swift example apps with dynamic font size sliders that scale label heights to prevent clipping. Feedback on these changes highlights a critical crash risk in ViewController.m caused by mutating a required constraint's priority at runtime, which should be resolved by deactivating the constraint instead. Additionally, it is recommended to use the newly introduced public font constants in AppDelegate.m rather than duplicating raw string literals.
| UIView* renderPanelRef = nil; | ||
| for (NSLayoutConstraint* c in self.view.constraints) { | ||
| // Match only scrollView.top == <renderPanel>.bottom. Checking the other | ||
| // end's attribute avoids accidentally matching scrollView.top anchored to | ||
| // the safe area layout guide's top, which would pin the slider off-screen. | ||
| if (c.firstItem == self.scrollView && c.firstAttribute == NSLayoutAttributeTop | ||
| && c.secondAttribute == NSLayoutAttributeBottom) { | ||
| renderPanelRef = c.secondItem; | ||
| c.priority = UILayoutPriorityDefaultLow; | ||
| break; | ||
| } else if (c.secondItem == self.scrollView && c.secondAttribute == NSLayoutAttributeTop | ||
| && c.firstAttribute == NSLayoutAttributeBottom) { | ||
| renderPanelRef = c.firstItem; | ||
| c.priority = UILayoutPriorityDefaultLow; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Mutating the priority of an active constraint after it has been added to a view is only supported if the priority remains in the non-required range (i.e., less than 1000). Since constraints created in Interface Builder/XIBs default to a priority of 1000 (Required), attempting to change c.priority to UILayoutPriorityDefaultLow at runtime will throw an exception and crash the application. Deactivating the constraint by setting c.active = NO; is the safe and standard way to replace a constraint at runtime.
UIView* renderPanelRef = nil;
for (NSLayoutConstraint* c in self.view.constraints) {
// Match only scrollView.top == <renderPanel>.bottom. Checking the other
// end's attribute avoids accidentally matching scrollView.top anchored to
// the safe area layout guide's top, which would pin the slider off-screen.
if (c.firstItem == self.scrollView && c.firstAttribute == NSLayoutAttributeTop
&& c.secondAttribute == NSLayoutAttributeBottom) {
renderPanelRef = c.secondItem;
c.active = NO;
break;
} else if (c.secondItem == self.scrollView && c.secondAttribute == NSLayoutAttributeTop
&& c.firstAttribute == NSLayoutAttributeBottom) {
renderPanelRef = c.firstItem;
c.active = NO;
break;
}
}There was a problem hiding this comment.
Fixed in cb98a7d. XIB constraints are required (1000), and mutating an active constraint's priority to/from required throws — so the original scrollView.top==renderPanel.bottom constraint is now deactivated (c.active = NO) instead. The new required scrollView.top==slider.bottom constraint replaces it.
| static NSString *const kMacFontNames[] = { | ||
| @"Latin Modern Math", @"TeX Gyre Termes", @"XITS Math", | ||
| @"New Computer Modern", @"TeX Gyre Pagella", @"STIX Two", | ||
| @"Fira Math", @"Noto Sans Math", | ||
| }; | ||
| static NSString *const kMacFontKeys[] = { | ||
| @"latinmodern-math", @"texgyretermes-math", @"xits-math", | ||
| @"newcm-math", @"texgyrepagella-math", @"stixtwo-math", | ||
| @"firamath", @"notosansmath", | ||
| }; | ||
| static const NSUInteger kMacFontCount = 8; |
There was a problem hiding this comment.
Instead of duplicating the raw string literals for the font keys in a global static array, we can define kMacFontKeys locally inside the changeFont: method using the newly exposed public constants (MTFontNameLatinModern, etc.) from MTFontManager.h. This improves maintainability and keeps the implementation consistent with ViewController.m.
static NSString *const kMacFontNames[] = {
@"Latin Modern Math", @"TeX Gyre Termes", @"XITS Math",
@"New Computer Modern", @"TeX Gyre Pagella", @"STIX Two",
@"Fira Math", @"Noto Sans Math",
};
static const NSUInteger kMacFontCount = 8;There was a problem hiding this comment.
Fixed in cb98a7d, using the public MTFontName* constants to match the iOS example. The array had to move local to changeFont: rather than stay a file-scope static — the constants are runtime extern NSString *const values, not compile-time constants, so they can't initialize a static array.
| - (void)changeFont:(NSPopUpButton *)sender | ||
| { | ||
| NSInteger i = sender.indexOfSelectedItem; | ||
| if (i < 0 || (NSUInteger)i >= kMacFontCount) return; | ||
| NSString* key = kMacFontKeys[i]; | ||
| for (MTMathUILabel* label in self.demoLabels) { | ||
| label.font = [[MTFontManager fontManager] fontWithName:key size:label.font.fontSize]; | ||
| } | ||
| for (MTMathUILabel* label in self.labels) { | ||
| label.font = [[MTFontManager fontManager] fontWithName:key size:label.font.fontSize]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Define the kMacFontKeys array locally using the public constants from MTFontManager.h to avoid hardcoded string literals.
- (void)changeFont:(NSPopUpButton *)sender
{
NSInteger i = sender.indexOfSelectedItem;
if (i < 0 || (NSUInteger)i >= kMacFontCount) return;
NSString *const kMacFontKeys[] = {
MTFontNameLatinModern, MTFontNameTermes, MTFontNameXITS,
MTFontNameNewComputerModern, MTFontNamePagella, MTFontNameSTIXTwo,
MTFontNameFiraMath, MTFontNameNotoSansMath,
};
NSString* key = kMacFontKeys[i];
for (MTMathUILabel* label in self.demoLabels) {
label.font = [[MTFontManager fontManager] fontWithName:key size:label.font.fontSize];
}
for (MTMathUILabel* label in self.labels) {
label.font = [[MTFontManager fontManager] fontWithName:key size:label.font.fontSize];
}
}There was a problem hiding this comment.
Done in cb98a7d — kMacFontKeys is now a local array in changeFont: built from MTFontNameLatinModern, MTFontNameTermes, etc.
…S font keys via public constants iOS: changing an active required (1000) XIB constraint's priority to/from required throws at runtime; deactivate it instead. macOS: replace hardcoded font-key string literals with the public MTFontName* constants (local array, since they're runtime extern consts), matching the iOS example. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ight cards
Testing feedback on the Playground/Examples tabs:
- Size control drifted as the font picker's width changed; replace the
Slider with a Stepper pinned to the trailing edge (Spacer + fixedSize).
- The stepper also shows the current size ("Size: NN"), which the slider did not.
- Examples cards used a guessed fixed-height table that clipped tall formulae
(e.g. Rogers-Ramanujan) at every font size; drop the fixed frame and let the
label size to its intrinsic content height. Removes the now-unused per-formula
height/fontSize metadata.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…: drop orphan XIB views
iOS testing feedback:
- Replaced the font-size slider with a stepper in the top row beside the font
and colour pickers, showing the current point size ("15pt"). The font field's
fixed width was removed so it absorbs the row's slack and the stepper stays
fully on-screen and tappable at any width (incl. iPhone 16 Pro). Removed the
redundant "Font" label to make room.
- Formulae no longer clip horizontally when the font size grows: the content
view may now exceed the viewport width (driven by each label's intrinsic
width) so wide formulae scroll horizontally instead of being cut off.
macOS testing feedback:
- Removed the stray blue box: an orphaned MTMathUILabel (plus an old "Update!"
button and text field) left in MainMenu.xib from the original template, which
AppDelegate never used. Also removed the matching stale outlet/action
connections (screen, inputTextField, clickUpdateButton:).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces global font size controls and expands the supported math fonts across the macOS, iOS, and SwiftMath examples. It refactors the layout logic to dynamically scale label heights and content views, enabling horizontal scrolling for wide formulas and preventing clipping. The feedback highlights a potential issue where arrays in ViewController.m are not initialized if instantiated via initWithCoder:, which could lead to silent failures. Additionally, the test formulas at indices 8 and 9 in both the macOS and iOS examples should be scaled using their specific baseline font sizes (30 and 10) rather than a hardcoded baseline of 20.0 to ensure correct layout heights.
| self.demoHeightConstraints = [[NSMutableArray alloc] init]; | ||
| self.testHeightConstraints = [[NSMutableArray alloc] init]; | ||
| self.demoBaseHeights = [[NSMutableArray alloc] init]; | ||
| self.testBaseHeights = [[NSMutableArray alloc] init]; |
There was a problem hiding this comment.
Since this View Controller is defined in View.xib (and potentially used in storyboards), it can be instantiated via initWithCoder: instead of initWithNibName:bundle:. If that happens, these arrays will remain nil, causing silent failures when trying to add or update labels. Consider implementing initWithCoder: or moving the initialization of these arrays to a common initializer method (e.g., commonInit) called by both initWithNibName:bundle: and initWithCoder:.
There was a problem hiding this comment.
This VC is created in exactly one place — AppDelegate.m: [[ViewController alloc] initWithNibName:@"View" bundle:nil]. There is no storyboard (no UIMainStoryboardFile/NSMainNibFile in Info.plist), and View.xib archives the view hierarchy with File's Owner = ViewController, not the controller object itself, so initWithCoder: is never invoked here. The nil-array path the comment guards against does not exist in this app, so I'm leaving the single designated initializer as-is rather than adding a commonInit for an unreachable instantiation path (YAGNI).
| documentHeight += 30; // gap between sections | ||
| for (NSUInteger i = 0; i < self.labels.count; i++) { | ||
| self.labels[i].fontSize = size; | ||
| CGFloat h = self.testBaseHeights[i].doubleValue * (size / 20.0); |
There was a problem hiding this comment.
The test formulas at indices 8 and 9 have baseline font sizes of 30 and 10 respectively, rather than the default 20. Scaling their heights using a hardcoded baseline of 20.0 will cause incorrect layout heights (potentially clipping at smaller sizes or leaving excessive empty space). Consider scaling them using their correct baseline font sizes, similar to the logic implemented in the Swift example.
CGFloat baseline = 20.0;
if (i == 8) baseline = 30.0;
else if (i == 9) baseline = 10.0;
CGFloat h = self.testBaseHeights[i].doubleValue * (size / baseline);There was a problem hiding this comment.
This doesn't apply to the macOS example as it currently stands. The per-label labels[8].fontSize = 30 / labels[9].fontSize = 10 overrides were removed in this PR (see the PR description) — current code sets only textAlignment for those indices. Every test label now renders at the default MTMathUILabel.fontSize of 20 (MTMathUILabel.m:44), so size / 20.0 is correct for all of them. Special-casing 30/10 here would scale indices 8/9 against a baseline they no longer use and clip/over-size them. The per-entry baselines still legitimately exist in the Swift gallery because those formula structs carry their own fontSize; iOS/macOS no longer do.
| total += 30; // gap between sections | ||
| for (NSUInteger i = 0; i < self.labels.count; i++) { | ||
| self.labels[i].fontSize = size; | ||
| CGFloat h = self.testBaseHeights[i].doubleValue * (size / 20.0); |
There was a problem hiding this comment.
The test formulas at indices 8 and 9 have baseline font sizes of 30 and 10 respectively, rather than the default 20. Scaling their heights using a hardcoded baseline of 20.0 will cause incorrect layout heights (potentially clipping at smaller sizes or leaving excessive empty space). Consider scaling them using their correct baseline font sizes, similar to the logic implemented in the Swift example.
CGFloat baseline = 20.0;
if (i == 8) baseline = 30.0;
else if (i == 9) baseline = 10.0;
CGFloat h = self.testBaseHeights[i].doubleValue * (size / baseline);There was a problem hiding this comment.
Same as the macOS comment: the labels[8].fontSize = 30 / labels[9].fontSize = 10 overrides were removed in this PR, so all test labels render at the default MTMathUILabel.fontSize of 20 (MTMathUILabel.m:44). size / 20.0 is correct for every test label; reintroducing a 30/10 baseline for indices 8/9 would mis-scale them since those labels no longer render at 30/10.
1 participant
Goal
All three example apps list all 8 fonts and gain a font-size slider (range 10–40, default 15) that applies one global override size to every label.
Plan:
docs/plans/2026-05-30-add-math-fonts.md(items 15–17)LLD:
docs/lld/2026-05-30-add-math-fonts.mdStack
Commits
[item 15]iOS example: 8-font wheel + global font-size slider[item 16]SwiftMathExample: 8 fonts (.menu) + shared font-size slider[item 17]macOS example: code-built font popup + font-size sliderChanges per app
iOS (
iosMathExample):fontNames/kFontKeysextended to 8 entries;UISlider(10–40pt, default 15) added in code above the scroll view;sizeChanged:action loops all labels; per-labellabels[8].fontSize=30/labels[9].fontSize=10overrides removed.Swift (
SwiftMathExample): 5 newMathFontenum cases; Picker style changed from.segmentedto.menu;@State fontSize: CGFloat = 15shared across all tabs;Slider(value: $fontSize, in: 10...40)in PlaygroundTab; ExampleCard and GalleryTab use sharedfontSize.macOS (
MacOSMathExample):NSPopUpButton+NSSlideradded as a code-built 36pt header above the scroll view;changeFont:/changeSize:actions wired; per-label size overrides removed.Testing
All three apps build (
BUILD SUCCEEDED). No storyboard/XIB edits.