Community
Participate
Working Groups
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, 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.
Created attachment 288110 [details] Snippet to reproduce
Created attachment 288111 [details] Snippet to reproduce
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/191138
IIRC on Windows the forceFocus does not work, see Bug 192036. Is this already fixed?
OS.SetFocus will activate the receiving window when it is attached to the thread. So, currently, when Control.setFocus is called, the containing shell is activated. The Control.setFocus call should not be calling OS.SetFocus when the shell is not active. i.e. Control.isActive() should be checking if the shell is the active shell on the current thread, not only checking if there is a modal shell shown. That is, a check like 'if (display.getActiveShell() != shell) return false' should be added in Control.isActive(). https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setfocus
(In reply to Rolf Theunissen from comment #5) > OS.SetFocus will activate the receiving window when it is attached to the > thread. > So, currently, when Control.setFocus is called, the containing shell is > activated. > > The Control.setFocus call should not be calling OS.SetFocus when the shell > is not active. i.e. Control.isActive() should be checking if the shell is > the active shell on the current thread, not only checking if there is a > modal shell shown. > > That is, a check like 'if (display.getActiveShell() != shell) return false' > should be added in Control.isActive(). > > > https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser- > setfocus Do you mean like in my patch?
(In reply to Thomas Singer from comment #6) > Do you mean like in my patch? I missed your patch before. But No. I think your patch is trying to add behavior to setFocus that is already implemented by Shell#forceActive. Moreover, I think that the behavior you observer is Win32 only, or are other platforms affected? e.g. I expect that setFocus on GTK does not raise the window. I would expect a one/two-liner change in Control#isActive() (line 1832) + if (display.getActiveShell() != shell) return false; Some cleanup w.r.t shell = getShell() calls.
(In reply to Rolf Theunissen from comment #7) > (In reply to Thomas Singer from comment #6) > > Do you mean like in my patch? > > I missed your patch before. > > But No. I think your patch is trying to add behavior to setFocus that is > already implemented by Shell#forceActive. Moreover, I think that the > behavior you observer is Win32 only, or are other platforms affected? e.g. I > expect that setFocus on GTK does not raise the window. This problem/behavior is reproducible on macOS/GTK, too. > I would expect a one/two-liner change in Control#isActive() (line 1832) > + if (display.getActiveShell() != shell) return false; > Some cleanup w.r.t shell = getShell() calls. That's what I've tried initially, but it caused problems selecting something by clicking with the mouse.
At least the behavior is consistent on all platforms. Can you update the "Hardware" section to reflect that this is an issue on all platforms. Then I understand you patch as backward compatibility is required.
*** Bug 563073 has been marked as a duplicate of this bug. ***
(In reply to Rolf Theunissen from comment #10) > *** Bug 563073 has been marked as a duplicate of this bug. *** From Bug 563073, on Windows the Shell is shown on top of all windows the first time setFocus is called, otherwise it is only shown on top of the windows of the same application.
Stale Bug 210410 seems related too, but that mouse hover focusing is not commonly used on Windows. How does GTK react on mouse hover focusing, i.e., does the window get focus without getting to the foreground?
(In reply to Rolf Theunissen from comment #12) > Stale Bug 210410 seems related too, but that mouse hover focusing is not > commonly used on Windows. > How does GTK react on mouse hover focusing, i.e., does the window get focus > without getting to the foreground? For GTK Stale Bug 413650 seems related.
Looking at Bug 209198 and Bug 277009, the behavior that the windows jumps to the front might not be the correct default. We could consider to change the default too, but I have no clue how many regressions that could introduce, where people assume different behavior.
My patch should not change the existing behavior, except of the addition of restoreFocus in GTK-Shell. Otherwise it only adds new overloaded setFocus/forceFocus methods that allow to not bring the shell to front.
(In reply to Thomas Singer from comment #15) > My patch should not change the existing behavior, except of the addition of > restoreFocus in GTK-Shell. Otherwise it only adds new overloaded > setFocus/forceFocus methods that allow to not bring the shell to front. Yes, that is true. But depends on what you like to accomplish here or what your ambition is. Your patch does only solve your current issue by adding additional API, i.e., increase complexity. While there is the opportunity to fix long standing usability issues (> 15 years) in the platform. Though, that comes with higher risk of course.
Please feel free to provide a different patch.
I think, the existing parameter-less setFocus/forceFocus methods are needed if backward compatibility is required. If that is the case, we need new API anyway to prevent the implicit bring-shell-to-front. The other bugs might need this new API to be resolved, so maybe I should rename the title to "Add new API for setFocus/forceFocus without bringing shell to front". What do you think about the suggested boolean parameter? Would you rather prefer separate methods, e.g. requestFocus() instead of setFocus(false)?
I have reconsidered too. We are talking about keyboard-focus here. So if you call 'setFocus' you expect that a control gets keyboard focus or not. For this it makes a lot of sense that the window made the active window too, which will raise it to the front (depending on the OS or window manager). What you like to do is set the focus control for a window, when it is active, set the focus to that control. However, when it is inactive, set focus to that control when the window becomes active, i.e., restoreFocus. I was also thinking to call this something like requestFocus, i.e., this control will get focus when the window is or becomes active. I don't know if the (public) API of forceFocus should be changed, i.e., if forceFocus return 'true' you expect that that control really has the keyboard focus. I would rename to something like: "Add new API for setFocus/forceFocus without activating the shell". Now it is implicit that activating the shell brings it to the front too. Another bug might want to change that behavior too (if someone feels the need for it) Note1: There can be three behaviors w.r.t. the Z-order: 1) do not change it, 2) bring on top for the application, 3) bring on top for all applications. Note2: besides setFocus and forceFocus, there is also setRadioFocus and setTabGroupFocus that might need updates.
We are now using our patch (https://github.com/eclipse-platform/eclipse.platform.swt/commit/8ffb9ef5602a6ebeaf00204c5977e27cdee28587) since a couple of months and the problem of SmartGit coming unintended to front when changing the focused control while in the background has been resolved. Feel free to do whatever you like with it. We are maintaining our own clone of the SWT repository at https://github.com/syntevo/eclipse.platform.swt to fix problems like this.
(In reply to Thomas Singer from comment #20) > We are now using our patch > (https://github.com/eclipse-platform/eclipse.platform.swt/commit/ > 8ffb9ef5602a6ebeaf00204c5977e27cdee28587) since a couple of months and the > problem of SmartGit coming unintended to front when changing the focused > control while in the background has been resolved. Feel free to do whatever > you like with it. We are maintaining our own clone of the SWT repository at > https://github.com/syntevo/eclipse.platform.swt to fix problems like this. Thomas, feel free to provide PR for this. Or is there any reason you won't this to be merged?
The last comments sounded like my fix would be rejected.
My pull request at https://github.com/eclipse-platform/eclipse.platform.swt/issues/450 waits since 2 weeks to be reviewed.
(In reply to Thomas Singer from comment #23) > My pull request at > https://github.com/eclipse-platform/eclipse.platform.swt/issues/450 waits > since 2 weeks to be reviewed. Can you ask Alexandr to have a look? IIRC you guys are working together. Marking as moved, as platform does not use bugzilla anymore, only Github issues and PR https://github.com/eclipse-platform/eclipse.platform.swt/issues/450
