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

Reset foreground and background while only comma is specified #742

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@SilverRainZ

SilverRainZ commented Aug 31, 2017

Currently irssi can not correctly process ^C, which is a valid mirc color code.

For ^C4,2ir^C,ssi, the foreground color was inherited and left a comma:

before

But ^C, has neither foreground nor background, it should be treated as same as ^C:

after

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 31, 2017

does anyone have a mIRC ready to compare if it should be ir,ssi or irssi? does this fix issue #740 ?

@SilverRainZ

This comment has been minimized.

SilverRainZ commented Aug 31, 2017

@ailin-nemui I have not a mIRC for testing so I only tested on hexchat and srain.
It really fixes #740.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 31, 2017

we had this bug: http://bugs.irssi.org/index.php?do=details&task_id=250

here is the comparison: we need to fix something, but maybe differently: mirc irssi

@SilverRainZ

This comment has been minimized.

SilverRainZ commented Aug 31, 2017

The one on the right is mIRC? It does't regard the comma as part of color code in ^C,, which means that the foreground color is never inherited -- I think it is unreasonable.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 31, 2017

I think mIRC is the "de facto" standard on mIRC colour codes so we may need to accept the fact that the foreground can not be inherited, ^C, is ^C then,...

@dequis

This comment has been minimized.

Member

dequis commented Aug 31, 2017

irssi:

scrot

mirc 7.47:

scrot

hexchat

scrot

weechat

scrot

After doing this I realized there are other more interesting test cases up there. Welp. I'll get to that later.

Regardless of the decisions made here: any changes to this code must go through fuzzing before they are merged. This is particularly fragile code.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Aug 31, 2017

I think the changes itself are good code wise but they don't match mirc

@SilverRainZ

This comment has been minimized.

SilverRainZ commented Sep 21, 2017

Well, now it act like mirc.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Sep 21, 2017

how about this:

diff --git a/src/fe-common/core/formats.c b/src/fe-common/core/formats.c
index 005e6fb7..a340bffa 100644
--- a/src/fe-common/core/formats.c
+++ b/src/fe-common/core/formats.c
@@ -1072,7 +1072,8 @@ static void get_mirc_color(const char **str, int *fg_ret, int *bg_ret)
 	fg = fg_ret == NULL ? -1 : *fg_ret;
 	bg = bg_ret == NULL ? -1 : *bg_ret;
 
-	if (!i_isdigit(**str) && **str != ',') {
+	if (!i_isdigit(**str)) {
+		/* turn off color */
 		fg = -1;
 		bg = -1;
 	} else {
@@ -1085,11 +1086,8 @@ static void get_mirc_color(const char **str, int *fg_ret, int *bg_ret)
 				(*str)++;
 			}
 		}
-		if (**str == ',') {
+		if ((*str)[0] == ',' && i_isdigit((*str)[1])) {
 			/* background color */
-			if (!i_isdigit((*str)[1]))
-				bg = -1;
-			else {
 				(*str)++;
 				bg = **str-'0';
 				(*str)++;
@@ -1097,7 +1095,6 @@ static void get_mirc_color(const char **str, int *fg_ret, int *bg_ret)
 					bg = bg*10 + (**str-'0');
 					(*str)++;
 				}
-			}
 		}
 	}
 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment