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

Shell32.SetCurrentProcessExplicitAppUserModelID #680

Merged
merged 4 commits into from
Jul 16, 2016
Merged

Shell32.SetCurrentProcessExplicitAppUserModelID #680

merged 4 commits into from
Jul 16, 2016

Conversation

rednoah
Copy link
Contributor

@rednoah rednoah commented Jul 13, 2016

Added functions to Shell32 wrapper which are useful Desktop integration for Java GUI applications (on Windows 7 and higher).

  • SetCurrentProcessExplicitAppUserModelID
  • GetCurrentProcessExplicitAppUserModelID

…plicitAppUserModelID functions to Shell32 wrapper
HRESULT r2 = Shell32.INSTANCE.GetCurrentProcessExplicitAppUserModelID(ppszAppID);
assertEquals(WinError.S_OK, r2);

assertEquals(appUserModelID, ppszAppID.getPointer().getWideString(0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please free the string, as this is expected from the documentation? For the unittest this is not actually necessary, but people looking at the code might be mislead, that you don't need to free the string.

@matthiasblaesing
Copy link
Member

Thank you! I added a few line notes. Please also add an entry to the FEATURES section in CHANGES.md - see the other entries as a template.

…plicitAppUserModelID functions to Shell32 wrapper (fix documentation)
…plicitAppUserModelID functions to Shell32 wrapper (update CHANGES.md)
@rednoah
Copy link
Contributor Author

rednoah commented Jul 15, 2016

Fixed.

@matthiasblaesing
Copy link
Member

Thank you for the changes however this can't be merged - running the unittest immediatly fails:

Testsuite: com.sun.jna.platform.win32.Shell32Test
Tests run: 10, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1,895 sec

Testcase: testCurrentProcessExplicitAppUserModelID(com.sun.jna.platform.win32.Shell32Test): FAILED
expected:<[com.sun.jna.platform.win32.Shell32Test]> but was:<[??À?]>
junit.framework.ComparisonFailure: expected:<[com.sun.jna.platform.win32.Shell32Test]> but was:<[??À?]>
    at com.sun.jna.platform.win32.Shell32Test.testCurrentProcessExplicitAppUserModelID(Shell32Test.java:247)

This is easily explained: PointerByReference#getPointer retrieves the Pointer to the PointerBuffer, not the Pointer value that is placed inside the buffer. Both occurences (line 247+ 249) need to be modified.

As the code needs another round I'd like to ask you to review the CHANGES.md entry:

  • please move your entry to the end of the FEATURES section (new entries are appended)
  • I'd reword it - you implemented both setter+getter, this modification is not only usable for running under javaw, but will also work with other launchers. (not critical!)

As a final note - it would be good if you'd squash the commits into one commit, so that history is not cluttered.

Thank you for your patience!

…plicitAppUserModelID functions to Shell32 wrapper (fix unit test and CHANGES.md)
@rednoah
Copy link
Contributor Author

rednoah commented Jul 15, 2016

Done, but squashing commits... for the love of my life i couldn't figure out how to do that on my branch... Doesn't github have a merge+squash button?

@rednoah
Copy link
Contributor Author

rednoah commented Jul 15, 2016

... no idea why CI fails. That test definitely works on my Windows VM now.

@matthiasblaesing matthiasblaesing merged commit 6c52731 into java-native-access:master Jul 16, 2016
mstyura pushed a commit to mstyura/jna that referenced this pull request Sep 9, 2024
Motivation:
Ability to get the RTT as requested in java-native-access#678

Modification:
Add ability to get the path stats using quiche_conn_path_stats

Result:
The change adds the API to get the path stats.
---------

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
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.

2 participants