From 9fec9452e221152240ce51ffc5719e8c3b731927 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Wed, 19 Aug 2026 16:03:35 +0200 Subject: [PATCH 1/7] Make FontRegistry's table of font data thread safe FontRegistry keeps its symbolic-name-to-FontData mapping in a plain HashMap that is read and written from arbitrary threads: - put(String, FontData[]) is public and carries no UI-thread restriction, unlike the methods that hand out Font instances, - getFontData(), getDescriptor(), getKeySet() and hasValueFor() read it and are likewise unrestricted, with getKeySet() even handing out a live view of the table, and - createFont() writes to it (via the internal put() overload) from whichever thread realizes a font, i.e. from any SWT Display's thread. Unsynchronized HashMap mutation from several threads can corrupt the table itself, not merely produce stale reads. Use a ConcurrentHashMap instead, so concurrent access is safe and getKeySet() returns a weakly-consistent view rather than one that may fail arbitrarily while being iterated. Also fold the internal put()'s read-compare-write of that table into a single atomic Map#put: the previous get()/put() pair could interleave so that two threads both concluded the mapping was unchanged, or that the mapping they replaced was one that had already been overwritten. Storing the given array when it is content-equal to the existing one is a no-op for every reader, so this does not change behavior. The table of realized FontRecords is deliberately left alone here: it is guarded by the documented UI-thread restriction of the methods returning Font instances. Assisted-by: Claude Opus 5 --- .../src/org/eclipse/jface/resource/FontRegistry.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 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..baf1e92f66d 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 @@ -196,7 +196,7 @@ void addAllocatedFontsToStale(Font defaultFont) { * (key type: String, * value type: org.eclipse.swt.graphics.FontData[]). */ - private final Map stringToFontData = new HashMap<>(7); + private final Map stringToFontData = new ConcurrentHashMap<>(7); /** * Collection of Fonts that are now stale to be disposed @@ -816,14 +816,15 @@ private void put(String symbolicName, FontData[] fontData, boolean update) { Assert.isNotNull(symbolicName); Assert.isNotNull(fontData); - FontData[] existing = stringToFontData.get(symbolicName); + // single atomic read-modify-write; replacing an equal mapping with the + // given, content-equal one is a no-op for every reader + FontData[] existing = stringToFontData.put(symbolicName, fontData); if (Arrays.equals(existing, fontData)) { return; } FontRecord oldFont = stringToFontRecord .remove(symbolicName); - stringToFontData.put(symbolicName, fontData); if (update) { fireMappingChanged(symbolicName, existing, fontData); } From e5f012d3f098efffcc22b40b46b3e44f7e2775c5 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Wed, 19 Aug 2026 16:04:16 +0200 Subject: [PATCH 2/7] Guard FontRegistry.createFont against filterData returning null filterData() is documented to return null for an empty font list, and does so, but createFont() dereferences its result unconditionally to check for a zero length. Registering an empty FontData[] under a symbolic name and then looking that name up therefore fails with java.lang.NullPointerException: Cannot read the array length because "validData" is null instead of falling back to the default font the way an unresolvable name otherwise does. Treat null like the empty result it stands for, and add a regression test registering an empty FontData[] and asserting the default-font fallback. Note that filterData() never actually returns a zero-length array - it falls back to the first entry when nothing matches - so the existing length check alone was dead code. Assisted-by: Claude Opus 5 --- .../org/eclipse/jface/resource/FontRegistry.java | 2 +- .../jface/tests/resources/FontRegistryTest.java | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) 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 baf1e92f66d..7da5a12b6be 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 @@ -497,7 +497,7 @@ private FontRecord createFont(String symbolicName, FontData[] fonts) { } FontData[] validData = filterData(fonts, display); - if (validData.length == 0) { + if (validData == null || validData.length == 0) { //Nothing specified return null; } 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 a2857e620ec..c77f9c08fbd 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 @@ -187,6 +187,20 @@ public void get_forNameThatWasNeverRegistered_returnsDefaultFontAndIsStableAfter assertSame(first, second); } + @Test + public void get_forNameRegisteredWithoutAnyFontData_returnsDefaultFont() { + FontRegistry fontRegistry = new FontRegistry(); + // an empty array leaves nothing to filter, so filterData() yields no usable data at all + fontRegistry.put("fontWithoutData", new FontData[0]); + + Font first = fontRegistry.get("fontWithoutData"); + Font second = fontRegistry.get("fontWithoutData"); + + assertSame(fontRegistry.get(JFaceResources.DEFAULT_FONT), first, + "a name that cannot be resolved to any font data must fall back to the default font"); + assertSame(first, second); + } + @Test public void getBoldAndGetItalic_returnSameInstanceOnRepeatedCalls() { FontRegistry fontRegistry = new FontRegistry(); From 19d886f0373e030f39712bb7ba2f2522bbe9d231 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Wed, 19 Aug 2026 15:10:38 +0200 Subject: [PATCH 3/7] Avoid eagerly creating the default font as a side effect of put() When put() replaced an already-realized font, it obtained the current default font via defaultFontRecord() purely to compare it against the replaced font's instances for staleness. defaultFontRecord() does not just look up an already-realized default font, it also creates and caches one if none exists yet. As a result, replacing an unrelated symbolic name's font could silently allocate and cache a native default-font handle earlier than it would otherwise have been needed, merely as a side effect of an identity comparison. Creating that font is not the only consequence. createFont() registers the data it used under the symbolic name it created the font for, so replacing an unrelated font also registered font data for the default font that no client ever asked for. That is directly observable: hasValueFor(JFaceResources.DEFAULT_FONT) flipped to true, and getKeySet() started reporting the default font, purely because some other name had been re-put. Look up the already-cached default font record directly instead, tolerating that it may not exist yet (in which case the replaced font's instances are unconditionally treated as stale, same as before, since they can never equal a still-unrealized default font). Add a regression test asserting that replacing a realized font leaves the default font unregistered. Beyond the observable effect above, this also matters once the font cache stops being a single shared cache, so that comparing against a display's default font does not have the side effect of allocating that default font on an unrelated display. Assisted-by: Claude Opus 5 --- .../org/eclipse/jface/resource/FontRegistry.java | 4 +++- .../jface/tests/resources/FontRegistryTest.java | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) 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 7da5a12b6be..56ce389a29f 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 @@ -830,7 +830,9 @@ private void put(String symbolicName, FontData[] fontData, boolean update) { } if (oldFont != null) { - oldFont.addAllocatedFontsToStale(defaultFontRecord().getBaseFont()); + FontRecord defaultRecord = stringToFontRecord.get(JFaceResources.DEFAULT_FONT); + Font defaultFont = defaultRecord != null ? defaultRecord.getBaseFont() : null; + oldFont.addAllocatedFontsToStale(defaultFont); } } 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 c77f9c08fbd..ccca67ead11 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 @@ -285,6 +285,22 @@ public void put_withNewData_invalidatesPreviouslyCachedFont() { assertEquals(18, updated.getFontData()[0].getHeight()); } + @Test + public void put_replacingRealizedFont_doesNotRegisterDefaultFontAsSideEffect() { + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + fontRegistry.get("myfont"); // realize it, so replacing it has fonts to retire + + // retiring those fonts compares them against the default font, which must not + // realize and register a default font that nobody has asked for yet + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 18, SWT.NORMAL) }); + + assertFalse(fontRegistry.hasValueFor(JFaceResources.DEFAULT_FONT), + "replacing an unrelated font must not register default font data as a side effect"); + assertFalse(fontRegistry.getKeySet().contains(JFaceResources.DEFAULT_FONT), + "replacing an unrelated font must not add the default font to the key set"); + } + @Test public void hasValueForAndGetKeySet_reflectOnlyRegisteredNames() { FontRegistry fontRegistry = new FontRegistry(); From 199aaf3dd5de93c5489f5a7e9ffd6c77eb2a5131 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Fri, 14 Aug 2026 17:41:57 +0200 Subject: [PATCH 4/7] Centralize FontRegistry font-record caching in createFont The mapping from a symbolic font name to its realized FontRecord was cached at each call site (defaultFontRecord() and getFontRecord()) after invoking createFont(), rather than by createFont() itself. As a side effect, a symbolic name that had never been explicitly registered and only ever resolved through the default-font fallback got cached as an alias pointing at the very same FontRecord instance used for the default font. Registering real data for such a name later would then remove only that alias, but still treat the (still live and cached) default record as replaced, incorrectly queuing its already-realized bold/italic fonts for disposal even though the default font remains in active use under its own name. Move the caching into createFont(), where the record is actually created, so every symbolic name is cached exactly once, at the single place responsible for creating it. This removes the accidental aliasing and the incorrect staleness it could cause. Note that with the registry's current single, display-wide cache, this inconsistency has no externally observable effect: disposing any display already tears down the entire cache and all stale fonts together in one step, so the aliased default font and its "stale" bold/italic variants are always disposed at the same time regardless. The fix still removes the incorrect internal state, and matters once disposal is no longer coupled that way, e.g. for a per-display cache where a font may otherwise be readable, writable and disposed of from separate places. With the caching gone from the end of getFontRecord(), the early return in its non-UI-thread branch no longer skips anything: it and the final return became identical. Drop it, together with the comment explaining that it avoids caching the default font under the requested name. What that comment promises still holds, a later lookup from the UI thread still creates the proper font, but that is now a consequence of createFont() doing the caching rather than of this return. Add a characterization test pinning that put() on a name only ever resolved via the default-font fallback does not disturb the default font's already-realized bold/italic instances. It does not fail without this fix given the current coupling described above, but documents the intended contract and guards against regressions once that coupling changes. Assisted-by: Claude Sonnet 5 --- .../org/eclipse/jface/resource/FontRegistry.java | 8 +++----- .../jface/tests/resources/FontRegistryTest.java | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 5 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 56ce389a29f..4e89ff06843 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 @@ -505,7 +505,9 @@ private FontRecord createFont(String symbolicName, FontData[] fonts) { //Do not fire the update from creation as it is not a property change put(symbolicName, validData, false); Font newFont = new Font(display, validData); - return new FontRecord(newFont, validData); + FontRecord newRecord = new FontRecord(newFont, validData); + stringToFontRecord.put(symbolicName, newRecord); + return newRecord; } private Display getDisplayAndHookForDisposal() { @@ -582,7 +584,6 @@ record = createFont(JFaceResources.DEFAULT_FONT, fontData); record = createFont(JFaceResources.DEFAULT_FONT, defaultFont.getFontData()); defaultFont.dispose(); } - stringToFontRecord.put(JFaceResources.DEFAULT_FONT, record); return record; } @@ -698,13 +699,10 @@ private FontRecord getFontRecord(String symbolicName) { if (Display.getCurrent() == null) { // log error but don't throw an exception to preserve existing functionality String msg = "Unable to create font \"" + symbolicName + "\" in a non-UI thread. Using default font instead."; //$NON-NLS-1$ //$NON-NLS-2$ Policy.logException(new SWTException(msg)); - return fontRecord; // don't add it to the cache; if later asked from UI thread, a proper font will be created } } - stringToFontRecord.put(symbolicName, fontRecord); return fontRecord; - } @Override 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 ccca67ead11..a54fa29f969 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 @@ -140,6 +140,22 @@ public void defaultFont_isStableAcrossLookupsOfOtherNames() { assertSame(defaultFont, fontRegistry.get(JFaceResources.DEFAULT_FONT)); } + @Test + public void put_onNameOnlyResolvedViaDefaultFallback_doesNotStaleDefaultFontsBoldAndItalic() { + FontRegistry fontRegistry = new FontRegistry(); + Font defaultBold = fontRegistry.getBold(JFaceResources.DEFAULT_FONT); + Font defaultItalic = fontRegistry.getItalic(JFaceResources.DEFAULT_FONT); + + // never explicitly registered, so this only ever resolves via the default-font fallback + fontRegistry.get("neverRegisteredName"); + + // registering data for that name must not disturb the still-live default font record + fontRegistry.put("neverRegisteredName", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + + assertSame(defaultBold, fontRegistry.getBold(JFaceResources.DEFAULT_FONT)); + assertSame(defaultItalic, fontRegistry.getItalic(JFaceResources.DEFAULT_FONT)); + } + @Test public void get_fontFromNonUIThreadFallback_doesNotOverwriteDefaultFont() throws Throwable { FontRegistry fontRegistry = new FontRegistry(); From b05c12eafcb61f32c170e4d6a70a2cac4b6fd9fd Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Thu, 20 Aug 2026 17:26:10 +0200 Subject: [PATCH 5/7] 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 | 85 +++++++++---------- .../tests/resources/FontRegistryTest.java | 16 ++++ 2 files changed, 55 insertions(+), 46 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 4e89ff06843..a035dfefbeb 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) { @@ -679,19 +680,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) { @@ -717,31 +718,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,17 +805,26 @@ private void put(String symbolicName, FontData[] fontData, boolean update) { return; } - FontRecord oldFont = stringToFontRecord - .remove(symbolicName); + invalidate(symbolicName); if (update) { fireMappingChanged(symbolicName, existing, fontData); } + } - if (oldFont != null) { - FontRecord defaultRecord = stringToFontRecord.get(JFaceResources.DEFAULT_FONT); - Font defaultFont = defaultRecord != null ? defaultRecord.getBaseFont() : null; - oldFont.addAllocatedFontsToStale(defaultFont); + /** + * 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; } + FontRecord defaultRecord = stringToFontRecord.get(JFaceResources.DEFAULT_FONT); + Font defaultFont = defaultRecord != null ? defaultRecord.getBaseFont() : null; + 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 a54fa29f969..0de2bac14f5 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 @@ -244,6 +244,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"); From 94500d176c659e4ba6cd78a8d36d96e1d4338c66 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Fri, 14 Aug 2026 20:46:10 +0200 Subject: [PATCH 6/7] Scope FontRegistry font caching and disposal to the Display that created each font FontRegistry cached fonts in a single, display-agnostic table and disposed all of them whenever any one Display it had been used from was disposed. With more than one SWT Display in play, the first Display to be disposed therefore tore down the fonts of every other Display as well, leaving live Displays with disposed fonts. Which Display that is happens to be arbitrary. The registry now keeps one font cache per Display, populated and disposed independently, so disposing a Display only disposes its own fonts. Fonts replaced via put() are likewise queued as stale per display and disposed together with the display that owns them. Note that this is about ownership and lifetime, not about which Display a font may be used on. Handing a font realized on one Display to another is not by itself a problem; having it disposed underneath that other Display is. The Display the registry was created for is kept as the main display and assumed to outlive all others, which makes its fonts the ones that are always safe to fall back to. A thread without a Display of its own can neither scope a lookup nor create a font, so it falls back to the default font realized there instead of failing outright. Disposal of the main display still disposes the whole registry, so clearCaches() keeps the "disposes all currently allocated resources" contract it inherits from ResourceRegistry. The per-display caches are concurrent, since Displays run on their own threads and put() invalidates the records of every display from whichever thread it is called on. For a single-display application this is behavior-preserving: the per-display cache then holds exactly what the global one did, and that Display is the main display. Since a record is now only reachable through the cache of the display it belongs to, and can only have got there via createFont(), which already hooked that display for disposal, FontRecord no longer needs the registry to obtain its display when realizing a bold or italic font. It uses the device of its own base font, which also fixes where those fonts end up: they used to be created on whatever display was current, and on a thread without one SWT silently substituted Display.getDefault(), so a styled font could be allocated on a different display than the record holding it. With the stale-font handling already moved out, that leaves no reference to the enclosing registry, so the class becomes static. An assertion states who may realize a style: the display owning the record, or a caller without a display of its own. Since such a caller reads the record while the owning display's thread may still be realizing styles on it, the record's font fields become volatile. Assisted-by: Claude Opus 5 --- .../eclipse/jface/resource/FontRegistry.java | 245 +++++++++++++----- .../tests/resources/FontRegistryTest.java | 104 ++++++-- 2 files changed, 260 insertions(+), 89 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 a035dfefbeb..484b092d51b 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 @@ -18,13 +18,14 @@ import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.MissingResourceException; +import java.util.Queue; import java.util.ResourceBundle; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; import org.eclipse.core.runtime.Assert; import org.eclipse.jface.util.Policy; @@ -67,13 +68,16 @@ public class FontRegistry extends ResourceRegistry { * FontRecord is a private helper class that holds onto a font * and can be used to generate its bold and italic version. */ - private class FontRecord { + private static class FontRecord { - Font baseFont; + // volatile: a caller without a display of its own is handed the record of + // the display owning it, so it reads these fields while that display's + // thread may still be writing the lazily realized styles + volatile Font baseFont; - Font boldFont; + volatile Font boldFont; - Font italicFont; + volatile Font italicFont; FontData[] baseData; @@ -119,9 +123,12 @@ public Font getBoldFont() { return boldFont; } + // only the display owning this record, or a caller without a display of + // its own, ever reaches this point; either way the font is realized on + // the display the record belongs to + Assert.isTrue(Display.getCurrent() == null || Display.getCurrent() == baseFont.getDevice()); FontData[] boldData = getModifiedFontData(SWT.BOLD); - Display display = getDisplayAndHookForDisposal(); - boldFont = new Font(display, boldData); + boldFont = new Font(baseFont.getDevice(), boldData); return boldFont; } @@ -156,9 +163,12 @@ public Font getItalicFont() { return italicFont; } + // only the display owning this record, or a caller without a display of + // its own, ever reaches this point; either way the font is realized on + // the display the record belongs to + Assert.isTrue(Display.getCurrent() == null || Display.getCurrent() == baseFont.getDevice()); FontData[] italicData = getModifiedFontData(SWT.ITALIC); - Display display = getDisplayAndHookForDisposal(); - italicFont = new Font(display, italicData); + italicFont = new Font(baseFont.getDevice(), italicData); return italicFont; } @@ -182,12 +192,66 @@ List getAllocatedFonts() { } } + private final Map displayToFontRecords = new ConcurrentHashMap<>(); + /** - * Table of known fonts, keyed by symbolic font name - * (key type: String, - * value type: FontRecord. + * Table of known fonts realized on one particular display, keyed by symbolic + * font name (key type: String, value type: + * FontRecord). There is one such table per {@link Display} the + * registry has been used from, since a {@link Font} is only valid on the + * display that created it. Also keeps that display's fonts that were + * replaced by {@link FontRegistry#put(String, FontData[])} but may still be + * in use elsewhere, so their disposal is deferred until the display itself + * is disposed. + *

