When building some UI menus, I wanted to get a list of defined roles. Config::getRoleNameByIndex() only returned the resolved colorspace name.

--snip--
Replaced config->getRoleNameByIndex (which returned the colorspace name, not the role name) with the following:

- getRoleName(int): get the role name at index, this will return values like 'scene_linear', 'compositing_log'. Returns NULL if index is out of range

- getRoleColorSpaceName(int): get the actual colorspace name at role index. Returns NULL when index is out of range

- getRoleColorSpaceName(char*): get the actual colorspace name from a role name. Returns NULL if roles is not defined
--snip--

https://github.com/malcolmhumphreys/OpenColorIO/commit/d7c8094f6aad57f980060e87d0f547ff36d844b7

.malcolm

Thanks!

I like the intent, but have two minor issues:

First, the trivial: I'd prefer to never allow NULL values as return
types from const char * functions. Please use an empty string ""
instead.

The reason being, OCIO expects the client to copy the returned string
value, if the client intends to hang onto it. So it's often convenient
in your code to do a directly assign the function result to a
std::string,

std::string rolename = config->getRoleNameByIndex(i);

If the API promises to never return a NULL value, this will work
great. However, if OCIO can return a null pointer than the above code
will crash. Thus for all char * return functions the user would be
required to explicitly add a null check. This is probably counter to
user expectations, and I expect that a few would slip through and
result in occasionally crashy client code. (This would be likely
because most of the time it *would* work fine. Only on invalid
configurations, or other corner cases, would this sharp corner be run
into. I'd like to keep OCIO as friendly as reasonable.)

(See http://github.com/imageworks/OpenColorIO/issues#issue/20 for more
thoughts on this topic).

The second issue is that you've exposed two helper functions
(getRoleColorSpaceName), that are not quite as helpful as they appear.
Is the client supposed to use these at runtime to determine which
colorspace to use for a given role?

Allow me to answer... "No!"

:)

ConstColorSpaceRcPtr getColorSpace(const char * name) const;
already does this in the 'correct' way, and this should be the *only*
allowed mechanism for getting a ColorSpace from a name. It
encapsulates a bit of extra mojo your new function does not, such as
looking at the 'strictParsing', and if allowed returning the DEFAULT
colorspace role in fall-through situations. It also resolves the
ambiguity where a role name and colorspace name overlap, and having
this resolution only defined in a single place seems preferable.

What if we renamed your functions to make it super clear that they are
not intended for color-space resolution, but instead as configuration
setup/ui purposes?

I.e.
setRole(const char * role, const char * colorSpaceName);
const char * getRoleName(int index);
const char * getRoleValue(int index);

Even though the role 'value' is a colorspace name, I think the
function would be far less tempting for the naive API user to
accidentally call.

If we'd like a convenience function to do the resolution, we could add
const char * getColorSpaceName(const char * name) const;

which would internally rely on config::getColorSpace.

-- Jeremy

On Sun, Jan 16, 2011 at 8:22 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

When building some UI menus, I wanted to get a list of defined roles. Config::getRoleNameByIndex() only returned the resolved colorspace name.

--snip--
Replaced config->getRoleNameByIndex (which returned the colorspace name, not the role name) with the following:

 - getRoleName(int): get the role name at index, this will return values like 'scene_linear', 'compositing_log'. Returns NULL if index is out of range

 - getRoleColorSpaceName(int): get the actual colorspace name at role index. Returns NULL when index is out of range

 - getRoleColorSpaceName(char*): get the actual colorspace name from a role name. Returns NULL if roles is not defined
--snip--

https://github.com/malcolmhumphreys/OpenColorIO/commit/d7c8094f6aad57f980060e87d0f547ff36d844b7

.malcolm

Thanks!

I like the intent, but have two minor issues:

First, the trivial: I'd prefer to never allow NULL values as return
types from const char * functions. Please use an empty string ""
instead.

The reason being, OCIO expects the client to copy the returned string
value, if the client intends to hang onto it. So it's often convenient
in your code to do a directly assign the function result to a
std::string,

std::string rolename = config->getRoleNameByIndex(i);

If the API promises to never return a NULL value, this will work
great. However, if OCIO can return a null pointer than the above code
will crash. Thus for all char * return functions the user would be
required to explicitly add a null check. This is probably counter to
user expectations, and I expect that a few would slip through and
result in occasionally crashy client code. (This would be likely
because most of the time it *would* work fine. Only on invalid
configurations, or other corner cases, would this sharp corner be run
into. I'd like to keep OCIO as friendly as reasonable.)

(See http://github.com/imageworks/OpenColorIO/issues#issue/20 for more
thoughts on this topic).

Sure that sounds great, i'll fix that.

The second issue is that you've exposed two helper functions
(getRoleColorSpaceName), that are not quite as helpful as they appear.
Is the client supposed to use these at runtime to determine which
colorspace to use for a given role?

Allow me to answer... "No!"

:)

ConstColorSpaceRcPtr getColorSpace(const char * name) const;
already does this in the 'correct' way, and this should be the *only*
allowed mechanism for getting a ColorSpace from a name. It
encapsulates a bit of extra mojo your new function does not, such as
looking at the 'strictParsing', and if allowed returning the DEFAULT
colorspace role in fall-through situations. It also resolves the
ambiguity where a role name and colorspace name overlap, and having
this resolution only defined in a single place seems preferable.

What if we renamed your functions to make it super clear that they are
not intended for color-space resolution, but instead as configuration
setup/ui purposes?

I.e.
setRole(const char * role, const char * colorSpaceName);
const char * getRoleName(int index);
const char * getRoleValue(int index);

