Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BringWindowToTop to c.s.j.p.win32.User32 #1352

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

kahgoh
Copy link
Contributor

@kahgoh kahgoh commented Jun 4, 2021

@kahgoh kahgoh changed the title Add BringWindowToTop to User32 Add BringWindowToTop to c.s.j.p.win32.User32 Jun 4, 2021
@kahgoh kahgoh marked this pull request as ready for review June 4, 2021 14:18
@dbwiddis
Copy link
Contributor

dbwiddis commented Jun 4, 2021

Thanks for this contribution! The mapping looks good.

The one question I have revolves around the test case. You've added a test which always fails which isn't a bad thing, but there's no test that includes a successful execution of the function.

However, it's probably not a good idea to alter the Window Z order for a test. But perhaps we could find the topmost window already and execute this function on that handle. WindowUtils.getAllWindows() I believe returns windows in Z order; it's possible that you could insert this test in WindowUtilsTest() somewhere where we know we have the top window. Open to any other ideas/suggestions, even just adding to a demo somewhere to demonstrate that it works even if it's not part of CI.

@kahgoh
Copy link
Contributor Author

kahgoh commented Jun 5, 2021

Thanks for the feedback!

I had a look at WindowUtilsTest. I noticed that there were some tests that create their own JFrame for testing. So I took inspiration from that. The test now also creates its own 'JFrame' and brings that to the top.

@dbwiddis
Copy link
Contributor

dbwiddis commented Jun 5, 2021

LGTM. Will merge later today.

@dbwiddis dbwiddis merged commit 4032b5e into java-native-access:master Jun 5, 2021
CHANGES.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants