Community
Participate
Working Groups
Created attachment 272753 [details] Screenshot showing the issue The Eclips Update Notification is not correctly styled in the dark theme. It has a light color. The cause is that in protected PopupDialog#getBackground() SWT.COLOR_INFO_BACKGROUND is used. Sub-Classes of PopupDialog (e.g. org.eclipse.equinox.internal.p2.ui.sdk.scheduler.AutomaticUpdatesPopup could overright getBackground (as e.g. the jface ContentProposalAdapter does) but I think it's a better ways to provide a meaningful default in PopupDialog#getBackground(). For PopupDialog#getDefaultForeground() the same applies. Do we already have color-preference keys that can be used as background / foreground color? I only found "Content Assist background / foreground color" or "Information background / foreground color". Also see attachement "update_before.png"
New Gerrit change created: https://git.eclipse.org/r/117749
Created attachment 272754 [details] Screenshot with the fix
*** Bug 531269 has been marked as a duplicate of this bug. ***
Very nice, fixes also the issues in the light theme reported with Bug 531269.
(In reply to Lars Vogel from comment #3) > *** Bug 531269 has been marked as a duplicate of this bug. *** Are u sure that this is really the same issue? Is this a Linux specific issue? At least on Mac this issue does not exist.
(In reply to Matthias Becker from comment #5) > Are u sure that this is really the same issue? > Is this a Linux specific issue? Yes, it is a Linux specific issue and I'm sure that this is the same issue. I also tested your patch and with it the correct colors is used for Linux.
Did u also test the update-dialog shows up in the correct colors?
Created attachment 272758 [details] Screenshot with the fix in the light theme
Created attachment 272759 [details] Screenshot before in the light theme
(In reply to Lars Vogel from comment #6) > (In reply to Matthias Becker from comment #5) > > Are u sure that this is really the same issue? > > Is this a Linux specific issue? > > Yes, it is a Linux specific issue and I'm sure that this is the same issue. > I also tested your patch and with it the correct colors is used for Linux. I am not quite sure about my implementation. I have added a question in gerrit. @Lars: Can you have a look at that question?
(In reply to Matthias Becker from comment #10) > @Lars: Can you have a look at that question? Via a text search for SWT.FOCUS, I did not find a usage of this flag in eclipse.platform.ui for popup dialogs. Also in general I don't understand why we anyway should be using a different hard-coded color based on the SWT.FOCUS flag. I think it is OK to merge this and if someone finds an issue with this, we can revisted.
Gerrit change https://git.eclipse.org/r/117749 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=cc4cae9821dba55ca6b90fb0a39c90b35f5774aa
I suggest to add this to the N&N. If you add an entry for the dark theme, I can add the info that the light theme is Linux is also fixed with a screenshot.
New Gerrit change created: https://git.eclipse.org/r/117841
Gerrit change https://git.eclipse.org/r/117841 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=db9369c649dec6e1c23b2bbdc24e39210ed1a251
Thanks, Matthias. These UI touches are quite annoying and it's a relief to see them fixed.
(In reply to Lars Vogel from comment #13) > I suggest to add this to the N&N. If you add an entry for the dark theme, I > can add the info that the light theme is Linux is also fixed with a > screenshot. Looking at the entry, an extra Linux entry seems unnecessary.
Please revert that change asap. The white background of the Quick Outline is now replaced by a yellow hover-like background, which is not OK.
(In reply to Dani Megert from comment #18) > Please revert that change asap. The white background of the Quick Outline is > now replaced by a yellow hover-like background, which is not OK. There was a good reason for the if ((getShellStyle() & SWT.NO_FOCUS) != 0) {
My 50 cents (as having implemented the INFORMATION_ color...): So in general INFORMATION_ color is intended to replace use of "TOOLTIP_", it's generally not a replacement for LIST_*, as is the case here: https://git.eclipse.org/r/#/c/117749/ (the patch above merges TOOLTIP and LIST, which should be kept separate I think?) With that said, keep in mind: - on win/cocoa, INFORMATION_ by default translates to TOOLTIP_* - Gtk, INFORMATION_ by default translates to LIST_* So on gtk, if you use INFORMATION_ and LIST_*, you get the same color. The fix I think should utilize INFORMATION_ where 'tooltip' was used, and perhaps a slightly different case when focus is used (e.g make color darker etc..).
New Gerrit change created: https://git.eclipse.org/r/117920
New Gerrit change created: https://git.eclipse.org/r/117921
(In reply to Eclipse Genie from comment #21) > New Gerrit change created: https://git.eclipse.org/r/117920 Should we really go for revert? IMHO Leo's comment 20 is the right way forward.
(In reply to Alexander Kurtakov from comment #23) > (In reply to Eclipse Genie from comment #21) > > New Gerrit change created: https://git.eclipse.org/r/117920 > > Should we really go for revert? IMHO Leo's comment 20 is the right way > forward. I revert it and will then will take my time to provide a new change.
(In reply to Leo Ufimtsev from comment #20) > The fix I think should utilize INFORMATION_ where 'tooltip' was used, and > perhaps a slightly different case when focus is used (e.g make color darker > etc..). But in the old code TOOLTIP* was not used. I did not quite get your point. Do you have a pointer to place where you did a similar change?
(In reply to Matthias Becker from comment #25) > (In reply to Leo Ufimtsev from comment #20) > > The fix I think should utilize INFORMATION_ where 'tooltip' was used, and > > perhaps a slightly different case when focus is used (e.g make color darker > > etc..). > > But in the old code TOOLTIP* was not used. I did not quite get your point. > Do you have a pointer to place where you did a similar change? TOOLTIP* should have been COLOR_INFO*. If you look at COLOR_INFO_BACKGROUND javadoc it says "System color used to paint tooltip background areas (value is 29)."
Gerrit change https://git.eclipse.org/r/117920 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=dd14b9d625582a7025f9fd8ae3890e311e3d590e
(In reply to Dani Megert from comment #19) > (In reply to Dani Megert from comment #18) > > Please revert that change asap. The white background of the Quick Outline is > > now replaced by a yellow hover-like background, which is not OK. > > There was a good reason for the > if ((getShellStyle() & SWT.NO_FOCUS) != 0) { With quick-outline you mean Ctrl-O e.g. in the java editor? For this case if ((getShellStyle() & SWT.NO_FOCUS) != 0) also is false (as well as for the update popup. So the cause of this error is not the removal of this condition but the fact that i picked the wrong color definition.
(In reply to Matthias Becker from comment #28) > (In reply to Dani Megert from comment #19) > > (In reply to Dani Megert from comment #18) > > > Please revert that change asap. The white background of the Quick Outline is > > > now replaced by a yellow hover-like background, which is not OK. > > > > There was a good reason for the > > if ((getShellStyle() & SWT.NO_FOCUS) != 0) { > > With quick-outline you mean Ctrl-O e.g. in the java editor? Yes. > For this case if ((getShellStyle() & SWT.NO_FOCUS) != 0) also is false (as > well as for the update popup. > So the cause of this error is not the removal of this condition but the fact > that i picked the wrong color definition. Right. I'm fine to replace COLOR_INFO_FOREGROUND/tooltop colors. But the LIST color must either be kept or a new color (constant) needs to be defined.
(In reply to Dani Megert from comment #29) > (In reply to Matthias Becker from comment #28) > > (In reply to Dani Megert from comment #19) > > > (In reply to Dani Megert from comment #18) > > > > Please revert that change asap. The white background of the Quick Outline is > > > > now replaced by a yellow hover-like background, which is not OK. > > > > > > There was a good reason for the > > > if ((getShellStyle() & SWT.NO_FOCUS) != 0) { > > > > With quick-outline you mean Ctrl-O e.g. in the java editor? > > Yes. > > > For this case if ((getShellStyle() & SWT.NO_FOCUS) != 0) also is false (as > > well as for the update popup. > > So the cause of this error is not the removal of this condition but the fact > > that i picked the wrong color definition. > > Right. I'm fine to replace COLOR_INFO_FOREGROUND/tooltop colors. But the > LIST color must either be kept or a new color (constant) needs to be defined. Sorry for askting again. Maybe I don't see a very simple point. So this would be wrong? private Color getDefaultBackground() { if ((getShellStyle() & SWT.NO_FOCUS) != 0) { return getShell().getDisplay().getSystemColor(SWT.COLOR_INFO_BACKGROUND); } return JFaceResources.getColorRegistry().get(JFacePreferences.CONTENT_ASSIST_BACKGROUND_COLOR); } If JFacePreferences.INFORMATION_BACKGROUND_COLOR is the replacement for SWT.COLOR_INFO_BACKGROUND what is the replacement for SWT.COLOR_LIST_BACKGROUND?
CONTENT_ASSIST_BACKGROUND should be good match in place of COLOR_LIST_BACKGROUND as content assist and quick outline should match in colors. aka private Color getDefaultBackground() { if ((getShellStyle() & SWT.NO_FOCUS) != 0) { return JFaceResources.getColorRegistry().get(JFacePreferences.INFORMATION_BACKGROUND_COLOR); } return JFaceResources.getColorRegistry().get(JFacePreferences.CONTENT_ASSIST_BACKGROUND_COLOR); }
New Gerrit change created: https://git.eclipse.org/r/117943
(In reply to Alexander Kurtakov from comment #31) > CONTENT_ASSIST_BACKGROUND should be good match in place of > COLOR_LIST_BACKGROUND as content assist and quick outline should match in > colors. > > aka > > private Color getDefaultBackground() { > if ((getShellStyle() & SWT.NO_FOCUS) != 0) { > return > JFaceResources.getColorRegistry().get(JFacePreferences. > INFORMATION_BACKGROUND_COLOR); > } > return > JFaceResources.getColorRegistry().get(JFacePreferences. > CONTENT_ASSIST_BACKGROUND_COLOR); > } I implemented this with my new change. On macOS the update dialog, the quick outline and that no-focus that appreas when you do Window > Appearance > Toggle Full Screen Mode all look good in light and dark them. @Dani: Can you pls check on windows if it now is ok? @Lars: How does it look on Linux
Gerrit change https://git.eclipse.org/r/117943 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=86ba71a63e8fa5d1992e242aac4cea705781a823
N&N still needs to be merged.
(In reply to Dani Megert from comment #35) > N&N still needs to be merged. It's still in. What was pending (because of a merge conflict) was the revert of the N&N. But we no longer need the revert. Thanks for testing and the advice.
*** Bug 501781 has been marked as a duplicate of this bug. ***
