[RFE] Callback factory methods and their location

[EnergyRay]

Hello!

The following "issues" I have with the current callback construction are opinionated, and not strictly issues.

So, the current situation with the callback creation, and especially with the various factory methods, leave much to be desired. I will use GLFW and its error callback as an example:

GLFWErrorCallback cb = new GLFWErrorCallback() {
    public void invoke(int error, long description) {}
};
GLFWErrorCallback cb = GLFW.GLFWErrorCallback(Handler::onError);
GLFWErrorCallback cb = Callbacks.errorCallbackThrow();

There is not much to be done about the first example, but the others have some "flaws" to a greater or lesser degree.
Firstly, the second example is a bit ambiguous and confusing looking at a quick glance, especially to someone new to Java, not to mention the loss in code-readability due to the method name being identical to the type. This situation is not helped by using static imports, something that is quite commonly used with GLFW or OpenGL code:

GLFWErrorCallback cb = GLFWErrorCallback(Handler::onError);

Secondly, I think the callback factory methods accepting the SAM-interface pollute the GLFW class, and thirdly, while the existence of the Callbacks class is nice, the factory methods could be defined in a better place.

This brings me to the feature request itself: the various factory methods should be defined in the corresponding callback classes. As an example:

GLFWErrorCallback cb = GLFWErrorCallback.of(Handler::onError);
GLFWErrorCallback cb = GLFWErrorCallback.ofThrow();

This should reduce ambiguity and confusion, if there has been any, in addition to making the code more readable. This also provides an excellent platform for providing specializations (string / buffer variants, etc.), and the ability to easily find said specializations (as they are in one class).


What are your opinions and/or suggestions regarding callbacks in LWJGL 3.

[quew8]

While I don't have any problems with the callbacks as they stand, what you've suggested could be an improvement. The SAM wrapper functions certainly don't conform to Java naming conventions and if I had a problem, that would probably be it. Otherwise, readability wise I think it's all fine (which is not to say it couldn't be improved).

However, in terms of "specialization" see this thread http://forum.lwjgl.org/index.php?topic=5932.0 for a brief description of versatility versus simplicity. Also I believe that much of these are automatically generated classes and specialization might not fit well into it the build model (but I don't know about that).

[EnergyRay]

Well, just to make it clear, I am not suggesting replacing the low-level interface with a higher-level one (pointer with buffer or string). Rather, I made the observation that, if need be, these specializations, or higher-level functions, can be provided in the abstract class directly. Additionally, there are some utility methods in the Callbacks class that make it easier to acquire higher-level access, and these could have companion wrappers providing that access directly:

public interface SAMString {
    void invoke(int error, String description);
}

public static final GLFWErrorCallback of(SAMString sam) {
    return new GLFWErrorCallback() {
        public void invoke(int error, long description) {
            sam.invoke(error, Callbacks.errorCallbackDescriptionString(description);
        }
    }
}

...

GLFWErrorcallback cb = GLFWErrorCallback.of((int error, String description) -> ...);

Even if many use the lower-level functionality directly (myself included), there are still plenty of people who would like a clear, Java-styled, API in some form or another. This is what the request was for: improving the current situation of factory and utility methods being scattered between different classes.

I hope my intent of this request is more clear.

[spasi]

Firstly, the second example is a bit ambiguous and confusing looking at a quick glance, especially to someone new to Java, not to mention the loss in code-readability due to the method name being identical to the type. This situation is not helped by using static imports, something that is quite commonly used with GLFW or OpenGL code:

GLFWErrorCallback cb = GLFWErrorCallback(Handler::onError);

This is exactly how you're supposed to use it, LWJGL always assumes static import. I know this is non-standard Java and I've said before that I borrowed the idea from Kotlin, which doesn't have the new keyword and you can do "tricks" like that without it looking weird.

Secondly, I think the callback factory methods accepting the SAM-interface pollute the GLFW class

Agreed.

This brings me to the feature request itself: the various factory methods should be defined in the corresponding callback classes. As an example:

GLFWErrorCallback cb = GLFWErrorCallback.of(Handler::onError);
GLFWErrorCallback cb = GLFWErrorCallback.ofThrow();

This should reduce ambiguity and confusion, if there has been any, in addition to making the code more readable. This also provides an excellent platform for providing specializations (string / buffer variants, etc.), and the ability to easily find said specializations (as they are in one class).

I'm fine with this suggestion but it can't be very complicated because, as quew8 said, the callback classes are machine-generated. I can probably move the SAM conversion methods there, but the methods in Callbacks will probably stay where they are.

Well, just to make it clear, I am not suggesting replacing the low-level interface with a higher-level one (pointer with buffer or string). Rather, I made the observation that, if need be, these specializations, or higher-level functions, can be provided in the abstract class directly.

Another option would be to make the SAM interfaces "high-level". If you need low-level access (should be rare), implementing the abstract class shouldn't be an issue.

[spasi]

How would you feel if the factory method was called "by" instead of "of"? That is:

GLFWErrorCallback cb = GLFWErrorCallback.by(Handler::onError);

This would better express that there's delegation involved, i.e. a new GLFWErrorCallback instance is created that delegates the callback to the lambda/method reference argument. Kotlin also uses the "by" clause for class and property delegates.

[spasi]

I also don't like ofThrow/ofPrint or byThrow/byPrint. The former is ugly, the latter is also ugly and there's no delegation there. Any suggestions?

GLFWErrorCallback.asThrowException()
GLFWErrorCallback.asPrintToStream(PrintStream)
GLFWErrorCallback.asPrintToSystemErr() // equivalent to GLFWErrorCallback.asPrintToStream(System.err)

or simply:

GLFWErrorCallback.throwException()
GLFWErrorCallback.printToStream(PrintStream)
GLFWErrorCallback.printToSystemErr() // equivalent to GLFWErrorCallback.printToStream(System.err)

[spasi]

Progress update: Went with the inelegant solution of dumping Java code in a string and simply writing it out at generation time. This basically allows any code to be included with the generated callback classes:

- Moved the "SAM constructors" from GLFW/etc to the callback classes.
- The Callbacks class has been cleaned up accordingly.
- We can now have multiple SAM interfaces and corresponding delegate implementations.

The problem with the 3rd point is that having multiple SAM interfaces is awkward. You must either cast the lambda to the corresponding interface or specify the argument types explicitly. Example code:

// works
glfwSetErrorCallback(GLFWErrorCallback.by((SAM)(error, description) -> { /* ... */ })); // description is a long
glfwSetErrorCallback(GLFWErrorCallback.by((SAMString)(error, description) -> { /* ... */ })); // description is a String

// works
glfwSetErrorCallback(GLFWErrorCallback.by((int error, long description) -> { /* ... */ }));
glfwSetErrorCallback(GLFWErrorCallback.by((int error, String description) -> { /* ... */ }));

// ambiguous error
glfwSetErrorCallback(GLFWErrorCallback.by((error, description) -> { /* ... */ }));

private static void handleError(int error, long description) { /* ... */ }
private static void handleError(int error, String description) { /* ... */ }
private static void handleErrorL(int error, long description) { /* ... */ }
private static void handleErrorS(int error, String description) { /* ... */ }

// works
glfwSetErrorCallback(GLFWErrorCallback.by((SAM)MyClass::handleError));
glfwSetErrorCallback(GLFWErrorCallback.by((SAMString)MyClass::handleError));
glfwSetErrorCallback(GLFWErrorCallback.by(MyClass::handleErrorL));
glfwSetErrorCallback(GLFWErrorCallback.by(MyClass::handleErrorS));

// ambiguous error
glfwSetErrorCallback(GLFWErrorCallback.by(MyClass::handleError));

The question is if it's worth having this trouble. Two options:

a) Have multiple interfaces. Method references are not affected that much, because you'd only have a single implementation and that works without ambiguities.
b) Have a single interface, preferring the high-level one (String description in this case).

[EnergyRay]

To start of, both the "by" and "of" are fine, but I would argue that "of" is slightly better.
Firstly, I think some implementation details (e.g. delegation) are obvious enough to not need any mentions or, if significant or important, are mentioned in the documentation, but often there is no such details (for better or worse), as you are well aware.
Secondly, and here I decided to search through the JRE and JDK for some interesting statistics (more details further down the post), I was unable to find a single "by*" factory method. I did find plenty of "of*" factory methods, but only 2 for constructing instances from delegates. Not to say that is the (or even a) reason why one should not use "by".

