Issue with ns_color_defined makeIndex argument

Issue with ns_color_defined makeIndex argument

[Alex]

I'm currently generalizing some window system-specific code into
terminal hooks, but I've run into an issue regarding ns_color_defined.

In image.c when HAVE_NS is defined, x_defined_color is defined to be:

  ns_defined_color (f, name, color_def, alloc, 0)

But in xfaces.c, defined_color calls ns_defined_color as follows:

  ns_defined_color (f, color_name, color_def, alloc, true);

My intention is to remove defined_color in xfaces.c and the #define
x_defined_color calls in image.c and to replace each call with:

 FRAME_TERMINAL (f)->defined_color_hook (f, color_name, color_def, alloc)

The issue here is that the calls in image.c have makeIndex as 0, and the
calls in xfaces.c as true; is this the intended behaviour?

If so, does anyone have any ideas to rectify this besides adding an
extra argument that non-NS window systems just ignore? I'd rather avoid
doing that.

Re: Issue with ns_color_defined makeIndex argument

[Alan Third]

On Thu, Apr 11, 2019 at 10:05:28AM -0600, Alex wrote:
> I'm currently generalizing some window system-specific code into
> terminal hooks, but I've run into an issue regarding ns_color_defined.
> 
> In image.c when HAVE_NS is defined, x_defined_color is defined to be:
> 
>   ns_defined_color (f, name, color_def, alloc, 0)
> 
> But in xfaces.c, defined_color calls ns_defined_color as follows:
> 
>   ns_defined_color (f, color_name, color_def, alloc, true);
> 
> My intention is to remove defined_color in xfaces.c and the #define
> x_defined_color calls in image.c and to replace each call with:
> 
>  FRAME_TERMINAL (f)->defined_color_hook (f, color_name, color_def, alloc)
> 
> The issue here is that the calls in image.c have makeIndex as 0, and the
> calls in xfaces.c as true; is this the intended behaviour?

I’m not familiar with this area of the code, and a review hasn’t done
anything to enlighten me. This code has been in since the NS port was
merged, so there’s no explanatory reasoning in commit messages.

If I’m reading the code right, when we lookup a colour in image.c, we
store a 32bit integer representation of the colour in color_def.pixel,
but when doing a lookup from xfaces.c, we store an index into a lookup
table in color_def.pixel.

> If so, does anyone have any ideas to rectify this besides adding an
> extra argument that non-NS window systems just ignore? I'd rather avoid
> doing that.

I have no ideas, I’m afraid.
-- 
Alan Third

Re: Issue with ns_color_defined makeIndex argument

[Alex Gramiak]

Alan Third <alan@idiocy.org> writes:

> I’m not familiar with this area of the code, and a review hasn’t done
> anything to enlighten me. This code has been in since the NS port was
> merged, so there’s no explanatory reasoning in commit messages.
>
> If I’m reading the code right, when we lookup a colour in image.c, we
> store a 32bit integer representation of the colour in color_def.pixel,
> but when doing a lookup from xfaces.c, we store an index into a lookup
> table in color_def.pixel.
>
>> If so, does anyone have any ideas to rectify this besides adding an
>> extra argument that non-NS window systems just ignore? I'd rather avoid
>> doing that.
>
> I have no ideas, I’m afraid.

Alright, I just made a wrapper procedure to accommodate NS instead.

Thanks for looking at this.