Skip to content

Fix icons of PropertyGrid control are not scaled well on 100% secondary monitor #14828

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ricardobossan wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −4
Open

Fix icons of PropertyGrid control are not scaled well on 100% secondary monitor #14828

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/System.Windows.Forms/System/Windows/Forms/Controls/PropertyGrid/PropertyGrid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1753,12 +1753,19 @@ private PropertyGridToolStripButton CreatePushButton(
EventHandler eventHandler,
bool useRadioButtonRole = false)
{
// ToolStripItem/ToolStripButton bake Margin and the minimum button width from
// ScaleHelper.InitialSystemDpi (the process's startup DPI) at construction time. Re-apply both
// explicitly so a button (re)created after a runtime DPI change reflects the grid's actual current
// DPI; DeviceDpi routes through ToolStripButton's own existing, correct DeviceDpi setter override.
// See https://github.com/dotnet/winforms/issues/8268.
PropertyGridToolStripButton button = new(this, useRadioButtonRole)
{
Text = toolTipText,
AutoToolTip = true,
DisplayStyle = ToolStripItemDisplayStyle.Image,
ImageIndex = imageIndex
ImageIndex = imageIndex,
Margin = ScaleHelper.ScaleToDpi(new Padding(0, 1, 0, 2), DeviceDpi),
DeviceDpi = DeviceDpi
};

button.Click += eventHandler;
Expand Down Expand Up @@ -2106,7 +2113,7 @@ private void EnsureLargeButtons()
ImageSize = s_largeButtonSize
};

if (ScaleHelper.IsScalingRequired)
if (ScaleHelper.IsScalingRequirementMet)
{
AddLargeImage(_alphaBitmap);
AddLargeImage(_categoryBitmap);
Expand Down Expand Up @@ -2140,7 +2147,7 @@ private void EnsureLargeButtons()
}
}

// This method should be called only inside a if (DpiHelper.IsScalingRequired) clause.
// This method should be called only inside a if (ScaleHelper.IsScalingRequirementMet) clause.
private void AddLargeImage(Bitmap? originalBitmap)
{
if (originalBitmap is null)
Expand Down Expand Up @@ -3874,12 +3881,19 @@ private void SetupToolbar(bool fullRebuild)
{
_normalButtonImages?.Dispose();
_normalButtonImages = new ImageList();
if (ScaleHelper.IsScalingRequired)
if (ScaleHelper.IsScalingRequirementMet)
{
_normalButtonImages.ImageSize = s_normalButtonSize;
}
}

// A ToolStripItem's size is computed once, at creation, and never revisited on its own; set
// ImageScalingSize (drives ToolStripItem.PreferredImageSize) before CreatePushButton runs below.
if (!LargeButtons)
{
_toolStrip.ImageScalingSize = _normalButtonImages.ImageSize;

@LeafShi1 LeafShi1 Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This early ImageScalingSize assignment looks redundant — the later unconditional set (_toolStrip.ImageScalingSize = LargeButtons ? ... : _normalButtonImages!.ImageSize;) already runs before the newly created buttons are added to the ToolStrip and laid out, and for the !LargeButtons case both assign the identical value (_normalButtonImages.ImageSize).

The stated rationale ("set before CreatePushButton runs, since a ToolStripItem's size is computed once at creation") doesn't seem to hold here: PropertyGridToolStripButton doesn't set the ToolStrip as its Owner at construction (it only stores the grid in _owningPropertyGrid), so during CreatePushButton the button's Owner is null. Since ToolStripItem.PreferredImageSize reads Owner.ImageScalingSize dynamically and returns Size.Empty when Owner is null, the effective size is bound at layout time — which happens after _toolStrip.Items.Add(...), i.e. after the later assignment has already set the correct value.

Could you double-check whether dropping this if (!LargeButtons) block (keeping only the later assignment) still fixes the issue with a "start 100% → move to 225%" repro? If so, this block and its comment can likely be removed. If there is a layout-cache subtlety that makes it necessary, it'd be great to capture that in the comment instead.

}

// Setup our event handlers.
EventHandler tabButtonHandler = OnViewTabButtonClick;
EventHandler sortButtonHandler = OnViewSortButtonClick;
Expand Down Expand Up @@ -4013,6 +4027,9 @@ private void SetupToolbar(bool fullRebuild)

_toolStrip.ImageList = LargeButtons ? _largeButtonImages : _normalButtonImages;

// Covers LargeButtons only (_largeButtonImages isn't ready earlier); a no-op otherwise.
_toolStrip.ImageScalingSize = LargeButtons ? _largeButtonImages!.ImageSize : _normalButtonImages!.ImageSize;

@LeafShi1 LeafShi1 Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (readability + DRY): since _toolStrip.ImageList was just assigned on the line above, and ToolStrip.ImageList's getter simply returns the backing field, you can derive the scaling size directly from it and drop the duplicated LargeButtons ternary:

_toolStrip.ImageScalingSize = _toolStrip.ImageList!.ImageSize;

This is runtime-equivalent to the current line, but keeps ImageScalingSize tied to the exact ImageList that was set, removing the risk that the two LargeButtons ? _largeButtonImages : _normalButtonImages expressions diverge in a future edit. The ! is still needed since ImageList is typed ImageList? (both branches are guaranteed non-null here, so it won't throw).


using (SuspendLayoutScope scope = new(_toolStrip))
{
_toolStrip.Items.Clear();
Expand Down
Loading