As for "as" vs. no prefix, the former is too similar to the conversion and view methods, and the latter is just too ambiguous.
Generally, if you have a method that throws an exception, it would be called "throwException", and you wouldn't expect it to return a callback. The same goes for the PrintStream variants.


Now, some statistics.

I did a quick search through the JRE, including internal packages, for various prefixes:

"of": 96 results
"of*": 241 results, of which 63 are "offer*" and 25 are "offset*"

"by": 0 results
"by*": 138 results, of which 120 are "byte*"

"as": 1 result
"as*": 511 results, of which 43 are "assert*" and 29 are "assign*" (most of the remaining are conversion or view methods)

"from": 46 results
"from*": 229 results (mostly conversion methods)

"with": 49 results
"with*": 161 results (interestingly, there were a few "without*" methods)

(Note, however, that not all are factory methods, especially in the larger result sets, as I did not have time to start filtering out results.)
(I did wish to search for "to*" methods, but quickly realized it would take several hours to sift through the ~3500 results with the tools I am using.)

As I mentioned above, I could find only 2 "of*" factory methods accepting delegates, and after a quick search, 1 "with*" factory method for delegates:

Collector.of(Supplier, BiConsumer, BinaryOperator, Characteristics...)
Collector.of(Supplier, BiConsumer, BinaryOperator, Function, Characteristics...)
ThreadLocal.withInitial(Supplier)

Returning to the "specializations", "with" could be used as the prefix, especially if some redundancy is removed from the method names:

GLFWErrorCallback.withPrintToStream(PrintStream)
GLFWErrorCallback.withPrintToSystemErr()

GLFWErrorCallback.withPrintStream(PrintStream)
GLFWErrorCallback.withSystemErr()

Although, the shortened names could be used with really any prefix.


EDIT: Reply to the last post from spasi:

As for the SAM-interface situation, I would go with multiple interfaces as it would allow the usage of lambdas and method references for single line implementations, instead of having to extend the abstract class.
I wouldn't also mind having a suffix in the factory method describing the type:

GLFWErrorCallback.by((error, description) -> {}); //pointer
GLFWErrorCallback.byPointer((error, description) -> {}); //Pointer
GLFWErrorCallback.byString((error, description) -> {}); //String

Replacing the prefix and suffixes with whatever will be decided on.

Does "Consumer" have a semantic meaning (that I have not thought of), or is there some other reason why the SAMs are not named as such?
Strictly speaking, they do in fact consume the callback input, and are allowed to have side-effects.

EDIT 2: Fixed sentence not making any sense whatsoever.

[spasi]

I haven't looked for examples in the JDK because, well, I'd be familiar with those already. I'm mainly looking for input from people that have used factories in other libraries and/or languages. Especially libraries that have gone through the Java 8 transition and offer better APIs for lambdas.

Returning to the "specializations", "with" could be used as the prefix, especially if some redundancy is removed from the method names

I have associated "with" with the builder pattern: called multiple times for configuration, then a terminal operation that returns the object.

As for the SAM-interface situation, I would go with multiple interfaces as it allows the usage of lambdas and method references in a single line.

Not sure what you mean here. A single interface (vs many) would still allow both lambda and method references, just of a specific signature only.

I wouldn't also mind having a suffix in the factory method describing the type

That's another solution but no better than casting/explicit types and also "bad java".

Does "Consumer" have a semantic meaning (that I have not thought of), or is there some other reason why the SAMs are not named as such?
Strictly speaking, they do in fact consume the callback input, and are allowed to have side-effects.

I chose "SAM" because they don't add anything to the API and their only purpose is to enable Java 8 SAM compatibility; exactly what the name says. Note the DropConsumer interfaces in Callbacks (now GLFWDropCallback.Consumer), those do offer something extra so are named accordingly.

[EnergyRay]

