Using ImageHandleManager for managing multiple Image handles in Image class

[amartya4256]

This PR introduces ImageHandleManager, an inner class to Image responsible for managing the storage, creation and manipulation of ImageHandles. This class provides methods which can be used to dead with ImageHandle instances inside an image for managing it with convenience while hiding the complexity of how the ImageHandle lifecycle is managed inside itself.

All the places where zoomToImageHandle map is accessed for any purposes is replaced by the wrapper methods provided by ImageHandleManager. Each image initializes a final field imageHandleManager with the instance of ImageHandleManager. Every instance of ImageHandle created within the Image object which is meant to be stored per zoom is registered in the manager itself and the manager is responsible for cleaning the dangling resources when are not supposed to be used anymore or when a cleanup is needed.

This contributes to vi-eclipse/Eclipse-Platform#562

[github-actions]

Test Results (win32)

   34 files  ±0     34 suites  ±0   4m 30s ⏱️ +31s
4 628 tests ±0  4 555 ✅ ±0  73 💤 ±0  0 ❌ ±0 
  167 runs  ±0    164 ✅ ±0   3 💤 ±0  0 ❌ ±0 

Results for commit fec96c6. ± Comparison against base commit 892be33.

♻️ This comment has been updated with latest results.

[HeikoKlare]

The changes look like a good refactoring to better encapsulate the management of handles for zooms and in general are fine fore me. I still have some proposals for improvements.

[HeikoKlare]

The hasImageHandle method is only used here and rather indicates a missing local state handling, I would propose to remove that method and manage the state locally instead.
The method is used to find out, if this HandleAtSize has created the handle on its own or whether it was retrieved from the ImageHandleManager. So the HandleAtSize could just store that it created the handle on its own (inside refresh()) and then just use that information here to determine if the handleContainer needs to be destroyed or not instead of indirectly retrieving that information again by querying the ImageHandleManager for whether it contains that handle.

[amartya4256]

You are right. As I can see, HandleAtSize doesn't register any ImageHandle explicitly and init is called with -1, which from the previous convention also means that it wasn't event intended to be added to the map. And as I can see, it is never added in the ImageHandleManager with the current implementation so I would simply remove this check as it is always true. And I will get rid of the method.

[HeikoKlare]

This is not correct. The handleContainer may be a handle from the ImageHandleManager, as refresh() may call getOrCreateImageHandleAtClosestSize(width, height) which can retrieve a handle from there. So it is not guaranteed that this handle is locally managed. The latest change may lead to a handle inside the ImageHandleManager being disposed.

[HeikoKlare]

Maybe we should add a check somewhere to better find accidental changes to the implementation that destroys handles from the manager outside of it? E.g., ImageHandle#destroy() may check for the handle not being contained in the ImageHandleManager.

Or may we can even better do that in an OO-safe way: remove the destroy method from ImageHandle and create a DestroyableImageHandle or the like. Then ImageHandleManager could only expose ImageHandle instances while internally managing destroyable ones. And the places where handles are created locally would maintain destroyable instances as well. Since we pass ImageHandles over to GC as well that would even increase safety as those other code places not not accidentally destroy handles anymore as well.

[amartya4256]

Although with this approach here we still need to check if it is in the manager because either we obtain it from the manager (Exposed Imagehandle) or create a temporary (DestroyableImageHandle)

[amartya4256]

Also the method which creates ImageHandle is consumed by both temporary user and permanent user, so we can utilize factory pattern here to create the instance of a specific class depending on who the consumer is.

Then we can have the following:

1. ImageHandle (no destroy, created by imageHandleManager, destroyed by a specific method inside the manager)
2. TemporaryHandle extends ImageHandle, has a destroy method.

• we can check if the instance is a temporaryhandle then destroy, in the case above. If type already know, destroying is straight forward.

[HeikoKlare]

Why are all these changes to GC necessary?

[amartya4256]

• PlainImageProviderWrapper#newImageHandle needs to perform GC#init amidst the creation of ImageHandle using ImageHandleManager#getOrCreate which is a synchronous call.
• GC#init calls Image.win32_getHandle which tries to create the handle since the current ImageHandle is not added to the map yet. As getOrCreate at first waits for the operation to complete and return a handle and then adds it to the map. We could provide a method like register(ImageHandle) which adds the ImageHandle amidst the creation of the ImageHandle using this method but then we put the manager at risk that some other consumer inside Image can misuse it. And what if newImageHandle (where the ImageHandle creation takes place) is called by some other consumer? Then the guarantee of synchronous ImageHandle creation will be lost.
• Hence, this takes a ImageHandle instead of the Image and perform the operation using the handle and not Image object.