Support user-defined styles via tb_set_uattr_func and TB_UATTR

[adsr]

See #120 (review)

[txgk]

Hi, Adam

I like the idea very much and I feel eager to apply this functionality in Newsraft.

However, I see several points that could be improved in the current patch.

1. UATTR doesn't speak for itself very clearly. I'd suggest TB_USERATTR, fn_userattr, tb_set_userattr_func, etc. but that's just a nitpick.

2. The API is not flexible enough for developer to decide what functionality of terminal they want to overwrite with their user attributes. Currently you propose to overwrite all upper 32 bits with hard-coded mask of 0xffffffff00000000. Despite the fact that it doesn't cause any inconvenience now, it seems to me that in the future several useful attributes may easily appear in the upper 32 bits, and I would like to continue using both these new attributes and my custom user attributes. The idea is simple:

Make developer decide the mask for their user attributes. Imagine they only need 3 custom attributes on bits 42, 51 and 53 (i.e. TB_USERATTR_MASK of 0x28040000000000) and don't want to overwrite anything else. They could just return the mask they need for that from global.fn_userattr and termbox2 will kindly disable only these attributes selected by the developer, and not all 32 bits which may contain some useful stuff in the future. It also looks nice from the architecture standpoint - we only use as much bits as we need.

3. I don't see the explanation of ownership/lifetime for string returned from global.fn_userattr. In the current patch the developer is responsible for freeing up the memory of out since it's not done in send_attr, but they don't know when they are allowed to do the cleaning (i.e. when termbox2 has finished processing it). My vision for the solution is the following:

To avoid introducing callbacks for memory management, I'd make developer write to the global.out from global.fn_userattr directly. We just need to give them a function pointer to termbox2 function which does the bytebuf_nputs(&global.out, buf, len).

Here's an example:

int my_mega_userattr_callback(uintattr_t fg, uintattr_t bg, uintattr_t *mask, int (*add)(const char *, size_t))
{
	bool is_attr1_set = fg & (1 << 42);
	bool is_attr2_set = fg & (1 << 51);
	bool is_attr3_set = fg & (1 << 53);

	char *my_sequence = generate_sequence(is_attr1_set, is_attr2_set, is_attr3_set);

	// termbox2 gives us the function to write data into internal buffer
	int status = add(my_sequence, strlen(my_sequence));

	// all memory management is done by the user
	free(my_sequence);

	*mask = (1 << 42) | (1 << 51) | (1 << 53);

	return status;
}

Let me know what you think :^)

Best regards, Grigory

[adsr]

Hi Grigory, thanks for the review. I haven't forgot about your comment, just haven't had time yet to review it.

[NikitaIvanovV]

I wonder if one could implement sixel support using this...

[adsr]

3. I don't see the explanation of ownership/lifetime for string returned from global.fn_userattr. In the current patch the developer is responsible for freeing up the memory of out since it's not done in send_attr, but they don't know when they are allowed to do the cleaning (i.e. when termbox2 has finished processing it). My vision for the solution is the following:

Agree, good point. My first thought was to pass a generously sized buffer to the callback, however it may not be big enough if the caller wants to output a large sixel image or something. At the moment I can't think of a better solution than what you've provided.

2. ... I would like to continue using both these new attributes and my custom user attributes.

What do you think about not applying a mask at all? The caller can simply avoid whichever attrs they don't need, and steal those bits for use in their callback. We can define a macro that resembles the first available bit for those that don't want to sacrifice any built-in ones. Any downside to this?

1. UATTR doesn't speak for itself very clearly. I'd suggest TB_USERATTR, fn_userattr, tb_set_userattr_func, etc. but that's just a nitpick.

Fair. I was going for something like udata which is a common name for "user data", or like the array_udiff function in PHP which takes a user callback, but maybe not everyone finds this intuitive.

Thanks again for the feedback and apologies for the long delay!