-
Notifications
You must be signed in to change notification settings - Fork 233
Proper selection coloring in JFace Structured Viewers #3659
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,199 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /******************************************************************************* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright (c) 2024 SAP SE. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * This program and the accompanying materials | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * are made available under the terms of the Eclipse Public License 2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * which accompanies this distribution, and is available at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * https://www.eclipse.org/legal/epl-2.0/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * SPDX-License-Identifier: EPL-2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Contributors: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * SAP SE - initial API and implementation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| *******************************************************************************/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package org.eclipse.jface.viewers; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Arrays; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.jface.resource.ColorRegistry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.jface.resource.JFaceResources; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.SWT; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.graphics.Color; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.graphics.GC; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.graphics.RGB; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.widgets.Control; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.widgets.Event; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.widgets.Listener; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.widgets.Scrollable; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * EraseItem event listener that provides custom selection coloring for JFace | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * viewers. This listener only activates when no custom owner draw label | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * provider is registered, ensuring it doesn't conflict with existing custom | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * drawing implementations. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <p> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+34
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * EraseItem event listener that provides custom selection coloring for JFace | |
| * viewers. This listener only activates when no custom owner draw label | |
| * provider is registered, ensuring it doesn't conflict with existing custom | |
| * drawing implementations. | |
| * <p> | |
| * {@code SWT.EraseItem} event listener that provides custom selection coloring | |
| * for JFace viewers. | |
| * <p> | |
| * The listener participates in drawing as long as there are no additional | |
| * custom {@code SWT.EraseItem} listeners installed on the underlying control | |
| * (other than the one used internally by {@link OwnerDrawLabelProvider}). | |
| * This ensures that existing custom drawing implementations which handle | |
| * {@code SWT.EraseItem} themselves can fully control the rendering, including | |
| * selection colors, without interference from this listener. | |
| * </p> | |
| * <p> |
Christopher-Hermann marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasAdditionalEraseItemListeners() documentation says it detects listeners “registered after this listener”, but the implementation doesn’t check registration order—any extra EraseItem listener (even one registered earlier) will disable this selection drawing. Either implement an order-aware check or revise the method comment/behavior to avoid surprising disabling of selection coloring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Color backgroundColor; | |
| Color foregroundColor; | |
| if (control.isFocusControl()) { | |
| backgroundColor = getSelectionColor(COLOR_SELECTION_BG_FOCUS, event.display); | |
| foregroundColor = getSelectionColor(COLOR_SELECTION_FG_FOCUS, event.display); | |
| } else { | |
| backgroundColor = getSelectionColor(COLOR_SELECTION_BG_NO_FOCUS, event.display); | |
| foregroundColor = getSelectionColor(COLOR_SELECTION_FG_NO_FOCUS, event.display); | |
| } | |
| final Color backgroundColor = getSelectionColor( | |
| control.isFocusControl() ? COLOR_SELECTION_BG_FOCUS : COLOR_SELECTION_BG_NO_FOCUS, event.display); | |
| final Color foregroundColor = getSelectionColor( | |
| control.isFocusControl() ? COLOR_SELECTION_FG_FOCUS : COLOR_SELECTION_FG_NO_FOCUS, event.display); |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drawSelection(Event) clears SWT.SELECTED and SWT.BACKGROUND in event.detail unconditionally. When this utility is called from SWT.PaintItem (as in TableOwnerDrawSupport), mutating event.detail is unexpected and can interfere with downstream logic that still inspects event.detail during painting; consider only clearing flags when handling SWT.EraseItem (or document this side effect clearly).
| // overwriting our custom colors | |
| event.detail &= ~(SWT.SELECTED | SWT.BACKGROUND); | |
| // overwriting our custom colors. This is only relevant for EraseItem | |
| // events; other event types (e.g. PaintItem) may still rely on detail. | |
| if (event.type == SWT.EraseItem) { | |
| event.detail &= ~(SWT.SELECTED | SWT.BACKGROUND); | |
| } |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drawSelection(Event) Javadoc tags this method as @since 3.32, but the class (and method) are introduced here with @since 3.39. Please correct the @since tag to the version where this API is actually added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private static Color getSelectionColor(String key, org.eclipse.swt.graphics.Device device) { | |
| ColorRegistry registry = JFaceResources.getColorRegistry(); | |
| if (registry.hasValueFor(key)) { | |
| return registry.get(key); | |
| } | |
| RGB systemColor; | |
| switch (key) { | |
| case COLOR_SELECTION_BG_FOCUS: | |
| systemColor = device.getSystemColor(SWT.COLOR_TITLE_BACKGROUND).getRGB(); | |
| break; | |
| case COLOR_SELECTION_FG_FOCUS: | |
| systemColor = device.getSystemColor(SWT.COLOR_WHITE).getRGB(); | |
| break; | |
| case COLOR_SELECTION_BG_NO_FOCUS: | |
| systemColor = device.getSystemColor(SWT.COLOR_TITLE_INACTIVE_BACKGROUND).getRGB(); | |
| break; | |
| case COLOR_SELECTION_FG_NO_FOCUS: | |
| systemColor = device.getSystemColor(SWT.COLOR_TITLE_INACTIVE_FOREGROUND).getRGB(); | |
| break; | |
| default: | |
| systemColor = device.getSystemColor(SWT.COLOR_LIST_SELECTION).getRGB(); | |
| break; | |
| } | |
| registry.put(key, systemColor); | |
| return registry.get(key); | |
| } | |
| private static Color getSelectionColor(String key, org.eclipse.swt.graphics.Device device) { | |
| ColorRegistry registry = JFaceResources.getColorRegistry(); | |
| if (!registry.hasValueFor(key)) { | |
| RGB systemColor = device.getSystemColor(idToColor(key)).getRGB(); | |
| registry.put(key, systemColor); | |
| } | |
| return registry.get(key); | |
| } | |
| private static int idToColor(String id) { | |
| return switch (id) { | |
| case COLOR_SELECTION_BG_FOCUS -> SWT.COLOR_TITLE_BACKGROUND; | |
| case COLOR_SELECTION_FG_FOCUS -> SWT.COLOR_WHITE; | |
| case COLOR_SELECTION_BG_NO_FOCUS -> SWT.COLOR_TITLE_INACTIVE_BACKGROUND; | |
| case COLOR_SELECTION_FG_NO_FOCUS -> SWT.COLOR_TITLE_INACTIVE_FOREGROUND; | |
| default -> SWT.COLOR_LIST_SELECTION; | |
| }; | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /******************************************************************************* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright (c) 2024 SAP SE. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * This program and the accompanying materials | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * are made available under the terms of the Eclipse Public License 2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * which accompanies this distribution, and is available at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * https://www.eclipse.org/legal/epl-2.0/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * SPDX-License-Identifier: EPL-2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Contributors: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * SAP SE - initial API and implementation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| *******************************************************************************/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package org.eclipse.ui.internal.themes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Hashtable; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.core.runtime.IConfigurationElement; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.core.runtime.IExecutableExtension; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.SWT; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.graphics.RGB; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.swt.widgets.Display; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.eclipse.ui.themes.IColorFactory; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Color factory for viewer selection colors that adapts to the OS/desktop | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * theme. Provides default colors based on system colors for focused and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * unfocused selections. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <p> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * The default colors are based on system title bar colors which automatically | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * adapt to light/dark themes and high contrast modes. Themes can override these | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * defaults to provide custom styling. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * </p> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @since 3.39 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class ColumnViewerSelectionColorFactory implements IColorFactory, IExecutableExtension { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private String color = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public RGB createColor() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Display display = Display.getDefault(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Display display = Display.getDefault(); | |
| Display display = Display.getCurrent(); | |
| if (display == null) { | |
| display = Display.getDefault(); | |
| } |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Focused selection foreground is hard-coded to SWT.COLOR_WHITE, while the other defaults are based on title-bar system colors. On platforms/themes where the title background is light, this can produce unreadable selection text; use a matching system foreground (e.g., SWT.COLOR_TITLE_FOREGROUND or SWT.COLOR_LIST_SELECTION_TEXT) instead of forcing white.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something more concise like ...
| public RGB createColor() { | |
| Display display = Display.getDefault(); | |
| if ("SELECTED_CELL_BACKGROUND".equals(color)) { //$NON-NLS-1$ | |
| return display.getSystemColor(SWT.COLOR_TITLE_BACKGROUND).getRGB(); | |
| } else if ("SELECTED_CELL_FOREGROUND".equals(color)) { //$NON-NLS-1$ | |
| return display.getSystemColor(SWT.COLOR_WHITE).getRGB(); | |
| } else if ("SELECTED_CELL_BACKGROUND_NO_FOCUS".equals(color)) { //$NON-NLS-1$ | |
| return display.getSystemColor(SWT.COLOR_TITLE_INACTIVE_BACKGROUND).getRGB(); | |
| } else if ("SELECTED_CELL_FOREGROUND_NO_FOCUS".equals(color)) { //$NON-NLS-1$ | |
| return display.getSystemColor(SWT.COLOR_TITLE_INACTIVE_FOREGROUND).getRGB(); | |
| } else { | |
| return new RGB(0, 0, 0); | |
| } | |
| } | |
| @Override | |
| public RGB createColor() { | |
| return Display.getDefault().getSystemColor(idToColor(color)).getRGB(); | |
| } | |
| private int idToColor(String id) { | |
| return switch (id) { | |
| case "SELECTED_CELL_BACKGROUND" -> SWT.COLOR_TITLE_BACKGROUND; //$NON-NLS-1$ | |
| case "SELECTED_CELL_FOREGROUND" -> SWT.COLOR_WHITE; //$NON-NLS-1$ | |
| case "SELECTED_CELL_BACKGROUND_NO_FOCUS" -> SWT.COLOR_TITLE_INACTIVE_BACKGROUND; //$NON-NLS-1$ | |
| case "SELECTED_CELL_FOREGROUND_NO_FOCUS" -> SWT.COLOR_TITLE_INACTIVE_FOREGROUND; //$NON-NLS-1$ | |
| default -> SWT.COLOR_BLACK; | |
| }; | |
| } |
?
With one change: default (or null) now yield SWT.COLOR_BLACK, which should also work as expected (please double check this).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -371,6 +371,15 @@ Color.revisionNewestDesc=Background color for the newest revision shown in a tex | |
| Color.revisionOldest=Oldest revision background color | ||
| Color.revisionOldestDesc=Background color for the oldest revision shown in a text editor's ruler. Together with the newest revision background color this defines a gradient used for all revision from newest to oldest. | ||
|
|
||
| Color.viewerSelectionBackgroundFocused=Viewer selection background (focused) | ||
| Color.viewerSelectionBackgroundFocusedDesc=Background color for selected items in viewers when the control has focus. Defaults to system highlight color. (No effect on Linux) | ||
| Color.viewerSelectionForegroundFocused=Viewer selection foreground (focused) | ||
| Color.viewerSelectionForegroundFocusedDesc=Foreground color for selected items in viewers when the control has focus. (No effect on Linux) | ||
| Color.viewerSelectionBackgroundNoFocus=Viewer selection background (no focus) | ||
| Color.viewerSelectionBackgroundNoFocusDesc=Background color for selected items in viewers when the control does not have focus. (No effect on Linux) | ||
| Color.viewerSelectionForegroundNoFocus=Viewer selection foreground (no focus) | ||
| Color.viewerSelectionForegroundNoFocusDesc=Foreground color for selected items in viewers when the control does not have focus. (No effect on Linux) | ||
|
Comment on lines
+374
to
+381
|
||
|
|
||
| Color.showKeysForeground=Keys foreground color | ||
| Color.showKeysForegroundDesc=Color for foreground of the control to visualize pressed keys | ||
| Color.showKeysBackground=Keys background color | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operator formatting (
event.detail&= ...) is inconsistent with the surrounding style in this file (spaces around&=). Please keep the existing formatting to avoid noisy diffs and match project conventions.