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..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 @@ -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; @@ -64,17 +64,24 @@ @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. */ - 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; @@ -106,26 +113,56 @@ 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; } + // 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; } + /** + * 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. @@ -152,68 +189,122 @@ private FontData[] getModifiedFontData(int style) { * base font to get it. * @return Font */ - public Font getItalicFont() { + private Font getItalicFont() { if (italicFont != null) { 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.get(FontStyle.NORMAL) : 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 +375,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 +459,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 +593,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; - } - 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; } /** @@ -543,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); } /** @@ -566,8 +672,17 @@ public FontDescriptor getDescriptor(String symbolicName) { /** * Returns the default font record. */ - private FontRecord defaultFontRecord() { - FontRecord record = stringToFontRecord.get(JFaceResources.DEFAULT_FONT); + private FontRecord defaultFontRecord(FontStyle style) { + FontRecord record = getExistingFontRecord(JFaceResources.DEFAULT_FONT, style); + if (record == null && Display.getCurrent() == null) { + // 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); + } + } if (record != null) { return record; } @@ -582,15 +697,69 @@ 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 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, FontStyle style) { + FontRecord recordOnCurrentDisplay = null; + Display currentDisplay = Display.getCurrent(); + 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 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; } /** @@ -627,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); } /** @@ -647,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); } /** @@ -667,44 +834,41 @@ 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); - Object result = stringToFontRecord.get(symbolicName); - if (result != null) { - return (FontRecord) result; + FontRecord existingRecord = getExistingFontRecord(symbolicName, style); + if (existingRecord != null) { + return existingRecord.get(style); } - result = stringToFontData.get(symbolicName); + FontData[] existingFontData = stringToFontData.get(symbolicName); FontRecord fontRecord; - if (result == null) { - fontRecord = defaultFontRecord(); + if (existingFontData == null) { + fontRecord = defaultFontRecord(style); } else { - fontRecord = createFont(symbolicName, (FontData[]) result); + 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; // 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; - + return fontRecord.get(style); } @Override @@ -719,37 +883,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 +976,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 +985,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..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 @@ -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,17 +97,173 @@ 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"); + + 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"); 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(); 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 +307,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 +371,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 +412,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 +485,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();