[Win32] Consolidate tool bar image list release and disposal - #3513
Open
HeikoKlare wants to merge 3 commits into
Open
[Win32] Consolidate tool bar image list release and disposal#3513HeikoKlare wants to merge 3 commits into
HeikoKlare wants to merge 3 commits into
Conversation
Contributor
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step3
branch
from
August 14, 2026 10:58
d510326 to
ec54e1b
Compare
HeikoKlare
changed the title
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step3
branch
2 times, most recently
from
August 14, 2026 13:47
84bd44e to
9eabb55
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors Win32 ToolBar image-list lifecycle management by centralizing creation, mutation, refresh (native synchronization), and disposal of the normal/hot/disabled ImageList trio. It reduces duplicated code across ToolBar.destroyItem(), ToolBar.releaseWidget(), and ToolBar.updateOrientation(), while updating ToolItem to interact with image lists only through ToolBar helpers.
Changes:
- Replace
ToolItem’s directImageListmanipulation withToolBar.clearImage(...),ToolBar.addImage(...), andToolBar.putImage(...). - Introduce
ToolBarhelpers for image list creation/mutation (addImage,putImage) and native synchronization (refreshImageLists). - Consolidate disposal/release logic via
clearAndReleaseImageLists(...)andreleaseImageLists(...).
Reviewed changes
Copilot reviewed 2 out of 2 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 | Routes image list updates through ToolBar helpers instead of manipulating lists directly. |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java | Encapsulates image list state, adds refresh/release helpers, and updates callers to use consolidated 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-step3
branch
from
August 14, 2026 15:03
9eabb55 to
79d3acc
Compare
HeikoKlare
marked this pull request as ready for review
August 14, 2026 15:18
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step3
branch
from
August 14, 2026 16:02
79d3acc to
7ee6864
Compare
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
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step3
branch
from
August 14, 2026 17:37
7ee6864 to
71bcb33
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'sdestroyItem(when the last button is removed) andreleaseWidgeteach released the three image lists and cleared their native references with near-identical, duplicated code;updateOrientationreleased and swapped them inline as well. Consolidates all three into sharedclearAndReleaseImageLists/releaseImageListshelpers built on top ofrefreshImageLists. All of them refresh withitemsChanged=false: indestroyItemandreleaseWidgetno tool bar buttons remain that the drop-down padding workaround would have to protect, and inupdateOrientationthe buttons are only re-pointed at their migrated images rather than added or recreated.updateOrientationreleased 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, matchingclearAndReleaseImageLists.Related to #3466
Note: This change is human-crafted and was only slightly revised and documented with the help of AI
This is the third of multiple incremental steps to enhance ImageList handling and consistency inside ToolBar. This is supposed to be merged for 2026-12 M1.