I haven't looked for examples in the JDK because, well, I'd be familiar with those already. I'm mainly looking for input from people that have used factories in other libraries and/or languages. Especially libraries that have gone through the Java 8 transition and offer better APIs for lambdas.

Finding uses in the JRE started to interest me personally, and not being entirely familiar with many parts of the standard library (and especially any internals), I decided to search whatever prefixes came to mind. I decided to share the results of the few quick searches I did instead of keeping them to myself.

Not sure what you mean here. A single interface (vs many) would still allow both lambda and method references, just of a specific signature only.

I wrote that part of the reply hastily and did not proof-read it, so thank you for pointing it out. I fixed the sentence, but I'll quote the fixed version here as well:

As for the SAM-interface situation, I would go with multiple interfaces as it would allow the usage of lambdas and method references for single line implementations, instead of having to extend the abstract class.

What I mean by that, is that no matter the use-case (pointer, String, ByteBuffer, etc.) you would not have to extend the callback (in a named class) or create an anonymous class just to get access to the method. So, in other words, if the only SAM-interface is the string variant, and you wished to use the pointer variant (which would be only available through the method in the callback class itself), you would have to create a concrete class and extend the callback class, or create an anonymous class. Both of these two options are cumbersome compared to the alternative.
Additionally, I feel it is good to have uniformity, and as such having an interface for both high- and low-level would be better.

Hopefully this was clear enough.

That's another solution but no better than casting/explicit types and also "bad java".

It may be "bad Java", but in some cases it is the lesser of two evils, especially if there are generics involved (luckily not the case).
While having nothing to do with factory methods, the few times I have had to use PrivilegedActions have usually resulted in even "worse Java" than if there had been a suffix to differentiate two "doPrivileged" methods...

I have associated "with" with the builder pattern: called multiple times for configuration, then a terminal operation that returns the object.

Are you going to discard it as an option due to that association?
Example of the "with*" prefix outside the builder pattern is in ThreadLocal:

ThreadLocal.withInitial(Supplier)

[spasi]

Finding uses in the JRE started to interest me personally, and not being entirely familiar with many parts of the standard library (and especially any internals), I decided to search whatever prefixes came to mind. I decided to share the results of the few quick searches I did instead of keeping them to myself.

I'm sorry, probably sounded too negative there. I very much appreciate you taking the time to do the research (or even just replying here).

What I mean by that, is that no matter the use-case (pointer, String, ByteBuffer, etc.) you would not have to extend the callback (in a named class) or create an anonymous class just to get access to the method. So, in other words, if the only SAM-interface is the string variant, and you wished to use the pointer variant (which would be only available through the method in the callback class itself), you would have to create a concrete class and extend the callback class, or create an anonymous class. Both of these two options are cumbersome compared to the alternative.
Additionally, I feel it is good to have uniformity, and as such having an interface for both high- and low-level would be better.

Hopefully this was clear enough.

Thanks, it's clear now. And I think I agree you, but just for argument's sake let's assume that most users go with the high-level interface. Most callbacks are invoked very infrequently (or while debugging only) and any overhead won't matter much, so that's pretty likely. If that's the case, isn't it better to only keep a high-level version and any user wanting to go low-level has to extend the abstract class instead?

Are you going to discard it as an option due to that association?
Example of the "with*" prefix outside the builder pattern is in ThreadLocal:

ThreadLocal.withInitial(Supplier)

That's a good example. I'm not discarding any options and would love to hear more voices here.

[EnergyRay]

Thanks, it's clear now. And I think I agree you, but just for argument's sake let's assume that most users go with the high-level interface. Most callbacks are invoked very infrequently (or while debugging only) and any overhead won't matter much, so that's pretty likely. If that's the case, isn't it better to only keep a high-level version and any user wanting to go low-level has to extend the abstract class instead?

What would the benefit of dropping the other factory methods, aside from an ever-so slight reduction in complexity, be? Even if a handful of people use the low-level interface, it would still be boiler-plate code that each would have to write and, I would hazard a guess, the produced code would be nearly identical. This would essentially defer it to the user's codebase instead of it being defined in the one common location. Obviously, you have to draw the line somewhere, as you cannot add utilities for everything, but I would say the String and pointer variants, at the very least, are both common enough.

