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

registering new window classes and creating windows #163

Merged
merged 5 commits into from
Dec 12, 2012
Merged

registering new window classes and creating windows #163

merged 5 commits into from
Dec 12, 2012

Conversation

wolftobias
Copy link
Contributor

Create new code for registering new window classes and creating window
example. Listening for win32 window events. Listening for logoff/logon
events. Created also new api for 'Wtsapi32.dll'.

Create new code for registering new window classes and creating window
example. Listening for win32 window events. Listening for logoff/logon
events. Created also new api for 'Wtsapi32.dll'.
@wolftobias wolftobias closed this Dec 2, 2012
@wolftobias wolftobias reopened this Dec 2, 2012
@dblock
Copy link
Member

dblock commented Dec 2, 2012

This is good. You need unit tests for this to be merged.

You wrote a demo app in Win32WindowTest.java, but it's not a test. For every Win32 API interface implementation that you're adding, please write a corresponding testXYZ JUnit unit test. We have pretty much 100% coverage in the Win32 layer.

Move the demo app out into contrib demos and please also edit CHANGES.

- Now the java guid structure can be used directly as alternative to
'Ole32Util.getGUIDFromString()'.
- The listening for usb devices is now working also with filter usage
@wolftobias
Copy link
Contributor Author

Done as you told me. Want to ask to get merged.

@dblock
Copy link
Member

dblock commented Dec 8, 2012

Thank you, this is very close.

A few issues:

  • contrib/native_window_msg/classes/com/sun/jna/platform/win32/Win32WindowDemo.class is checked in, it's a generated file
  • you've made changes to GUID, converting to/from string - thank you, but those need tests, please
  • you still need to describe your changes in the CHANGES file

@dblock
Copy link
Member

dblock commented Dec 8, 2012

Also, the demo needs to be buildable/runnable, it should have all the usual build.xml, .project, etc.

@wolftobias
Copy link
Contributor Author

please have another check on my latest code. I`ve done all changes as you suggested.

@dblock
Copy link
Member

dblock commented Dec 9, 2012

Sorry to be a pest, but we want this to be perfect, especially since a large amount of people are going to prefer the native GUID to the Ole32 functions. So, testGUID doesn't test anything. It does conversions and prints them out on System.out. A unit test should assert something and then would fail when the implementation is broken. A test also shouldn't not call code that it's not involved with (Ole32Util.getGUIDFromString) unless it's trying to assert something relating to that function.

I would write these (as separate test functions).

  • testGUIDFromString: load a GUID from string and verify that the guid returned has the expected values in each byte
  • testNewGUID: same with the constructor, there're several flavors of this
  • testGUIDFromBinary: same from binary data
  • testGUIDtoByteArray: Create a GUID, call guid.toByteArray(); and verify that each byte has the expected values.
  • test that GUID behaves in a similar way as Ole32Util.getGUIDFromString
  • etc.

Take a stab at this and I'll fix/add/edit anything else that I find after that.

@wolftobias
Copy link
Contributor Author

No, thats ok for me :-). Ive written the code yesterday during a hurry and youre absolutely right with the unit test. I think its totally ok to give suggestion to other project workers. That makes the success of open source software.
So no problem at all.
Ive added now some test cases, as you proposed, I was using the method from Ole32Util to show that the guid structure can be used vice versa with the build-in windows function and that the result is exactly the same. Ive figured out through the development of the usb listener class, that it is not so easy to deal with the jna guid class, because my filter for the usb listener was not working and the reason was that the given guid was not working correctly and trapping that issue was difficult.
Ive added also a new function for generating a new guid, just in case its needed. I made a reengineering from the java UUID class. Do you think that this is be ok?

@wolftobias wolftobias closed this Dec 10, 2012
@wolftobias wolftobias reopened this Dec 10, 2012
@wolftobias
Copy link
Contributor Author

sorry I`ve closed the pull request as a mistake. I still want to get merged!

@wolftobias
Copy link
Contributor Author

Would you please be so kind to have a check please

@dblock
Copy link
Member

dblock commented Dec 11, 2012

The code looks good. I need to check it on a PC when I have one in front of me, definitely this week.

@dblock dblock merged commit b78eef7 into java-native-access:master Dec 12, 2012
@dblock
Copy link
Member

dblock commented Dec 12, 2012

Merged. I rewrote the CHANGES to be proper markdown. Thanks for contributing! Looking forward to more.

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.

None yet

2 participants