Control.setFocus must not bring the application to front if it is in the background

[tmssngr]

Please run the attached snippet. It opens 3 shells, simulates some loading and then wants to set the focused control for each of the shells. Unfortunately, (sometimes) it brings the shell to front, even if it was in the background. If I would want to bring a shell to front, I would invoke shell.setActive(), but I just want to change the focuses control inside a shell.

import org.eclipse.swt.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class FocusToFrontTest {

	public static void main(String[] args) {
		final FocusToFrontTest test = new FocusToFrontTest();
		test.openShell();
		test.openShell();
		test.openShell();
		test.show();
	}

	private final Display display = new Display();

	private int count;

	private FocusToFrontTest() {
	}

	private void openShell() {
		count++;
		final Shell shell = new Shell(display);
		final int number = count;
		shell.setText("Window " + number);
		shell.setLayout(new FillLayout());
		shell.addListener(SWT.Dispose, event -> count--);
		shell.addListener(SWT.Activate, event -> System.out.println("Activated " + number));
		shell.setSize(400, 300);

		final Text text1 = new Text(shell, SWT.BORDER | SWT.MULTI);
		text1.setText("Loading...");

		final Text text2 = new Text(shell, SWT.BORDER | SWT.MULTI | SWT.WRAP);

		shell.open();

		display.timerExec(1_000 * number, () -> {
			if (shell.isDisposed()) {
				return;
			}

			text1.setText("");
			text2.setText("Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.");
			text2.setFocus();
		});
	}

	private void show() {
		while (count > 0) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}

		display.dispose();
	}
}

This ticket is related to https://bugs.eclipse.org/bugs/show_bug.cgi?id=578929, but instead of adding new API it should fix the existing one.

[tmssngr]

Hi Alexander, do you need further input from me to accept this fix?

[akurtakov]

Honestly, it just got lost in the pile of other issues/prs.

[akurtakov]

Should be fixed via #451

[iloveeclipse]

Caused regression, see #502

[iloveeclipse]

I see a bigger regression now, that would break different clients.

Ctrl+Shift+T + start typing doesn't work anymore: the opened dialog does not have focus set in the text field.
The problem is that dialog is created & focus is set before the shell has been opened / focused!

We should revert this commit until we have a plan how to fix that.

[sratz]

We also have almost a hundred of assorted unit tests of our product failing after this change.

I suppose it would be better to add a 2nd API method (or parameterize setFocus(...)) to give consumers a choice, but avoid changing existing behavior, as originally proposed in the bugzilla.

[iloveeclipse]

I also see it in Egit "create branch" dialog etc - in all places where a new shell is opened and something should have a focus...

[mickaelistria]

I suppose it would be better to add a 2nd API method (or parameterize setFocus(...)) to give consumers a choice, but avoid changing existing behavior, as originally proposed in the bugzilla.

I don't think so; giving focus in 1 app is a different from pushing an application to front.
And I suspect your tests wouldn't be failing even if setFocus() behaves in a way that doesn't bring control to front; they're more likely failing for the reason described by @iloveeclipse which seems to be a bug in the current proposed implementation; but when the implementation doesn't have such bugs, there wouldn't be such issue and no such need for a different setFocus() method.

@iloveeclipse thanks for identifying that case. Do you think it can be easily turned into a test case in SWT code base? If so, it would be nice to add it now to not take risk of regressing the same way again with other attempts at fixing this issue.

[iloveeclipse]

I believe automated test could work, but not sure because handling focus is the most sensible part of UI testing. I'm pretty sure it will be unstable and/or very platform specific.

Beside this, I've prepared PR #503 with revert.
As said, if @tmssngr doesn't find a solution, I would like to merge it this evening, so next SDK build is usable again.

[iloveeclipse]

Other manifestation of same issue: switching from one shell by clicking on a link in the first shell to another opened shell isn't working anymore, because the "other one" isn't forcing the focus anymore.

Open editor in IDE, show revision information, hover over left ruler to see some commit: shows a shell with a diff -> show revision information -> opens an editor in background and does NOT hides the shell / shows the editor as it was before.

[tmssngr]

On what operating system you reproduce the problems? Could you post a tiny snippet where I can reproduce it easily in SWT (not Eclipse)?

[iloveeclipse]

RHEL 7.9 GTK 3.22.
No, no time. Please try to create shell from other shell and focus widget in the new one before opening. That would not work because new shell is not active yet.

[tmssngr]

I've tried to reproduce with this snippet, but it works fine on latest Manjaro/Gnome:

