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 support of function SHAppBarMessage from ShellApi #245

Closed
wants to merge 4 commits into from

Conversation

bsorrentino
Copy link
Contributor

i've implemented what i asked in Issue #241

I've tested all the stuff on Windows 7 x64

@dblock
Copy link
Member

dblock commented Jun 19, 2013

It's a good start, thank you. A few things:

  • Fix comments in SHAppBarMessage to be processable by JavaDoc.
  • Fix spacing and tabs to look like the rest of the code. Ex, int ABE_LEFT = 0; is not aligned with the rest.
  • Tests need to be self-contained and test what you expect to happen. You have something going on in setup, then you're doing a bunch of System.out.printf and a Thread.sleep. This looks like a demo, more than a test, I am not sure how to rewrite this, but happy to help.
  • Add a line to CHANGES.md.

@bsorrentino
Copy link
Contributor Author

You absolutely right.. i've worked so fast and, as consequence, have missed these important details

Probably my test was more a kind of demo rather unit test

thx for feedbacks ... i'll working on

javadoc refactoring
unit test refactoring
CHANGES.md update
@@ -7,6 +7,7 @@ NOTE: JNI native support is typically incompatible between minor versions, and a

Features
--------
* [#241](https://github.com/twall/jna/issues/241) - Add support function SHAppBarMessage from ShellApi
Copy link
Member

Choose a reason for hiding this comment

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

Add your name, similar to the other items below.

@dblock
Copy link
Member

dblock commented Jun 22, 2013

You have to merge from upstream, otherwise this looks good to go.

@bsorrentino
Copy link
Contributor Author

Ok, thanks for feedbacks

Don't worry, indeed sorry for the problems of code formatting but from my ide (eclipse on mac) all seemed good

i'll go to merge and fix, hoping that my little work will become part of next release of this great project

@bsorrentino
Copy link
Contributor Author

hi

i've fetched & merged from upstream ( 46fb75c)

i've fixed code formatting & put ABM_* constants as DWORD (1646c3c)

After that, I've checked out if other constants in the project was declared as ''IntegerType' rather than "int" and, i've reverted the ABM_* updates (27a5176)

@dblock
Copy link
Member

dblock commented Jun 23, 2013

Sorry to be annoying, click on the files changed diff here. The changelog is completely reformatted spaces/tabs. That will make history unusable. Could you please fix that? I would take the original file, use notepad or something to edit it. LMK if you can't figure it out and I'll do it by cherry-picking your changes.

@bsorrentino
Copy link
Contributor Author

Again don't worry

Since the great part of problems seems relates to code formatting and they are increasing ....

what do you think whether i send you the (only) three updated files and you put directly on your branch ?

i'll provide to make a fresh fork to avoid problem for the future

@dblock
Copy link
Member

dblock commented Jun 23, 2013

Merged via e0d2079. You should be able to reset your master branch to an older commit, eg git reset --hard 3ba1fd097559179bfbf3ca71a6dfc12f0aec470f, then merge from upstream git pull upstream master, then force push that to your master git push origin master -f. That gives you parity with JNA upstream. Next time, before making changes make a branch first, git checkout -b feature-branch, this way it's a bit easier to abandon. Thanks for contributing and being patient with my bossy demands :)

@bsorrentino
Copy link
Contributor Author

Thx for useful feedbacks. Don't worry i love to contribute to the project and i'll go ahead

i'll follow your suggestion ... hoping, next time, will go better

@dblock dblock closed this Jul 2, 2013
mstyura pushed a commit to mstyura/jna that referenced this pull request Sep 9, 2024
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