Re: [EGIT PATCH 3/8] Dispose of allocated colors on finalize()
- From
Shawn O. Pearce <spearce@spearce.org>
- Date
- Oct 1, 2008, 14:37 UTC
- Message-ID
- <20081001143718.GU21310@spearce.org>
- In-Reply-To
- <1222818823-22780-3-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> ---
Missing SBO.
Show 21 quoted lines
> .../egit/ui/internal/history/SWTPlotRenderer.java | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
> index 23ec255..a58b3bf 100644
> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
> @@ -45,6 +45,17 @@ SWTPlotRenderer(final Display d) {
> sys_darkblue = d.getSystemColor(SWT.COLOR_DARK_BLUE);
> }
>
> + @Override
> + protected void finalize() throws Throwable {
> + sys_black.dispose();
> + sys_blue.dispose();
> + sys_gray.dispose();
> + sys_darkblue.dispose();
> + sys_yellow.dispose();
> + sys_green.dispose();
> + sys_white.dispose();
> + }I think this is wrong. Any color that we get from Display.getSystemColor() must not be disposed of by the application, its in use by the Display and/or other parts of the application.
>From the Javadocs:
This color should not be free'd because it was allocated by the system, not the application.
I take that to mean that the color should not be disposed of.
Though reading the Gtk SWT sources it seems that the system color objects aren't pooled, each call to getSystemColor causes a new Color instace to be allocated. So it may be harmless on Gtk.
What was the rationale for disposing of these resources? Did you identify that this is a resource leak somewhere? Because I'd like to make sure I actually understand the SWT resource model better so I don't commit mistakes in the future.
-- Shawn.