#450/Bug 578929 - Control.setFocus brings windows to front #553

tmssngr · 2023-02-16T10:55:04Z

If the application is in the background and the GUI is updated, especially a different control is getting focused, it might happen that the shell will be brought to front. This patch should fix that.

To get the previous behavior (activate the shell on control.forceFocus(), set the system property
"org.eclipse.swt.internal.activateShellOnForceFocus" to "true").

If you want an application window be brought to front, instead of relying on control.forceFocus() to bring the shell to front, use shell.forceActive().

Shell.open: an explicit bringToTop() is needed now, because the implicit one from setFocus/forceFocus() is not happening any more. For the first shell it needs to be forced, because otherwise it won't do anything (display.activeShell is null).

lshanmug · 2023-03-12T18:55:33Z

@elsazac @deepika-u Can you please verify the PR?

github-actions · 2023-03-18T12:23:29Z

Test Results

    299 files ±0     299 suites ±0 6m 45s ⏱️ +55s
  4 090 tests +1   4 082 ✔️ +1   8 💤 ±0 0 ❌ ±0
12 182 runs +3 12 111 ✔️ +3 71 💤 ±0 0 ❌ ±0

Results for commit 9e43c26. ± Comparison against base commit 37c187a.

♻️ This comment has been updated with latest results.

deepika-u · 2023-03-20T13:32:47Z

@tmssngr
Attempted with FocusToFrontTest.java snippet.

As per my understanding, expected behavior is :
3 windows are being opened and asked to load something. 3rd window being the last once finished loading needs to continue to be focused. (1st and 2nd windows since started loading in the background should never get focused (irrespective of finishing) till they are exclusively focused by user/invoked programatically.)

Before the fix :
3 windows are opened and asked to load something in the sequence(window 1 followed by window 2 followed by window 3).
Window 3 is on focus right now.
Then from window 3 focus goes to window 1 as it finished loading.
Then window 2 gets the focus as it finished loading.
Then window 3 gets the focus as it finished lastly and remains focused.

After the fix :
It is working as expected and focus continues to remain on window 3(though it finishes loading after window 1 and window 2).

This fix looks fine for me on windows 11. Please correct/add if my understanding is fine.

elsazac · 2023-03-20T13:35:09Z

@tmssngr
Attempted with FocusToFrontTest.java snippet on MacOS aarch 64.

As per my understanding, Expected Behavior is:
Window 1 shows up and starts loading.
Window 2 shows up and starts loading.
Window 3 shows up and starts loading.
Now focus remains on window 3, window 1 finished loading in the background, then window 2 finished loading in the background, window 3 finished loading on the foreground and focus continues to remain on window 3.

Without PR :
Window 1 and Window 2 doesn’t show up and hence looks like loading/not loading.
Window 3 shows up and starts loading.
Then window 1 finished loading and shows up focused.
Then window 2 finished loading and shows up focused.
Then window 3 finished loading and shows up focused.
Now focus remains on window 3.

With PR:
only the window 3 shows up loading and finished loading. After that , only if we perform an external action (for eg minimise the window 3 or close the the window 3)we will see window 2. When window 2 is minimised or closed window 1 shows up.

The behaviour in Mac is totally different from that in windows.
Irrespective of the fix, window 1 and window 2 are not showing up. Looks like this is another issue.
Focus is not shifting from window 3( irrespective of window 1, window 2 loading) and continues to remain on window 3.So this fix looks good to me.

tmssngr · 2023-03-22T14:25:19Z

To what FocusToFrontTest.java you are referring?

elsazac · 2023-03-22T14:30:31Z

The first snippet that was provided along with the issue.

tmssngr · 2023-03-30T07:38:18Z

Sorry for the delay. On my Mac mini M1 (macOS 13.2.1) the FocusToFrontTest test behaves the same as on all other platforms. Without patch, 3 windows open that get activated for each "loading finished". With patch the windows open but the top most shell remains the active one independent of the others.

elsazac · 2023-03-30T12:54:20Z

This is the behaviour without patch.

When I run this snippet window 1 and 2 doesn't come up.Window 3 comes up and starts loading.Then window 1 finishes loading and comes to the front followed by window 2 and window 3.Focus remains on window 3.