+ * Both collections are concurrent: a display's records are read and written + * by its own thread, but {@link FontRegistry#put(String, FontData[])} + * invalidates the records of every display from whichever thread it + * is called on, and may do so while that display is disposing itself. + *

*/ - private final Map stringToFontRecord = new HashMap<>(7); + private static class DisplayFontRecords { + + private final Map records = new ConcurrentHashMap<>(); + + private final Queue staleFonts = new ConcurrentLinkedQueue<>(); + + FontRecord get(String symbolicName) { + return records.get(symbolicName); + } + + void put(String symbolicName, FontRecord record) { + records.put(symbolicName, record); + } + + /** + * Drop the record for the given symbolic name, if any, and defer + * disposal of the fonts it had realized until this display is disposed, + * since they may still be in use. The display's default font is kept, + * as it stays in use under its own symbolic name. + */ + void invalidate(String symbolicName) { + FontRecord replacedRecord = records.remove(symbolicName); + if (replacedRecord == null) { + return; + } + FontRecord defaultRecord = records.get(JFaceResources.DEFAULT_FONT); + Font defaultFont = defaultRecord != null ? defaultRecord.getBaseFont() : null; + replacedRecord.getAllocatedFonts().stream().filter(font -> font != defaultFont).forEach(staleFonts::add); + } + + void dispose() { + records.values().forEach(FontRecord::dispose); + records.clear(); + // drained rather than iterated and cleared, so that a font enqueued by a + // concurrent put() is either disposed here or still queued afterwards, + // but never dropped undisposed + for (Font staleFont = staleFonts.poll(); staleFont != null; staleFont = staleFonts.poll()) { + staleFont.dispose(); + } + } + + } /** * Table of known font data, keyed by symbolic font name @@ -196,22 +260,20 @@ List getAllocatedFonts() { */ private final Map stringToFontData = new ConcurrentHashMap<>(7); - /** - * Collection of Fonts that are now stale to be disposed - * when it is safe to do so (i.e. on shutdown). - * @see List - */ - private final List staleFonts = new ArrayList<>(); - /** * Runnable that cleans up the manager on disposal of the display. */ protected Runnable displayRunnable = this::clearCaches; - private final Set displayDisposeHooked = ConcurrentHashMap.newKeySet(); - private final boolean cleanOnDisplayDisposal; + /** + * The display this registry was created for. It is assumed to outlive every + * other display the registry is used from, so its fonts can serve as a + * fallback for callers that have no display of their own. + */ + private final Display mainDisplay; + /** * Creates an empty font registry. *

@@ -282,14 +344,14 @@ public FontRegistry() { */ public FontRegistry(String location, ClassLoader loader) throws MissingResourceException { - Display display = Display.getCurrent(); - Assert.isNotNull(display); + mainDisplay = Display.getCurrent(); + Assert.isNotNull(mainDisplay); // FIXE: need to respect loader //readResourceBundle(location, loader); readResourceBundle(location); - cleanOnDisplayDisposal = true; - hookDisplayDispose(display); + displayToFontRecords.put(mainDisplay, new DisplayFontRecords()); + hookMainDisplayDispose(mainDisplay); } /** @@ -368,15 +430,17 @@ public FontRegistry(Display display) { * whether all fonts allocated by this FontRegistry * 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 + * itself; the fonts it allocated for any display are retained + * until {@link #clearCaches()} disposes them * @since 3.1 */ public FontRegistry(Display display, boolean cleanOnDisplayDisposal) { Assert.isNotNull(display); + this.mainDisplay = display; + displayToFontRecords.put(display, new DisplayFontRecords()); this.cleanOnDisplayDisposal = cleanOnDisplayDisposal; if (cleanOnDisplayDisposal) { - hookDisplayDispose(display); + hookMainDisplayDispose(display); } } @@ -503,23 +567,22 @@ private FontRecord createFont(String symbolicName, FontData[] fonts) { return null; } - //Do not fire the update from creation as it is not a property change + // Do not fire the update from creation as it is not a property change. + // Note that this drops any record other displays had realized for this + // name, should filterData() have narrowed the registered data. That is + // intended: the registered data is shared by all displays, so a record + // built from outdated data must not survive on any of them. put(symbolicName, validData, false); Font newFont = new Font(display, validData); - FontRecord newRecord = new FontRecord(newFont, validData); - stringToFontRecord.put(symbolicName, newRecord); - return newRecord; - } - - private Display getDisplayAndHookForDisposal() { - Display display = Display.getCurrent(); - if (display == null) { - return null; - } - if (cleanOnDisplayDisposal && !displayDisposeHooked.contains(display)) { - hookDisplayDispose(display); + FontRecord record = new FontRecord(newFont, validData); + // the display's records may have been torn down concurrently, e.g. by + // clearCaches() running on another display's thread, in which case there is + // nothing left to cache the record in and the next lookup realizes it again + DisplayFontRecords displayRecords = displayToFontRecords.get(display); + if (displayRecords != null) { + displayRecords.put(symbolicName, record); } - return display; + return record; } /** @@ -570,7 +633,18 @@ public FontDescriptor getDescriptor(String symbolicName) { * Returns the default font record. */ private FontRecord defaultFontRecord() { - FontRecord record = stringToFontRecord.get(JFaceResources.DEFAULT_FONT); + FontRecord record = getExistingFontRecord(JFaceResources.DEFAULT_FONT); + if (record == null && Display.getCurrent() == null) { + // No display for the current thread, so there is none to scope the + // lookup to and none to create a font on. Fall back to the default + // font already realized on the main display, which outlives every + // other display, rather than fail outright. Reached e.g. from + // getFontRecord()'s non-UI-thread fallback. + DisplayFontRecords mainDisplayRecords = displayToFontRecords.get(mainDisplay); + if (mainDisplayRecords != null) { + record = mainDisplayRecords.get(JFaceResources.DEFAULT_FONT); + } + } if (record != null) { return record; } @@ -588,6 +662,20 @@ record = createFont(JFaceResources.DEFAULT_FONT, defaultFont.getFontData()); return record; } + /** + * Looks up an already-realized font record for the given symbolic name on + * the current display. Returns null if there is none, in which + * case the caller is responsible for creating one on the current display. + */ + private FontRecord getExistingFontRecord(String symbolicName) { + Display currentDisplay = Display.getCurrent(); + if (currentDisplay == null) { + return null; + } + DisplayFontRecords currentDisplayRecords = displayToFontRecords.get(currentDisplay); + return currentDisplayRecords != null ? currentDisplayRecords.get(symbolicName) : null; + } + /** * Returns the default font data. Creates it if necessary. */ @@ -680,7 +768,7 @@ public Font getItalic(String symbolicName) { */ private FontRecord getFontRecord(String symbolicName) { Assert.isNotNull(symbolicName); - FontRecord existingRecord = stringToFontRecord.get(symbolicName); + FontRecord existingRecord = getExistingFontRecord(symbolicName); if (existingRecord != null) { return existingRecord; } @@ -718,22 +806,44 @@ public boolean hasValueFor(String fontKey) { @Override protected void clearCaches() { - stringToFontRecord.values().forEach(FontRecord::dispose); - stringToFontRecord.clear(); - staleFonts.forEach(Font::dispose); - staleFonts.clear(); - - displayDisposeHooked.remove(Display.getCurrent()); + // disposes every display's fonts, not only the current display's: the + // contract is to dispose all allocated resources, and this also runs on + // disposal of the main display, which outlives all other displays + for (Display display : displayToFontRecords.keySet()) { + unhookDisplayAndDisposeFonts(display); + } } /** - * Hook a dispose listener on the SWT display. + * Hook a dispose listener on the SWT display this registry was created for. + * Since that display outlives all others, its disposal tears down the whole + * registry, not just its own fonts. */ - private void hookDisplayDispose(Display display) { - displayDisposeHooked.add(display); + private void hookMainDisplayDispose(Display display) { display.disposeExec(displayRunnable); } + private Display getDisplayAndHookForDisposal() { + Display display = Display.getCurrent(); + if (display == null) { + return null; + } + displayToFontRecords.computeIfAbsent(display, newDisplay -> { + if (cleanOnDisplayDisposal) { + newDisplay.disposeExec(() -> unhookDisplayAndDisposeFonts(newDisplay)); + } + return new DisplayFontRecords(); + }); + return display; + } + + private void unhookDisplayAndDisposeFonts(Display display) { + DisplayFontRecords records = displayToFontRecords.remove(display); + if (records != null) { + records.dispose(); + } + } + /** * Checks whether the given font is in the list of fixed fonts. */ @@ -789,7 +899,7 @@ public void put(String symbolicName, FontData[] fontData) { * * @param symbolicName the symbolic font name * @param fontData an Array of FontData - * @param update - fire a font mapping changed if true. False + * @param update - fire a property change if true. False * if this method is called from the get method as no setting * has changed. */ @@ -805,28 +915,19 @@ private void put(String symbolicName, FontData[] fontData, boolean update) { return; } - invalidate(symbolicName); + // the font data is shared by all displays, so the font has to be + // invalidated on all of them. Stale fonts are queued per display, so + // each display disposes its own replaced fonts when it is itself + // disposed, instead of every display's replaced fonts only being + // disposed together with one particular display + for (DisplayFontRecords records : displayToFontRecords.values()) { + records.invalidate(symbolicName); + } if (update) { fireMappingChanged(symbolicName, existing, fontData); } } - /** - * 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; - } - FontRecord defaultRecord = stringToFontRecord.get(JFaceResources.DEFAULT_FONT); - Font defaultFont = defaultRecord != null ? defaultRecord.getBaseFont() : null; - replacedRecord.getAllocatedFonts().stream().filter(font -> font != defaultFont).forEach(staleFonts::add); - } - /** * Reads the resource bundle. This puts FontData[] objects * in the mapping table. These will lazily be turned into 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 0de2bac14f5..70a0ceea1ad 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 @@ -60,6 +60,17 @@ public void testBug544026() { assertArrayEquals(fontData, JFaceResources.getDefaultFont().getFontData()); } + @Test + public void multipleDisplayDispose_noDisposeOtherThreadFonts() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + Font mainFont = fontRegistry.get("myfont"); + testMultipleDisplayDispose(fontRegistry::defaultFont); + assertFalse(mainFont.isDisposed(), "FontRegistry should not dispose fonts on other displays"); + } + @Test public void multipleDisplayDispose() { assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); @@ -86,6 +97,65 @@ public void multipleDisplayDispose_italicFont() { testMultipleDisplayDispose(() -> fontRegistry.getItalic(JFaceResources.DEFAULT_FONT)); } + @Test + public void put_invalidatesCachedFont_onAllDisplays() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + fontRegistry.get("myfont"); // only realizes the NORMAL style on the main display + + Display secondDisplay = initializeDisplayInSeparateThread(); + try { + // a style the main display has not realized, so the second display is + // guaranteed to hold a font record of its own; only then does this + // actually cover invalidation across displays + Font boldFontOnSecondDisplay = secondDisplay.syncCall(() -> fontRegistry.getBold("myfont")); + assertEquals(secondDisplay, boldFontOnSecondDisplay.getDevice(), + "the second display must have realized a font of its own"); + + // changing the font data must invalidate the cached font on every display, not just the main one + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 20, SWT.NORMAL) }); + + Font updatedBoldFontOnSecondDisplay = secondDisplay.syncCall(() -> fontRegistry.getBold("myfont")); + assertNotEquals(boldFontOnSecondDisplay, updatedBoldFontOnSecondDisplay, + "put() must invalidate the cached font on the second display too"); + assertEquals(20, updatedBoldFontOnSecondDisplay.getFontData()[0].getHeight()); + } finally { + secondDisplay.syncExec(secondDisplay::dispose); + } + } + + @Test + public void cleanOnDisplayDisposalFalse_cachesFontAcrossRepeatedCalls() { + FontRegistry fontRegistry = new FontRegistry(Display.getCurrent(), false); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + + Font first = fontRegistry.get("myfont"); + Font second = fontRegistry.get("myfont"); + + assertEquals(first, second, + "repeated get() calls must return the cached font, not a new one, when cleanOnDisplayDisposal is false"); + } + + @Test + public void cleanOnDisplayDisposalFalse_doesNotAutoDisposeFontsOnSecondDisplay() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(Display.getCurrent(), false); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + + Display secondDisplay = initializeDisplayInSeparateThread(); + Font fontOnSecondDisplay = secondDisplay.syncCall(() -> fontRegistry.get("myfont")); + Font sameFontOnSecondDisplay = secondDisplay.syncCall(() -> fontRegistry.get("myfont")); + assertEquals(fontOnSecondDisplay, sameFontOnSecondDisplay, + "font must be cached per display even when cleanOnDisplayDisposal is false"); + + secondDisplay.syncExec(secondDisplay::dispose); + assertFalse(fontOnSecondDisplay.isDisposed(), + "fonts must not be disposed automatically when cleanOnDisplayDisposal is false"); + } + private static void testMultipleDisplayDispose(Supplier fontSupplier) { assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); @@ -94,9 +164,8 @@ private static void testMultipleDisplayDispose(Supplier fontSupplier) { Font fontOnThisDisplayBeforeSecondDisplayDispose = fontSupplier.get(); Device displayOfFontOnSecondDisplay = fontOnSecondDisplay.getDevice(); - // font registry returns same font for every display assertEquals(secondDisplay, displayOfFontOnSecondDisplay); - assertEquals(fontOnThisDisplayBeforeSecondDisplayDispose, fontOnSecondDisplay); + assertNotEquals(fontOnThisDisplayBeforeSecondDisplayDispose, fontOnSecondDisplay); // after disposing font's display, registry should reinitialize the font secondDisplay.syncExec(secondDisplay::dispose); @@ -162,25 +231,26 @@ public void get_fontFromNonUIThreadFallback_doesNotOverwriteDefaultFont() throws fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); Font defaultFont = fontRegistry.get(JFaceResources.DEFAULT_FONT); - AtomicReference fontFromNonUIThread = new AtomicReference<>(); - AtomicReference failureFromNonUIThread = new AtomicReference<>(); - Thread nonUiThread = new Thread(() -> { - try { - fontFromNonUIThread.set(fontRegistry.get("myfont")); - } catch (Throwable t) { - failureFromNonUIThread.set(t); - } - }); - nonUiThread.start(); - nonUiThread.join(); + Font fontFromNonUIThread = callOnNonUIThread(() -> fontRegistry.get("myfont")); - if (failureFromNonUIThread.get() != null) { - throw failureFromNonUIThread.get(); - } - assertSame(defaultFont, fontFromNonUIThread.get()); + assertSame(defaultFont, fontFromNonUIThread); assertSame(defaultFont, fontRegistry.get(JFaceResources.DEFAULT_FONT)); } + @Test + public void getBold_fromNonUIThreadFallback_reusesMainDisplaysBoldDefaultFont() throws Throwable { + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + // a thread without a display of its own can neither create a font nor scope a + // lookup, so the main display's default font record is all it can fall back to + Font defaultBold = fontRegistry.getBold(JFaceResources.DEFAULT_FONT); + + Font boldFromNonUIThread = callOnNonUIThread(() -> fontRegistry.getBold("myfont")); + + assertSame(defaultBold, boldFromNonUIThread); + assertSame(defaultBold, fontRegistry.getBold(JFaceResources.DEFAULT_FONT)); + } + @Test public void get_returnsSameFontInstanceOnRepeatedCalls() { FontRegistry fontRegistry = new FontRegistry(); From 3cb9bead8fc4f41fa26bb0ca6af83f90df5d36d7 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 24 Aug 2026 17:37:24 +0200 Subject: [PATCH 7/7] Reuse the main display's font across displays when already available Now that each display keeps its own font cache, a symbolic name requested from several displays ends up with a separate native font handle per display, even for names that never vary between displays, such as the JFace default font. Prefer a font already realized on the registry's main display whenever it has the exact style requested; only when it does not does the requesting display realize and cache one of its own. This keeps resource usage close to what a single shared cache would give, while remaining correct per display. Callers without a display of their own, which so far could only fall back to the default font, reuse any already realized font the same way. Handing a font owned by the main display to another display is safe because the main display outlives every other display the registry is used from, so the font cannot be disposed while another display still uses it. The flip side is that the font returned for a symbolic name is no longer necessarily owned by the caller's display. A display that had to realize a font itself keeps getting that one, so repeated lookups do not silently switch to a different instance once the main display catches up. That costs nothing, since such a copy only exists where it had to be created anyway. Reusing only a record that already carries the requested style also keeps the lazy creation of styled fonts confined to the display owning the record, so it needs no synchronization of its own. Since all of this depends on which style is being asked for, the style is now known while a record is looked up, rather than being applied to the record only afterwards. Tests cover reuse when the style is already available on the main display, falling back to a display's own font when it is not, and reuse starting once the main display realizes the style later. Assisted-by: Claude Opus 5 --- .../eclipse/jface/resource/FontRegistry.java | 147 +++++++++++++----- .../tests/resources/FontRegistryTest.java | 98 ++++++++++++ 2 files changed, 210 insertions(+), 35 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 484b092d51b..bd1d00be733 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 @@ -64,6 +64,10 @@ @NoExtend public class FontRegistry extends ResourceRegistry { + private enum FontStyle { + NORMAL, BOLD, ITALIC + } + /** * FontRecord is a private helper class that holds onto a font * and can be used to generate its bold and italic version. @@ -109,16 +113,29 @@ void dispose() { * Return the base Font. * @return Font */ - public Font getBaseFont() { + private Font getBaseFont() { return baseFont; } + /** + * Return the font for the given style, creating it lazily if necessary. + * @param style the requested style + * @return the font + */ + public Font get(FontStyle style) { + return switch (style) { + case NORMAL -> getBaseFont(); + case BOLD -> getBoldFont(); + case ITALIC -> getItalicFont(); + }; + } + /** * Return the bold Font. Create a bold version * of the base font to get it. * @return Font */ - public Font getBoldFont() { + private Font getBoldFont() { if (boldFont != null) { return boldFont; } @@ -132,6 +149,20 @@ public Font getBoldFont() { return boldFont; } + /** + * Returns whether the given style has already been realized for this + * record. + * @param style the style to check + * @return whether the given style is already available + */ + public boolean has(FontStyle style) { + return switch (style) { + case NORMAL -> baseFont != null; + case BOLD -> boldFont != null; + case ITALIC -> italicFont != null; + }; + } + /** * Get a version of the base font data with the specified * style. @@ -158,7 +189,7 @@ private FontData[] getModifiedFontData(int style) { * base font to get it. * @return Font */ - public Font getItalicFont() { + private Font getItalicFont() { if (italicFont != null) { return italicFont; } @@ -236,7 +267,7 @@ void invalidate(String symbolicName) { return; } FontRecord defaultRecord = records.get(JFaceResources.DEFAULT_FONT); - Font defaultFont = defaultRecord != null ? defaultRecord.getBaseFont() : null; + Font defaultFont = defaultRecord != null ? defaultRecord.get(FontStyle.NORMAL) : null; replacedRecord.getAllocatedFonts().stream().filter(font -> font != defaultFont).forEach(staleFonts::add); } @@ -609,7 +640,16 @@ Font calculateDefaultFont() { * @return Font */ public Font defaultFont() { - return defaultFontRecord().getBaseFont(); + return defaultFont(FontStyle.NORMAL); + } + + /** + * Return the default font in the given style, creating it if necessary. + * @param style the requested style + * @return the font + */ + private Font defaultFont(FontStyle style) { + return defaultFontRecord(style).get(style); } /** @@ -632,14 +672,12 @@ public FontDescriptor getDescriptor(String symbolicName) { /** * Returns the default font record. */ - private FontRecord defaultFontRecord() { - FontRecord record = getExistingFontRecord(JFaceResources.DEFAULT_FONT); + private FontRecord defaultFontRecord(FontStyle style) { + FontRecord record = getExistingFontRecord(JFaceResources.DEFAULT_FONT, style); if (record == null && Display.getCurrent() == null) { - // No display for the current thread, so there is none to scope the - // lookup to and none to create a font on. Fall back to the default - // font already realized on the main display, which outlives every - // other display, rather than fail outright. Reached e.g. from - // getFontRecord()'s non-UI-thread fallback. + // no display to scope the lookup to and none to create a font on: use the + // main display's record even if the requested style is not realized there + // yet, so the style gets realized on that display rather than failing DisplayFontRecords mainDisplayRecords = displayToFontRecords.get(mainDisplay); if (mainDisplayRecords != null) { record = mainDisplayRecords.get(JFaceResources.DEFAULT_FONT); @@ -663,24 +701,65 @@ record = createFont(JFaceResources.DEFAULT_FONT, defaultFont.getFontData()); } /** - * Looks up an already-realized font record for the given symbolic name on - * the current display. Returns null if there is none, in which - * case the caller is responsible for creating one on the current display. + * Looks up an already-realized font record for the given symbolic name and + * style. A record the current display has already realized the exact style + * on is used first, so repeated lookups from that display keep returning the + * same font. Otherwise the main display's record is used if it already has + * that style, so callers from any display, including a thread with no + * Display of its own, reuse it instead of allocating a duplicate. Failing + * both, the current display's own record is returned even though it lacks + * the requested style, so that the style gets realized on the display owning + * the record; null is returned only if the current display has + * no record for the name at all, in which case the caller is responsible for + * creating one on it. + *

+ * Handing out the main display's font to another display is safe because + * the main display is assumed to outlive every other display the registry + * is used from, so the font cannot be disposed while another display is + * still using it. + *

+ *

+ * Requiring the exact style to be present is also what keeps the lazy + * creation of styled fonts single-threaded: a record is only ever handed to + * a foreign display once the style it asks for has been realized, so only + * the record's own display ever reaches the creating branch of + * {@link FontRecord#get(FontStyle)}. Note that for {@link FontStyle#NORMAL} + * every existing record matches, since that font is realized when the record + * is created. + *

*/ - private FontRecord getExistingFontRecord(String symbolicName) { + private FontRecord getExistingFontRecord(String symbolicName, FontStyle style) { + FontRecord recordOnCurrentDisplay = null; Display currentDisplay = Display.getCurrent(); - if (currentDisplay == null) { - return null; + if (currentDisplay != null) { + DisplayFontRecords currentDisplayRecords = displayToFontRecords.get(currentDisplay); + if (currentDisplayRecords != null) { + recordOnCurrentDisplay = currentDisplayRecords.get(symbolicName); + // a style this display already realized itself stays the one it gets, so + // repeated lookups keep returning the same font instance + if (recordOnCurrentDisplay != null && recordOnCurrentDisplay.has(style)) { + return recordOnCurrentDisplay; + } + } } - DisplayFontRecords currentDisplayRecords = displayToFontRecords.get(currentDisplay); - return currentDisplayRecords != null ? currentDisplayRecords.get(symbolicName) : null; + + DisplayFontRecords mainDisplayRecords = displayToFontRecords.get(mainDisplay); + if (mainDisplayRecords != null) { + FontRecord recordOnMainDisplay = mainDisplayRecords.get(symbolicName); + // Only return main display record if exact font style already exists + if (recordOnMainDisplay != null && recordOnMainDisplay.has(style)) { + return recordOnMainDisplay; + } + } + + return recordOnCurrentDisplay; } /** * Returns the default font data. Creates it if necessary. */ private FontData[] defaultFontData() { - return defaultFontRecord().baseData; + return defaultFontRecord(FontStyle.NORMAL).baseData; } /** @@ -717,8 +796,7 @@ public FontData[] getFontData(String symbolicName) { * @return the font */ public Font get(String symbolicName) { - - return getFontRecord(symbolicName).getBaseFont(); + return getFont(symbolicName, FontStyle.NORMAL); } /** @@ -737,8 +815,7 @@ public Font get(String symbolicName) { * @since 3.0 */ public Font getBold(String symbolicName) { - - return getFontRecord(symbolicName).getBoldFont(); + return getFont(symbolicName, FontStyle.BOLD); } /** @@ -757,20 +834,20 @@ public Font getBold(String symbolicName) { * @since 3.0 */ public Font getItalic(String symbolicName) { - - return getFontRecord(symbolicName).getItalicFont(); + return getFont(symbolicName, FontStyle.ITALIC); } /** - * Return the font record for the key. + * Return the font for the given key and style. * @param symbolicName The key for the record. - * @return FontRecord + * @param style the requested style + * @return the font */ - private FontRecord getFontRecord(String symbolicName) { + private Font getFont(String symbolicName, FontStyle style) { Assert.isNotNull(symbolicName); - FontRecord existingRecord = getExistingFontRecord(symbolicName); + FontRecord existingRecord = getExistingFontRecord(symbolicName, style); if (existingRecord != null) { - return existingRecord; + return existingRecord.get(style); } FontData[] existingFontData = stringToFontData.get(symbolicName); @@ -778,20 +855,20 @@ private FontRecord getFontRecord(String symbolicName) { FontRecord fontRecord; if (existingFontData == null) { - fontRecord = defaultFontRecord(); + fontRecord = defaultFontRecord(style); } else { fontRecord = createFont(symbolicName, existingFontData); } if (fontRecord == null) { - fontRecord = defaultFontRecord(); + fontRecord = defaultFontRecord(style); if (Display.getCurrent() == null) { // log error but don't throw an exception to preserve existing functionality String msg = "Unable to create font \"" + symbolicName + "\" in a non-UI thread. Using default font instead."; //$NON-NLS-1$ //$NON-NLS-2$ Policy.logException(new SWTException(msg)); } } - return fontRecord; + return fontRecord.get(style); } @Override 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 70a0ceea1ad..bbb9ff0f257 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 @@ -97,6 +97,101 @@ public void multipleDisplayDispose_italicFont() { testMultipleDisplayDispose(() -> fontRegistry.getItalic(JFaceResources.DEFAULT_FONT)); } + @Test + public void multipleDisplay_reusesMainDisplayFont_whenStyleAlreadyCached() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + Font mainFont = fontRegistry.get("myfont"); + + Display secondDisplay = initializeDisplayInSeparateThread(); + try { + Font fontFromSecondDisplayThread = secondDisplay.syncCall(() -> fontRegistry.get("myfont")); + assertEquals(mainFont, fontFromSecondDisplayThread, + "a font already realized on the main display should be reused from any other display's thread"); + assertEquals(Display.getCurrent(), fontFromSecondDisplayThread.getDevice(), + "the reused font is still owned by the main display"); + } finally { + secondDisplay.syncExec(secondDisplay::dispose); + } + } + + @Test + public void multipleDisplay_createsOwnFont_whenStyleNotYetCachedOnMainDisplay() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + fontRegistry.get("myfont"); // only realizes the NORMAL style on the main display + + Display secondDisplay = initializeDisplayInSeparateThread(); + Font boldFontOnSecondDisplay; + try { + boldFontOnSecondDisplay = secondDisplay.syncCall(() -> fontRegistry.getBold("myfont")); + + assertEquals(secondDisplay, boldFontOnSecondDisplay.getDevice(), + "bold style is not yet cached on the main display, so it must be created on the requesting display"); + } finally { + secondDisplay.syncExec(secondDisplay::dispose); + } + assertTrue(boldFontOnSecondDisplay.isDisposed(), + "fonts created for the second display must be disposed together with it"); + } + + @Test + public void multipleDisplay_reusesMainDisplayFont_onceStyleBecomesCachedThere() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + fontRegistry.get("myfont"); // only realizes the NORMAL style on the main display + + Display firstSecondDisplay = initializeDisplayInSeparateThread(); + Font boldFontOnFirstSecondDisplay = firstSecondDisplay.syncCall(() -> fontRegistry.getBold("myfont")); + assertEquals(firstSecondDisplay, boldFontOnFirstSecondDisplay.getDevice(), + "bold style is not yet cached on the main display, so it must be created on the requesting display"); + firstSecondDisplay.syncExec(firstSecondDisplay::dispose); + + // the main display now also realizes the bold style + Font boldFontOnMainDisplay = fontRegistry.getBold("myfont"); + + Display secondSecondDisplay = initializeDisplayInSeparateThread(); + try { + Font boldFontOnSecondSecondDisplay = secondSecondDisplay.syncCall(() -> fontRegistry.getBold("myfont")); + assertEquals(boldFontOnMainDisplay, boldFontOnSecondSecondDisplay, + "once the main display has realized the requested style, later lookups from any display must reuse it"); + } finally { + secondSecondDisplay.syncExec(secondSecondDisplay::dispose); + } + } + + @Test + public void multipleDisplay_keepsOwnFont_whenMainDisplayRealizesItLater() { + assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); + + FontRegistry fontRegistry = new FontRegistry(); + fontRegistry.put("myfont", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + + Display secondDisplay = initializeDisplayInSeparateThread(); + try { + // the second display realizes the font before the main display has one to reuse + Font fontOnSecondDisplay = secondDisplay.syncCall(() -> fontRegistry.get("myfont")); + assertEquals(secondDisplay, fontOnSecondDisplay.getDevice(), + "nothing to reuse yet, so the second display must realize a font of its own"); + + // the main display realizing the same font afterwards must not change what + // the second display gets, or it would silently switch instances mid-flight + fontRegistry.get("myfont"); + + Font fontOnSecondDisplayAgain = secondDisplay.syncCall(() -> fontRegistry.get("myfont")); + assertSame(fontOnSecondDisplay, fontOnSecondDisplayAgain, + "a display that realized a font itself must keep getting that same instance"); + } finally { + secondDisplay.syncExec(secondDisplay::dispose); + } + } + @Test public void put_invalidatesCachedFont_onAllDisplays() { assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); @@ -160,6 +255,9 @@ private static void testMultipleDisplayDispose(Supplier fontSupplier) { assumeTrue(OS.isWindows(), "multiple Display instance only allowed on Windows"); Display secondDisplay = initializeDisplayInSeparateThread(); + // the second display is asked first on purpose: the requested font is not + // realized on the main display yet, so there is nothing to reuse and the + // second display has to realize (and own) a font of its own Font fontOnSecondDisplay = secondDisplay.syncCall(fontSupplier::get); Font fontOnThisDisplayBeforeSecondDisplayDispose = fontSupplier.get();