Bug 329872

Comment 1 Fan Peng 2010-11-10 04:19:05 EST

Created attachment 182792 [details]
An simple eclipse project to reproduce this issue

An simple eclipse project to reproduce this issue

Comment 2 Fan Peng 2010-11-10 04:36:06 EST

1 Import the attachment project into eclipse
2 Run the Main() method in MainApp.java
3 Click the "Push Me" button, child dialog popup.
4 Press F9 in mac, switch the focus to the parent dialog, the issue comes out.

Comment 3 Scott Kovatch 2010-11-17 13:08:09 EST

Since this is on Carbon and it works on Cocoa we won't be addressing it for 3.7.

Comment 4 David Klein 2011-01-28 19:36:21 EST

We're seeing defects come in regarding this flicker scenario from three of our Eclipse based products so far (using SWT 3.6.1).  Am asking that someone take a look to see if a fix can be found for this carbon problem.

Our Mac development team has traced down what they believe to be the change that has caused this regression in 3.6, to code which was added to the swt Shell class, kEventWindowActivated method.  Basically, the if (!active) logic which is deactivating shells is initiating the behavior causing the flicker (but don't have any further insight).  Admittedly this logic is to fix another problem, but perhaps the logic can be modified to account for carbon's behavior.

int kEventWindowActivated (int nextHandler, int theEvent, int userData) {
	int result = super.kEventWindowActivated (nextHandler, theEvent, userData);
	if (result == OS.noErr) return result;
	/*
	* Bug in the Macintosh.  Despite the that a window has scope
	* kWindowActivationScopeNone, it gets kEventWindowActivated
	* events but does not get kEventWindowDeactivated events.  The
	* fix is to ignore kEventWindowActivated events.
	*/
	int [] outScope = new int [1];
	OS.GetWindowActivationScope (shellHandle, outScope); 
	if (outScope [0] == OS.kWindowActivationScopeNone) return result;
	if (!active) {
		Shell[] shells = display.getShells ();
		for (int i = 0; i < shells.length; i++) {
			Shell shell = shells [i];
			if (shell.active && !shell.isDisposed ()) shell.kEventWindowDeactivated ();
		}
...

Comment 5 Scott Kovatch 2011-02-02 18:56:06 EST

This turns out to be a regression caused by the change for 316238. I'll reopen this but it may be too late for 3.6.2.

Comment 6 Scott Kovatch 2011-02-02 19:08:22 EST

(In reply to comment #4)
>         Shell[] shells = display.getShells ();
>         for (int i = 0; i < shells.length; i++) {
>             Shell shell = shells [i];
>             if (shell.active && !shell.isDisposed ())
> shell.kEventWindowDeactivated ();
>         }
> ...

This part of the fix for 316238 can be removed without breaking the fix. It's not clear what this change does for the original fix.

Comment 7 Scott Kovatch 2011-02-02 19:13:45 EST

I'll have to take this, since Silenio is away until Feb. 21.

This will require a pretty high bar for inclusion in 3.6.2.

Comment 8 Scott Kovatch 2011-02-02 19:14:15 EST

Created attachment 188204 [details]
Patch for 3.6.2

Comment 9 Scott Kovatch 2011-02-02 19:16:07 EST

I'm requesting reviews and PMC approval, but be forewarned we are building 3.6.2 tonight.

Comment 10 Lakshmi P Shanmugam 2011-02-03 04:22:29 EST

(In reply to comment #6)
> (In reply to comment #4)
> >         Shell[] shells = display.getShells ();
> >         for (int i = 0; i < shells.length; i++) {
> >             Shell shell = shells [i];
> >             if (shell.active && !shell.isDisposed ())
> > shell.kEventWindowDeactivated ();
> >         }
> > ...
> 
> This part of the fix for 316238 can be removed without breaking the fix. It's
> not clear what this change does for the original fix.

Hi Scott,
Display.appleEventProc() too has the code changes to call kEventWindowDeactivated() on shells. But this OS.kEventAppDeactivated case gets called only when the application is deactivated. 
Hence, I think, the above code in Shell.kEventWindowActivated() that calls kEventWindowDeactivated() on shells is required when the windows in the application are deactivated but the application itself is not.

With the patch, I cannot see the original main menu lost problem problem with eclipse and 'open new window'. But when I  minimize and maximize the windows in a simple SWT application without switching to a different application, the main menu is lost.

Comment 11 Lakshmi P Shanmugam 2011-02-03 04:38:46 EST

Here is the simple snippet I used for testing:
1) Open main window and sub window.
2) Minimize sub window and then minimize main window.
3) click on sub window.
4) click on main window. The main window's menu doesn't appear.
The Deactivate event for main window is not sent at step2 or step3 (because Shell.kEventWindowDeactivated() is not called for the main window)
With the original fix, Deactivate event is sent at step3 and main window's menu appears in step4.

public class Bug_316238 {
	public static void main(String[] args) {
		final Display display = new Display();
		final Shell shell = new Shell(display);
		shell.setLayout(new GridLayout(1, false));
		Button button = new Button(shell, SWT.PUSH);
		button.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false,false));
		button.setText("open new window");
		button.addSelectionListener(new SelectionAdapter() {
			public void widgetSelected(SelectionEvent e) {
				Shell shell2 = new Shell(display, SWT.SHELL_TRIM);
				shell2.setLayout(new GridLayout(1, false));
				shell2.setText("sub window");
				shell2.setSize(100,100);
				shell2.open();
			}
		});
		display.addFilter(SWT.Activate, new Listener() {
			public void handleEvent(Event event) {
				if (event.widget instanceof Shell) System.out.println("window activated" + event.widget);
			}
		});
		display.addFilter(SWT.Deactivate, new Listener() {
			public void handleEvent(Event event) {
				if (event.widget instanceof Shell) System.out.println("window deactivated" + event.widget);
			}
		});
		Menu bar = new Menu(shell, SWT.BAR);
		MenuItem item = new MenuItem(bar, SWT.PUSH);
		item.setText("my menu");
		shell.setMenuBar(bar);
		shell.setText("main window");
		shell.pack();
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}
}

