Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add color support for input bar #764

Merged
merged 5 commits into from Feb 5, 2018
Merged

Conversation

@GinjaNinja32
Copy link
Contributor

@GinjaNinja32 GinjaNinja32 commented Oct 6, 2017

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)

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;

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

s/0/NULL/

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

if (*col != color) {

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

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

This comment has been minimized.

@GinjaNinja32

GinjaNinja32 Oct 6, 2017
Author Contributor

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?

This comment has been minimized.

@LemonBoy

LemonBoy Oct 7, 2017
Member

That's much better!

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

Missing space after for.

@@ -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,

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

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

This comment has been minimized.

@GinjaNinja32

GinjaNinja32 Oct 6, 2017
Author Contributor

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?

This comment has been minimized.

@LemonBoy

LemonBoy Oct 7, 2017
Member

That's it, sorry for the noise.

(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,

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

Ditto as before.

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

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

Stray newline.

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

This comment has been minimized.

@LemonBoy

LemonBoy Oct 6, 2017
Member

s/1/TRUE/

This comment has been minimized.

@GinjaNinja32

GinjaNinja32 Oct 6, 2017
Author Contributor

I assume FALSE on L1109 too?

This comment has been minimized.

@@ -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,

This comment has been minimized.

@LemonBoy

LemonBoy Oct 7, 2017
Member

That's it, sorry for the noise.

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

This comment has been minimized.

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

if (*col != color) {

This comment has been minimized.

@LemonBoy

LemonBoy Oct 7, 2017
Member

That's much better!

@@ -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)

This comment has been minimized.

@LemonBoy

LemonBoy Oct 7, 2017
Member

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.

@@ -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)

This comment has been minimized.

@LemonBoy

LemonBoy Oct 7, 2017
Member

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.

This comment has been minimized.

@GinjaNinja32

GinjaNinja32 Oct 7, 2017
Author Contributor

Done in 8326588

@ailin-nemui
Copy link
Contributor

@ailin-nemui ailin-nemui commented Oct 11, 2017

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
Copy link
Member

@LemonBoy LemonBoy commented Oct 11, 2017

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
Copy link
Contributor

@ailin-nemui ailin-nemui commented Oct 11, 2017

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
Copy link
Contributor Author

@GinjaNinja32 GinjaNinja32 commented Oct 11, 2017

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
Copy link
Contributor

@ailin-nemui ailin-nemui commented Oct 13, 2017

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
Copy link
Contributor

@ailin-nemui ailin-nemui commented Oct 23, 2017

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

@ailin-nemui ailin-nemui force-pushed the GinjaNinja32:colorful-input branch from 8326588 to 2ff6a07 Jan 10, 2018
@ailin-nemui
Copy link
Contributor

@ailin-nemui ailin-nemui commented Jan 10, 2018

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

@ailin-nemui ailin-nemui force-pushed the GinjaNinja32:colorful-input branch 3 times, most recently from 99e2257 to 3e4e52b Jan 18, 2018
@ailin-nemui
Copy link
Contributor

@ailin-nemui ailin-nemui commented Jan 18, 2018

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

@ailin-nemui ailin-nemui force-pushed the GinjaNinja32:colorful-input branch from 5f89de5 to 7ce9de4 Jan 19, 2018
@ailin-nemui
Copy link
Contributor

@ailin-nemui ailin-nemui commented Jan 19, 2018

@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 ailin-nemui force-pushed the GinjaNinja32:colorful-input branch from 7ce9de4 to 9072e98 Feb 2, 2018
@ailin-nemui
Copy link
Contributor

@ailin-nemui ailin-nemui commented Feb 2, 2018

@irssi/developers I think we should merge this

@ailin-nemui ailin-nemui merged commit 8183180 into irssi:master Feb 5, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ailin-nemui ailin-nemui added this to the 1.2.0 milestone Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.