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

Reset foreground and background while only comma is specified #742

Closed
wants to merge 1 commit into from

Conversation

SilverRainZ
Copy link

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

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

@SilverRainZ
Copy link
Author

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

@ailin-nemui
Copy link
Contributor

ailin-nemui commented Aug 31, 2017

we had this bug: irssi-import/bugs.irssi.org#250

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

@SilverRainZ
Copy link
Author

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

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

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

@SilverRainZ
Copy link
Author

Well, now it act like mirc.

@ailin-nemui
Copy link
Contributor

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants