[Win32] Consolidate tool bar image list native synchronization - #3508
Open
HeikoKlare wants to merge 2 commits into
Open
[Win32] Consolidate tool bar image list native synchronization #3508HeikoKlare wants to merge 2 commits into
HeikoKlare wants to merge 2 commits into
Conversation
Contributor
HeikoKlare
marked this pull request as ready for review
August 14, 2026 06:46
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step2
branch
from
August 14, 2026 09:34
82224f9 to
675e221
Compare
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step2
branch
from
August 14, 2026 10:44
675e221 to
70fd7e2
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors Win32 ToolBar image-list synchronization by centralizing native TB_SET*IMAGELIST updates into ToolBar.refreshImageList(s) and routing ToolItem image updates through ToolBar methods (addImage/putImage/clearImage) instead of manipulating the three ImageList instances directly. This supports the ongoing effort to make DPI-change image handling more consistent and maintainable (per the linked issue/series).
Changes:
- Replace
ToolBar’s per-list getter/setter sync methods with a consolidatedrefreshImageList/refreshImageListsimplementation. - Introduce
ToolBar.addImage/putImage/clearImageand updateToolItemto use them. - Adjust image-list refresh call sites to use the consolidated batch refresh approach.
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 | Switches image-list manipulation to ToolBar helper methods for add/put/clear operations. |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/ToolBar.java | Adds consolidated image-list refresh helpers and new internal image-list mutation APIs; updates orientation/DPI paths to use them. |
💡 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-step2
branch
4 times, most recently
from
August 14, 2026 14:34
c823e39 to
64ecb9c
Compare
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step2
branch
from
August 14, 2026 16:01
64ecb9c to
637edb7
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
HeikoKlare
force-pushed
the
toolbar-imagelists-cleanup-step2
branch
from
August 14, 2026 17:36
637edb7 to
d0fd66f
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 #3504
ToolBar'ssetImageList/setHotImageList/setDisabledImageListeach carried a near-identical copy of the logic to compare the currentTB_GET*IMAGELISThandle against the ImageList's handle for the current zoom, and, if different, apply it viaTB_SET*IMAGELISTwhile togglingsetDropDownItemsaround it to avoid a Windows layout glitch.handleDPIChangeandaddImageeach called all three setters in turn whenever any one image list might have changed.This change consolidates that duplicated logic into a single
refreshImageListsmethod operating on the current field values, replacing the three getters and three setters.ToolBar's representation is still three separateImageListfields. 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:
refreshImageListstogglessetDropDownItemsat 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 forupdateOrientation. Both the toggling and the native updates areskipped 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 #3466
Note: This change is human-crafted and was only slightly revised and documented with the help of AI
This is the second of multiple incremental steps to enhance ImageList handling and consistency inside ToolBar. This is supposed to be merged for 2026-12 M1.