Allow selection of what kind of activity targets to ignore v2 #779

vague666 · 2017-10-24T10:07:37Z

Syntax:

 ::all	 	        Ignore activity in all windows
 ::channels       	Ignore activity in all channels
 ::queries  		Ignore activity in all queries
 ::dccqueries    	Ignore activity in all dcc chats
 #chan|[=]nick  	Ignore activity in named target(channel, query, dcc chat)
 tag/::all	        Ignore all activity on network 'tag'
 tag/::channels 	Ignore activity in all channels on network 'tag'
 tag/::queries	        Ignore activity in all queries on network 'tag'
 tag/::dccqueries	Ignore activity in all dcc chats on network 'tag'
 tag/#chan|[=]nick	Ignore activity in named channel/query/dcc chat on network 'tag'

dequis · 2017-10-25T01:28:31Z

src/fe-common/core/fe-common-core.c

+
+	char **tmp;
+	for (tmp = array; *tmp != NULL; tmp++) {
+		char *tagtarget;


Set this to NULL, otherwise it's uninitialized access down there

LemonBoy · 2017-10-25T19:09:39Z

src/fe-common/core/fe-common-core.c

@@ -462,25 +462,73 @@ void fe_common_core_finish_init(void)
 gboolean strarray_find_dest(char **array, const TEXT_DEST_REC *dest)
 {
 	g_return_val_if_fail(array != NULL, FALSE);
+	WI_ITEM_REC *item = window_item_find_window(dest->window, dest->server, dest->target);
+	int channel_type = module_get_uniq_id_str("WINDOW ITEM TYPE", "CHANNEL");


This, together with query_type, can be made static and moved to the top of the function since we don't expect those to change between invocations.

LemonBoy · 2017-10-25T19:12:32Z

src/fe-common/core/fe-common-core.c

@@ -462,25 +462,73 @@ void fe_common_core_finish_init(void)
 gboolean strarray_find_dest(char **array, const TEXT_DEST_REC *dest)
 {
 	g_return_val_if_fail(array != NULL, FALSE);
+	WI_ITEM_REC *item = window_item_find_window(dest->window, dest->server, dest->target);


This line assumes that:

dest is not NULL

dest->window is not NULL

dest->target is not NULL

I don't know right off the bat how likely is to end up in those situations but I'd add some checks to avoid a nasty crash.

LemonBoy · 2017-10-25T19:15:58Z

src/fe-common/core/fe-common-core.c

-		g_free(tagtarget);
-		if (ret != -1)
-			return TRUE;
+		if (item->type && item->type == channel_type) {


What's the item->type check for?

It's a perl-ism :P I'll add a check for not null

LemonBoy · 2017-10-25T19:18:24Z

src/fe-common/core/fe-common-core.c


-	if (strarray_find(array, "*") != -1)
-		return TRUE;
+		if (dest->server_tag != NULL) {


I think that you can just return FALSE if server_tag == NULL since without that you can't really match anything (beside the "bare" #, = and @) and it'd allows to simplify the following code quite a bit.
Better ask @ailin-nemui @dequis

LemonBoy · 2017-10-25T19:23:16Z

src/fe-common/core/fe-common-core.c

+			g_free(tagtarget);
+		}
+		else if (item->type && item->type == query_type) {
+			if (g_str_has_prefix(dest->target, "=")) {


If you stare at the two branches of the if you can see that the code is the same, minus the "=" vs "@".
Can you fold the two branches together?

dequis

This changed a lot since the last time I looked at it, nice. Haven't verified that it still does what it is intended to do.

dequis · 2017-10-26T11:24:16Z

src/fe-common/core/fe-common-core.c

-	if (strarray_find(array, dest->target) != -1)
-		return TRUE;
+	for (tmp = array; *tmp != NULL; tmp++) {
+		if (dest->server_tag != NULL && g_str_has_prefix(g_ascii_strdown(*tmp, -1), g_ascii_strdown(dest->server_tag, -1))) {


Both g_ascii_strdown leak, consider g_ascii_strncasecmp() with the length parameter set to the short one

dequis · 2017-10-26T11:24:50Z

src/fe-common/core/fe-common-core.c

-		return TRUE;
+	for (tmp = array; *tmp != NULL; tmp++) {
+		if (dest->server_tag != NULL && g_str_has_prefix(g_ascii_strdown(*tmp, -1), g_ascii_strdown(dest->server_tag, -1))) {
+			*tmp = (g_strsplit(*tmp, "/", 2))[1];


g_strsplit leaks (requires g_strfreev), consider strchr() + null result check + add 1 to get the string after /

LemonBoy

I like this, the cpp-namespace-ish syntax not so much :)

LemonBoy · 2017-11-02T08:23:12Z

src/fe-common/core/fe-common-core.c

-	if (strarray_find(array, "*") != -1)
-		return TRUE;
+	WI_ITEM_REC *item = window_item_find_window(dest->window, dest->server, dest->target);
+	g_return_val_if_fail(item != NULL, FALSE);


Turn this into a plain check + return, we use the g_return_val form to assert the pre-conditions on the arguments

LemonBoy · 2017-11-02T08:23:16Z

src/fe-common/core/fe-common-core.c

+		if (*str == '\0') {
+			continue;
+		}
+		if (dest->server_tag != NULL && (slash = strchr(str, '/'))) {


// compute this only once outside the loop int server_tag_len = dest->server_tag ? strlen(dest->server_tag) : 0; ... if (server_tag_len && !g_ascii_strncasecmp(str, dest->server_tag, server_tag_len) && str[server_tag_len] == '/') { str += server_tag_len + 1; }

LemonBoy · 2017-11-02T08:23:36Z

src/fe-common/core/fe-common-core.c

-		return TRUE;
+	for (tmp = array; *tmp != NULL; tmp++) {
+		char *str = *tmp;
+		if (*str == '\0') {


Nice catch!

LemonBoy · 2017-11-02T08:23:53Z

src/fe-common/core/fe-common-core.c

-		int ret = strarray_find(array, tagtarget);
-		g_free(tagtarget);
-		if (ret != -1)
+		if (g_strcmp0(str, "") == 0 || g_strcmp0(str, "::all") == 0) {


Please use the !g_strcmp0 form

@LemonBoy please don't confuse everybody, we use == 0 everywhere in Irssi

LemonBoy · 2017-11-02T08:23:56Z

src/fe-common/core/fe-common-core.c

 			return TRUE;
+		} else if (item->type == query_type &&
+			(dest->target[0] == '=' ?
+			g_strcmp0(str, "::dccqueries") == 0 : g_strcmp0(str, "::queries") == 0)) {


You can shorten the code by swapping the ternary operator and the second argument to g_strcmp0:

g_strcmp0(str, (dest->target[0] == '=') ? "::dccqueries: : "::queries")

Initialize tagtarget on declaration move code around for better flow, extra checks for uninitialized values remove unnecessary item->type checks don't strdup sign add braces around if statements, use strcmp0 with single characters and remove g_str_has_prefix refactoring changed g_ascii_strcasecmp to g_strcmp0 Add networktag/ shorthand fixed memory leaks changed from #@= to ::channels, ::queries and ::dccqueries check for empty string and continue; if found fixed bug with empty string check Clean up code

ailin-nemui · 2018-01-04T23:42:32Z

@LemonBoy @dequis is this good to go?

LemonBoy · 2018-01-05T11:16:54Z

LGTM, but I think another pair of eyes should have a look before it's merged

ailin-nemui · 2018-01-06T19:02:55Z

@irssi/developers

fix mixed decls

dequis reviewed Oct 25, 2017

View reviewed changes

LemonBoy reviewed Oct 25, 2017

View reviewed changes

LemonBoy mentioned this pull request Oct 25, 2017

Allow selection of what kind of activity targets to ignore #612

Closed

dequis reviewed Oct 26, 2017

View reviewed changes

LemonBoy reviewed Nov 2, 2017

View reviewed changes

Update fe-common-core.c

4d5982f

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

ailin-nemui and others added 2 commits January 6, 2018 20:17

Update fe-common-core.c

121a497

fix mixed decls

fe-common-core: fix redeclaration of server_tag_len

f9c8365

ailin-nemui merged commit f83ba5a into irssi:master Jan 8, 2018

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

Allow selection of what kind of activity targets to ignore v2 #779

Allow selection of what kind of activity targets to ignore v2 #779

Uh oh!

Conversation

vague666 commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dequis left a comment

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!

LemonBoy left a comment

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 Jan 4, 2018

Uh oh!

LemonBoy commented Jan 5, 2018

Uh oh!

ailin-nemui commented Jan 6, 2018

Uh oh!

Uh oh!

vague666 commented Oct 24, 2017 •

edited

Loading