-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Make DropDownWidth calculation Dpi aware and account for vertical scrollbar #11864
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.
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
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but a few nits
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do share the same question as @mstv asked in #11864 (comment). ComboBoxExtensions
appears to be only used in GitUI project, so we should move these extensions in the GitUI project.
Alternatively, we could consider linking the source instead of duplicating it. Though, my preference is to move the type.
public int cbSize; | ||
public RECT rcItem; | ||
public RECT rcButton; | ||
public ComboBoxButtonState buttonState; | ||
public IntPtr hwndCombo; | ||
public IntPtr hwndEdit; | ||
public IntPtr hwndList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public int cbSize; | |
public RECT rcItem; | |
public RECT rcButton; | |
public ComboBoxButtonState buttonState; | |
public IntPtr hwndCombo; | |
public IntPtr hwndEdit; | |
public IntPtr hwndList; | |
public uint cbSize; | |
public RECT rcItem; | |
public RECT rcButton; | |
public ComboBoxButtonState buttonState; | |
public nuint hwndCombo; | |
public nuint hwndEdit; | |
public nuint hwndList; |
DWORD
maps to uint
, see https://learn.microsoft.com/dotnet/standard/native-interop/best-practices.
HWND
maps to UIntPtr
, and nuint
is a new better alias for UIntPrt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really not my field.. Are you sure about HWND
mapping to UIntPtr
and not IntPtr
? https://learn.microsoft.com/en-us/windows/apps/develop/ui-input/retrieve-hwnd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. I'm having a brainfart moment....
HWND == IntPtr == nint.
|
||
private static int CalculateVerticalScrollBarWidth(ComboBox comboBox) | ||
{ | ||
NativeMethods.COMBOBOXINFO cboInfo = NativeMethods.COMBOBOXINFO.Create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NativeMethods.COMBOBOXINFO cboInfo = NativeMethods.COMBOBOXINFO.Create(); | |
NativeMethods.COMBOBOXINFO cboInfo = new(); | |
cboInfo.cbSize = sizeof(COMBOBOXINFO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetComboBoxInfo
should probably accept a pointer instead of a ref
too....
NativeMethods.COMBOBOXINFO* tConfig = new();
tConfig->cbSize = sizeof(NativeMethods.COMBOBOXINFO);
There are examples in dotnet/winforms codebase in release/6.0 or release/7.0 (before they moved to CsWin32 definitions - something we should probably do too...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but "should probably accept a pointer" part is way over my head, I have not seen it done that way. Also there are several interop methods accepting ref
params. I feel being pushed to do something new and which was not done in GE before instead of continuing in the steps of patterns that are already present.
Usually "stylistic" changes, or changes in approach are done in a sweeping "lets change all the places at once" manner. Sorry if I am misunderstanding something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but "should probably accept a pointer" part is way over my head, I have not seen it done that way. Also there are several interop methods accepting
ref
params. I feel being pushed to do something new and which was not done in GE before instead of continuing in the steps of patterns that are already present. Usually "stylistic" changes, or changes in approach are done in a sweeping "lets change all the places at once" manner. Sorry if I am misunderstanding something.
Using blittable types (i.e., those that map directly from the managed to the unmanaged memory) and the unsafe code gains perf. Some of our interop definitions are old and, thus, carry perf overhead.
[StructLayout(LayoutKind.Sequential)]
internal struct COMBOBOXINFO
{
public uint cbSize;
public RECT rcItem;
public RECT rcButton;
public ComboBoxButtonState buttonState;
public nint hwndCombo;
public nint hwndEdit;
public nint hwndList;
}
internal enum ComboBoxButtonState : uint
{
STATE_SYSTEM_NONE = 0,
STATE_SYSTEM_INVISIBLE = 0x00008000,
STATE_SYSTEM_PRESSED = 0x00000008
}
[DllImport("user32.dll")]
internal static extern BOOL GetComboBoxInfo(nint hWnd, COMBOBOXINFO* pcbi);
private static unsafe int CalculateVerticalScrollBarWidth(ComboBox comboBox)
{
NativeMethods.COMBOBOXINFO cboInfo = new()
{
cbSize = (uint)sizeof(NativeMethods.COMBOBOXINFO)
};
fixed (NativeMethods.COMBOBOXINFO* pcboInfo = cboInfo)
{
if (!NativeMethods.GetComboBoxInfo(comboBox.Handle, &pcboInfo))
{
return 0;
}
}
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -21,7 +21,7 @@ public FormOpenDirectory(IGitModule? currentModule) | |||
|
|||
IList<Repository> repositoryHistory = ThreadHelper.JoinableTaskFactory.Run(RepositoryHistoryManager.Locals.LoadRecentHistoryAsync); | |||
_NO_TRANSLATE_Directory.DataSource = GetDirectories(currentModule, repositoryHistory); | |||
_NO_TRANSLATE_Directory.ResizeDropDownWidth(AppSettings.BranchDropDownMinWidth, AppSettings.BranchDropDownMaxWidth); | |||
_NO_TRANSLATE_Directory.ResizeDropDownWidth(DpiUtil.Scale(AppSettings.BranchDropDownMinWidth), DpiUtil.Scale(AppSettings.BranchDropDownMaxWidth)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are updating all callsites. Any reason not to update the implementation to scale the passed arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it would be more convenient and produce less diffs I think that semantically this is correct - this is a straightforward utility method and the caller has control and responsibility to understand what is being done. Unless we'd add ResizeDropDownWidthDpiAware
..
Anyhow, I see your point. I personally would lean towards semantic correctness (which I guess could also be debated :) ) and leave it the way it is at the same time this is not the hill I'm ready to die on so if you'll tell me to change this, I will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a scenario where we'd want to resize a dropdown without scaling for the current dpi? If not, then I can't see any reason why we shouldn't be doing scaling implicitly - i.e., not rely on a caller to remember to scale before resizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One scenario came up, it's when you want to adjust the width based on some other control width which is already scaled. You'd need to de-scale it which is not pretty. I solved it by introducing an optional parameter bool dpiScaleBounds = true
and this one place which didn't require scaling sets it to false
There are quite a few extension methods in Thus I've linked |
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
internal static partial class NativeMethods | ||
{ | ||
[DllImport("user32.dll")] | ||
internal static extern int GetWindowLong(nint hwnd, int nIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is a correct import. On 64-bit systems, it should be GetWindowLongPtrW
:
https://github.com/dotnet/winforms/blob/698cfb403515150b08ca8cd1f201b5d0aacde813/src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.GetWindowLong.cs#L11-L26
...and this would be a good enough reason to move those "UI-specific" into GitUI, no? :) Not forcing the change, just can't help but think the argument is weak... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least this of Igor's comments has not been addressed yet.
@mstv The link doesn't lead to a comment for me, which comment would that be? Also I've just pushed a new commit so maybe it's been addressed already. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I've just pushed a new commit so maybe it's been addressed already.
It has.
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/app/GitUI/CommandsDialogs/BrowseDialog/FormOpenDirectory.cs
Outdated
Show resolved
Hide resolved
public static void ResizeDropDownWidth(this ToolStripComboBox comboBox, int minWidth, int maxWidth, bool dpiScaleBounds = true) | ||
{ | ||
if (comboBox is null) | ||
ArgumentNullException.ThrowIfNull(comboBox); | ||
|
||
if (dpiScaleBounds) | ||
{ | ||
throw new ArgumentNullException(nameof(comboBox)); | ||
minWidth = DpiUtil.Scale(minWidth); | ||
maxWidth = DpiUtil.Scale(maxWidth); | ||
} | ||
|
||
int calculatedWidth = GetPreferredDropDownWidth(comboBox.Control); | ||
int calculatedWidth = GetPreferredDropDownWidth((ComboBox)comboBox.Control); | ||
comboBox.DropDownWidth = Math.Min(Math.Max(calculatedWidth, minWidth), maxWidth); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to reuse the other API to avoid code duplication?
public static void ResizeDropDownWidth(this ToolStripComboBox comboBox, int minWidth, int maxWidth, bool dpiScaleBounds = true) | |
{ | |
if (comboBox is null) | |
ArgumentNullException.ThrowIfNull(comboBox); | |
if (dpiScaleBounds) | |
{ | |
throw new ArgumentNullException(nameof(comboBox)); | |
minWidth = DpiUtil.Scale(minWidth); | |
maxWidth = DpiUtil.Scale(maxWidth); | |
} | |
int calculatedWidth = GetPreferredDropDownWidth(comboBox.Control); | |
int calculatedWidth = GetPreferredDropDownWidth((ComboBox)comboBox.Control); | |
comboBox.DropDownWidth = Math.Min(Math.Max(calculatedWidth, minWidth), maxWidth); | |
} | |
public static void ResizeDropDownWidth(this ToolStripComboBox comboBox, int minWidth, int maxWidth, bool dpiScaleBounds = true) | |
{ | |
ArgumentNullException.ThrowIfNull(comboBox); | |
ResizeDropDownWidth((ComboBox)comboBox.Control, minWidth, maxWidth, dpiScaleBounds); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works because toolStripComboBox.DropDownWidth = 123
actually redirects the setting of DropDownWidth
to underlying wrapped (ComboBox)comboBox.Control
.
Updated.
60f73fa
to
08a2eee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have not run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Thank you for your patience and support along the way 😊 |
Proposed changes
DisplayMember
into accountScreenshots
Before
Combo box is not adjusted for DPI
Max width at 1x scale is 600px, screenshot at 2x scaling
After
Commit:
Call ResizeDropDownWidth with Dpi adjusted valuesWeird; why is it so wide?..

DisplayMember
Turns out some combo boxes have their
DisplayMember
set to something other than a value of.ToString()
and the width is measured based on a wrong valueBranches.DisplayMember = nameof(IGitRef.LocalName);
After
Commit:
Measure combo box item's displayed value widthWS_VSCROLL
Turns out, when setting combo box drop-down width
comboBox.DropDownWidth = ...
, one needs to take vertical scrollbar's width into accountAfter
Commit:
Take vertical scrollbar width into account when setting drop down widthNice :)

High value of
AppSettings.BranchDropDownMinWidth = 300
Maybe this value was picked by someone not running at 1x scaling? It seems too high.
After
Commit:
Adjust AppSettings.BranchDropDownMinWidthTest methodology
Test environment(s)
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.