-
Notifications
You must be signed in to change notification settings - Fork 189
Adjusted computation of horizontal toolbar size. #2879
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
base: master
Are you sure you want to change the base?
Conversation
The first part ensures the toolbar becomes tall enough to fit controls like Combo or Text, whose height is often larger than the native toolbar button height returned by TB_GETITEMRECT. Without this check, the toolbar height is based only on Windows’ button metrics, which are smaller, causing embedded controls to be clipped. By taking the max of the toolbar height and each control’s actual height, the final toolbar size always accommodates the tallest embedded control. In the second part, the original code centered the control vertically, but when the control (like a combo box) was taller than the toolbar item height, the centering calculation produced a negative offset, shifting the control upward and causing clipping. Your fix uses Math.max(0, …) to prevent negative offsets, ensuring the control is never positioned above the item’s top. As a result, taller controls are aligned safely without being cut off.
4ab4e5c to
dec0c6e
Compare
|
If such a toolbar is used now in a view stack, do we get #936 (comment) again? (If you need a view with such a toolbar, try EGit's staging view or history view. When the window is wide enough so that there is enough horizontal space to the right of the view stack's tabs to accomodate the toolbar's width, the toolbar should be on the same line as the tabs.) If that layout issue re-appears: maybe the CTabFolder needs layout changes. |
I also went through the EGit views when trying to find a toolbar that we might easily use for testing, but none of those seems to contain a combo, do they? We only notices this issue with combos so far, but they seem to be rarely used in toolbars. Only usage I know about is the zoom field of GEF. |
That's right. They do contain text input fields, though. If the issue occurs only with combos, that won't help then. IIRC the text inputs are also cut off at the bottom on Linux (but this change here is Windows only). |
|
In general Toolbar with controls are often work quite poor and the best "workaround" is to add a large enough image (what the would make the toolbar higher). A similar issue (and maybe easier to reproduce?) occurs when one adds buttons with images of different size, e.g. add a tool item with a 16x16 image and one with a 32x32 image then both are shrink to 16x16px. By the way according to the image, the vertical (!) size is to small or do I misunderstand the issue? |
|
I have tested this change and could see this fixes the issue in GEF zoom field. I have a question about the changes in the
I dont understand how this can happen as in the current code the height of tool item is the max height of all the controls, so why is the second part required in toolitem to adjust toolitems y? |
@arunjose696 In the case of a combo box, its height can exceed the height of the toolbar. The existing logic vertically centers the control by computing an offset based on the height difference. When the combo box is taller than the toolbar, this calculation produces a negative Y offset, causing the control to be positioned partially outside the toolbar and resulting in the top being clipped. By using Math.max(0, itemRect.height - rect.height) / 2, we clamp the offset to a non-negative value, ensuring the control is never positioned at a negative Y coordinate and preventing the top portion from being cut off. |
The first part ensures the toolbar becomes tall enough to fit controls like Combo or Text, whose height is often larger than the native toolbar button height returned by TB_GETITEMRECT. Without this check, the toolbar height is based only on Windows’ button metrics, which are smaller, causing embedded controls to be clipped. By taking the max of the toolbar height and each control’s actual height, the final toolbar size always accommodates the tallest embedded control.
In the second part, the original code centered the control vertically, but when the control (like a combo box) was taller than the toolbar item height, the centering calculation produced a negative offset, shifting the control upward and causing clipping. Your fix uses Math.max(0, …) to prevent negative offsets, ensuring the control is never positioned above the item’s top. As a result, taller controls are aligned safely without being cut off.
To Reproduce
Expected behavior
Combobox is displayed correctly
Result
Before Fix

After Fix

Fixes #935