[Win32] Consolidate tool bar image lists in ToolBarImageLists - #3515
Open
HeikoKlare wants to merge 4 commits into
Open
[Win32] Consolidate tool bar image lists in ToolBarImageLists#3515HeikoKlare wants to merge 4 commits into
HeikoKlare wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs an internal Win32 refactor of ToolBar image-list handling by consolidating the normal/hot/disabled ImageList trio into a single ToolBarImageLists unit, reducing duplicated bookkeeping while keeping the public ToolBar/ToolItem image APIs unchanged.
Changes:
- Introduces
ToolBarImageListsto own and operate on the three image lists as one aligned unit. - Updates
ToolBarto use the consolidated image-list owner for creation, refresh (native sync), migration on orientation updates, and release. - Updates
ToolItemto route image add/put/clear operations throughToolBar’s internal API instead of manipulating individual lists.
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 | Replaces direct list manipulation with ToolBar internal methods (addImage/putImage/clearImage). |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBarImageLists.java | New helper class encapsulating normal/hot/disabled ImageList operations as a single unit. |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java | Replaces three separate image-list fields and related logic with a single ToolBarImageLists field plus consolidated refresh/release/migration logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step4
branch
from
August 14, 2026 16:02
3d29497 to
1b762a2
Compare
HeikoKlare
marked this pull request as ready for review
August 14, 2026 17:02
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 eclipse-platform#3466
ToolBar's setImageList/setHotImageList/setDisabledImageList each carried a near-identical copy of the logic to compare the current TB_GET*IMAGELIST handle against the ImageList's handle for the current zoom, and, if different, apply it via TB_SET*IMAGELIST while toggling setDropDownItems around it to avoid a Windows layout glitch. handleDPIChange and addImage each called all three setters in turn whenever any one image list might have changed. This change consolidates that duplicated logic into a single refreshImageLists method operating on the current field values, replacing the three getters and three setters. ToolBar's representation is still three separate ImageList fields. It prepares the ground for introducing a class that owns the three image lists as one unit. This is not a purely behavior-preserving refactoring: refreshImageLists toggles setDropDownItems at most once per refresh batch instead of once per list, and only when the tool bar's buttons are actually being added or recreated (addImage, handleDPIChange) rather than for updateOrientation. Both the toggling and the native updates are skipped entirely when the image lists already set on the tool bar are the current ones, which in particular avoids any native calls for tool bars without images. Related to eclipse-platform#3466
destroyItem (when the last button is removed) and releaseWidget each released the three image lists and cleared their native references with near-identical, duplicated code; updateOrientation released and swapped them inline as well. Consolidates all three into shared clearAndReleaseImageLists/releaseImageLists helpers built on top of refreshImageLists. All of them refresh with itemsChanged=false: in destroyItem and releaseWidget no tool bar buttons remain that the drop-down padding workaround would have to protect, and in updateOrientation the buttons are only re-pointed at their migrated images rather than added or recreated. updateOrientation released the old image lists before pointing the native tool bar at the new ones, leaving a window where the control could reference an already-disposed image list. It now assigns the fields to the freshly created lists upfront, migrating each button's images from the old lists (kept in local variables) into them, and only refreshes the native references and releases the old lists afterwards, matching clearAndReleaseImageLists. Related to eclipse-platform#3466
ToolBar's three image lists were still three separate fields that were created, filled, cleared, moved and released individually in addImage, putImage, destroyItem, releaseWidget and updateOrientation, even though they only ever change together and have to stay index-aligned. This change introduces ToolBarImageLists, which owns the three ImageLists as one unit and offers the operations on them as single calls. Synchronizing the image lists with the native control remains in ToolBar, which retrieves the handles to set from ToolBarImageLists. ToolBar's addImage/putImage/clearImage API (and therefore ToolItem, which only calls that API) is unaffected; this change is entirely internal to ToolBar. This is a behavior-preserving internal refactor; it does not itself change what is rendered. It lays the groundwork for fixing the image-list index-alignment problems tracked in eclipse-platform#3466
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step4
branch
from
August 14, 2026 17:37
1b762a2 to
693bd51
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
This change is based on and should thus be merged after:
ToolBar's three image lists were still three separate fields that were created, filled, cleared, moved and released individually inaddImage,putImage,destroyItem,releaseWidgetandupdateOrientation, even though they only ever change together and have to stay index-aligned.This change introduces
ToolBarImageLists, which owns the threeImageListsas one unit and offers the operations on them as single calls. Synchronizing the image lists with the native control remains inToolBar, which retrieves the handles to set fromToolBarImageLists.ToolBar'saddImage/putImage/clearImageAPI (and thereforeToolItem, which only calls that API) is unaffected; this change is entirely internal toToolBar.This is a behavior-preserving internal refactor; it does not itself change what is rendered.
Related to #3466
Note: This change is human-crafted and was only slightly revised and documented with the help of AI
This is the fourth of multiple incremental steps to enhance ImageList handling and consistency inside ToolBar. This is supposed to be merged for 2026-12 M1.