This is the behaviour with patch

Only window 3 opens up and finishes loading.If I close or minimise the window 3 only then I could see window 2.If I close or minimise window 2 , only then window 1 shows up. Else I cannot see window 1 or window 2 are open or not.

In both the cases opening of window 1 and window 2 doesn't show up at all.Only after loading finished window 1 and window 2 shows up without patch .These 2 windows (window 1 and window 2 before loading)show up on windows but not on Mac.

tmssngr · 2023-03-30T14:52:23Z

For me it looks before patch:

and with patch:

tmssngr · 2023-03-30T19:23:34Z

Are these failing builds caused by my pull request?

tmssngr · 2023-04-04T11:11:09Z

What else is needed from my side to merge this PR?

iloveeclipse · 2023-04-04T16:15:24Z

What else is needed from my side to merge this PR?

A positive review from @SyntevoAlex?

deepika-u · 2023-04-06T08:44:05Z

For me it looks before patch: and with patch:

My vote goes for the fix.

SyntevoAlex

I support the idea of this change, but I feel uneasy about potential regressions. I guess the only way to move forward here is trial&error - merge, see what happens, revert/fix/reapply as needed.

I would really like to see a set of UnitTests here (one per each fixed problem and discovered regression). Because

there was a number of regressions
multiple platforms are affected
the change has potentially far-reaching consequences
I find it likely that there will be some future tweaks to this logic

In your testing snippet, there are 1000ms pauses. Try to fix that when converting to unit test.
Maybe you will need to add SwtTestUtil.processEvents() somewhere instead of those sleeps.

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Shell.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java

SyntevoAlex · 2023-05-21T22:52:52Z

I would still very much like to see unit tests here.
Otherwise, it would be too hard to continue iterating on it if there's yet another regression. Also, due to how non-obvious things are (especially how other code relies on old behavior) I fear that things will get broken later.

tmssngr · 2023-05-25T13:07:52Z

Please look at this unit test. It reliably produces an NPE which might be caused by the disposing of the display inside the shell's dispose listener. I'm quite sure this NPE is independent of the patch.

…front If the application is in the background and the GUI is updated, especially a different control is getting focused, it might happen that the shell will be brought to front. This patch should fix that. To get the previous behavior (activate the shell on control.forceFocus(), set the system property "org.eclipse.swt.internal.activateShellOnForceFocus" to "true"). If you want an application window be brought to front, instead of relying on control.forceFocus() to bring the shell to front, use shell.setActive(). Shell.open: an explicit bringToTop() is needed now, because the implicit one from setFocus/forceFocus() is not happening any more. It needs to be forced, because otherwise it won't do anything (display.activeShell is null). If a shell is activated, restore the focus which has been set when it was inactive.

SyntevoAlex · 2023-07-02T18:51:50Z

Let's give it a try. In master now.

@iloveeclipse @lshanmug this PR is kind of breaking change. When app sets focus to a control and relies (incorrectly) that Shell will also become active, this will no longer be the case. Explicit Shell.setActive() will need to be added. I guess I'll have to ... uh, "volunteer"... to fix these issues.

elsazac · 2023-07-04T06:09:48Z

@SyntevoAlex

There is a test failure in the build pertaining to this PR.
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Shell

tmssngr · 2023-07-04T08:18:02Z

@elsazac Where can the stacktrace be found?

elsazac · 2023-07-04T08:21:32Z

For x86
https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.swt.tests_ep429I-unit-mac64-java17_macosx.cocoa.x86_64_17.html

for arch 64
https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.swt.tests_ep429I-unit-macM1-java17_macosx.cocoa.aarch64_17.html

iloveeclipse · 2023-07-04T08:21:55Z

@elsazac Where can the stacktrace be found?

See latest nightly build root page:
https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testResults.php

Go from there to Mac swt results

java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Shell.test_Issue450_NoShellActivateOnSetFocus(Test_org_eclipse_swt_widgets_Shell.java:962)

