SWT - ChartComposite - removeAllMenuItems method

A discussion forum for JFreeChart (a 2D chart library for the Java platform).
Locked
Rex Feral
Posts: 11
Joined: Tue Feb 23, 2016 8:48 am
antibot: No, of course not.

SWT - ChartComposite - removeAllMenuItems method

Post by Rex Feral » Mon Mar 07, 2016 2:38 pm

I worked a little with ChartComposite for an Eclipse plugin application and have noticed that it never disposes its MenuItems from the popupMenu. While this doesn't normally hurt, if you happen to create over about 2000-3000 ChartComposites (depending on RAM probably) over the application's lifetime, it will crash with the exception org.eclipse.swt.SWTError: No more handles.

I noticed the problem was with Menu and MenuItems never getting disposed by using a profiler.

Now what I did was to extend ChartComposite and create a method removeAllMenuItems() which goes like this:

Code: Select all

	public void removeAllMenuItems() {
		if (popupMenu != null) {
		    for (MenuItem menuItem : popupMenu.getItems()) {
		        menuItem.removeSelectionListener(this);
			menuItem.dispose();
		    }
		    popupMenu.dispose();
               }
	}
It's made to work like this: If you want a chartComposite disposed (or a Composite containing multiple chartComposites) first you call this method and then you proceed to dispose the composite. This way you can dispose safely without worrying of leaving objects undisposed.

Is there a better method to do this? I currently working with 1.0.19 but maybe you guys have something better in store for the next version.

david.gilbert
JFreeChart Project Leader
Posts: 11734
Joined: Fri Mar 14, 2003 10:29 am
antibot: No, of course not.
Contact:

Re: SWT - ChartComposite - removeAllMenuItems method

Post by david.gilbert » Tue Mar 08, 2016 6:19 am

Hi Rex,

Thanks for the feedback. First thing to note is that the SWT code (the ChartComposite etc) has moved into a separate project:

https://github.com/jfree/jfreechart-swt

Second thing is that I don't use SWT myself, so I am not an expert at all in how the API works. But I'm wondering if it makes sense to follow the same approach as I see already in the code where a dispose listener is added to the underlying canvas:

Code: Select all

        this.canvas.addDisposeListener(new DisposeListener() {
            public void widgetDisposed(DisposeEvent e) {
                org.eclipse.swt.graphics.Image img;
                img = (org.eclipse.swt.graphics.Image) canvas.getData("double-buffer-image");
                if (img != null) {
                    img.dispose();
                }
            }
        });
The ChartComposite also has an addDisposeListener() method, so would it make sense to add a listener that disposes the menus?
David Gilbert
JFreeChart Project Leader

:idea: Read my blog
:idea: Support JFree via the Github sponsorship program

Rex Feral
Posts: 11
Joined: Tue Feb 23, 2016 8:48 am
antibot: No, of course not.

Re: SWT - ChartComposite - removeAllMenuItems method

Post by Rex Feral » Tue Mar 08, 2016 9:29 am

Ah yes, adding a DisposeListener should do, although I can't say for certain where and how, since I can't see the ChartComposite code (I only I have the class file and no source code). Might try later overriding some methods (addSWTListener?) or adding by constructor.

What also works is overriding the dispose() method BUT the ChartComposite objects you created have to be disposed one by one by their method (e.g. myChartCompositeObject.dispose() ) and since they would be a lot of them, they should be added to a List that is then iterated for their disposal. Disposing the parent composite of the chartComposites does not dispose them by the overriden method so that's why they'd have to be disposed one by one.

Here's how the dispose() method would look:

Code: Select all

	@Override
	public void dispose() {
		if (popupMenu != null) {
			for (MenuItem item : popupMenu.getItems()) {
				item.removeSelectionListener(this);
				item.dispose();
			}
			popupMenu.dispose();
		}
		super.dispose();
	}
Adding a DisposeListener as chartCompositeObject.addDisposeListener(...) would be trickier since the ChartComposite doesn't have a getPopupMenu() method in order to get the menu and all it's items.

Locked