What you mentioned in the "LWJGL 3 debug output gives pointer to string" thread (http://forum.lwjgl.org/index.php?topic=5932.0) also has some relevance here:

Another issue is text encoding, which is not specified in the spec. I used UTF-8 here, which covers ASCII, but it could be anything I guess. With a String argument you're stuck with whatever LWJGL implements. With the raw pointer, you can use ASCII decoding for efficiency or anything else if UTF-8 happens to produce garbage. This is the reason why "unsafe" function versions are publicly exposed in LWJGL 3: if LWJGL does something stupid, in any API and any function, you can be sure that you will be able to implement a workaround without having to wait for LWJGL to fix it.

Let us assume you, as a user, would like to switch from the String variant to the pointer one for some reason (efficiency or anything else). In that case, having the factory method for the pointer variant would simplify refactoring.


To keep things positive: I would like to thank you for hearing out these opinions, and continuing creating and updating an amazing library!

[spasi]

What would the benefit of dropping the other factory methods, aside from an ever-so slight reduction in complexity, be?

The only issue is that multiple factories means forcing a cast or explicit type arguments at the call site (this affects all call sites and all users). This is kind of similar to the issue of buffer overloads in the bindings, for example:

// you cannot say:
glTexImage2D(..., null);
// you have to say:
glTexImage2D(..., (ByteBuffer)null);

but at least there's a "workaround" there:

glTexImage2D(..., NULL); // NULL is MemoryUtil.NULL = 0L

So the question is if the low-level factories will be used frequently enough to justify having this issue.

[spasi]

I just promoted build #32 to stable and will be working on API breaking changes next: the callback and struct refactorings.

If you've got any further thoughts on this topic, please post soon. I will probably come to a decision and start pushing changes in the next couple of days.

[EnergyRay]

It is a bit unfortunate that this topic has not received comments from others, but I digress...

My opinion regarding the matter has not changed all that much since my last post and, just for the sake of it, I will recap my opinions and thoughts here:


Static factory methods should be declared in their respective callback classes (the actual issue).
Various other utilities can be kept elsewhere.
This reduces code pollution and ambiguity, and simplifies finding relevant functionality (currently spread between 2-3 classes).

Functional interfaces should be provided for both low- and high-level access.
The entire reason for the SAM-interfaces is to provide some sort of support for Java 8, and forcing the user to extend an abstract class misses this point. I would argue that the pointer and String variants are common enough to be provided, instead of deferring either of these to the user's code base as boiler-plate.
This may force a cast at the call site, depending on the factory naming convention.

SAM-interfaces should be named more descriptively and following (more) closely to Java conventions.
By its simplest definition, the term "consumer" describes these interfaces accurately (paraphrasing Java): "Represents an operation that accepts input and returns no result".
Using the term "consumer" may cause some issues with utility methods in the Callbacks class regarding the file drop callback, and there may be a better alternative.

Factory methods should specify the type (pointer, String, etc.) in the name.
This will avoid forcing casts or explicit type arguments.
Can be considered "bad" Java to some degree, but is fairly commonly used. With many IDEs, the difference between two different factory methods is one or two button presses, so name length is not an issue.

Common "specializations" should be provided where applicable.
This one also goes back to picking and choosing implementations that are common enough to be provided, and ones to omit and defer to the user's code base.

Prefixes should be used to distinguish factory methods (as is convention).
It may also be desirable to describe the relation between the SAM-interfaces and the callback classes proper with the prefix.
Deciding the exact prefix(es) is the least of the problems, but here are some suggestions out of a myriad of possibilities: "of", "with", "create".


Example:

//Basic factories
//SAM-interface names are a combination of the names of the callback class and provided type
GLFWErrorCallback.createPointerCallback(ErrorPointerConsumer)
GLFWErrorCallback.createStringCallback(ErrorStringConsumer)

//Specializations:
GLFWErrorCallback.createThrowCallback() //createThrowingCallback?
GLFWErrorCallback.createPrintStreamCallback(PrintStream) //createPrintingCallback?
GLFWErrorCallback.createSystemErrorCallback()

I may have forgotten something from this ramble of a post, and I will try to edit and add anything that comes to mind.