Please also check the windows fail in a different test (which I haven't seen failing before):

https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.swt.tests_ep429I-unit-win32-java17_win32.win32.x86_64_17.html

expected:<01[]567890> but was:<01[234]567890>

org.junit.ComparisonFailure: expected:<01[]567890> but was:<01[234]567890>
at org.junit.Assert.assertEquals(Assert.java:117)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Text.test_cut(Test_org_eclipse_swt_widgets_Text.java:374)

iloveeclipse · 2023-07-04T08:24:56Z

Note, there is also now a fail in org.eclipse.jface.tests.databinding

https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.jface.tests.databinding_ep429I-unit-win32-java17_win32.win32.x86_64_17.html

java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at org.eclipse.jface.tests.internal.databinding.swt.ControlObservableValueTest.testObserveFocus(ControlObservableValueTest.java:223)

that seem be directly related to this PR.

deepika-u · 2023-07-04T08:34:02Z

Is anyone aware of the cause of the windows failure? seeing for the first time. Else please let me know if i might have to open new issue for it if it is unrelated.

https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.swt.tests_ep429I-unit-win32-java17_win32.win32.x86_64_17.html

tmssngr · 2023-07-04T11:36:46Z

For x86 https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.swt.tests_ep429I-unit-mac64-java17_macosx.cocoa.x86_64_17.html

for arch 64 https://download.eclipse.org/eclipse/downloads/drops4/I20230703-1800/testresults/html/org.eclipse.swt.tests_ep429I-unit-macM1-java17_macosx.cocoa.aarch64_17.html

I wonder what is different for this test machine than for the one which ran yesterday successfully for this (unmerged) pull request.

deepika-u · 2023-07-04T14:57:01Z

Attempted with FocusToFrontTest.java snippet.

Expected behavior is :
3 windows are being opened and asked to load something. 3rd window being the last once finished loading needs to continue to be focused. (1st and 2nd windows since started loading in the background should never get focused (irrespective of finishing) till they are exclusively focused by user/invoked programatically.)

This is how it is exactly working for me even.

I have verified on below environment
Eclipse SDK
Version: 2023-09 (4.29)
Build id: I20230703-1800
OS: Windows 11, v.10.0, x86_64 / win32
Java vendor: Eclipse Adoptium
Java runtime version: 20+36
Java version: 20

SyntevoAlex · 2023-07-04T16:17:47Z

Going to investigate test failures now.

SyntevoAlex · 2023-07-04T16:18:49Z

Attempted with FocusToFrontTest.java snippet.

Expected behavior is : 3 windows are being opened and asked to load something. 3rd window being the last once finished loading needs to continue to be focused. (1st and 2nd windows since started loading in the background should never get focused (irrespective of finishing) till they are exclusively focused by user/invoked programatically.)

This is how it is exactly working for me even.

@deepika-u sorry failed to parse. Do you mean that it's good after the patch?

SyntevoAlex · 2023-07-04T17:27:25Z

test_Issue450_NoShellActivateOnSetFocus failures on macOS will hopefully be fixed by #725

SyntevoAlex · 2023-07-04T21:47:26Z

Test_org_eclipse_swt_widgets_Text.test_cut

Fails for a long time, unrelated to this Issue, see #106

SyntevoAlex · 2023-07-04T22:57:21Z

org.eclipse.jface.tests.databinding

The problem is in test only.
It can be seen how it struggles to activate Shell:

https://github.com/eclipse-platform/eclipse.platform.ui/blob/f0ce6e75ebab0142f7ff9f13afda67554e0f3818/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/jface/tests/internal/databinding/swt/ControlObservableValueTest.java#L217-L221

And here (old logs before patch) it can be seen that this never worked on Windows:
https://download.eclipse.org/eclipse/downloads/drops4/I20230612-1800/testresults/ep429I-unit-win32-java17_win32.win32.x86_64_17/org.eclipse.jface.tests.databinding.BindingTestSuite.txt

ControlObservableValueTest.testObserveFocus() start active shell: null
active shell (2): null
active shell (3): null

So the test was always kind of broken.

I think that test needs to wait for SWT.Activate properly, like we did here:
https://github.com/syntevo/eclipse.platform.swt/blob/9e43c2659d35e66a350260999e25b432c1344f50/tests/org.eclipse.swt.tests/JUnit%20Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java#L942

deepika-u · 2023-07-05T06:10:57Z

Attempted with FocusToFrontTest.java snippet.
Expected behavior is : 3 windows are being opened and asked to load something. 3rd window being the last once finished loading needs to continue to be focused. (1st and 2nd windows since started loading in the background should never get focused (irrespective of finishing) till they are exclusively focused by user/invoked programatically.)
This is how it is exactly working for me even.

@deepika-u sorry failed to parse. Do you mean that it's good after the patch?

From the past iteration of testing, i have understood that expected behavior is like that. So it is working as per that after the fix is delivered into master(leaving the junit failures aside).

iloveeclipse · 2023-07-05T07:20:27Z

test_Issue450_NoShellActivateOnSetFocus failures on macOS will hopefully be fixed by #725

Unfortunately not:

Shell did not become active

java.lang.AssertionError: Shell did not become active
at org.junit.Assert.fail(Assert.java:89)
at org.eclipse.swt.tests.junit.SwtTestUtil.waitShellActivate(SwtTestUtil.java:418)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Shell.test_Issue450_NoShellActivateOnSetFocus(Test_org_eclipse_swt_widgets_Shell.java:961)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

iloveeclipse · 2023-07-05T07:24:12Z

I've created dedicated issue for the fail, we don't need to abuse this PR anymore: #727

iloveeclipse · 2023-07-05T07:53:37Z

org.eclipse.jface.tests.databinding
The problem is in test only. It can be seen how it struggles to activate Shell:

Thanks for analysis

So the test was always kind of broken.

But now it consistently fail, which is not nice... Anyway, moving to dedicated issue: eclipse-platform/eclipse.platform.ui#910

Due to the revert of eclipse-platform/eclipse.platform.swt/pull/553, the utilities added in eclipse-platform#933 became obsolete. The utilities are code duplications from the SWT repository, which are faulty and have already been updated in the SWT repository. This change thus reverts the addition of the duplicated and now unnecessary utilities. It reverts commit 4194b8f from PR eclipse-platform#933 for issue eclipse-platform#910.

Due to the revert of eclipse-platform/eclipse.platform.swt/pull/553, the utilities added in #933 became obsolete. The utilities are code duplications from the SWT repository, which are faulty and have already been updated in the SWT repository. This change thus reverts the addition of the duplicated and now unnecessary utilities. It reverts commit 4194b8f from PR #933 for issue #910.

Due to the revert of eclipse-platform/eclipse.platform.swt/pull/553, the utilities added in eclipse-platform#933 became obsolete. The utilities are code duplications from the SWT repository, which are faulty and have already been updated in the SWT repository. This change thus reverts the addition of the duplicated and now unnecessary utilities. It reverts commit 4194b8f from PR eclipse-platform#933 for issue eclipse-platform#910.

tmssngr requested review from lshanmug, niraj-modi, iloveeclipse, sravanlakkimsetti and akurtakov as code owners February 16, 2023 10:55

tmssngr mentioned this pull request Feb 16, 2023

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

Closed

tmssngr force-pushed the feature/450-prevent-shell-activation-on-forcefocus branch 4 times, most recently from 393c42d to 4c4b239 Compare February 22, 2023 10:43

laeubi force-pushed the feature/450-prevent-shell-activation-on-forcefocus branch from 4c4b239 to 25efc17 Compare March 18, 2023 12:05

tmssngr force-pushed the feature/450-prevent-shell-activation-on-forcefocus branch from 25efc17 to 7aa5920 Compare March 30, 2023 07:38

SyntevoAlex reviewed Apr 29, 2023

View reviewed changes

tmssngr force-pushed the feature/450-prevent-shell-activation-on-forcefocus branch from 7aa5920 to 1c20ec8 Compare May 1, 2023 08:50

tmssngr force-pushed the feature/450-prevent-shell-activation-on-forcefocus branch from 1c20ec8 to a29ee88 Compare May 25, 2023 12:58

tmssngr force-pushed the feature/450-prevent-shell-activation-on-forcefocus branch from 1895a35 to 9e43c26 Compare July 2, 2023 18:30

SyntevoAlex merged commit 960a322 into eclipse-platform:master Jul 2, 2023

iloveeclipse mentioned this pull request Jul 5, 2023

Shell.test_Issue450_NoShellActivateOnSetFocus fails on Mac #727

Closed

iloveeclipse mentioned this pull request Jul 5, 2023

ControlObservableValueTest.testObserveFocus fails on Windows eclipse-platform/eclipse.platform.ui#910

Closed

iloveeclipse mentioned this pull request Jul 11, 2023

Editor Tabs: chevron menu not closing anymore eclipse-platform/eclipse.platform.ui#929

Closed

HeikoKlare mentioned this pull request Jul 20, 2023

Remove obsolete test utilities duplicates #933 eclipse-platform/eclipse.platform.ui#950

Merged

tmssngr deleted the feature/450-prevent-shell-activation-on-forcefocus branch October 7, 2023 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#450/Bug 578929 - Control.setFocus brings windows to front #553

#450/Bug 578929 - Control.setFocus brings windows to front #553

tmssngr commented Feb 16, 2023

lshanmug commented Mar 12, 2023

github-actions bot commented Mar 18, 2023 •

edited

Loading

deepika-u commented Mar 20, 2023

elsazac commented Mar 20, 2023

tmssngr commented Mar 22, 2023

elsazac commented Mar 22, 2023

tmssngr commented Mar 30, 2023

elsazac commented Mar 30, 2023

tmssngr commented Mar 30, 2023

tmssngr commented Mar 30, 2023

tmssngr commented Apr 4, 2023

iloveeclipse commented Apr 4, 2023

deepika-u commented Apr 6, 2023

SyntevoAlex left a comment

SyntevoAlex commented May 21, 2023

tmssngr commented May 25, 2023

SyntevoAlex commented Jul 2, 2023 •

edited

Loading

elsazac commented Jul 4, 2023

tmssngr commented Jul 4, 2023

elsazac commented Jul 4, 2023

iloveeclipse commented Jul 4, 2023

iloveeclipse commented Jul 4, 2023

deepika-u commented Jul 4, 2023

tmssngr commented Jul 4, 2023

deepika-u commented Jul 4, 2023 •

edited

Loading

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

deepika-u commented Jul 5, 2023

iloveeclipse commented Jul 5, 2023

iloveeclipse commented Jul 5, 2023

iloveeclipse commented Jul 5, 2023

#450/Bug 578929 - Control.setFocus brings windows to front #553

#450/Bug 578929 - Control.setFocus brings windows to front #553

Conversation

tmssngr commented Feb 16, 2023

lshanmug commented Mar 12, 2023

github-actions bot commented Mar 18, 2023 • edited Loading

Test Results

deepika-u commented Mar 20, 2023

elsazac commented Mar 20, 2023

tmssngr commented Mar 22, 2023

elsazac commented Mar 22, 2023

tmssngr commented Mar 30, 2023

elsazac commented Mar 30, 2023

tmssngr commented Mar 30, 2023

tmssngr commented Mar 30, 2023

tmssngr commented Apr 4, 2023

iloveeclipse commented Apr 4, 2023

deepika-u commented Apr 6, 2023

SyntevoAlex left a comment

Choose a reason for hiding this comment

SyntevoAlex commented May 21, 2023

tmssngr commented May 25, 2023

SyntevoAlex commented Jul 2, 2023 • edited Loading

elsazac commented Jul 4, 2023

tmssngr commented Jul 4, 2023

elsazac commented Jul 4, 2023

iloveeclipse commented Jul 4, 2023

iloveeclipse commented Jul 4, 2023

deepika-u commented Jul 4, 2023

tmssngr commented Jul 4, 2023

deepika-u commented Jul 4, 2023 • edited Loading

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

SyntevoAlex commented Jul 4, 2023

deepika-u commented Jul 5, 2023

iloveeclipse commented Jul 5, 2023

iloveeclipse commented Jul 5, 2023

iloveeclipse commented Jul 5, 2023

github-actions bot commented Mar 18, 2023 •

edited

Loading

SyntevoAlex commented Jul 2, 2023 •

edited

Loading

deepika-u commented Jul 4, 2023 •

edited

Loading