Skip to content

Conversation

mdonatas
Copy link
Contributor

Proposed changes

  • Take DPI into account when setting ComboBox width
  • Take DisplayMember into account
  • Take vertical scrollbar into account
  • Allow for narrower drop downs

Screenshots

Before

Combo box is not adjusted for DPI
Max width at 1x scale is 600px, screenshot at 2x scaling

image

After

Commit: Call ResizeDropDownWidth with Dpi adjusted values

Weird; why is it so wide?..
image


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 value
Branches.DisplayMember = nameof(IGitRef.LocalName);

After

Commit: Measure combo box item's displayed value width

image


WS_VSCROLL

Turns out, when setting combo box drop-down width comboBox.DropDownWidth = ..., one needs to take vertical scrollbar's width into account

After

Commit: Take vertical scrollbar width into account when setting drop down width

Nice :)
image


High value of AppSettings.BranchDropDownMinWidth = 300

Maybe this value was picked by someone not running at 1x scaling? It seems too high.

image

After

Commit: Adjust AppSettings.BranchDropDownMinWidth

image

Test methodology

  • Manually with 1x and 2x scaling

Test environment(s)

  • Windows 11 4K 200% scaling

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.

@mdonatas

This comment was marked as off-topic.

Copy link
Member

@mstv mstv left a 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

mdonatas and others added 5 commits September 1, 2024 14:22
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
Copy link
Member

@RussKie RussKie left a 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.

Comment on lines 10 to 16
public int cbSize;
public RECT rcItem;
public RECT rcButton;
public ComboBoxButtonState buttonState;
public IntPtr hwndCombo;
public IntPtr hwndEdit;
public IntPtr hwndList;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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
image

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm sure. The link that I shared contains all the relevant information:
image

Copy link
Contributor Author

@mdonatas mdonatas Sep 3, 2024

Choose a reason for hiding this comment

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

I believe this table should not be read in rows, there are two columns of related types and not a mapping between column A and column B. And HWND is listed in a column which has IntPtr in the header.
image

Copy link
Member

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();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
NativeMethods.COMBOBOXINFO cboInfo = NativeMethods.COMBOBOXINFO.Create();
NativeMethods.COMBOBOXINFO cboInfo = new();
cboInfo.cbSize = sizeof(COMBOBOXINFO);

Copy link
Member

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...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sizeof(COMBOBOXINFO); won't do
image

I've proceeded with calculating it once and then
cboInfo.cbSize = NativeMethods.COMBOBOXINFO.SizeOf;

Copy link
Contributor Author

@mdonatas mdonatas Sep 2, 2024

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.

Copy link
Member

Choose a reason for hiding this comment

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

sizeof(COMBOBOXINFO); won't do image

I've proceeded with calculating it once and then cboInfo.cbSize = NativeMethods.COMBOBOXINFO.SizeOf;

The screenshot contains the answer as well. The method must be marked as unsafe.

Copy link
Member

@RussKie RussKie Sep 3, 2024

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;
            }
        }

        // ...
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get it working with the fixed block, please help :)
See Interop adjustments based on PR review commit

image

@@ -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));
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Sep 2, 2024
@mdonatas
Copy link
Contributor Author

mdonatas commented Sep 2, 2024

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.

There are quite a few extension methods in GitExtUtils which only serve the needs of GitUI so looking at how utility methods are introduced, this seems to be the place. Also extension methods inside of GitExtUtils are more generic in nature while the ones in GitUI are more closely linked to specific custom user control or specific functionality.

Thus I've linked RECT.cs into GitExtUtils.

@mdonatas mdonatas requested review from mstv and RussKie September 2, 2024 17:23
mdonatas and others added 2 commits September 2, 2024 21:57
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);
Copy link
Member

Choose a reason for hiding this comment

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

@RussKie
Copy link
Member

RussKie commented Sep 3, 2024

There are quite a few extension methods in GitExtUtils which only serve the needs of GitUI so looking at how utility methods are introduced, this seems to be the place. Also extension methods inside of GitExtUtils are more generic in nature while the ones in GitUI are more closely linked to specific custom user control or specific functionality.

...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...

@mdonatas mdonatas changed the title Use static types in GetPreferredDropDownWidth Make DropDownWidth calculation Dpi aware and account for vertical scrollbar Sep 5, 2024
Copy link
Member

@mstv mstv left a 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.

@mdonatas
Copy link
Contributor Author

mdonatas commented Sep 8, 2024

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.

Copy link
Member

@mstv mstv left a 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>
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 49 to 61
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);
}
Copy link
Member

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?

Suggested change
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);
}

Copy link
Contributor Author

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.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

have not run

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Nice!

@RussKie RussKie removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Sep 16, 2024
@RussKie RussKie merged commit f089673 into gitextensions:master Sep 16, 2024
4 checks passed
@RussKie RussKie added this to the 5.0.1 milestone Sep 16, 2024
@mdonatas
Copy link
Contributor Author

Thank you for your patience and support along the way 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants