Make FontData key in map of ScalingSWTFontRegistry immutable

[fedejeanne]

As mentioned in point Nr. 3 of #98 (comment) (pasted below), this could lead to problems if someone changes the internal state of fontData.

3. org.eclipse.swt.internal.ScalingSWTFontRegistry.fontKeyMap uses FontData as key

(Same problem for org.eclipse.swt.internal.DefaultSWTFontRegistry.fontsMap).

FontData is not immutable which may lead to information getting lost.

Example

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

  FontData fontData = new FontData("f1", 10, 10);
  map.put(fontData, "found!");

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

  fontData.height += 1;

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

Produces the output:

found!
null

Task

Make the key in the registry map immutable. Possible options are:

• Clone the FontData used as key so that no one outside the registry has access to it and may modify it
• Find some other kind of key that is both unique and immutable

[ShahzaibIbrahim]

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

[HeikoKlare]

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;

[fedejeanne]

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.