ContextAttribs and PixelFormat builder without unexpected behaviour #4
Conversation
withXXXX() methods no longer create a new instance. They instead modify the instance, and return this. This removes any possible misunderstanding is using the withXXXX() calls individually (without chaining them). This change also ensures all the 'OpenGL 3.2 and newer' examples on the wiki set up the display correctly (the examples assume mutability) Also see http://lwjgl.org/forum/index.php/topic,4708.0.html
|
I forgot to add that making these two classes immutable seems to give no real benefit, so making them mutable to make the API less surprising should be a good thing :-) |
|
The idea behind the current ContextAttribs API is: a) It makes clear that once you create a context using it, there's no way to modify its settings. The only way to change anything is by destroying the context and recreating it with another ContextAttribs instance. b) There's currently no way to get a ContextAttribs instance from an existing context. The only way to have that information available is to hang on to the instance you used when creating the context. Without immutability, there'd be no guarantees that information would be correct. Anyway, this a low-risk change and I understand how the withXXXX pattern might be confusing if someone hasn't used it before. But if I were to do it again, I'd keep ContextAttribs immutable and provide a Builder/Factory for creating instances of it. What do you think? |
ghost
assigned
|
Addressing your points, I would say that It's only clear if you've read the API code/docs fairly carefully that it's immutable - reading code that uses the API gives no indication it's immutable, or that changing the ContextAttribs after the context has been created will have no effect. My own preference would be to avoid the factory, leave the object to be mutable, and add the ability to get a read-only version of ContextAttribs from the existing context - perhaps an interface already exists, I can't remember and I'm at work ATM :-) The use of a fluent API here works quite well, but only if it's mutable (in Java at least). |
|
We could use a fluid builder pattern for it. There is just no benefit in having modifiable objects (and only downsides). |
|
In a language where immutable objects are the norm, then I'd agree. In Java, the norm is to have mutable objects. If the class is to be immutable, then any methods that create new instances should be named in a more obvious manner: e.g. newWithXXX() or createWithXXX() (though that wouldn't read as well as what there is now). (Of course I'm not saying you should never use immutable objects in Java, just that it should be clear what's happening) |
|
@HaggisChan according to josh blochs "Effective Java" book it's a good idea to make all your classes immutable by default since this makes them much simpler to use/maintain. I kinda don't agree that in Java "mutable classes" are the default :) As Spasi said:
+1 |
If fluid modification/building is your goal you should write a fluid builder that spits out immutable objects, this saves you having to worry about synchronisation of a mutable object (because that would be mandatory in this case). |
|
Ok, so people here are big on immutability ;-) I've committed versions of PixelFormat and ContextAttribs that are both fully immutable, and now use builders. The downside is that it's a breaking change. Thoughts? |
| } | ||
|
|
||
| public Builder(int bpp, int alpha, int depth, int stencil, int samples, int num_aux_buffers, int accum_bpp, int accum_alpha, boolean stereo, boolean floating_point) { | ||
| this.bpp = bpp; |
There was a problem hiding this comment.
We dont check for < 0 here.
|
Should probably be consistent on how we handle those values. |
Edited after original change was questioned.
I've committed versions of PixelFormat and ContextAttribs that are both fully immutable, and now use builders. The downside is that it's a breaking change.