I don't remind removing them altogether.

I was really just adding them to match the old functionality of getRoleNameByIndex() which returned the colorspace name not the role name, I wasn't sure if you were needing the colorspace name.

Calling getColorSpace(getRoleName(0)) seems nice an clean to me.

.malcolm

Even though the role 'value' is a colorspace name, I think the
function would be far less tempting for the naive API user to
accidentally call.

If we'd like a convenience function to do the resolution, we could add
const char * getColorSpaceName(const char * name) const;

which would internally rely on config::getColorSpace.

-- Jeremy

On Sun, Jan 16, 2011 at 8:22 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

When building some UI menus, I wanted to get a list of defined roles. Config::getRoleNameByIndex() only returned the resolved colorspace name.

--snip--
Replaced config->getRoleNameByIndex (which returned the colorspace name, not the role name) with the following:

- getRoleName(int): get the role name at index, this will return values like 'scene_linear', 'compositing_log'. Returns NULL if index is out of range

- getRoleColorSpaceName(int): get the actual colorspace name at role index. Returns NULL when index is out of range

- getRoleColorSpaceName(char*): get the actual colorspace name from a role name. Returns NULL if roles is not defined
--snip--

https://github.com/malcolmhumphreys/OpenColorIO/commit/d7c8094f6aad57f980060e87d0f547ff36d844b7

.malcolm

Replaced config->getRoleNameByIndex() with config->getRoleName()
- getRoleNameByIndex returned the colorspace name not the role name, also ..ByIndex seemed redundant in the signature.

Added hasRole(role) convenience function

Commit:
https://github.com/malcolmhumphreys/OpenColorIO/commit/4825ef1c7d105378ef5ebd41609f5d3b49c2a6ac

.malcolm

LGTM - committed.

-- Jeremy

On Wed, Jan 26, 2011 at 10:34 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

Replaced config->getRoleNameByIndex() with config->getRoleName()
 - getRoleNameByIndex returned the colorspace name not the role name, also ..ByIndex seemed redundant in the signature.

Added hasRole(role) convenience function

Commit:
https://github.com/malcolmhumphreys/OpenColorIO/commit/4825ef1c7d105378ef5ebd41609f5d3b49c2a6ac

.malcolm

On 19/01/2011, at 11:12 AM, Malcolm Humphreys wrote:

Thanks!

I like the intent, but have two minor issues:

First, the trivial:  I'd prefer to never allow NULL values as return
types from const char * functions. Please use an empty string ""
instead.

The reason being, OCIO expects the client to copy the returned string
value, if the client intends to hang onto it. So it's often convenient
in your code to do a directly assign the function result to a
std::string,

std::string rolename = config->getRoleNameByIndex(i);

If the API promises to never return a NULL value, this will work
great. However, if OCIO can return a null pointer than the above code
will crash.  Thus for all char * return functions the user would be
required to explicitly add a null check.   This is probably counter to
user expectations, and I expect that a few would slip through and
result in occasionally crashy client code.  (This would be likely
because most of the time it *would* work fine.  Only on invalid
configurations, or other corner cases, would this sharp corner be run
into.  I'd like to keep OCIO as friendly as reasonable.)

(See http://github.com/imageworks/OpenColorIO/issues#issue/20 for more
thoughts on this topic).

Sure that sounds great, i'll fix that.

The second issue is that you've exposed two helper functions
(getRoleColorSpaceName), that are not quite as helpful as they appear.
Is the client supposed to use these at runtime to determine which
colorspace to use for a given role?

Allow me to answer...  "No!"

:)

ConstColorSpaceRcPtr getColorSpace(const char * name) const;
already does this in the 'correct' way, and this should be the *only*
allowed mechanism for getting a ColorSpace from a name.   It
encapsulates a bit of extra mojo your new function does not, such as
looking at the 'strictParsing', and if allowed returning the DEFAULT
colorspace role in fall-through situations. It also resolves the
ambiguity where a role name and colorspace name overlap, and having
this resolution only defined in a single place seems preferable.

What if we renamed your functions to make it super clear that they are
not intended for color-space resolution, but instead as configuration
setup/ui purposes?

I.e.
setRole(const char * role, const char * colorSpaceName);
const char * getRoleName(int index);
const char * getRoleValue(int index);

I don't remind removing them altogether.

I was really just adding them to match the old functionality of getRoleNameByIndex() which returned the colorspace name not the role name, I wasn't sure if you were needing the colorspace name.

Calling getColorSpace(getRoleName(0)) seems nice an clean to me.

.malcolm

Even though the role 'value' is a colorspace name, I think the
function would be far less tempting for the naive API user to
accidentally call.

If we'd like a convenience function to do the resolution, we could add
      const char * getColorSpaceName(const char * name) const;

which would internally rely on config::getColorSpace.

-- Jeremy

On Sun, Jan 16, 2011 at 8:22 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

When building some UI menus, I wanted to get a list of defined roles. Config::getRoleNameByIndex() only returned the resolved colorspace name.

--snip--
Replaced config->getRoleNameByIndex (which returned the colorspace name, not the role name) with the following:

- getRoleName(int): get the role name at index, this will return values like 'scene_linear', 'compositing_log'. Returns NULL if index is out of range

- getRoleColorSpaceName(int): get the actual colorspace name at role index. Returns NULL when index is out of range

- getRoleColorSpaceName(char*): get the actual colorspace name from a role name. Returns NULL if roles is not defined
--snip--

https://github.com/malcolmhumphreys/OpenColorIO/commit/d7c8094f6aad57f980060e87d0f547ff36d844b7

.malcolm