Add color support for input bar #764

GinjaNinja32 · 2017-10-06T17:44:00Z

This is mostly a rebase of this patch, with a few bugfixes (a segfault in gui_entry_transpose_words, and a pointer-to-int assignment).

Allows scripts to specify that ranges of characters in the input bar should be rendered in specific colors; the original intent for this was to implement a spellchecker, my use is to colorise nicks in the input bar.

Example script using this, mostly based on my rewritten colorize_nicks.pl: https://thanatos.gn32.uk/f/colorise_input.pl (I'll likely clean up the code a little and submit for scripts.irssi.org, assuming this PR gets merged)

LemonBoy · 2017-10-06T19:10:41Z

src/fe-text/gui-entry.c

 	term_set_color(root_window, ATTR_RESET);
 	term_move(root_window, xpos, entry->ypos);

-	for (i = entry->scrstart + pos; i < entry->text_len; i++) {
+	col = entry->scrstart + pos < entry->text_len ?
+			entry->colors + entry->scrstart + pos : 0;


s/0/NULL/

LemonBoy · 2017-10-06T19:11:53Z

src/fe-text/gui-entry.c

@@ -266,14 +274,19 @@ static void gui_entry_draw_from(GUI_ENTRY_REC *entry, int pos)
 		if (xpos > end_xpos)
 			break;

+		if (*col != color) {


If col is set to NULL above you get a NPE.

I think the intention was that col is only set to NULL if the loop won't run.

Perhaps this should instead use entry->colors[i], since col is equal to entry->colors + i at all points during the loop?

That's much better!

LemonBoy · 2017-10-06T19:13:57Z

src/fe-text/gui-entry.c

@@ -505,6 +522,10 @@ void gui_entry_insert_text(GUI_ENTRY_REC *entry, const char *str)
 		}
 	}

