Drawing custom menu items for Menu Bar Dark Theme Only #1405

ShahzaibIbrahim · 2024-08-14T10:49:22Z

Using gc to draw menu item, moving the responsibility from OS due to wrong scaling of menu bar horizontally in Dark Theme. Snippet 373 can be used to test menuItems with images.

Additional points covered in this PR:

Changing the text color from a grayish tone to white.
When ALT key is pressed mnemonics are underlined and work as a toggle, the behavior which was missing from dark theme previously.

Screenshot of the fix:

HOW TO TEST

Run the RunTimeWorkspace with the following VM Arguments

-Dswt.autoScale=quarter
-Dswt.autoScale.updateOnRuntime=true

Go to "Windows" -> "Preferences" -> "General" -> "Appearance"
Change theme to "Dark"
Click "Apply and Close"
Move the window from 100 -> 175 (or anything 100+) zoom level
See if menu bar is properly aligned.

EXPECTED BEHAVIOUR

The menu bar should be scaled properly.

contributes to #62 and #127

github-actions · 2024-08-14T11:00:04Z

Test Results

284 files - 201 284 suites - 201 5m 22s ⏱️ - 3m 29s
4 158 tests ± 0 4 150 ✅ + 1 8 💤 ± 0 0 ❌ - 1
8 246 runs - 8 140 8 188 ✅ - 8 105 58 💤 - 34 0 ❌ - 1

Results for commit 3cf5e46. ± Comparison against base commit 02e20d6.

♻️ This comment has been updated with latest results.

HeikoKlare

I tested the changes with different scale factors and auto-scale settings (quarter/integer200). The scaling results look fine, in constrast to the existing scaling behavior that was broken.

I have some detailed comments and a general question: are there snippets to test the behavior also in combination with images? Our default applications only have menus with text but it would also be interested to test the changes with images, so we should either have or provide a snippet with images for validation.

Please also improve the commit message according to the recommendations, such that it explains the existing issue and more precisely describes the fix. E.g., it currently sounds as if the menu is always draw in a custom way by this change, but it only affects the behavior when using dark theme.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

HeikoKlare

Thank you for the recent improvements. There are several improvements in the new version, that's great. In particular, this change also improves the visualization of menus with images. This is how it looked before:

And this is how it looks now:

Only problem is that the left margin is always applied to the text, even if an image is present. But in that case, the margin should be applied to the image (and maybe some additional small margin needs to be placed beetween image and text). This is, e.g., how it looks for the light theme:

One additional issue I found concerns the handling of mnemonics, i.e., when pressing Alt+$ for accesing menu item with menmonic $.

The visualization is broken: the background color of the menu items does not fit when pressing the "Alt" button (it is black while the rest of the menu is grayish). See the screenshot below.
Mnemonics are always displayed (underlined) and not only when Alt is pressed. This is, actually, existing behvior, but differs from default behavior, such as in light theme, where the mnemonics are only underlined when Alt is pressed. Maybe we could address this as a follow up, as no regression is introduced. On the contrary, in the current behavior, pressing Alt hides the mnemonics, so badly inverts the intended behavior.
Most severe issue: there is no reaction to key bindings for mnemonics. E.g., pressing Alt+F should open the "File" menu, but does nothing.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java

HeikoKlare · 2024-09-02T15:44:54Z

Something seems to be wrong with the PR now. No matter whether I rebase on #1349, I see two issues:

The left margin of each menu item is quite large
There are unexpected menu items, like another "File" entry with only a single entry in the menu that opens up from it

Can you please check that, @ShahzaibIbrahim?

HeikoKlare · 2024-09-04T14:05:38Z

I retested and found that the faulty second "File" entry is because of a broken workbench.xmi on my side. Still, it shows some placement issue, as the text is cut off (hapenning for a "File" entry without a mnemonic, while the original "File" entry with mnemonic is not cut off). That may be investigated. You can easily reproduce by manually adding two menus (with at least one dummy menu item in each of them to have the menu shown) through the Model Spy:

HeikoKlare · 2024-09-05T15:13:31Z

Please check your latest commit. Only considering the diff, it does not seem to be in the intended/final state. There is a System.out.println() statement and some some added magic number in addition to the essential change:

ShahzaibIbrahim · 2024-09-06T08:57:29Z

Please check your latest commit. Only considering the diff, it does not seem to be in the intended/final state. There is a System.out.println() statement and some some added magic number in addition to the essential change:

Yes sorry, that was a leftover.

HeikoKlare · 2024-09-06T10:41:19Z

I've quickly retested again, but still see the large left margin for every menu I have mentioned before. Is that intended and is it reproducible for you?

In addition, I see that when activating mnemonics mode (pressing Alt) only the texts but not the complete menus are highlighted. That's different to existing behavior and to behavior of the light theme. Anyway: is the behavior intended or is that by accident?

When switched to Dark Theme, using gc to draw menu item, moving the responsibility from OS due to wrong scaling of menu bar vertically. Also changing the text color from a grayish tone to white. Also when ALT key is pressed mnemonics are underlined and work as a toggle, the behavior which was missing from dark theme previously.

HeikoKlare

The proposed change works really great now! It fixes the existing issues with scaling the menu bar in dark theme and it also improves the appearance of the menu bar in dark theme overall.

I only have one behavioral concern left, which is about the statically defined margins that (1) seem to be inappropriate and (2) are scaled incorrectly. The other comments are regarding code style / design and rather nitpicky.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

HeikoKlare · 2024-09-18T14:03:30Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

-			/*
-			 * Weirdness in Windows. Setting `HBMMENU_CALLBACK` causes
-			 * item sizes to mean something else. It seems that it is
-			 * the size of left margin before the text. At the same time,
-			 * if menu item has a mnemonic, it's always drawn at a fixed
-			 * position. I have tested on Win7, Win8.1, Win10 and found
-			 * that value of 5 works well in matching text to mnemonic.
-			 * NOTE: autoScaleUpUsingNativeDPI() is used to avoid problems
-			 * with applications that disable automatic scaling.
-			 */


Isn't this relevant anymore, so that the width can be set to a different value?

This information still seems relevant on explaining the difference between left margin and item size. Also we are back to using value of 5 again to match the margins between items in menu bar. I would suggest to keep this comment as is.

Okay, then please re-add it, because currently this PR will remove it. Only the information on autoScaleUpUsingNativeDPI should be revised, as that method is not used anymore.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

HeikoKlare · 2024-09-18T14:07:24Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

+	final static int LEFT_TEXT_MARGIN = 15;
+	final static int ADDED_ITEM_WIDTH = 15;


How are these values derived? The margins look rather different to the existing implementation and to the light theme. In the existing implementation, a value of "5" was hardcoded for the left margin at some point, maybe that would fit as well?

I am going back to the value 5 for item width as before (because of larger gap) and left margin value was derived as a result of testing against different values and finding out the closest to the light theme.

HeikoKlare

Code quality improvements are overall fine. The behavioral concerns in the last review have not been addressed yet. Please check them again.

In addition, testing Snippet 373 shows a misplacement of the image:

One minor finding that may be addressed independently as it is not part of this PR:
the image is not scaled correctly when using rescaleOnRuntime:

HeikoKlare · 2024-09-19T06:50:32Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

-			/*
-			 * Weirdness in Windows. Setting `HBMMENU_CALLBACK` causes
-			 * item sizes to mean something else. It seems that it is
-			 * the size of left margin before the text. At the same time,
-			 * if menu item has a mnemonic, it's always drawn at a fixed
-			 * position. I have tested on Win7, Win8.1, Win10 and found
-			 * that value of 5 works well in matching text to mnemonic.
-			 * NOTE: autoScaleUpUsingNativeDPI() is used to avoid problems
-			 * with applications that disable automatic scaling.
-			 */


Okay, then please re-add it, because currently this PR will remove it. Only the information on autoScaleUpUsingNativeDPI should be revised, as that method is not used anymore.

HeikoKlare · 2024-09-19T06:54:09Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java

+			gc.setBackground(isSelected? display.getSystemColor(SWT.COLOR_DARK_GRAY) : parent.getBackground());
+			gc.fillRectangle(menuItemArea);
+
+			int xPoisitonText = DPIUtil.scaleDown(x + LEFT_TEXT_MARGIN, zoom) + (this.image != null ? this.image.getBounds().width : 0);


