[RFE] Callback factory methods and their location

Started by EnergyRay, September 08, 2015, 15:12:06

Previous topic - Next topic

Kai

Quote from: quew8 on September 08, 2015, 16:12:40
While I don't have any problems with the callbacks as they stand...
Exactly, neither do I.

Quote from: EnergyRay on September 22, 2015, 12:07:39
It is a bit unfortunate that this topic has not received comments from others, but I digress...
Okay, then let me just write some response. :)

Personally, I will be fine with *any* solution for registering callbacks, because that is just such an insignificant aspect of my applications. However the callback mechanism will turn out, it will always be encapsulated/isolated in a single class and refactoring to go from one form to another will take five minutes.

I am absolutely 100% fine with using anonymous inner classes as they are now, and I will be fine with using any form of static factory methods, however they are named.

I think you are trying to shoot to eagerly for Lambdas to make use of some Java 8 feature. If you think that is necessary for LWJGL, then just go for it.
But honestly, it does not add any benefit for me as user. Personally, I find anonymous classes just as easy to read as a lambda method when registering a single callback.

A completely different case is when Lambda methods and method references are being used to support a functional programming-style where you build new functions by composing many other function applications together. See Java 8's Stream API for example. Or see a real functional programming language like Haskell and in particular its "pointfree" programming style.

In my opinion this is where lambdas (and more correctly: method references) shine and that's where using Lambdas and method references indeed makes sense and makes those function applications easier to read.
But trying to squeeze a single callback registration into Lambda form does not add any value for me as LWJGL user, IMHO.

Cornix

I completely agree with Kai. I dont care how exactly the things are called because I will wrap them around my own code anyways. I will look it up once in a tutorial and go with it, however it may be called.

spasi

This change is about having a cleaner API with better discoverability, so that you don't have to look it up in a tutorial. Besides, all code and every tutorial that's been written so far with LWJGL 3 will be broken/incorrect.

The first attempt is now up with build 3.0.0b #36.

SHC

What I observed in the latest nightly: (Note, I was using nightly #27 previously).

Introduction of glfw.dll

The GLFW dlls are not present before, so can I ask why this change is made instead of statically linking GLFW? It's not a problem though.

Removal of callback name functions from GLFW class

Previously there used to be static functions that accept SAM interfaces with the same name as the callback in the GLFW class, now that they are moved into the callback class with the name as create, for example, keyCallback = GLFWKeyCallback.create(this::myKeyCallback);

This is more better than the previous one in my opinion, with more readability. I really like this.

Removal of Callbacks.errorCallbackDescriptionString() and Callbacks.dropCallbackNamesString()

These are now gone, and the new functions are now present in the respective callback classes itself. Callbacks.errorCallbackDescriptionString() is now not available in the GLFWErrorCallback class, instead there is now GLFWErrorCallback.create(SAMString) that gives a String object as the description parameter instead of the long.

I think I could also use MemoryUtil.memDecodeUTF8(long) method as the alternative, am I right? Are there any other alternatives for this method? To avoid confusion to the newbies, I suggest to add a GLFWErrorCallback.getDescription(long) method that does the same thing, it would be better.

And regarding Callbacks.dropCallbackNamesString(), there is now GLFWDropCallback.getNames(count, names) method that I'm now using, and it is fine.

Are there any changes that I've missed other than these?

spasi

Thank you for the feedback.

Quote from: SHC on September 23, 2015, 16:48:59Introduction of glfw.dll

The GLFW dlls are not present before, so can I ask why this change is made instead of statically linking GLFW?

This was done for flexibility. GLFW is slowing moving to configurable window and context APIs (at runtime). For example you will be able to use both "legacy" and EGL contexts with a single binary. The work for this is partially done in GLFW currently, but there's no API to make the choice yet (it may be added in GLFW 3.2). So you're still stuck with whatever choice gets build into the GLFW binary.

By moving GLFW to a separate shared llibrary, an LWJGL user can build GLFW with the configuration of their choice and drop it in. It will work without further LWJGL changes.

Quote from: SHC on September 23, 2015, 16:48:59I think I could also use MemoryUtil.memDecodeUTF8(long) method as the alternative, am I right? Are there any other alternatives for this method? To avoid confusion to the newbies, I suggest to add a GLFWErrorCallback.getDescription(long) method that does the same thing, it would be better.

Yes, memDecodeUTF8(long) will work. Adding getDescription is a good suggestion.

Quote from: SHC on September 23, 2015, 16:48:59Are there any changes that I've missed other than these?

EGL and OpenGL ES bindings are now included in the default distribution.

Kai

Quote from: spasi on September 23, 2015, 17:26:24
By moving GLFW to a separate shared llibrary, an LWJGL user can build GLFW with the configuration of their choice and drop it in. It will work without further LWJGL changes.
Having GLFW as a separate drop-in library is a fantastic choice!
It also allows people to include an own GLFW fork with custom changes not (yet) in the GLFW repo with LWJGL.

EDIT: And I even have the impression that with the latest version of LWJGL, applications load a bit faster, too, from opening the window to a first visible frame. Hmmm...

spasi

String decoding helper methods have been added to all callback classes (build 3.0.0b #37).