From 719d8476493a503edb4a8259a76737e7741e2005 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Thu, 20 Aug 2026 17:26:10 +0200 Subject: [PATCH] Clean up FontRegistry's legacy collection handling FontRegistry still handles its collections the way it did before generics: explicit Iterators, casts through Object, and one variable reused for two unrelated values. It also lets FontRecord reach into the registry to retire its own fonts, mixing up who owns that decision. Simplify all of that, and write down what cleanOnDisplayDisposal == false already promises. No behavior change, other than put() now invalidating the replaced record entirely before notifying listeners rather than partly after, so the registry is consistent by the time they run. That listeners already see the new font when notified was untested, so a test now covers it. Assisted-by: Claude Opus 5 --- .../eclipse/jface/resource/FontRegistry.java | 82 +++++++++---------- .../tests/resources/FontRegistryTest.java | 16 ++++ 2 files changed, 54 insertions(+), 44 deletions(-) diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java index ec360f9dcca..8254e6887e1 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java @@ -19,7 +19,6 @@ import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; -import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.MissingResourceException; @@ -164,23 +163,22 @@ public Font getItalicFont() { } /** - * Add any fonts that were allocated for this record to the - * stale fonts. Anything that matches the default font will - * be skipped. - * @param defaultFont The system default. + * Return all of the fonts allocated by the receiver, that is the base + * font and whichever styled variants have been realized so far. + * @return the allocated fonts, never null */ - void addAllocatedFontsToStale(Font defaultFont) { - //Return all of the fonts allocated by the receiver. - //if any of them are the defaultFont then don't bother. - if (defaultFont != baseFont && baseFont != null) { - staleFonts.add(baseFont); + List getAllocatedFonts() { + List allocatedFonts = new ArrayList<>(3); + if (baseFont != null) { + allocatedFonts.add(baseFont); } - if (defaultFont != boldFont && boldFont != null) { - staleFonts.add(boldFont); + if (boldFont != null) { + allocatedFonts.add(boldFont); } - if (defaultFont != italicFont && italicFont != null) { - staleFonts.add(italicFont); + if (italicFont != null) { + allocatedFonts.add(italicFont); } + return allocatedFonts; } } @@ -368,7 +366,10 @@ public FontRegistry(Display display) { * the Display * @param cleanOnDisplayDisposal * whether all fonts allocated by this FontRegistry - * should be disposed when the display is disposed + * should be disposed when the display is disposed. If + * false, this registry never disposes a font by + * itself; the fonts it allocated are retained until + * {@link #clearCaches()} disposes them * @since 3.1 */ public FontRegistry(Display display, boolean cleanOnDisplayDisposal) { @@ -678,19 +679,19 @@ public Font getItalic(String symbolicName) { */ private FontRecord getFontRecord(String symbolicName) { Assert.isNotNull(symbolicName); - Object result = stringToFontRecord.get(symbolicName); - if (result != null) { - return (FontRecord) result; + FontRecord existingRecord = stringToFontRecord.get(symbolicName); + if (existingRecord != null) { + return existingRecord; } - result = stringToFontData.get(symbolicName); + FontData[] existingFontData = stringToFontData.get(symbolicName); FontRecord fontRecord; - if (result == null) { + if (existingFontData == null) { fontRecord = defaultFontRecord(); } else { - fontRecord = createFont(symbolicName, (FontData[]) result); + fontRecord = createFont(symbolicName, existingFontData); } if (fontRecord == null) { @@ -719,31 +720,14 @@ public boolean hasValueFor(String fontKey) { @Override protected void clearCaches() { - - Iterator iterator = stringToFontRecord.values().iterator(); - while (iterator.hasNext()) { - Object next = iterator.next(); - ((FontRecord) next).dispose(); - } - - disposeFonts(staleFonts.iterator()); + stringToFontRecord.values().forEach(FontRecord::dispose); stringToFontRecord.clear(); + staleFonts.forEach(Font::dispose); staleFonts.clear(); displayDisposeHooked.remove(Display.getCurrent()); } - /** - * Dispose of all of the fonts in this iterator. - * @param iterator over Collection of Font - */ - private void disposeFonts(Iterator iterator) { - while (iterator.hasNext()) { - Object next = iterator.next(); - ((Font) next).dispose(); - } - } - /** * Hook a dispose listener on the SWT display. */ @@ -821,16 +805,26 @@ private void put(String symbolicName, FontData[] fontData, boolean update) { return; } - FontRecord oldFont = stringToFontRecord - .remove(symbolicName); stringToFontData.put(symbolicName, fontData); + invalidate(symbolicName); if (update) { fireMappingChanged(symbolicName, existing, fontData); } + } - if (oldFont != null) { - oldFont.addAllocatedFontsToStale(defaultFontRecord().getBaseFont()); + /** + * Drop the realized font record for the given symbolic name, if any, and + * defer disposal of the fonts it had allocated until it is safe to dispose + * them, since they may still be in use. The default font is kept, as it + * stays in use under its own symbolic name. + */ + private void invalidate(String symbolicName) { + FontRecord replacedRecord = stringToFontRecord.remove(symbolicName); + if (replacedRecord == null) { + return; } + Font defaultFont = defaultFontRecord().getBaseFont(); + replacedRecord.getAllocatedFonts().stream().filter(font -> font != defaultFont).forEach(staleFonts::add); } /** diff --git a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java index 7aa7f7dfcd0..93f8cd5808c 100644 --- a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java +++ b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java @@ -214,6 +214,22 @@ public void put_firesPropertyChangeOnlyWhenDataActuallyChanges() { assertEquals(2, events.size()); } + @Test + public void put_notifiesListenersOnlyAfterTheNewFontIsInEffect() { + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + Font originalFont = fontRegistry.get("myfont"); + + AtomicReference fontSeenByListener = new AtomicReference<>(); + fontRegistry.addListener(event -> fontSeenByListener.set(fontRegistry.get("myfont"))); + + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 18, SWT.NORMAL) }); + + assertNotEquals(originalFont, fontSeenByListener.get(), + "a listener must not still see the replaced font when it is notified"); + assertEquals(18, fontSeenByListener.get().getFontData()[0].getHeight()); + } + @Test public void put_withNewData_disposesOldFontOnlyOnDisplayDispose() { assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows");