Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 430166

Summary: [Graphics] Improve view menu button to make it look good on a dark background
Product: [Eclipse Project] Platform Reporter: Lars Vogel <Lars.Vogel>
Component: UIAssignee: Lars Vogel <Lars.Vogel>
Status: CLOSED DUPLICATE QA Contact:
Severity: enhancement    
Priority: P3 CC: andrea.guarinoni, daniel.rolka, daniel_megert, Lars.Vogel, markus.kell.r, tmccrary, tmccrary
Version: 4.4   
Target Milestone: ---   
Hardware: PC   
OS: All   
Whiteboard:
Bug Depends on: 466511, 495033, 501491    
Bug Blocks:    
Attachments:
Description Flags
Screenshot
none
Screenshot
none
New icon on light background
none
Picture with correct colors and alignment
none
CTabFolder icons on Kepler/Win7
none
CTabFolder icons on Luna/Win7 none

Description Lars Vogel CLA 2014-03-12 06:51:23 EDT
Current icon looks bad, see screenshot.

Anyone knows where this icon is located?
Comment 1 Lars Vogel CLA 2014-03-12 06:51:44 EDT
Created attachment 240801 [details]
Screenshot
Comment 2 Lars Vogel CLA 2014-03-12 07:01:26 EDT
Got the info from Daniel Rolka that this icon is defined in CTabFolderRenderer.
Comment 3 Lars Vogel CLA 2014-03-12 07:07:29 EDT
Looks like this one is not an icon but drawn directly.

void drawChevron(GC gc, Rectangle chevronRect, int chevronImageState) {
		if (chevronRect.width == 0 || chevronRect.height == 0) return;
		int selectedIndex = parent.selectedIndex;
		// draw chevron (10x7)
		Display display = parent.getDisplay();
		Point dpi = display.getDPI();
		int fontHeight = 72 * 10 / dpi.y;
		FontData fd = parent.getFont().getFontData()[0];
		fd.setHeight(fontHeight);
		Font f = new Font(display, fd);
		int fHeight = f.getFontData()[0].getHeight() * dpi.y / 72;
		int indent = Math.max(2, (chevronRect.height - fHeight - 4) /2);
		int x = chevronRect.x + 2;
		int y = chevronRect.y + indent;
		int count;
		int itemCount = parent.getItemCount();
		if (parent.single) {
			count = selectedIndex == -1 ? itemCount : itemCount - 1;
		} else {
			int showCount = 0;
			while (showCount < parent.priority.length && parent.items[parent.priority[showCount]].showing) {
				showCount++;
			}
			count = itemCount - showCount;
		}
		String chevronString = count > 99 ? "99+" : String.valueOf(count); //$NON-NLS-1$
		switch (chevronImageState & (SWT.HOT | SWT.SELECTED)) {
			case SWT.NONE: {
				Color chevronBorder = parent.single ? parent.getSelectionForeground() : parent.getForeground();
				gc.setForeground(chevronBorder);
				gc.setFont(f);
				gc.drawLine(x,y,     x+2,y+2);
				gc.drawLine(x+2,y+2, x,y+4);
				gc.drawLine(x+1,y,   x+3,y+2);
				gc.drawLine(x+3,y+2, x+1,y+4);
				gc.drawLine(x+4,y,   x+6,y+2);
				gc.drawLine(x+6,y+2, x+5,y+4);
				gc.drawLine(x+5,y,   x+7,y+2);
				gc.drawLine(x+7,y+2, x+4,y+4);
				gc.drawString(chevronString, x+7, y+3, true);
				break;
			}
			case SWT.HOT: {
				gc.setForeground(display.getSystemColor(BUTTON_BORDER));
				gc.setBackground(display.getSystemColor(BUTTON_FILL));
				gc.setFont(f);
				gc.fillRoundRectangle(chevronRect.x, chevronRect.y, chevronRect.width, chevronRect.height, 6, 6);
				gc.drawRoundRectangle(chevronRect.x, chevronRect.y, chevronRect.width - 1, chevronRect.height - 1, 6, 6);
				gc.drawLine(x,y,     x+2,y+2);
				gc.drawLine(x+2,y+2, x,y+4);
				gc.drawLine(x+1,y,   x+3,y+2);
				gc.drawLine(x+3,y+2, x+1,y+4);
				gc.drawLine(x+4,y,   x+6,y+2);
				gc.drawLine(x+6,y+2, x+5,y+4);
				gc.drawLine(x+5,y,   x+7,y+2);
				gc.drawLine(x+7,y+2, x+4,y+4);
				gc.drawString(chevronString, x+7, y+3, true);
				break;
			}
			case SWT.SELECTED: {
				gc.setForeground(display.getSystemColor(BUTTON_BORDER));
				gc.setBackground(display.getSystemColor(BUTTON_FILL));
				gc.setFont(f);
				gc.fillRoundRectangle(chevronRect.x, chevronRect.y, chevronRect.width, chevronRect.height, 6, 6);
				gc.drawRoundRectangle(chevronRect.x, chevronRect.y, chevronRect.width - 1, chevronRect.height - 1, 6, 6);
				gc.drawLine(x+1,y+1, x+3,y+3);
				gc.drawLine(x+3,y+3, x+1,y+5);
				gc.drawLine(x+2,y+1, x+4,y+3);
				gc.drawLine(x+4,y+3, x+2,y+5);
				gc.drawLine(x+5,y+1, x+7,y+3);
				gc.drawLine(x+7,y+3, x+6,y+5);
				gc.drawLine(x+6,y+1, x+8,y+3);
				gc.drawLine(x+8,y+3, x+5,y+5);
				gc.drawString(chevronString, x+8, y+4, true);
				break;
			}
		}
		f.dispose();
	}