+	for(i = 0; i < len; i++) {


Missing space after for.

LemonBoy · 2017-10-06T19:16:01Z

src/fe-text/gui-entry.c

@@ -700,6 +726,9 @@ void gui_entry_erase(GUI_ENTRY_REC *entry, int size, CUTBUFFER_UPDATE_OP update_
 	g_memmove(entry->text + entry->pos - size, entry->text + entry->pos,
 		  (entry->text_len-entry->pos+1) * sizeof(unichar));

+	g_memmove(entry->colors + entry->pos - size, entry->colors + entry->pos,


The memmove above has a +1 that's missing here, why is that?

I'm not entirely sure. This is straight from the original patch.

Is entry->text NULL-terminated, and entry->colors implicitly has the same number of elements?

That's it, sorry for the noise.

LemonBoy · 2017-10-06T19:16:19Z

src/fe-text/gui-entry.c

-		  (entry->text_len-entry->pos-size+1) * sizeof(unichar));
+	          (entry->text_len-entry->pos-size+1) * sizeof(unichar));
+
+	g_memmove(entry->colors + entry->pos, entry->colors + entry->pos + size,


Ditto as before.

LemonBoy · 2017-10-06T19:18:09Z

src/fe-text/gui-entry.c

+		g_free(first_color);
+		g_free(sep_color);
+		g_free(second_color);
+


Stray newline.

LemonBoy · 2017-10-06T19:19:24Z

src/fe-text/gui-entry.c

+	for (i = pos; i < end; i++) {
+		if (entry->colors[i] != color) {
+			entry->colors[i] = color;
+			update = 1;


s/1/TRUE/

I assume FALSE on L1109 too?

LemonBoy · 2017-10-07T07:53:01Z

src/fe-text/gui-entry.c

@@ -700,6 +726,9 @@ void gui_entry_erase(GUI_ENTRY_REC *entry, int size, CUTBUFFER_UPDATE_OP update_
 	g_memmove(entry->text + entry->pos - size, entry->text + entry->pos,
 		  (entry->text_len-entry->pos+1) * sizeof(unichar));

+	g_memmove(entry->colors + entry->pos - size, entry->colors + entry->pos,


That's it, sorry for the noise.

LemonBoy · 2017-10-07T07:53:18Z

src/fe-text/gui-entry.c

+	for (i = pos; i < end; i++) {
+		if (entry->colors[i] != color) {
+			entry->colors[i] = color;
+			update = 1;


LemonBoy · 2017-10-07T07:56:36Z

src/fe-text/gui-entry.c

@@ -266,14 +274,19 @@ static void gui_entry_draw_from(GUI_ENTRY_REC *entry, int pos)
 		if (xpos > end_xpos)
 			break;

+		if (*col != color) {


That's much better!

LemonBoy · 2017-10-07T08:05:35Z

src/fe-text/gui-entry.c

@@ -1042,3 +1100,31 @@ void gui_entry_redraw(GUI_ENTRY_REC *entry)
 	gui_entry_fix_cursor(entry);
 	gui_entry_draw(entry);
 }
+
+void gui_entry_set_color(GUI_ENTRY_REC *entry, int pos, int len, int color)


I think we should add a couple of checks to make sure pos and len are >= 0 to avoid some potential memory corruption bugs due to careless scripts.

LemonBoy · 2017-10-07T08:06:17Z

src/fe-text/gui-entry.c

@@ -1042,3 +1103,31 @@ void gui_entry_redraw(GUI_ENTRY_REC *entry)
 	gui_entry_fix_cursor(entry);
 	gui_entry_draw(entry);
 }
+
+void gui_entry_set_color(GUI_ENTRY_REC *entry, int pos, int len, int color)


I think we should add a couple of checks to make sure pos and len are >= 0 to avoid some potential memory corruption bugs due to careless scripts.

Done in 8326588

ailin-nemui · 2017-10-11T05:37:32Z

seems mostly ok, but I don't like having to use the internal colour attributes from XS -- maybe we can add in some functions to do colour code conversions between \004, % and this. And another thing: it doesn't do 24bit "true colour" (api is term_set_color2, didn't exist back then)

The more general question is if we like this implementation. Alternative would be to have in-line colours like in print text / gui prompt. But this code here is much simpler.

In general I think it would be a nice to have feature for your and the other use case

LemonBoy · 2017-10-11T07:44:58Z

maybe we can add in some functions to do colour code conversions between \004, % and this.

What if we reuse the same parsing logic that's used for themes formats? It's not that good but at least spares us from writing another parser and having the script writers learn a new set of specifiers.

Alternative would be to have in-line colours

The principle of separation between data and metadata applies here.
A script may as well render the inline colours though, that's an interesting idea.

other use case

I'm using it to turn the whole prompt red whenever there's a cmdchar with leading rubbish, no more accidental commands for me 😃

ailin-nemui · 2017-10-11T10:25:06Z

I'm using it to turn the whole prompt red whenever there's a cmdchar with leading rubbish, no more accidental commands for me

if we had something more generic like in line colours then it could offer more flexibility like also in line altering of the actual displayed text. think of a blinking "warning" appended to the prompt or an in line display of spelling suggestions after the unrecognised word

GinjaNinja32 · 2017-10-11T17:27:48Z

I'm not entirely sure how inline colors would work here.

Using format codes instead of ints sounds good, though. get_nick_color2 appears to return \004-style codes, so it'd be good if it accepts those - perhaps a string containing either a \cD.. code or a %#/%X## code?

I have no idea how I'd implement that, but I can take a look and poke around IRC and see if I can put something together.

ailin-nemui · 2017-10-13T15:25:57Z

just conceptually speaking, one way to think about inline colours would be to take the input

 /whois xxx

then call a function (e.g. XS) which would change it to

[NOT COMMAND!]  /whois xxx

and that's what would then be visible. This would demand the callback to transform "input" into "displayed input" to be called at basically every change to the prompt

colours would then be realised by "returning"

%R /whois xxx

from the signal handler. There /should/ be some methods in irssi (which may need to be moved to some util/misc function) that turn %codes into the appropriate attributes

ailin-nemui · 2017-10-23T12:35:25Z

todo: change XS to work on %-codes (maybe exposing some functions for \004 conversion at the same time)

ailin-nemui · 2018-01-10T23:47:01Z

first version, still some errors, feel free to play with it and fix the remaining bugs

ailin-nemui · 2018-01-18T22:28:51Z

Irssi::gui_input_clear_extents(pos, len)
Irssi::gui_input_get_extent(pos)
Irssi::gui_input_set_extents(pos, len, left, right)

ailin-nemui · 2018-01-19T00:01:44Z

http://anti.teamidiot.de/static/nei/*/Code/Irssi/spellcheck.pl

ailin-nemui · 2018-01-19T09:34:56Z

@GinjaNinja32 I changed the api again slightly,extent -> extents. hope you can test this again and discuss any usability issues that we may need to fix before merge

ailin-nemui · 2018-02-02T13:06:48Z

@irssi/developers I think we should merge this

LemonBoy reviewed Oct 6, 2017

View reviewed changes

src/fe-text/gui-entry.c Outdated

g_free(first_color);

g_free(sep_color);

g_free(second_color);

Copy link

Member

LemonBoy Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray newline.

LemonBoy reviewed Oct 6, 2017

View reviewed changes

LemonBoy reviewed Oct 7, 2017

View reviewed changes

ailin-nemui force-pushed the colorful-input branch from 8326588 to 2ff6a07 Compare January 10, 2018 23:46

GinjaNinja32 added 3 commits January 18, 2018 10:08

Add color support for input bar

925ae6e

Apply changes based on review feedback

13b93c8

Don't accept pos<0 or len<0 in gui_entry_set_color

0f1b387

ailin-nemui force-pushed the colorful-input branch 3 times, most recently from 99e2257 to 3e4e52b Compare January 18, 2018 22:27

ailin-nemui force-pushed the colorful-input branch from 5f89de5 to 7ce9de4 Compare January 19, 2018 00:21

ailin-nemui added needs review needs testing labels Jan 19, 2018

change color -> extents (malloc based)

9072e98

ailin-nemui force-pushed the colorful-input branch from 7ce9de4 to 9072e98 Compare February 2, 2018 12:57

ailin-nemui added the auto-merge This PR is scheduled for merge if no further comments are opened label Feb 2, 2018

up abi

2185c4d

ailin-nemui merged commit 8183180 into irssi:master Feb 5, 2018

ailin-nemui mentioned this pull request Jun 17, 2019

restore 8bit support in input entry #1057

Merged

ailin-nemui added this to the 1.2.0 milestone Jun 26, 2019

Add color support for input bar #764

Add color support for input bar #764

Uh oh!

Conversation

GinjaNinja32 commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ailin-nemui commented Oct 11, 2017

Uh oh!

LemonBoy commented Oct 11, 2017

Uh oh!

ailin-nemui commented Oct 11, 2017

Uh oh!

GinjaNinja32 commented Oct 11, 2017

Uh oh!

ailin-nemui commented Oct 13, 2017

Uh oh!

ailin-nemui commented Oct 23, 2017

Uh oh!

ailin-nemui commented Jan 10, 2018

Uh oh!

ailin-nemui commented Jan 18, 2018

Uh oh!

ailin-nemui commented Jan 19, 2018

Uh oh!

ailin-nemui commented Jan 19, 2018

Uh oh!

ailin-nemui commented Feb 2, 2018

Uh oh!

Uh oh!