Skip to content

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

Merged
dbwiddis merged 1 commit intojava-native-access:masterfrom
kahgoh:master
Jun 5, 2021
Merged

Add BringWindowToTop to c.s.j.p.win32.User32#1352
dbwiddis merged 1 commit intojava-native-access:masterfrom
kahgoh:master

Conversation

@kahgoh
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

dbwiddis commented Jun 5, 2021

LGTM. Will merge later today.

@dbwiddis dbwiddis merged commit 4032b5e into java-native-access:master Jun 5, 2021
Comment thread CHANGES.md
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