Comment 4 Paul Webster CLA 2014-03-12 09:11:59 EDT
We're responsible for the CTabFolder and others.

PW
Comment 5 Paul Webster CLA 2014-03-12 09:12:41 EDT
(In reply to Lars Vogel from comment #3)
> Looks like this one is not an icon but drawn directly.
> 
> void drawChevron(GC gc, Rectangle chevronRect, int chevronImageState) {

This is the chevron for when there are 2 many tabs to display, it's not the view dropdown menu button.

PW
Comment 6 Lars Vogel CLA 2014-03-12 10:22:10 EDT
(In reply to Paul Webster from comment #5)
> This is the chevron for when there are 2 many tabs to display, it's not the
> view dropdown menu button.
> 
> PW

Thanks. From the other bug it is StackRenderer.getViewMenuImage()
Comment 7 Lars Vogel CLA 2014-03-12 10:41:55 EDT
(In reply to Lars Vogel from comment #6)
> (In reply to Paul Webster from comment #5) 
> Thanks. From the other bug it is StackRenderer.getViewMenuImage()

With the help from Paul, I was able to replace the currently hand-drawn
Comment 8 Lars Vogel CLA 2014-03-12 10:43:56 EDT
I asked Tony, if he can create a fitting icon, meanwhile I did already the required code change. The original image was not disposed, I have not change that logic.

https://git.eclipse.org/r/23253
Comment 9 Lars Vogel CLA 2014-03-12 12:24:40 EDT
Created attachment 240820 [details]
Screenshot

Tony created an icon and I updated the Gerrit review. If there are no objections I commit this tomorrow.
Comment 10 Daniel Rolka CLA 2014-03-12 12:26:21 EDT
(In reply to Lars Vogel from comment #9)
> Created attachment 240820 [details]
> Screenshot
> 
> Tony created an icon and I updated the Gerrit review. If there are no
> objections I commit this tomorrow.

For me it looks great, thanks

Daniel
Comment 11 Lars Vogel CLA 2014-03-12 12:30:00 EDT
Created attachment 240822 [details]
New icon on light background

For comparison, this is the new icon on a light background
Comment 12 Lars Vogel CLA 2014-03-13 03:50:48 EDT
(In reply to Daniel Rolka from comment #10)
> (In reply to Lars Vogel from comment #9)
> > Created attachment 240820 [details]
> > Screenshot
> > 
> > Tony created an icon and I updated the Gerrit review. If there are no
> > objections I commit this tomorrow.
> 
> For me it looks great, thanks
> 
> Daniel

Thanks for the feedback Daniel. Fixed with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2cc39fc98fc6e7e4901c82cbeaab40ac60cde46b
Comment 13 Dani Megert CLA 2014-03-27 11:48:18 EDT
Lars, this doesn't look good on Windows 7 (too dark, not using the color scheme of min/max buttons) and the horizontal edge of the triangle is no longer horizontally aligned with the minimize and maximize button's top.
Comment 14 Lars Vogel CLA 2014-03-27 13:26:43 EDT
(In reply to Dani Megert from comment #13)
> Lars, this doesn't look good on Windows 7 (too dark, not using the color
> scheme of min/max buttons) and the horizontal edge of the triangle is no
> longer horizontally aligned with the minimize and maximize button's top.

Thanks. Tony can you update the icon?
Comment 15 Tony McCrary CLA 2014-03-27 14:17:02 EDT
Can we get a screenshot of the issue? I'm looking at the previous icon in Windows 7 and it looks wonky as well.
Comment 16 Dani Megert CLA 2014-03-28 07:31:03 EDT
Created attachment 241367 [details]
Picture with correct colors and alignment
Comment 17 Dani Megert CLA 2014-04-01 09:15:37 EDT
(In reply to Dani Megert from comment #16)
> Created attachment 241367 [details]
> Picture with correct colors and alignment

Any update on this one?
Comment 18 Lars Vogel CLA 2014-04-07 11:13:09 EDT
(In reply to Dani Megert from comment #17)
> (In reply to Dani Megert from comment #16)
> > Created attachment 241367 [details]
> > Picture with correct colors and alignment
> 
> Any update on this one?

I think the original color was SWT.COLOR_WIDGET_DARK_SHADOW. Where do I see the real color behind this constant?  I don't think it is 17.

public static final int COLOR_WIDGET_DARK_SHADOW = 17;
Comment 19 Dani Megert CLA 2014-04-07 11:22:53 EDT
(In reply to Lars Vogel from comment #18)
> (In reply to Dani Megert from comment #17)
> > (In reply to Dani Megert from comment #16)
> > > Created attachment 241367 [details]
> > > Picture with correct colors and alignment
> > 
> > Any update on this one?
> 
> I think the original color was SWT.COLOR_WIDGET_DARK_SHADOW. Where do I see
> the real color behind this constant?  I don't think it is 17.
> 
> public static final int COLOR_WIDGET_DARK_SHADOW = 17;

The image was computed to look right on all color schemes. To see how it was before, just look at the change ;-).
Comment 20 Lars Vogel CLA 2014-04-07 11:37:10 EDT
(In reply to Dani Megert from comment #19)
> The image was computed to look right on all color schemes. 

Except on a dark theme, in which is look extremely ugly. In the change I think I see that SWT.COLOR_WIDGET_DARK_SHADOW is used. Can you tell me where I find the mapping to the color for that constant?
Comment 21 Dani Megert CLA 2014-04-07 11:41:56 EDT
(In reply to Lars Vogel from comment #20)
> (In reply to Dani Megert from comment #19)
> > The image was computed to look right on all color schemes. 
> 
> Except on a dark theme, in which is look extremely ugly. In the change I
> think I see that SWT.COLOR_WIDGET_DARK_SHADOW is used. Can you tell me where
> I find the mapping to the color for that constant?

This is a system color read in by SWT [1]. That's the point of it. So, if in the dark scheme that color is the same as the background, then it's really a bug in the OS color scheme and not Eclipse.

[1] org.eclipse.swt.widgets.Display.getSystemColor(int)
Comment 22 Lars Vogel CLA 2014-04-07 12:05:58 EDT
(In reply to Dani Megert from comment #21)
> This is a system color read in by SWT [1]. That's the point of it. So, if in
> the dark scheme that color is the same as the background, then it's really a
> bug in the OS color scheme and not Eclipse.
> 
> [1] org.eclipse.swt.widgets.Display.getSystemColor(int)

Ah, sorry, I was talking about Eclipse CSS themes not system themes. I try to fix the image.
Comment 23 Andrea Guarinoni CLA 2014-04-07 12:56:21 EDT
On Windows 7 with Kepler (Build id: 20130614-0229), looks good to me. See screenshots.
With Luna Integration Build (Build id: I20140402-0100) there is a black border.

(The 'Close' icon in CTabFolder instead, looks ugly on any OS/platform/release)
Comment 24 Andrea Guarinoni CLA 2014-04-07 12:58:21 EDT
Created attachment 241687 [details]
CTabFolder icons on Kepler/Win7
Comment 25 Andrea Guarinoni CLA 2014-04-07 12:59:27 EDT
Created attachment 241688 [details]
CTabFolder icons on Luna/Win7
Comment 26 Dani Megert CLA 2014-04-08 08:30:41 EDT
(In reply to Dani Megert from comment #13)
> and the horizontal edge of the triangle is no
> longer horizontally aligned with the minimize and maximize button's top.

OK, that part already got broken a little bit in 4.0 and then even worse in later builds. But still something that needs to be fixed.
Comment 27 Dani Megert CLA 2014-04-15 11:14:14 EDT
Ping!
Comment 28 Dani Megert CLA 2014-04-24 06:08:29 EDT
I've reverted the change and also fixed the wrong horizontal alignment.
Comment 29 Lars Vogel CLA 2014-04-24 07:19:26 EDT
(In reply to Dani Megert from comment #28)
> I've reverted the change and also fixed the wrong horizontal alignment.

I though I still had today for this change? ;-) Thanks for the revert, I would potentially not be able to fix this tonight.
Comment 30 Markus Keller CLA 2015-04-21 09:19:56 EDT
The menu icon should not be replaced with a pre-computed image. It should look the same as the Minimize and Maximize buttons, and as long a those are drawn in org.eclipse.swt.custom.CTabFolderRenderer#drawMinimize(GC, Rectangle, int), the menu button should not be rendered differently.
Comment 31 Lars Vogel CLA 2015-04-21 09:33:21 EDT
(In reply to Markus Keller from comment #30)
> The menu icon should not be replaced with a pre-computed image. It should
> look the same as the Minimize and Maximize buttons, and as long a those are
> drawn in org.eclipse.swt.custom.CTabFolderRenderer#drawMinimize(GC,
> Rectangle, int), the menu button should not be rendered differently.

I agree that we should not replace it with a png but the drawing should be still improved. Compared to the minimize / maximize it looks fuzzy.
Comment 32 Lars Vogel CLA 2015-05-06 16:20:24 EDT
(In reply to Lars Vogel from comment #31)
> (In reply to Markus Keller from comment #30)
> > The menu icon should not be replaced with a pre-computed image. It should
> > look the same as the Minimize and Maximize buttons, and as long a those are
> > drawn in org.eclipse.swt.custom.CTabFolderRenderer#drawMinimize(GC,
> > Rectangle, int), the menu button should not be rendered differently.

As we are planning to support HDPI, I think replacing the view icon and the minimize and maximize custom drawing with images will help in having a consistent experience.
Comment 33 Lars Vogel CLA 2016-04-20 12:06:35 EDT
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
Comment 34 Lars Vogel CLA 2016-05-31 10:05:45 EDT
(In reply to Markus Keller from comment #30)
> The menu icon should not be replaced with a pre-computed image. It should
> look the same as the Minimize and Maximize buttons, and as long a those are
> drawn in org.eclipse.swt.custom.CTabFolderRenderer#drawMinimize(GC,
> Rectangle, int), the menu button should not be rendered differently.

Opened Bug 495033 for SWT.
Comment 35 Lars Vogel CLA 2016-09-13 09:42:49 EDT

*** This bug has been marked as a duplicate of bug 466511 ***