wxPopupWindow: prevent stealing foreground (PR #26937)
Bill Su
It seems wrong that displaying a popup window,
which is never a wxTopLevelWindow, changes the
foreground process.
Fix #26740
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26937
Commit Summary
- 1e57321 wxPopupWindow: prevent stealing foreground
File Changes
(1 file)
- M src/msw/popupwin.cpp (7)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26937.patch
- https://github.com/wxWidgets/wxWidgets/pull/26937.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- 9931666 wxPopupWindow: prevent stealing foreground
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Thanks, I'm fine with doing this but could you please simplify the code?
> @@ -170,7 +170,33 @@ bool wxPopupWindow::Show(bool show)
}
// and set it as the foreground window so the mouse can be captured
- ::SetForegroundWindow(GetHwnd());
+ // But if another application is in the foreground,
+ // don't take the foreground away from the other application.
+ static const auto GetForegroundProcessId = []() -> DWORD
Sorry, I don't understand why does this need to be so complex:
- Why do we need to return the foreground process PID instead of just returning true or false depending on whether this process is in the foreground?
- Why does this need to be a lambda at all instead of just plain code?
I think it should be either turned into some wxIsProcessInForeground() function (to be declared in wx/msw/private.h) or just be inlined here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -170,7 +170,33 @@ bool wxPopupWindow::Show(bool show)
}
// and set it as the foreground window so the mouse can be captured
- ::SetForegroundWindow(GetHwnd());
+ // But if another application is in the foreground,
+ // don't take the foreground away from the other application.
+ static const auto GetForegroundProcessId = []() -> DWORD
Sorry, I don't understand why does this need to be so complex:
- Why do we need to return the foreground process PID instead of just returning true or false depending on whether this process is in the foreground?
- Why does this need to be a lambda at all instead of just plain code?
I think it should be either turned into some
wxIsProcessInForeground()function (to be declared inwx/msw/private.h) or just be inlined here.
I realize the way I wrote this suggests Multiple Personality Disorder. On the one hand, I thought providing a general-purpose accessor to return the process ID of the foreground process might be useful. On the other hand, I wasn't sure there was any other use for the foreground process ID, so I wrote it as a lambda rather than making it a separate function. And then, I now think I exacerbated the MPD by putting logic that should have been specific to the wxPopupWindow::Show() into the theoretically general purpose implementation of the lambda.
My opinion (at this moment :-)) is that if this is declared in wx/msw/private.h, then it should be wxGetForegroundProcessID(), and return 0, not the current process ID if there is no foreground process ID.
Do you have a preference for whether this is inlined or declared in wx/msw/private.h?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
Bill Su
@wsu-cb pushed 1 commit.
- 8f8f8b3 fixup! Avoid initializing GTK if it has already been done
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -170,7 +170,33 @@ bool wxPopupWindow::Show(bool show)
}
// and set it as the foreground window so the mouse can be captured
- ::SetForegroundWindow(GetHwnd());
+ // But if another application is in the foreground,
+ // don't take the foreground away from the other application.
+ static const auto GetForegroundProcessId = []() -> DWORD
I rewrote the code to handle the foreground process ID inline.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.