If we see the implementation of getFontData(FontData, zoom) in ScalingSWTFontRegistry

@Override
public Font getFont(FontData fontData, int zoom) {
	ScaledFontContainer container;
	if (customFontsKeyMap.containsKey(fontData)) {
		container = customFontsKeyMap.get(fontData);
	} else {
		container = new ScaledCustomFontContainer(fontData);
		customFontsKeyMap.put(fontData, container);
	}
	return container.getScaledFont(zoom);
}

If we change the font data it creates a new ScaledCustomFont which for me is correct behavior. Even later if the Font is requested for 200% zoom we will use the modified fontData and then scale it to 200% zoom.

Even if we use the clone image data to store it as a key. It won't be updated by reference but the output for your example will still remain.

found!
null

My conclusion is that we don't really need to do anything here.

@fedejeanne @akoch-yatta

The issue is with the line adding to the map: customFontsKeyMap.put(fontData, container)

This results in the passed font data being placed inside the map, but the data be modified by the consumer that passed it in afterwards, i.e., something like:

registry.getFont(fontData, zoom);
fontData.height += 1;

My conclusion is that we don't really need to do anything here.

The resulting font might be fine but the fact that there is a dangling (unusable) entry in the map is the issue I described.

As Heiko mentioned, the issue described here is that when you change the internal state (a field) of the FontData that was used as a key, its hashCode and equals methods will return something different and therefore the call to if (customFontsKeyMap.containsKey(fontData)) will yield false even though the instance of FontData is still there.

If we make FontData immutable then the consumers will be forced to create a new instance every time they want to change its state, which would allow us to use a WeakHashMap and automatically remove the entry from the map once the (now unused) original FontData is nullified.

If we make FontData immutable then the consumers will be forced to create a new instance every time they want to change its state, which would allow us to use a WeakHashMap and automatically remove the entry from the map once the (now unused) original FontData is nullified.

Just to avoid confusion and clarify that we are on the same page: the goal of this is not to make FontData immutable but just to clone the FontData stored inside the map, such that no one will be able to modify it.

Just to avoid confusion and clarify that we are on the same page: the goal of this is not to make FontData immutable but just to clone the FontData stored inside the map, such that no one will be able to modify it.

But that would mean that you will still have dangling entries in the map.

Try this:

public static void main(String[] args) {
	Map<FontData, String> map = new HashMap<>();

	FontData fontData = new FontData("f1", 10, 10);
	FontData clone = new FontData(fontData.getName(), fontData.getHeight(), fontData.getStyle());

	// store the clone but search for the original one
	map.put(clone, "found!");

	System.out.println(map.get(fontData));

	// modify the original one and search for it
	fontData.height += 1;
	System.out.println(map.get(fontData));
}

The output is still:

found!
null

Let's talk about it in the daily.

Let's talk about it in the daily.

Discussed: since the idea is to support the use-case where one creates "the same" font at a later point in time and passes it as a parameter to the getFont(FontData fd, ...) then storing a clone of the FontData in the map is the correct approach.