Positioning is still wrong and inconsistent across different zoom level. Please correct the scaleDown calls as described before to solve this and adapt the LEFT_TEXT_MARGIN accordingly.

This is 100%:

This is 150%:

Please also fix the typo in the variable name xPoisitionText

ShahzaibIbrahim requested a review from niraj-modi as a code owner August 14, 2024 10:49

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch from 1829a6d to 2d1a006 Compare August 14, 2024 10:49

HeikoKlare reviewed Aug 19, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java Show resolved Hide resolved

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MenuItem.java Outdated Show resolved Hide resolved

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch from 2d1a006 to a9d2293 Compare August 20, 2024 13:22

ShahzaibIbrahim changed the title ~~Drawing custom menu items for Menu Bar~~ Drawing custom menu items for Menu Bar **Dark Theme Only** Aug 20, 2024

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch 2 times, most recently from e7017c6 to e10a814 Compare August 21, 2024 11:21

HeikoKlare requested changes Aug 23, 2024

View reviewed changes

akurtakov reviewed Aug 28, 2024

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Device.java Outdated Show resolved Hide resolved

ShahzaibIbrahim mentioned this pull request Sep 2, 2024

Adapt win32 theme calls to use dpi dependent variants vi-eclipse/Eclipse-Platform#102

Open

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch 3 times, most recently from 9949dbc to 3d1c1ce Compare September 2, 2024 11:29

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch 2 times, most recently from 674c379 to 3c16e7e Compare September 5, 2024 12:10

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch 2 times, most recently from 5ee53ab to e0d0e5b Compare September 6, 2024 08:56

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch 5 times, most recently from db0f7f1 to 14830a6 Compare September 13, 2024 15:15

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch from 14830a6 to e197195 Compare September 17, 2024 14:38

ShahzaibIbrahim force-pushed the use-dpi-dependent-win32-api-calls branch from e197195 to 8490a8f Compare September 17, 2024 14:42

HeikoKlare reviewed Sep 18, 2024

View reviewed changes

Review Comment Changes

7c00bbe

HeikoKlare reviewed Sep 19, 2024

View reviewed changes

ShahzaibIbrahim added 2 commits September 19, 2024 17:41

Usage of scale down and left margin comments fix

0afa016

Typo Fix

3cf5e46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drawing custom menu items for Menu Bar Dark Theme Only #1405

Drawing custom menu items for Menu Bar Dark Theme Only #1405

ShahzaibIbrahim commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

HeikoKlare left a comment

HeikoKlare left a comment

HeikoKlare commented Sep 2, 2024

HeikoKlare commented Sep 4, 2024

HeikoKlare commented Sep 5, 2024

ShahzaibIbrahim commented Sep 6, 2024

HeikoKlare commented Sep 6, 2024

HeikoKlare left a comment

HeikoKlare Sep 18, 2024

ShahzaibIbrahim Sep 18, 2024

HeikoKlare Sep 19, 2024

HeikoKlare Sep 18, 2024

ShahzaibIbrahim Sep 18, 2024

HeikoKlare left a comment

HeikoKlare Sep 19, 2024

HeikoKlare Sep 19, 2024

HeikoKlare Sep 19, 2024

		final static int LEFT_TEXT_MARGIN = 15;
		final static int ADDED_ITEM_WIDTH = 15;

Drawing custom menu items for Menu Bar **Dark Theme Only** #1405

Are you sure you want to change the base?

Drawing custom menu items for Menu Bar **Dark Theme Only** #1405

Conversation

ShahzaibIbrahim commented Aug 14, 2024 • edited Loading

HOW TO TEST

EXPECTED BEHAVIOUR

github-actions bot commented Aug 14, 2024 • edited Loading

Test Results

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare commented Sep 2, 2024

HeikoKlare commented Sep 4, 2024

HeikoKlare commented Sep 5, 2024

ShahzaibIbrahim commented Sep 6, 2024

HeikoKlare commented Sep 6, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drawing custom menu items for Menu Bar Dark Theme Only #1405

Drawing custom menu items for Menu Bar Dark Theme Only #1405

ShahzaibIbrahim commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading