[Win32] Keep tool bar normal, hot and disabled image lists index-aligned

[HeikoKlare]

Important

This change is based on and should thus be merged after:

• [Win32] Encapsulate ToolItem's access to tool bar image lists #3504
• [Win32] Consolidate tool bar image list native synchronization  #3508
• [Win32] Consolidate tool bar image list release and disposal #3513
• [Win32] Consolidate tool bar image lists in ToolBarImageLists #3515

Problem

On Windows a ToolItem could render another item's hot (hover) or disabled icon — no multiple monitors and no image disposal required. Setting an image, then a hot image, then clearing the image while adding a new item to the tool bar is already enough to trigger it.

A tool bar keeps three native image lists (normal, hot, disabled), but each button stores a single image index that addresses all three lists at once. The three lists therefore have to stay index-aligned. Two code paths in ToolItem.updateImages broke that invariant:

• Clearing the normal image while a hot image is still set freed the normal and disabled slots but kept the hot slot occupied. A later item that reused the freed normal slot then rendered the first item's stale hot icon on hover.
• Adding a first image appended it to each list with three independent add() calls. Once the lists' free-slot patterns diverged (as caused by the first defect), those calls could return different indices. Only the normal list's index is written to the button, so its slot could end up pointing at another item's hot or disabled image.

Contributes to #3466.

Fix

Enforce one index per item across all three lists:

• Free the hot slot too when the normal image is cleared, so the hot list's free-slot pattern no longer diverges from the normal and disabled lists.
• Derive the slot index once from the normal list and write the hot and disabled lists at that exact index (instead of relying on three independent add() calls returning matching indices). ToolBar.updateOrientation adopts the same discipline.

This is implemented with a small ImageList.putAt(index, image) that stores an image at a caller-chosen index (appending when the index equals the current size, otherwise replacing or, for a null image, clearing the slot). add(image) is now implemented on top of it by first computing the target index.

Reproduction

Run the snippet below and hover the mouse over the second (blue) button:

• Before the fix: the blue button turns green — it renders the first item's leftover hot image, even though it has no hot image of its own.
• After the fix: the blue button stays blue.

Before (wrong behavior):

After (corrected behavior):

/*
 * Reproduction for a Win32 tool bar item rendering another item's hot (hover)
 * icon, without any multiple monitors or image disposal involved.
 *
 * Steps performed below:
 *   - item0 gets a RED normal image and a GREEN hot image
 *   - item0's image is cleared (setImage(null)) while the hot image stays set
 *   - item1 gets a BLUE normal image and NO hot image
 *
 * Hover the mouse over the second (blue) button:
 *   Expected (fixed): it stays BLUE (item1 has no hot image).
 *   Bug:              it turns GREEN, i.e. item1 renders item0's leftover hot image.
 */
import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.Color;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Image;
import org.eclipse.swt.graphics.RGB;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Label;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.ToolBar;
import org.eclipse.swt.widgets.ToolItem;

public class ToolItemHotImageReproduction {

	public static void main(String[] args) {
		Display display = new Display();
		Image red = solidImage(display, new RGB(220, 40, 40));
		Image green = solidImage(display, new RGB(40, 180, 40));
		Image blue = solidImage(display, new RGB(40, 40, 220));

		Shell shell = new Shell(display);
		shell.setLayout(new GridLayout());
		shell.setText("ToolItem hot image reproduction");

		ToolBar bar = new ToolBar(shell, SWT.FLAT);

		ToolItem item0 = new ToolItem(bar, SWT.PUSH);
		item0.setImage(red);
		item0.setHotImage(green); // item0 shows GREEN on hover
		item0.setImage(null);     // clear item0's image; its hot image stays set

		ToolItem item1 = new ToolItem(bar, SWT.PUSH);
		item1.setImage(blue);     // item1: BLUE, no hot image

		Label hint = new Label(shell, SWT.WRAP);
		hint.setText("Hover the mouse over the second (blue) button.\n"
				+ "Expected: it stays BLUE (it has no hot image).\n"
				+ "Bug: it turns GREEN (item0's leftover hot image).");

		shell.setSize(360, 150);
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) display.sleep();
		}

		red.dispose();
		green.dispose();
		blue.dispose();
		display.dispose();
	}

	private static Image solidImage(Display display, RGB rgb) {
		Image image = new Image(display, 16, 16);
		GC gc = new GC(image);
		Color color = new Color(display, rgb);
		gc.setBackground(color);
		gc.fillRectangle(image.getBounds());
		gc.dispose();
		return image;
	}
}

[github-actions]

Test Results (win32)

   35 files  ±0     35 suites  ±0   4m 54s ⏱️ -25s
4 879 tests +8  4 803 ✅ +8  76 💤 ±0  0 ❌ ±0 
1 406 runs  +8  1 382 ✅ +8  24 💤 ±0  0 ❌ ±0 

Results for commit 63e1819. ± Comparison against base commit bb1b092.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

This PR fixes a Win32-specific toolbar rendering defect where a ToolItem could show another item’s hot (hover) or disabled image due to the native image lists (normal/hot/disabled) becoming index-misaligned. It enforces a single authoritative slot index per item across all three image lists to prevent stale or cross-item icon lookups.

Changes:

• ToolItem.updateImages: derive the slot index once from the normal image list and write hot/disabled images at that same index; additionally clear the hot slot when the normal image is cleared.
• ToolBar.updateOrientation: when rebuilding image lists, keep new hot/disabled lists aligned by writing at the normal list’s returned index (instead of independent add(...) calls).
• ImageList: introduce putAt(index, image) and implement add(image) on top of it.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolItem.java	Keeps normal/hot/disabled image list indices aligned and clears hot slots when the normal image is cleared.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java	Preserves index alignment across rebuilt image lists during orientation updates.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/internal/ImageList.java	Adds putAt(...) to support caller-chosen indices and refactors add(...) to use it.

[r-mennig]

ImageList now contains both a public putAt(int index, Image image) and a public put(int index, Image image) method where putAt allows appending and put does not. So maybe putOrAppend would be a better name for this method?

[HeikoKlare]

That a valid concern. I had originally named that method putOrAppend before I changed it, but I agree that given the already existing put, this is very misleading. Rather than renaming, I have now merged the two: putAt is gone and put now also accepts the index right after the last slot, appending there.

put previously ignored such an index silently, which is the surprising behaviour of the two. Map.put inserts when the key is absent, so store-or-append is closer to what the name suggests. add keeps its shape (find the first free slot, then put), which now describes exactly what it does.

The change is safe for existing callers: the sole caller outside ToolBar/ToolItem is TabFolder, whose index comes from indexOf and is therefore always inside the list, i.e., it cannot reach the new append path. Every other caller passes a button's iImage, likewise always in range.

I have also rebased this change/PR on a sequence of refactorings for properly encapsulating and aligning the three ImageLists of a ToolBar inside the ToolBar class and a specific three-list container. Only the last of the current commits compared to master covers the actual change of the PR.
I have also added a bunch of tests for preserving the enhanced ImageList behavior.