Comment 12 Scott Kovatch 2011-02-03 17:58:46 EST

Created attachment 188282 [details]
New patch

New patch -- avoid the back and forth of activate/deactivate by not reporting deactivate while handling activation.

Comment 13 Scott Kovatch 2011-02-03 18:04:57 EST

Lakshmi, give this a run and see what you think. It doesn't affect the original fix but instead tries to work around what seems to be a Carbon bug with Exposé and kWindowModalityWindowModal.

Comment 14 Lakshmi P Shanmugam 2011-02-04 04:52:33 EST

(In reply to comment #13)
> Lakshmi, give this a run and see what you think. It doesn't affect the original
> fix but instead tries to work around what seems to be a Carbon bug with Exposé
> and kWindowModalityWindowModal.

Hi Scott,
I tested the patch and it works well.

Comment 15 Bogdan Gheorghe 2011-02-04 10:19:06 EST

Patch released to 3.6.2 > 20110204

Comment 16 Scott Kovatch 2011-02-04 13:03:10 EST

*** Bug 335613 has been marked as a duplicate of this bug. ***

Comment 17 Markus Keller 2011-02-07 05:08:40 EST

Looks good in M20110204-1030. Filed bug 336485 for a different issue with this scenario on Cocoa.

Don't forget to release this fix to HEAD as well.

Comment 18 Scott Kovatch 2011-02-07 11:49:36 EST

Right. We wanted to make sure it was okay in 3.6.2 before committing to HEAD.

Fixed in HEAD > 20110207.

Comment 19 Dani Megert 2011-02-07 11:59:12 EST

(In reply to comment #18)
> Right. We wanted to make sure it was okay in 3.6.2 before committing to HEAD.
I understand that process given that 3.6.2 is close but want to mention that normally it should be done the other way around i.e. put it into HEAD, test that it works fine and then decide to backport.