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

Return previous callbacks #76

Merged
merged 4 commits into from
Jul 21, 2014
Merged

Return previous callbacks #76

merged 4 commits into from
Jul 21, 2014

Conversation

pwaller
Copy link
Member

@pwaller pwaller commented Jul 13, 2014

A second attempt of #75, but this time against the right branch.

@pwaller pwaller mentioned this pull request Jul 13, 2014
@dmitshur
Copy link
Member

Let me know when this is ready for review, there are still a few issues to be fixed:

  • Still a few instances of inconsistent spacing remaining.
  • A few return old need to be updated to return previous or else it won't compile.
  • CharacterCallback signature has changed.
  • Should analogously improve the API of the 2 new callbacks SetDropCallback and SetCharacterModsCallback to return previous callback func.
$ go build 
# github.com/go-gl/glfw3
./input.go:367: undefined: old
./input.go:387: cannot use w.fCharHolder (type func(*Window, rune)) as type CharacterCallback in assignment
./input.go:388: cannot use cbfun (type CharacterCallback) as type func(*Window, rune) in assignment
./input.go:394: undefined: old
./input.go:417: undefined: old
./input.go:438: undefined: old
./input.go:454: undefined: old
./input.go:469: undefined: old
./input.go:484: undefined: old

Once all those things are done, ideally this PR can be squashed into one or two commits.

Sorry about being very picky, this is one of my most favorite Go packages so I have really high standards for it. :D

@tapir
Copy link
Member

tapir commented Jul 13, 2014

@shurcooL No need to be sorry, pickier the better :)
@pwaller This looks like a good addition. I've checked glfw website to figure out why func returns were removed but couldn't find anything.

@dmitshur
Copy link
Member

I've checked glfw website to figure out why func returns were removed but couldn't find anything.

What do you mean by "func returns were removed"?

@pwaller
Copy link
Member Author

pwaller commented Jul 13, 2014

👍 on the pickiness!

@pwaller
Copy link
Member Author

pwaller commented Jul 13, 2014

I think I got everything. I can't build it because I'm on the old glfw so please let me know if there are any other problems.

@pwaller
Copy link
Member Author

pwaller commented Jul 13, 2014

I did just force again to squash in the "Make consistent spacing" commit.

@tapir
Copy link
Member

tapir commented Jul 13, 2014

We should also probably modify the documentation. Maybe we can add a line saying that it returns the previous callback func.

@shurcooL I meant callback funcs

@dmitshur
Copy link
Member

@pwaller The latest version compiles successfully and I'm not seeing any remaining problems. Thanks!

@tapir I'm not sure what you mean. In both 3.0.4 and latest master, glfw does return previous callback. See here. So I'm not seeing which callback funcs were removed? Or did you mean "not added to glfw3 the Go library"?

@dmitshur
Copy link
Member

And yes, adding something to the docs would be appropriate.

Returns the previously set callback, or nil if no callback was set or an error occurred.

I'm not sure if the "or an error occurred" part applies from GLFW.

@tapir
Copy link
Member

tapir commented Jul 13, 2014

@shurcooL My bad, I thought glfw changed it from 2.7 to 3.x but it was me who missed it. Nice catch.

@dmitshur
Copy link
Member

Other than docs, this has +1 from me to merge (and I'll mirror it at github.com/shurcooL/glfw3).

@tapir
Copy link
Member

tapir commented Jul 14, 2014

Once the docs are there I think we can merge it. Looks good.

@tapir
Copy link
Member

tapir commented Jul 14, 2014

How quickly is glfw 3.1 likely to find its way into distributions?

Ubuntu, at this date, is still on GLFW 2.7, so I think we should not bother ourselves with it. GLFW is very easy to build from source.

@pwaller
Copy link
Member Author

pwaller commented Jul 14, 2014

@tapir @shurcooL What do you guys expect in terms of docs - adding the same line to every affected function?

If so, how about this wording? Otherwise, any other suggestions?

The last callback handler is returned, or nil if there was no previous callback handler

@tapir
Copy link
Member

tapir commented Jul 14, 2014

@pwaller That's pretty much it.

@tapir
Copy link
Member

tapir commented Jul 21, 2014

I think let's get this one going. I'm planning to do a complete documentation sweep anyway. I can put it afterwards.

@pwaller
Copy link
Member Author

pwaller commented Jul 21, 2014

Hm, I thought I had finished this one off. I think I must have left uncommitted or unpushed code on a machine I don't have access to for a bit. But it isn't a big deal, so if you would just do it in passing then that's fine.

tapir added a commit that referenced this pull request Jul 21, 2014
@tapir tapir merged commit bd06f54 into devel_glfw3.1 Jul 21, 2014
@tapir tapir deleted the support-cb-return-devel branch July 21, 2014 10:06
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.

3 participants