import org.eclipse.swt.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class DialogFocusTest {

	public static void main(String[] args) {
		final Display display = new Display();

		final Shell mainShell = new Shell(display);
		mainShell.setLayout(new GridLayout(1, false));

		final Button button = new Button(mainShell, SWT.PUSH);
		button.setText("Open Dialog");
		button.addListener(SWT.Selection, event -> openDialog(mainShell));
		button.setLayoutData(new GridData(GridData.BEGINNING, GridData.BEGINNING, false, false));

		mainShell.setSize(500, 400);
		mainShell.open();

		while (!mainShell.isDisposed()) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}

		display.dispose();
	}

	private static void openDialog(Shell parent) {
		final Shell shell = new Shell(parent, SWT.DIALOG_TRIM | SWT.PRIMARY_MODAL);
		shell.setLayout(new GridLayout(1, false));

		createText(shell, "First");
		createText(shell, "Second").setFocus();

		shell.setSize(500, 300);
		shell.open();
	}

	private static Control createText(Composite parent, String message) {
		final Text text = new Text(parent, SWT.SINGLE | SWT.BORDER);
		text.setMessage(message);
		text.setLayoutData(new GridData(GridData.FILL, GridData.BEGINNING, true, false));
		return text;
	}
}

[akurtakov]

Do you run X11 or Wayland? Behavior on both differ significantly.

[iloveeclipse]

Snippet works for me with the patch (on X11). So you have to dig into IDE code to find where the difference to the plain SWT is.
May be the flags used to create shells.

[tmssngr]

I'd appreciate it if someone could share a snippet that reproduces the problem with the undone patch (commit 7d28a78).

[tmssngr]

Ping. I need a snippet to reproduce the problems with my pull request. Otherwise I can't fix it.

[iloveeclipse]

Ping. I need a snippet to reproduce the problems with my pull request. Otherwise I can't fix it.

Thomas, don't get me wrong, but you have to create that snippet by yourself.
Unfortunately my time is limited and I can't spend time on issues that are not relevant for me personally or my job.

Grab latest SDK. Debug that regression from Eclipse - use cases are plenty. Find out which JFace / IDE component is involved in all that mess. Try to reduce to plain SWT.

[tmssngr]

At least it would be helpful to have exact steps where exactly the problem was reproducible as a user of Eclipse. Please note that we are not used to Eclipse.

Other manifestation of same issue: switching from one shell by clicking on a link in the first shell to another opened shell isn't working anymore, because the "other one" isn't forcing the focus anymore.

Is a little bit too unspecific to me, because I don't know where to find such a link that opens another shell.

[merks]

Setting up an IDE is completely automated. Just drop this link on the installer or read the instructions for how to do that:

https://www.eclipse.org/setups/installer/?url=https://raw.githubusercontent.com/eclipse-platform/eclipse.platform.releng.aggregator/master/oomph/PlatformSDKConfiguration.setup&show=true

[tmssngr]

Thanks, @jukzi, for your comments. I think I have addressed most of them in the latest above commit (not yet sent a pull-request for the API changes).

[tmssngr]

OK, there is now new API setFocus(boolean) and forceFocus(boolean). Existing code, even in SWT, continues using setFocus() and forceFocus() and the challenging part would now be to use the new API. For some invocations, e.g. in CTabFolder.onKeyDown or StyledText.handleMouseDown I reckon it would be safe to use false as activateShell parameter. But what to use in Control.setTabItemFocus() or Control.fixFocus()? I would avoid adding a new activateShell parameter to all those methods to tunnel this information through. Maybe the new API idea does not work good in this situation?

[jukzi]

maybe just "never touch a running system" and only use new api where the old did not work

[ingomohr]

Is changing the behavior of old and existing API really a good idea?

I was just pointed to this issue by the N&N for 4.29M1. As far as I understand from that comment in the N&N the behavior of the existing API has changed w/ this issue - and that the behavior w/ the shell popping up had been there for quite a while.

Is it really a good idea to change the behavior of existing API when the old behavior was no recent bug?
Imho, this can cause regression problems with any SWT-based UI and, thus, increase the costs to migrate to a newer Eclipse version.

We imho really shouldn't want that. Not for SWT and not for any other API. API may have problems in what it documents vs what it does. But modifying it years later instead of just adding new API to fix the problems - in my opinion - drives client applications away from using those APIs, at all.

[tmssngr]

@ingomohr According to my knowledge @mickaelistria was so kind to merge my pull request as commit bc9c59c to change the default to the legacy (old) behavior. So code which relies on this incorrect old behavior will not break.

[mickaelistria]

@ingomohr I think the code is fine now; however, the N&N must probably be modified to mention that the default behavior hasn't changed, but that the "feature flag" can be set to enable it.

[ingomohr]

Thanks for the update, @mickaelistria !

[merks]

The new and noteworthy is wrong at right now:

I believe the flag org.eclipse.swt.internal.activateShellOnForceFocus effectively defaults to false:

So I will update the description accordingly. OK?

[merks]

Hmmm, I thought this was changed for this not to be the default, but I think I'm misreading the code. Or?

[syntevo]

Yes, the news and noteworthy is incorrect now as the default has been changed to the old (legacy) behavior.

[merks]

Yes, I looked again and of course the "true" argument to getProperty determines the default. I will reference this issue in the PR to update the N&N.