Add ImageList.UseMaskForTransparency for better image transparency#14238
Add ImageList.UseMaskForTransparency for better image transparency#14238xPaw wants to merge 1 commit intodotnet:mainfrom
ImageList.UseMaskForTransparency for better image transparency#14238Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ImageList.UseMaskForTransparency option intended to control whether ImageList uses a separate mask bitmap for transparency when adding/replacing bitmap images.
Changes:
- Added a new public
UseMaskForTransparencyproperty onImageList(defaulting totrue). - Updated bitmap add/replace code paths to conditionally skip creating a mask and instead use
Bitmap.GetHbitmap()with a null mask. - Added a new localized description string for the new property.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/System.Windows.Forms/System/Windows/Forms/Controls/ImageList/ImageList.ImageCollection.cs | Conditional logic for replacing an image now branches on UseMaskForTransparency and introduces a no-mask path. |
| src/System.Windows.Forms/System/Windows/Forms/Controls/ImageList/ImageList.cs | Adds the new public property and updates AddToHandle to optionally add bitmaps without generating a mask. |
| src/System.Windows.Forms/Resources/SR.resx | Adds a new SR description string for the property’s designer metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HBITMAP hMask = (HBITMAP)ControlPaint.CreateHBitmapTransparencyMask(bitmap); | ||
| HBITMAP hBitmap = (HBITMAP)ControlPaint.CreateHBitmapColorMask(bitmap, hMask); | ||
| bool ok; | ||
| try |
There was a problem hiding this comment.
In the UseMaskForTransparency code path, ok is declared twice. The inner bool ok; in the else block shadows the outer variable, so the if (!ok) check after the conditional refers to an unassigned variable when the else branch runs (won't compile). Remove the inner declaration and assign to the already-declared ok.
| HBITMAP hBitmap = (HBITMAP)bitmap.GetHbitmap(); | ||
| try | ||
| { | ||
| using var ilHandle = new DeleteObjectSafeHandle(_owner.Handle, ownsHandle: false); | ||
| ok = PInvoke.ImageList_Replace(ilHandle, index, hBitmap, default); | ||
| } |
There was a problem hiding this comment.
DeleteObjectSafeHandle is not defined in this project and (even if it existed) is not the right wrapper for a HIMAGELIST. Passing it to PInvoke.ImageList_Replace is also inconsistent with the existing PInvoke.ImageList.Replace(_owner, ...) wrapper which already handles keeping the owner alive. Use the existing PInvoke.ImageList.Replace(_owner, index, hBitmap, default) overload instead of introducing a new handle wrapper here.
| HBITMAP hBitmap = (HBITMAP)bitmap.GetHbitmap(); | ||
| try | ||
| { | ||
| using var ilHandle = new DeleteObjectSafeHandle(Handle, ownsHandle: false); | ||
| index = PInvoke.ImageList_Add(ilHandle, hBitmap, default); | ||
| } |
There was a problem hiding this comment.
This branch introduces DeleteObjectSafeHandle and calls PInvoke.ImageList_Add(ilHandle, ...). ImageList_Add expects a HIMAGELIST (see existing PInvoke.ImageList.Add<T>(T himl, ...) wrapper), so this is very likely a type mismatch / compile break. Use the existing PInvoke.ImageList.Add(this, hBitmap, default) wrapper instead (it already does GC.KeepAlive correctly) and drop the custom handle wrapper.
| /// <summary> | ||
| /// Gets or sets whether to use a mask for transparency. | ||
| /// </summary> | ||
| [SRCategory(nameof(SR.CatBehavior))] | ||
| [SRDescription(nameof(SR.ImageListUseMaskForTransparencyDescr))] | ||
| public bool UseMaskForTransparency { get; set; } = true; |
There was a problem hiding this comment.
The new UseMaskForTransparency property currently only changes how images are added/replaced, but CreateHandle() always creates the native image list with ILC_MASK. With ILC_MASK set, calling ImageList_Add/Replace with a null mask will still cause a mask to be generated by comctl32 (typically from the upper-left pixel), so the property likely doesn't do what its name/description promise. Consider wiring this property into handle creation flags (and recreating the handle when it changes) so that disabling masks truly disables mask-based transparency.
| /// <summary> | ||
| /// Gets or sets whether to use a mask for transparency. | ||
| /// </summary> | ||
| [SRCategory(nameof(SR.CatBehavior))] | ||
| [SRDescription(nameof(SR.ImageListUseMaskForTransparencyDescr))] | ||
| public bool UseMaskForTransparency { get; set; } = true; |
There was a problem hiding this comment.
New public behavior (UseMaskForTransparency) is introduced without any unit tests. There are already extensive ImageListTests; please add coverage for the default value and at least one scenario that exercises the new code path (e.g., adding/replacing a bitmap with transparency and verifying handle/image count or resulting bitmap characteristics).
|
You can ask copilot to make a new pull request. |
Fixes #14171. See that issue for more details.
This is a speculative PR because:
Proposed changes
Customer Impact
Regression?
Risk
Screenshots
Before
After
Test methodology
Accessibility testing
Test environment(s)