[Win32] Encapsulate ToolItem's access to tool bar image lists

ToolItem manipulated ToolBar's three normal/hot/disabled ImageLists
directly: reading them via ToolBar's getters, creating them inline
when absent, and calling add()/put() on each of them itself. This
spread the bookkeeping for a single item's images across both
classes and made ToolItem responsible for details that are really
ToolBar's to own, such as lazily creating the image lists sized to
the first image added.

With this change, ToolBar exposes addImage/putImage/clearImage
instead, and ToolItem goes through these instead of touching
ImageList directly. ToolBar's internal representation (three
separate ImageList fields, and their existing
setImageList/setHotImageList/setDisabledImageList synchronization
methods) is otherwise unchanged.

This is a behavior-preserving refactoring: the image lists are still
created, filled and synchronized with the same values as before.

The disabledImageList != null guard in ToolItem.updateImages is
dropped rather than moved: in that branch the item already has an
image index, so all three image lists necessarily exist, as they are
only ever created together in addImage and cleared together in
destroyItem and releaseWidget.

Related to
#3466

Author: HeikoKlare

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java

@@ -55,7 +55,7 @@ public class ToolBar extends Composite {
 	ToolItem [] items;
 	ToolItem [] tabItemList;
 	boolean ignoreResize, ignoreMouse;
-	ImageList imageList, disabledImageList, hotImageList;
+	private ImageList imageList, disabledImageList, hotImageList;
 	static final long ToolBarProc;
 	static final TCHAR ToolBarClass = new TCHAR (OS.TOOLBARCLASSNAME, true);
 	static {
@@ -143,6 +143,34 @@ public ToolBar (Composite parent, int style) {
 	}
 }
 
+/*
+ * The given image bounds are the bounds of the tool item's image and determine which shared image
+ * lists are used. They are intentionally not derived from the images actually added: for a disabled
+ * item with CHECK or RADIO style, those are the disabled images, which may have different bounds.
+ * Note that the icon size of an image list is defined by the first image added to it.
+ */
+int addImage(Rectangle imageBounds, Image image, Image hotImage, Image disabledImage) {
+	int listStyle = style & SWT.RIGHT_TO_LEFT;
+	if (imageList == null) {
+		imageList = display.getImageListToolBar(listStyle, imageBounds.width, imageBounds.height, getAutoscalingZoom());
+	}
+	if (hotImageList == null) {
+		hotImageList = display.getImageListToolBarHot(listStyle, imageBounds.width, imageBounds.height,
+				getAutoscalingZoom());
+	}
+	if (disabledImageList == null) {
+		disabledImageList = display.getImageListToolBarDisabled(listStyle, imageBounds.width, imageBounds.height,
+				getAutoscalingZoom());
+	}
+	int index = imageList.add(image);
+	hotImageList.add(hotImage);
+	disabledImageList.add(disabledImage);
+	setImageList(imageList);
+	setHotImageList(hotImageList);
+	setDisabledImageList(disabledImageList);
+	return index;
+}
+
 @Override
 long callWindowProc (long hwnd, int msg, long wParam, long lParam) {
 	if (handle == 0) return 0;
@@ -199,6 +227,10 @@ public void layout (boolean changed) {
 	super.layout(changed);
 }
 
+void clearImage(int index) {
+	putImage(index, null, null, null);
+}
+
 void clearSizeCache(boolean changed) {
 	// If changed, discard the cached layout information
 	if (changed) {
@@ -869,6 +901,18 @@ boolean mnemonicMatch (char ch) {
 	return findMnemonic (items [id [0]].text) != '\0';
 }
 
+void putImage(int index, Image image, Image hotImage, Image disabledImage) {
+	if (imageList != null) {
+		imageList.put(index, image);
+	}
+	if (hotImageList != null) {
+		hotImageList.put(index, hotImage);
+	}
+	if (disabledImageList != null) {
+		disabledImageList.put(index, disabledImage);
+	}
+}
+
 @Override
 void releaseChildren (boolean destroy) {
 	if (items != null) {

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolItem.java

@@ -535,12 +535,7 @@ void releaseImages () {
 	* an image and one is never assigned, this is not a problem.
 	*/
 	if ((info.fsStyle & OS.BTNS_SEP) == 0 && info.iImage != OS.I_IMAGENONE) {
-		ImageList imageList = parent.getImageList ();
-		ImageList hotImageList = parent.getHotImageList ();
-		ImageList disabledImageList = parent.getDisabledImageList();
-		if (imageList != null) imageList.put (info.iImage, null);
-		if (hotImageList != null) hotImageList.put (info.iImage, null);
-		if (disabledImageList != null) disabledImageList.put (info.iImage, null);
+		parent.clearImage(info.iImage);
 	}
 }
 
@@ -1099,21 +1094,7 @@ void updateImages (boolean enabled) {
 	info.dwMask = OS.TBIF_IMAGE;
 	OS.SendMessage (hwnd, OS.TB_GETBUTTONINFO, id, info);
 	if (info.iImage == OS.I_IMAGENONE && image == null) return;
-	ImageList imageList = parent.getImageList ();
-	ImageList hotImageList = parent.getHotImageList ();
-	ImageList disabledImageList = parent.getDisabledImageList();
 	if (info.iImage == OS.I_IMAGENONE) {
-		Rectangle boundsInPoints = image.getBounds();
-		int listStyle = parent.style & SWT.RIGHT_TO_LEFT;
-		if (imageList == null) {
-			imageList = display.getImageListToolBar (listStyle, boundsInPoints.width, boundsInPoints.height, getAutoscalingZoom());
-		}
-		if (disabledImageList == null) {
-			disabledImageList = display.getImageListToolBarDisabled (listStyle, boundsInPoints.width, boundsInPoints.height, getAutoscalingZoom());
-		}
-		if (hotImageList == null) {
-			hotImageList = display.getImageListToolBarHot (listStyle, boundsInPoints.width, boundsInPoints.height, getAutoscalingZoom());
-		}
 		Image disabled = disabledImage;
 		if (disabledImage == null) {
 			if (disabledImage2 != null) disabledImage2.dispose ();
@@ -1134,27 +1115,19 @@ void updateImages (boolean enabled) {
 		if ((style & (SWT.CHECK | SWT.RADIO)) != 0) {
 			if (!enabled) image2 = hot = disabled;
 		}
-		info.iImage = imageList.add (image2);
-		disabledImageList.add (disabled);
-		hotImageList.add (hot != null ? hot : image2);
-		parent.setImageList (imageList);
-		parent.setDisabledImageList (disabledImageList);
-		parent.setHotImageList (hotImageList);
+		info.iImage = parent.addImage(image.getBounds(), image2, hot != null ? hot : image2, disabled);
 	} else {
 		Image disabled = null;
-		if (disabledImageList != null) {
-			if (image != null) {
-				if (disabledImage2 != null) disabledImage2.dispose ();
-				disabledImage2 = null;
-				disabled = disabledImage;
-				if (disabledImage == null) {
-					disabled = image;
-					if (!enabled) {
-						disabled = disabledImage2 = new Image (display, image, SWT.IMAGE_DISABLE);
-					}
+		if (image != null) {
+			if (disabledImage2 != null) disabledImage2.dispose ();
+			disabledImage2 = null;
+			disabled = disabledImage;
+			if (disabledImage == null) {
+				disabled = image;
+				if (!enabled) {
+					disabled = disabledImage2 = new Image (display, image, SWT.IMAGE_DISABLE);
 				}
 			}
-			disabledImageList.put (info.iImage, disabled);
 		}
 		/*
 		* Bug in Windows.  When a tool item with the style
@@ -1167,12 +1140,8 @@ void updateImages (boolean enabled) {
 		if ((style & (SWT.CHECK | SWT.RADIO)) != 0) {
 			if (!enabled) image2 = hot = disabled;
 		}
-		if (imageList != null) {
-			imageList.put (info.iImage, image2);
-		}
-		if (hotImageList != null) {
-			hotImageList.put (info.iImage, hot != null ? hot : image2);
-		}
+
+		parent.putImage(info.iImage, image2, hot != null ? hot : image2, disabled);
 		if (image == null) info.iImage = OS.I_IMAGENONE;
 	}