Bug 253002 - [Contributions] CompoundContributionItem disposes old items on show
Summary: [Contributions] CompoundContributionItem disposes old items on show
Status: REOPENED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-31 13:16 EDT by Kim Horne CLA
Modified: 2019-09-06 16:13 EDT (History)
7 users (show)

See Also:
bokowski: review+


Attachments
CompoundContributionItem early disposal v01 (2.73 KB, patch)
2009-05-01 09:33 EDT, Paul Webster CLA
no flags Details | Diff
CompoundContributionItem early disposal v02 (2.67 KB, patch)
2009-05-06 13:34 EDT, Paul Webster CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kim Horne CLA 2008-10-31 13:16:36 EDT
I20080911-0800

CompoundContributionItem will dispose of old items, but only when the menu is shown.  This has the effect of temporarily leaking old items.  Could this not make use of IMenuListener2 to dispose of the items on menu close?
Comment 1 Paul Webster CLA 2009-05-01 09:33:49 EDT
Created attachment 134055 [details]
CompoundContributionItem early disposal v01
Comment 2 Boris Bokowski CLA 2009-05-04 16:24:30 EDT
Paul, can we look at this together tomorrow?
Comment 3 Boris Bokowski CLA 2009-05-06 13:05:06 EDT
	if (display.isDisposed()) {
		dispose.run();
	} else {
		display.asyncExec(dispose);
	}


Questions:
1. Can the display even be disposed at this point? This code is called from menuAboutToHide().
2. By doing the work in an async exec, could we end up with empty menus in some cases?
Comment 4 Paul Webster CLA 2009-05-06 13:34:50 EDT
Created attachment 134654 [details]
CompoundContributionItem early disposal v02

1) you're right, were in the UI thread event loop when this is called

This is the updated patch.

2) as it turns out, this doesn't have the same problem that I was talking about with PopupMenuExtender.  We copy out the items for dispose, so that when the asyncExec runs it cannot overrun the new items.

PW
Comment 5 Boris Bokowski CLA 2009-05-11 12:09:21 EDT
+1. Would be good to either check for display.isDisposed / PlatformUI.getWorkbench().isClosing() in the runnable, or add a comment that calling dispose on IContributionItem has nothing to do with SWT resources or Workbench resources.
Comment 6 Paul Webster CLA 2009-05-11 12:14:17 EDT
(In reply to comment #4)
> Created an attachment (id=134654) [details]
> CompoundContributionItem early disposal v02

Released to HEAD for I20090511-2000

I added a check for display.isDisposed() just to be careful.

PW
Comment 7 Paul Webster CLA 2009-05-15 09:15:12 EDT
In I20090514-2000
PW
Comment 8 Paul Webster CLA 2009-05-19 16:02:46 EDT
This caused bug 276525

Steve, what we were doing was creating an asycnExec in the Menu SWT.Hide.  On windows, the asyncExec Runnable would dispose the MenuItems before the submenu actually disappeared (even though the SWT.Hide has finished processing).

Should the menu not simply disappear after the SWT.Hide is finished?

PW
Comment 9 Steve Northover CLA 2009-05-19 16:10:00 EDT
Are you telling me that you got to watch the items be disposed when you did not do the asyncExec() on Windows but the behavior was different elsewhere?
Comment 10 Paul Webster CLA 2009-05-19 16:16:18 EDT
Steve ... hmmm.  I'll say right!

On windows, we launch the asyncExec from the SWT.Hide and it is processed before the submenu disappears.  We watch all of the menu items disappear, and then the submenu disappears.

On linux, it works as expected (the menu just hides normally).

PW
Comment 11 Paul Webster CLA 2009-09-10 09:17:15 EDT
Silenio we were talking about event ordering and menus disappearing on windows.

PW
Comment 12 Silenio Quarti CLA 2009-09-10 11:57:38 EDT
Paul, do you know what is the behavior on Mac (carbon and cocoa)?

Felipe, please investigate if we can make SWT.Hide come after the menu is hidden.
Comment 13 Felipe Heidrich CLA 2009-09-10 16:26:54 EDT
public static void main (String [] args) {
    Display display = new Display ();
    Shell shell = new Shell (display);
    final Menu menu = new Menu (shell, SWT.POP_UP);
    MenuItem item = new MenuItem(menu, SWT.PUSH);
    item.setText("File");
    item = new MenuItem(menu, SWT.PUSH);
    item.setText("Edit");
    item = new MenuItem(menu, SWT.PUSH);
    item.setText("view");
    shell.setMenu(menu);
    menu.addListener(SWT.Show, new Listener() {
		public void handleEvent(Event e) {
			System.out.println("show: " + menu.getVisible());
		}
	});
    menu.addListener(SWT.Hide, new Listener() {
		public void handleEvent(Event e) {
			System.out.println("hide: " + menu.getVisible());
		}
	});    
    shell.setSize(50, 50);
    shell.open ();
    while (!shell.isDisposed ()) {
        if (!display.readAndDispatch ()) display.sleep ();
    }
    display.dispose ();
}

Windows and cocoa:
show: true
hide: true

Gtk and carbon:
show: false
hide: false

SSQ, who is right ?
Comment 14 Eclipse Webmaster CLA 2019-09-06 16:13:56 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.