Fix memory-leak in FigureUtilities by disposing GC on font change

[arunjose696]

FigureUtilities create a GC once for an application and then reuse it to calculate font/text dimensions. Due to the modification of GC to store Operations, every operation on FigureUtilities that sets the font on the GC (i.e., that calls setFont()) creates a SetFontOperation in the GC. Since that GC is never dispose, these operation accumulate throughout the application lifecycle and constituted a memory leak

The current change addresses this issue by creating a single shell for the lifetime of the application. Whenever a different font needs to be set, the old GC is properly disposed and replaced with a new one created from the shell. This ensures that no GC objects persist indefinitely, effectively preventing the memory leak. The now unused getGC() method which was protected has been removed from the codebase as part of this change.

Steps to reproduce in vi-eclipse/Eclipse-Platform#443 (comment)

1. Start an Eclipse product with GEF installed
2. Open and close any GEF diagram (such as an empty Logic or Shapes example)
3. Take a memory heapdump, e.g., with VisualVM
4. Open and close the same GEF diagram one or multiple times
5. Take another memory heapdump and compare with the original one

You see that the number of org.eclipse.swt.graphics.GC$SetFontOperation objects in the heap increased (and increases with every GEF diagram that is opened) without this change

[ptziegler]

Even though getGC() is deprecated, it is still API and therefore must not be removed.

[ptziegler]

I wonder if it makes sense to keep the font functionality in the FigureUtilities or whether it should be moved to the TextUtilities. Perhaps it's possible to re-use the DrawableTextUtilities and have it be accessible from every Figure. Then the references to the FigureUtilities can be removed and the old methods be deprecated...

[fedejeanne]

I'm not a maintainer of GEF (so please double-check my proposals below) but I would definitely not remove the method since it's still API (being protected means that it can be accessed from subclasses or from the same package).

[arunjose696]

Even though getGC() is deprecated, it is still API and therefore must not be removed.

I’ve reintroduced the method in my change to preserve the previous behavior. However, since this method has been deprecated for over 15 years, it might make sense to mark it for removal if there isnt specific reason to keep it. Given that all methods in this class are static, it’s unlikely that the class is intended to be subclassed, and I don’t see any usage of this method in the Draw2D package. The new logic doesn’t rely on this method, so keeping it seems unnecessary.

I wonder if it makes sense to keep the font functionality in the FigureUtilities or whether it should be moved to the TextUtilities. Perhaps it's possible to re-use the DrawableTextUtilities and have it be accessible from every Figure. Then the references to the FigureUtilities can be removed and the old methods be deprecated...

DrawableTextUtilities has non-static methods and this class requires instantiation with a Control source. Looking at how font functionality is currently used, we generally don’t have access to a Control, so moving the logic there isn’t straightforward. Additionally, the TextUtilities parent class computes font information by calling FigureUtilities, so addressing this would also require fixing the existing memory leak.

[fedejeanne]

LGTM

[ptziegler]

I’ve reintroduced the method in my change to preserve the previous behavior. However, since this method has been deprecated for over 15 years, it might make sense to mark it for removal if there isnt specific reason to keep it.

That is true. Though I think all of the font-related methods in the FigureUtilities class don't work well when dealing with multiple zoom levels.

DrawableTextUtilities has non-static methods and this class requires instantiation with a Control source. Looking at how font functionality is currently used, we generally don’t have access to a Control, so moving the logic there isn’t straightforward. Additionally, the TextUtilities parent class computes font information by calling FigureUtilities, so addressing this would also require fixing the existing memory leak.

Each Figure generally has a RootFigure. This RootFigure belongs to a LightweightSystem, which is hooked to a Canvas. I've created #817 as an example. This would also fix the memory leak, because the GC is now disposed together with the canvas.

[ptziegler]

@arunjose696 Can you also update your commit message? This part no longer matches the current state:

The now-unused getGC() method which was protected has been removed from the codebase as part of this change.

[arunjose696]

@arunjose696 Can you also update your commit message? This part no longer matches the current state:

done

[ptziegler]

Thanks!