Update Color constructors to remove deprecated Device parameter#2288
Update Color constructors to remove deprecated Device parameter#2288vogella merged 1 commit intoeclipse-pde:masterfrom
Conversation
vogella
force-pushed
the
update-color-constructors
branch
from
dac13b3 to
9a56753
Compare
vogella
force-pushed
the
update-color-constructors
branch
2 times, most recently
from
9b44e5e to
1a12daf
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates SWT Color construction call sites to stop passing the deprecated Device/Display argument, aligning PDE/e4 tools code and templates with the SWT API change referenced in eclipse-platform SWT issue #3232.
Changes:
- Replaced
new Color(display, rgb)/new Color(Display.getCurrent(), rgb)with the newer constructors (new Color(rgb)/new Color(r,g,b)). - Removed now-unused
Display/RGBimports in the affected template/services files. - Updated a few UI components/templates to construct colors via the new API.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/parts/MessageLine.java | Updates error background color construction to the device-less Color constructor. |
| ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/editor/text/ColorManager.java | Updates preference-driven color caching to use the new Color(RGB) constructor. |
| ui/org.eclipse.pde.ui.templates/templates_3.1/extensibleEditor/java/$javaClassPrefix$PresentationReconciler.java | Updates generated reconciler template to use new color constructors and drops Display import. |
| ui/org.eclipse.pde.ui.templates/templates_3.1/editor/java/ColorManager.java | Updates generated color manager template to use new Color(RGB) constructor and drops Display import. |
| ui/org.eclipse.pde.spy.event/src/org/eclipse/pde/spy/event/internal/ui/CapturedEventTree.java | Updates custom tree item foreground color construction to the device-less Color(RGB) constructor. |
| e4tools/bundles/org.eclipse.e4.tools.services/src/org/eclipse/e4/tools/services/BasicResourceProvider.java | Updates service-based color creation to new Color(r,g,b) and removes the unused RGB import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (fErrorMsgAreaBackground == null) { | ||
| fErrorMsgAreaBackground = new Color(getDisplay(), ERROR_BACKGROUND_RGB); | ||
| fErrorMsgAreaBackground = new Color(ERROR_BACKGROUND_RGB); | ||
| } | ||
| setBackground(fErrorMsgAreaBackground); |
There was a problem hiding this comment.
fErrorMsgAreaBackground is created with new Color(...) but never disposed. The current dispose() implementation only nulls the reference, which will leak the OS color resource over the widget’s lifetime. Dispose the color (when non-null and not already disposed) before calling super.dispose().
| private void putColor(String property, RGB setting) { | ||
| Color oldColor = fColorTable.get(property); | ||
| if (oldColor != null) { | ||
| if (oldColor.getRGB().equals(setting)) { | ||
| return; | ||
| } | ||
| } | ||
| fColorTable.put(property, new Color(Display.getCurrent(), setting)); | ||
| fColorTable.put(property, new Color(setting)); | ||
| } |
There was a problem hiding this comment.
When the preference color changes, putColor(...) overwrites the existing entry in fColorTable without disposing the previous Color. This leaks SWT resources over time (and the map is never explicitly disposed). Dispose oldColor when it’s being replaced, and consider disposing all cached colors when counter drops to 0.
| private final TextAttribute tagAttribute = new TextAttribute(new Color(new RGB(0,0, 255))); | ||
| private final TextAttribute headerAttribute = new TextAttribute(new Color(new RGB(128,128,128))); |
There was a problem hiding this comment.
This template creates Color instances inline for TextAttribute fields but never disposes them. Generated editors will leak SWT color resources unless they explicitly dispose these colors. Prefer routing color creation through a ColorManager/ColorRegistry with a dispose() hook, or ensure the reconciler disposes the colors when the editor is disposed.
| final int g = Integer.parseInt(cols[1].trim()); | ||
| final int b = Integer.parseInt(cols[2].trim()); | ||
| return new Color(display, new RGB(r, g, b)); | ||
| return new Color(r, g, b); |
There was a problem hiding this comment.
getColor(Display display, ...) now returns new Color(r, g, b), which ignores the provided display. This can create the Color on the wrong SWT Device (or fail if there is no current display), while ResourceService pools resources by the passed-in display. Create the color on the supplied display’s UI thread (e.g., via display.syncExec) so the resource is associated with the correct device.
| return new Color(r, g, b); | |
| final Color[] createdColor = new Color[1]; | |
| display.syncExec(() -> createdColor[0] = new Color(display, r, g, b)); | |
| return createdColor[0]; |
…emove unused imports
vogella
force-pushed
the
update-color-constructors
branch
from
1a12daf to
885a4e7
Compare
|
Copilot needs an update, it continue to claims that colors must be disposed. All feedback is invalid. |
This PR updates Color constructors to remove the deprecated Device parameter, following the changes in SWT (eclipse-platform/eclipse.platform.swt#3232). It also removes the now unused imports of Display and RGB in the modified files.