Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #466511 +++ The custom drawing should be replaced with png icons. This would result in a more consistent user interface (IMHO the close button looks really bad on Mac). This should also help with the work of supporting high resolution displays, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=382972#c51 Modification is needed in CTabFolderRenderer#drawClose
*** Bug 538710 has been marked as a duplicate of this bug. ***
(In reply to Lars Vogel from comment #0) > IMHO the close button looks really bad on Mac I work daily with Eclipse on Mac (many hours a day, too). It has _never_ occurred to me that the close button looked bad. It's perfectly fine as it is for me.
Created attachment 276409 [details] Close icon on Windows in the dark theme Attached a screenshot of the close icon on Windows in the dark theme
Created attachment 277269 [details] Screenshot - Aliased vs Anti-Aliased close button
I just encountered this issue on Windows while working on a Dark Theme for our product. It turns out that the issue is just that the close shape needs to be drawn anti-aliased. Unfortunately for me, extending the behavior of CTabFolderRenderer is not a straightforward task due to the private nature of many required members and functions. Fortunately, fixing it on the SWT CTabFolderRenderer should be a rather simple change. In my custom renderer, I just called gc.setAntialiased(SWT.ON) at the beginning of drawClose and set it back to it's original value at the end of drawClose. I see that there is an antialias() function call, perhaps it would have been more appropriate to use that method.
Please provide a Gerrit patch to fix this in standard.
(In reply to Mike Marchand from comment #5) > I just encountered this issue on Windows while working on a Dark Theme for > our product. It turns out that the issue is just that the close shape needs > to be drawn anti-aliased. Mike, if you need help with Gerrit, please feel free to ping me via Lars.Vogel@vogella.com. I also documented it here: http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#exericse-eclipse-user-creation-and-gerrit-server-configuration In our hackathons the setup usually is done in 5-10 mins + the time required to clone the repo.
Created attachment 277389 [details] patch that draws antialised close button This patch will draw the close button anti-aliased.
(In reply to Mike Marchand from comment #8) > Created attachment 277389 [details] > patch that draws antialised close button > > This patch will draw the close button anti-aliased. Please provide a Gerrit ;-)
New Gerrit change created: https://git.eclipse.org/r/136043
Gerrit change https://git.eclipse.org/r/136043 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b96722424ff908aba3b0ce5a2b173f5080b3c281
(In reply to Eclipse Genie from comment #10) > New Gerrit change created: https://git.eclipse.org/r/136043 This caused a *very* bad regression on Windows, if themes are disabled (and so less repaints are triggered). Open two editors. Hover the mouse from right to left over the editor tabs. As soon as the mouse goes over the close button, original tab content disappears and is never drawn until the mouse hovers over the empty area. I'm going to revert this commit.
New Gerrit change created: https://git.eclipse.org/r/136170
Created attachment 277407 [details] disappearing tabs I've recorded video, for some reason mouse position is off by few pixels, it should be hovering over tabs.
Thanks Andrey. I can reproduce the problem. I'm not certain what the appropriate fix is... investigating.
The root cause of the drawClose looking so bad is simply because gc.fillPolygon and gc.drawPolygon do not draw to the same pixels with the same shape (When antialiasing is not enabled) There appears to be an off by negative one bug in the X coordinates. If I adjust the shape's x coordinates by +1 for the fillPoly, it will align correctly with the drawPoly. Lars, could you please advise? I'm not sure what the appropriate plan of attack here is. Should we file a Bug against the GC? To my knowledge, this is a problem on all platforms (Except Mac because to my knowledge Mac performs anti-aliasing by default)
(In reply to Mike Marchand from comment #16) > The root cause of the drawClose looking so bad is simply because > > gc.fillPolygon > and > gc.drawPolygon > > do not draw to the same pixels with the same shape (When antialiasing is not > enabled) > > There appears to be an off by negative one bug in the X coordinates. > > If I adjust the shape's x coordinates by +1 for the fillPoly, it will align > correctly with the drawPoly. > > Lars, could you please advise? I'm not sure what the appropriate plan of > attack here is. Should we file a Bug against the GC? > > To my knowledge, this is a problem on all platforms (Except Mac because to > my knowledge Mac performs anti-aliasing by default) I'm not sure I can follow without the code. So you mean, with same arguments given, the shapes of gc.drawPoligon and gc.fillPolygon differ? But is this not what expected, the "fill" should fill *inside* and "draw" the "border" of the polygon? What does the javadoc say?
(In reply to Andrey Loskutov from comment #17) > So you mean, with same arguments > given, the shapes of gc.drawPoligon and gc.fillPolygon differ? But is this > not what expected, the "fill" should fill *inside* and "draw" the "border" > of the polygon? What does the javadoc say? Javadoc says what I've expected: the "fill" paints *inside* the area. I plan to submit reverted patch today before next SDK build, because I can't work with disappearing tabs anymore. If you have a patch to share, feel free to do so. I've quickly played with the patch, one thing I wonder is: why is the *unrelated* tab disappears (we hover over the close button of *another* tab)? Looks like while painting of the close button we paint way more we should. This could be the hint to start fixing the problem.
+1 for revert, we can work on a correct fix after the revert.
Gerrit change https://git.eclipse.org/r/136170 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=04e7674bd4a82dc50ebd04d86747241b75845fbf
Created attachment 277416 [details] GC Polygon Drawing test This Bug is actually caused by Bug #139791.
(In reply to Mike Marchand from comment #21) > Created attachment 277416 [details] > GC Polygon Drawing test > > This Bug is actually caused by Bug #139791. Cool. Bug from 2006, *at least*.
Although the Antialiased solution seemed to make the 'x' look better, if you zoom in, you can still see that the fill is not drawn properly within the outline.
This is the Win32 GC fillPolygon fix. New Gerrit change created: https://git.eclipse.org/r/136209
It is worth mentioning that we do not notice this issue without a theme or on Light theme because the fill color is the same color as the background color. On Dark theme, the fill color is no longer the same color as the default color, which allows us to see that the fill is outside the outline.
Bug #139791 has been resolved and merged to Master. We should also be able to mark this Bug as resolved.
(In reply to Mike Marchand from comment #26) > Bug #139791 has been resolved and merged to Master. We should also be able > to mark this Bug as resolved. But your patch is reverted - don't we need it again?
(In reply to Andrey Loskutov from comment #27) > (In reply to Mike Marchand from comment #26) > > Bug #139791 has been resolved and merged to Master. We should also be able > > to mark this Bug as resolved. > > But your patch is reverted - don't we need it again? Oh, the bug we fixed does not resolve the disappearing tab problem. It resolves an issue with how fillPolygon works (That was the 13 year old bug). The close button always looked good without a theme because the close button's fill color and the background color were the same, which means we couldn't tell that there was a drawing glitch happening. On a dark theme, this became apparent because the background and fill are different colors.
But since we are no longer using my patch (with anti-aliasing), there wont be any problems (which wouldn't already exist beforehand) with disappearing tabs. Since you've mentioned it, I've seen a tab disappear once while using dark theme on a 4.10 installation. Not sure about the cause or how reproducible the problem is though.
FYI, the tab disappearing has already been reported in Bug #428697
*** Bug 560134 has been marked as a duplicate of this bug. ***
Created attachment 283014 [details] Close light Linux
Created attachment 283016 [details] New close button on dark Linux
New Gerrit change created: https://git.eclipse.org/r/163593
(In reply to Eclipse Genie from comment #34) > New Gerrit change created: https://git.eclipse.org/r/163593 With this change, we would use a tab close icon similar to VsCode, Browsers (Chrome close icon) and Android application. The basis for this icon is taken from the free material icons from Google for Android. WDYT?
(In reply to Lars Vogel from comment #35) > (In reply to Eclipse Genie from comment #34) > > New Gerrit change created: https://git.eclipse.org/r/163593 > > With this change, we would use a tab close icon similar to VsCode, Browsers > (Chrome > close icon) and Android > application. The basis for this icon is taken from the free material > icons from Google for Android. > > WDYT? I'm +1 for this change (once it's ready). I prefer the look of the new suggested close buttons compared to the old ones. Looks cleaner IMO. Also, would it be feasible to implement this in pure-gc? Meaning there's no icon file, but instead the icon is drawn? It would have the advantage of being themeable. However, maybe SVG's can be recoloured programmatically within Platform? If this is the case, then using a pure-gc approach seems redundant and a waste of ressources.
(In reply to Andrew Obuchowicz from comment #36) > Also, would it be feasible to implement this in pure-gc? Meaning there's no > icon file, but instead the icon is drawn? It would have the advantage of > being themeable. We use images almost everywhere, so I would prefer to remove this almost single usage of gc for creating icons.
(In reply to Andrew Obuchowicz from comment #36) > Also, would it be feasible to implement this in pure-gc? Meaning there's no > icon file, but instead the icon is drawn? It would have the advantage of > being themeable. We should rather add the feature to replace icons via themes.
(In reply to Matthias Becker from comment #38) > (In reply to Andrew Obuchowicz from comment #36) > > Also, would it be feasible to implement this in pure-gc? Meaning there's no > > icon file, but instead the icon is drawn? It would have the advantage of > > being themeable. > We should rather add the feature to replace icons via themes. +1
New Gerrit change created: https://git.eclipse.org/r/164647
Created attachment 284018 [details] Screenshot Also close item is dark on Mac and light on Window
Mass move out to 4.18
Created attachment 284402 [details] svg file
Created attachment 284403 [details] This is how it looks with patchset 16 on macOS with high resolution display
IMO, We should add a new API in CTabFolder something like below: CTabFolder.setCloseImage(Image image); Advantages of this approach: - Current implementation of drawing remains as default. - SWT only acts as an enabler and doesn't provide any images as part of the toolkit. - HiDPI close image management should remain in the client code.
Created attachment 284604 [details] Close with gc.drawString on dark Linux
Created attachment 284605 [details] Close with gc.drawString on light Linux
(In reply to Matthias Becker from comment #44) > Created attachment 284403 [details] > This is how it looks with patchset 16 on macOS with high resolution display Can you test the latest patchset on Mac?
Created attachment 284608 [details] Screenshot on macOS in classic, light, dark and system theme For me the "x" looks not quite right. Maybe a bit too small. What font is that? Maybe we could check how it looks when we make it "bold".
(In reply to Matthias Becker from comment #49) > Created attachment 284608 [details] > Screenshot on macOS in classic, light, dark and system theme > > For me the "x" looks not quite right. Maybe a bit too small. Looks fantastic to me. > What font is that? Maybe we could check how it looks when we make it "bold". It currently used the font of the receiver, but I think we should definitely fix it. I updated the Gerrit to use the chevronFont. Still looks fantastic to me. Can you maybe play with the patch direct? I think you sense for aesthetic is much higher than mine. :-) To position the x you have to modify the x and y calculation. int x = closeRect.x + Math.max(1, (closeRect.width-12)); int y = closeRect.y-1;
Matthias, unless you find a better font / setting I would suggest to merge the change after M2 so that we can fine tune it in graphics in M3.
(In reply to Lars Vogel from comment #51) > Matthias, unless you find a better font / setting I would suggest to merge > the change after M2 so that we can fine tune it in graphics in M3. I am on vacation til next week. So I won't be able to work on that.
Created attachment 284722 [details] Result with patchset 36 on macOS
(In reply to Matthias Becker from comment #53) > Created attachment 284722 [details] > Result with patchset 36 on macOS That's definitely ugly. Perhaps related to bug 568777?
I don't really get why use some arbitrary Unicode glyph for this. Yes it is convenient for scaling but seem to only bring more problems. There are already quite a lot problems with the limited amount of people involved in this bug. With the more diverse setups from all SWT users there will be more issues for sure. Apparently there is not even a guarantee on the close "button" color. In Matthias screenshot (and if I see this Unicode glyph in my browser) it can be red instead of the actual font color. What if I configured my tab to have a red background too? It also looks quite different on any platform where it is imo to large on Windows and in some cases it looks like someone made a tabTitle + " X". As it is part of the title rather than a distinct control element. (and it lost the outline and select feature even if the last one isn't very useful before) I would support Niraj's suggestion in comment 45. Btw. I have no HiDPI monitor and therefore no HiDPI experience in SWT but if you just want an image for scaling stuff it should be easy to let the existing code draw into an image. The code does not care if the GC is from the paint event or an image.
Hi Lars, Checking if still planned for M3 ?
(In reply to Niraj Modi from comment #56) > Hi Lars, > Checking if still planned for M3 ? Thanks for checking. Lets revisit this for 4.19.
Ping! Is this still in plan for 4.19 ?
Too late for 4.19. Removing target milestone as it's unclear to me whether anyone is working on it.
As using unicode or png images did not work on all platform, I suggest we adjust drawing with gc to provide an improved icon.
Created attachment 286919 [details] Gc solution on Linux - Dark theme
Created attachment 286920 [details] Gc solution on Linux - Light theme
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/183887
Created attachment 286924 [details] Screenshot with https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/183887 on macOS The new close icon looks really nice on macOS. Looks good on all themes. But I found one issue. The notification dialog still uses the old one.
(In reply to Matthias Becker from comment #64) > Created attachment 286924 [details] > Screenshot with > https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/183887 on macOS > > The new close icon looks really nice on macOS. > Looks good on all themes. > But I found one issue. The notification dialog still uses the old one. Please open a new bug for that. The notification dialog uses a png file for its close button see /org.eclipse.jface.notifications/icons/eview16/notification-close.png. We can either update the png file or try to configure the shell to use the regular close functionality.
(In reply to Lars Vogel from comment #62) > Created attachment 286920 [details] > Gc solution on Linux - Light theme The contrast seem to be worse with new code. Can we please have side to side screenshots old/new? Originally the bug was about "custom drawing should be replaced with png icons" (see comment 0). That was considered more consistent (consistency with what?). Now we change the way how they are drawn, quote from gerrit: "to look similar to common and popular tools like the Chrome browser or development tools like VScode." Can we please have a clear goal defined for this bug, with clear success metric, so anyone can understand if the proposed patch makes sense and matches that metric?
(In reply to Andrey Loskutov from comment #66) > The contrast seem to be worse with new code. Can we please have side to side > screenshots old/new? Yes the new one has finer lines compared to the current one. But the current one is really very heavy. I am unsure if the lines are too fine.
(In reply to Andrey Loskutov from comment #66) > (In reply to Lars Vogel from comment #62) > > Created attachment 286920 [details] > > Gc solution on Linux - Light theme > > The contrast seem to be worse with new code. Can we please have side to side > screenshots old/new? Sorry, screenshot of Linux is a bit old before the Gerrit was improved I push an update soon (with side by side). > Can we please have a clear goal defined for this bug, with clear success > metric, so anyone can understand if the proposed patch makes sense and > matches that metric? The target always was to make the close icon look better on Eclipse and more consistent with Eclipse itself (the close button on the shell) and other tools. Originally I though we can archive this with png icons, then it looks like unicode was the way to go but both alternatives did not work. As it turns out adjusting the gc results in a nicer and consistent icon. The new icon is similar to the close icon on the Windows of Eclipse.
Created attachment 286925 [details] Linux Light side by side
(In reply to Lars Vogel from comment #68) > > Can we please have a clear goal defined for this bug, with clear success > > metric, so anyone can understand if the proposed patch makes sense and > > matches that metric? > > The target always was to make the close icon look better on Eclipse don't get me wrong, but please define "better". > and more > consistent with Eclipse itself (the close button on the shell) and other > tools. OK, so please provide 3 icons side by side: original, new proposed, and the "close on the shell" that we are trying to match. Also if I understand it right, the "red" color change is coming on top of that as a bonus? I honestly hope we could have clear requirements and clear success criteria, especially if that is about "standards" of user interface in Eclipse.
(In reply to Andrey Loskutov from comment #70) > don't get me wrong, but please define "better". Please have a look at the screenshot. IMHO the old icon looks horrible and is not what the shell uses. > > and more > > consistent with Eclipse itself (the close button on the shell) and other > > tools. > > OK, so please provide 3 icons side by side: original, new proposed, and the > "close on the shell" that we are trying to match. See see new attachment. Shell close icon is top-right side and only included once at it does not change. > Also if I understand it right, the "red" color change is coming on top of that as a bonus? The old color was not strong enough for the new icon according to the feedback in the Gerrit.
(In reply to Lars Vogel from comment #71) > (In reply to Andrey Loskutov from comment #70) > > > don't get me wrong, but please define "better". > > Please have a look at the screenshot. IMHO the old icon looks horrible and > is not what the shell uses. Again, "horrible" is not defined. I find the old one just right. The new icons are harder to read, and please note, the original system icon has really high contrast by using orange background color. The old one did this by using "borders". The new one is too "fine" drawn for me, so if you define "horrible" as "good contrast", and "better" as "reduced contrast", I would vote against the change. Please define clear requirements and clear success criteria *without* using terms "better" or "horrible".
(In reply to Andrey Loskutov from comment #72) > (In reply to Lars Vogel from comment #71) > > Please define clear requirements and clear success criteria *without* using > terms "better" or "horrible". Similar looking to the standard close icon of the shell of Eclipse and other tools.
(In reply to Lars Vogel from comment #73) > (In reply to Andrey Loskutov from comment #72) > > (In reply to Lars Vogel from comment #71) > > > > Please define clear requirements and clear success criteria *without* using > > terms "better" or "horrible". > > Similar looking to the standard close icon of the shell of Eclipse and other > tools. Well, that is not attainable. On Mac, the close buttons on a shell are the red, yellow, and green dots. A better metric is: this is SWT. The look and feel of SWT widgets, also custom widgets provided by SWT, should be as native as possible. It can't be truly native since this is a custom widget, so some leeway must be given. I'd declare success if Eclipse's close button in non-hot mode looks close enough to whatever is used normally on the corresponding platform to not stick out, contrast is OK in both hot and non-hot modes, and hot mode neither changes the button's glyph shape no does other crazy things. (So if on hover it turns into an emoji or starts playing a song or some such, I'll -2 it :-) The current close button fails by this measure, on all three platforms. Native look on Mac for closeable tabs appears indeed to be this plain cross, as can be seen in the Finder's tabs. As hover effect, the background is made (in light mode) a darker gray. (Darker gray rectangle with rounded corners). Native look on Windows is also a plain cross, with the same hover effect. See for instance tabs in Microsoft Edge. Other web browsers seem to have converged on this same UI across platforms. (I only checked Firefox and Safari.) On Windows 10, the shell close button is also such a thin plain cross, as hover effect, the button gets a glaringly red background. The tab close button's cross is smaller, but appears to use also in non-hot mode a slightly thicker lines. Native look on GTK/Adwaita on CentOS 7 for tabs appears to be also a plain cross, but with thicker lines, as can be seen for instance in gedit with multiple files open. As hover effect, the background becomes a brighter shade of gray. The shell close icon uses the same cross. Firefox uses the same cross and hover effect (darker gray) as on other platforms. So I'd say this change is a step in the right direction. For me, the current implementation is good enough. If there is consensus that contrast is not sufficient, one could try with slightly thicker lines (+1?) in non-hot mode, or if it's the contrast of the red vs. light or dark background, one could certainly find a color shade of red that works well in both modes. I think a hover effect that makes the cross red is better than one that changes the background to dark gray. I find that *that* gives a really poor contrast. So I'd accept that deviation from apparently widely accepted standard.
Thanks Thomas for the detailed analysis on all supported platforms. It made it clear that we need to move in that direction, I would say even do it with current patch and tweak later as it's already in the right direction.
Created attachment 286934 [details] Strong close color Andrey requested a stronger close color and Thomas suggested to use RGB(240, 64, 64). Looks good to me. The suggestion is to merge this change and tweak to close color further if we see a need for it.
Created attachment 286936 [details] Size changes on hover I am unsure about the change in the line thickness.
Created attachment 286937 [details] make "hot" state as thin as "normal" state What about making the hot state as thin as the "normal " state?
Created attachment 286938 [details] making the normal state as thicl as the "hot " state What about making the normal state as thicl as the "hot " state?
Created attachment 286939 [details] hot line width used for non-hot state (In reply to Matthias Becker from comment #79) > Created attachment 286938 [details] > making the normal state as thicl as the "hot " state > > What about making the normal state as thicl as the "hot " state? This looks "too strong" on Linux, at least on non-HiDPI monitors. The current "non-hot" line width is 0 pixel, which GC then draws with 1 pixel, so adding 0 + 1 doesn't change anything, and adding 2 pixel like for the "hot" state draws too heavy icon, see attached picture.
(In reply to Andrey Loskutov from comment #80) > Created attachment 286939 [details] > hot line width used for non-hot state > > (In reply to Matthias Becker from comment #79) > > Created attachment 286938 [details] > > making the normal state as thicl as the "hot " state > > > > What about making the normal state as thicl as the "hot " state? > > This looks "too strong" on Linux, at least on non-HiDPI monitors. The > current "non-hot" line width is 0 pixel, which GC then draws with 1 pixel, > so adding 0 + 1 doesn't change anything, and adding 2 pixel like for the > "hot" state draws too heavy icon, see attached picture. I opened Bug 575398 for fine tuning the color and line width.
Also created Bug 575395 to update the notification png files.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/183887 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=9a8d78ed435b81eb4a7fa3583d084e8b7593c543
Thanks everyone for the discussion and thanks for everyone who tested the different approaches. As the consensus seems to be that the new close icon is an improvement, I merged the change so that we can test and fine tune it in M3 or RC1 via Bug 575398. I will also add this to N&N as soon as we have an official build with this change.
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/184062
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/184062 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=3639e2cd0808e66813c56abcf35f003d1c492d6c
I am trying to restore the old close button for my SWT application. We have lots of screenshots in our documentation and other UI elements based on the old close button. Can we make `void drawClose(GC gc, Rectangle closeRect, int closeImageState)` a protected method, rather than package-private? That way users can easily make a CTabFolderRenderer that overrides just the close button, and can put the old drawing code back in if they want: https://github.com/diffplug/eclipse.platform.swt/commit/9a8d78ed435b81eb4a7fa3583d084e8b7593c543#diff-2989afee4bfc49a4a96e2612dea5130deae11347e86395bf444e874563a44c0b The closest protected method is `protected void draw(int part, int state, Rectangle bounds, GC gc)`, but it is huge and makes extensive use of package-private variables and methods, which means that it isn't actually possible to override the close-button behavior to restore the previous behavior. I tried and you have to copy literally thousands of lines, and even still it doesn't work because you can't access package-private fields of CTabFolder.
(In reply to Ned Twigg from comment #87) > I am trying to restore the old close button for my SWT application. We have > lots of screenshots in our documentation and other UI elements based on the > old close button. > > Can we make `void drawClose(GC gc, Rectangle closeRect, int > closeImageState)` a protected method, rather than package-private? That way > users can easily make a CTabFolderRenderer that overrides just the close > button, and can put the old drawing code back in if they want: > > https://github.com/diffplug/eclipse.platform.swt/commit/ > 9a8d78ed435b81eb4a7fa3583d084e8b7593c543#diff- > 2989afee4bfc49a4a96e2612dea5130deae11347e86395bf444e874563a44c0b > > The closest protected method is `protected void draw(int part, int state, > Rectangle bounds, GC gc)`, but it is huge and makes extensive use of > package-private variables and methods, which means that it isn't actually > possible to override the close-button behavior to restore the previous > behavior. I tried and you have to copy literally thousands of lines, and > even still it doesn't work because you can't access package-private fields > of CTabFolder. This bug is closed, Ned. Please open another issue. However, before you do that, you must realize that the chance that we are going to make API changes because you don't want to update your documentation, is very slim.
