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..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,14 +18,14 @@ import java.util.Arrays; 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; +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; @@ -68,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; @@ -120,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; } @@ -157,63 +163,117 @@ 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; } /** - * 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; } } + 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 * (key type: String, * value type: org.eclipse.swt.graphics.FontData[]). */ - private final Map stringToFontData = new HashMap<>(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<>(); + private final Map stringToFontData = new ConcurrentHashMap<>(7); /** * 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. *

@@ -284,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,14 +428,19 @@ 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 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); } } @@ -497,26 +562,27 @@ 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; } - //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); - return new FontRecord(newFont, validData); - } - - private Display getDisplayAndHookForDisposal() { - Display display = Display.getCurrent(); - if (display == null) { - return null; + 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); } - if (cleanOnDisplayDisposal && !displayDisposeHooked.contains(display)) { - hookDisplayDispose(display); - } - return display; + return record; } /** @@ -567,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; } @@ -582,10 +659,23 @@ record = createFont(JFaceResources.DEFAULT_FONT, fontData); record = createFont(JFaceResources.DEFAULT_FONT, defaultFont.getFontData()); defaultFont.dispose(); } - stringToFontRecord.put(JFaceResources.DEFAULT_FONT, record); 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. */ @@ -678,19 +768,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 = getExistingFontRecord(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) { @@ -698,13 +788,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 @@ -719,37 +806,42 @@ public boolean hasValueFor(String fontKey) { @Override protected void clearCaches() { - - Iterator iterator = stringToFontRecord.values().iterator(); - while (iterator.hasNext()) { - Object next = iterator.next(); - ((FontRecord) next).dispose(); + // 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); } - - disposeFonts(staleFonts.iterator()); - stringToFontRecord.clear(); - staleFonts.clear(); - - displayDisposeHooked.remove(Display.getCurrent()); } /** - * Dispose of all of the fonts in this iterator. - * @param iterator over Collection of Font + * 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 disposeFonts(Iterator iterator) { - while (iterator.hasNext()) { - Object next = iterator.next(); - ((Font) next).dispose(); + 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; } - /** - * Hook a dispose listener on the SWT display. - */ - private void hookDisplayDispose(Display display) { - displayDisposeHooked.add(display); - display.disposeExec(displayRunnable); + private void unhookDisplayAndDisposeFonts(Display display) { + DisplayFontRecords records = displayToFontRecords.remove(display); + if (records != null) { + records.dispose(); + } } /** @@ -807,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. */ @@ -816,21 +908,24 @@ 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); + // 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); } - - if (oldFont != null) { - oldFont.addAllocatedFontsToStale(defaultFontRecord().getBaseFont()); - } } /** 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..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); @@ -140,31 +209,48 @@ 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(); 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(); @@ -187,6 +273,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(); @@ -214,6 +314,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"); @@ -271,6 +387,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();