lwjgl icon indicating copy to clipboard operation
lwjgl copied to clipboard

ContextAttribs and PixelFormat builder without unexpected behaviour

Open gmacd opened this issue 12 years ago • 9 comments

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.

gmacd avatar Nov 20 '12 22:11 gmacd

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 :-)

gmacd avatar Nov 21 '12 07:11 gmacd

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?

Spasi avatar Nov 21 '12 08:11 Spasi

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).

gmacd avatar Nov 21 '12 09:11 gmacd

We could use a fluid builder pattern for it. There is just no benefit in having modifiable objects (and only downsides).

grum avatar Nov 21 '12 20:11 grum

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)

gmacd avatar Nov 21 '12 21:11 gmacd

@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:

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?

+1

void256 avatar Nov 21 '12 21:11 void256

(Of course I'm not saying you should never use immutable objects in Java, just that it should be clear what's happening)

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).

grum avatar Nov 21 '12 21:11 grum

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?

gmacd avatar Nov 24 '12 14:11 gmacd

Should probably be consistent on how we handle those values.

philipwhiuk avatar Jul 12 '13 21:07 philipwhiuk