Deregister Light Editor columns

[ericmehl]

This adds three new methods to the Light Editor for removing columns : deregisterParameter(), deregisterAttribute() and deregisterColumn().

Really deregisterColumn() is all that's needed to do the actual work or removing a column, but the formatting of a parameter into a tuple seemed like just enough of a gotcha to deserve it's own function. And then deregisterAttribute() follows for completeness.

Checklist

• I have read the contribution guidelines.
• I have updated the documentation, if applicable.
• I have tested my change(s) in the test suite, and added new test cases where necessary.
• My code follows the Gaffer project's prevailing coding style and conventions.

[ericmehl]

This would be more succinct by returning above, instead of assigning to parameter and returning that.

[johnhaddon]

Sounds worth it to me.

[ericmehl]

Done in 6da2d7c

[johnhaddon]

Thanks Eric!

Really deregisterColumn() is all that's needed to do the actual work or removing a column, but the formatting of a parameter into a tuple seemed like just enough of a gotcha to deserve it's own function.

That seems entirely logical given the starting point, but I think the starting point was a bit odd in the first place. Way back when we only had registerParameter(), so the internal __columnRegistry was keyed by tuple (as described in the comment). I think that was a poor choice on my part, and makes no sense now that we're sometimes keying by string (for attributes) and sometimes by tuple (for parameters). So a simpler option might be to make [de]registerColumn require string keys. Then maybe it's not too much of a stretch to just pass a string as in deregisterColumn( renderer, "shader.parameter" )? Not sure I feel that strongly about this if you prefer the more fulsome API.

[johnhaddon]

We need to make sure both these are deregistered no matter how the test exits (failure, success etc). The easiest way might be to use self.addCleanup() here.

The same applies to the other two tests.

[ericmehl]

Fixed in 92dadb1

[johnhaddon]

We seem to have lost these during the later fixups.

[johnhaddon]

Sounds worth it to me.

[ericmehl]

Thanks John! I'm up for simplifying the API to just deregisterColumn(). From what I can tell, we'd also have to change the column registry key as you described, to use just a string instead of a tuple for parameter columns? I don't see how we could keep the key as a tuple and figure out whether deregisterColumn() is being given an attribute or parameter to deregister.

[johnhaddon]

From what I can tell, we'd also have to change the column registry key as you described, to use just a string instead of a tuple for parameter columns?

Yes, that is what I was expecting...

[ericmehl]

Yes, that is what I was expecting...

Great, that's done in 178dc6f. The commit history with the fixups is a bit funky with the __parseParameter() method getting moved around then removed in the end. But it will all wash out when I squash.

Note to self to check the commit messages for correctness when I squash down...

Ready for a new look.

[ericmehl]

One last fixup I made yesterday after learning about the LazyMethod.flush() capability, so now we don't need to pop up a window and waitForIdle() which is a nice improvement.

Now it's ready for a new look.

[johnhaddon]

Thanks Eric! I spotted a couple of minor omissions (see inline) while squashing the fixups in, so just addressed them as I went. Merging...

[johnhaddon]

I believe this should be "X" rather than "P.X", as that's what we asserted to exist higher up.

[johnhaddon]

We seem to have lost these during the later fixups.