Fix theme-race in decorator manager and modernize decorator package#16
Fix theme-race in decorator manager and modernize decorator package#16
Conversation
|
Please rebase on master, PR contains lot of unrelated changes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes several Eclipse UI components by migrating legacy logging and status utilities to the ILog and Status APIs, generifying collections, and adopting modern Java features like records. Notable functional improvements include adding sortable columns to the Smart Import wizard, a cancel icon in the Quick Access search field, and extracting Quick Access matching logic into a dedicated utility class for better testability. Additionally, several test suites were updated to JUnit 5. Feedback was provided regarding a potential StringIndexOutOfBoundsException in DecoratorManager.java caused by the switch from StringTokenizer to String.split when processing preference strings.
| for (String nextValuePair : preferenceValue.split(PREFERENCE_SEPARATOR)) { | ||
| // Strip out the true or false to get the id | ||
| String id = nextValuePair.substring(0, nextValuePair.indexOf(VALUE_SEPARATOR)); | ||
| if (nextValuePair.endsWith(P_TRUE)) { | ||
| enabledIds.add(id); | ||
| } else { | ||
| disabledIds.add(id); | ||
| } | ||
| } |
There was a problem hiding this comment.
The transition from StringTokenizer to String.split introduces a potential regression in robustness. String.split can return empty strings if the preference value contains leading or consecutive separators (e.g., ",id:true"), whereas StringTokenizer would skip them. If an empty string is processed, nextValuePair.indexOf(VALUE_SEPARATOR) will return -1, causing substring(0, -1) to throw a StringIndexOutOfBoundsException. It is safer to check if the separator exists before attempting to extract the ID.
for (String nextValuePair : preferenceValue.split(PREFERENCE_SEPARATOR)) {
int separatorIndex = nextValuePair.indexOf(VALUE_SEPARATOR);
if (separatorIndex != -1) {
String id = nextValuePair.substring(0, separatorIndex);
if (nextValuePair.endsWith(P_TRUE)) {
enabledIds.add(id);
} else {
disabledIds.add(id);
}
}
}
You may not have noticed that this is just living in my clone. So please ignore this until I'm really to share it with eclipse.platform.ui The link is unfortunately automatically created by Github. |
Behavior-preserving cleanup of the org.eclipse.ui.internal.decorators package: - DecorationResult: generify raw List fields to List<String>, replace ListIterator loops with enhanced-for. - DecoratorManager: replace explicit Iterator loop with enhanced-for, replace StringTokenizer with String.split, drop toArray with a pre-sized array in favour of toArray(new T[0]), remove the now unused imports. - LightweightDecoratorManager: convert the RunnableData holder to a record, drop toArray with a pre-sized array. In preparation for eclipse-platform#3926
DecorationResult holds Color and Font references obtained from the active theme's registries at the time a decoration is computed. When the workbench starts with a theme that differs from the default, the color registry is populated after some decoration jobs have already cached references, leaving rows that carry a decorator-provided background or foreground painted with stale values from the default theme. The same stale-reference issue shows up on a runtime theme switch. Install an IPropertyChangeListener on the workbench IThemeManager, which forwards property changes from the current theme's color and font registries and fires CHANGE_CURRENT_THEME on theme switches. When any of those events fire, clear the DecorationScheduler result cache and fire a general LabelProviderChangedEvent so viewers re-query decorations and pick up fresh colors and fonts. Fixes eclipse-platform#3926
vogella
force-pushed
the
fix/decorator-theme-race
branch
from
86e7d63 to
150eea8
Compare
2 participants
Tree rows with a decorator-provided background or foreground can render with stale colors when the workbench starts with a theme that differs from the default, because
DecorationResultcachesColorandFontreferences at decoration time and the registry is still being populated when early decoration jobs fire. The same issue shows up on a runtime theme switch.This branch first applies a behaviour-preserving cleanup of the decorator package (raw-type generics, enhanced-for,
String.split,RunnableDataas a record), then adds anIPropertyChangeListeneron the workbenchIThemeManagerinDecoratorManagerthat clears the decoration cache and fires a generalLabelProviderChangedEventwhen theme colors or fonts change, so viewers pick up fresh values.Target for the upstream work is eclipse-platform#3926. This PR is against the vogella fork so the branch can be built into an IDE and tested before proposing anything upstream.