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

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

Merged
merged 4 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@vague666
Member

vague666 commented Oct 24, 2017

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'
char **tmp;
for (tmp = array; *tmp != NULL; tmp++) {
char *tagtarget;

This comment has been minimized.

@dequis

dequis Oct 25, 2017

Member

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

This comment has been minimized.

@vague666
@@ -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 comment has been minimized.

@LemonBoy

LemonBoy Oct 25, 2017

Member

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.

@@ -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 comment has been minimized.

@LemonBoy

LemonBoy Oct 25, 2017

Member

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.

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

This comment has been minimized.

@LemonBoy

LemonBoy Oct 25, 2017

Member

What's the item->type check for?

This comment has been minimized.

@vague666

vague666 Oct 25, 2017

Member

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Oct 25, 2017

Member

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Oct 25, 2017

Member

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.

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

This comment has been minimized.

@dequis

dequis Oct 26, 2017

Member

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

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];

This comment has been minimized.

@dequis

dequis Oct 26, 2017

Member

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Nov 2, 2017

Member

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Nov 2, 2017

Member
// 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;
}
return TRUE;
for (tmp = array; *tmp != NULL; tmp++) {
char *str = *tmp;
if (*str == '\0') {

This comment has been minimized.

@LemonBoy

LemonBoy Nov 2, 2017

Member

Nice catch!

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

This comment has been minimized.

@LemonBoy

LemonBoy Nov 2, 2017

Member

Please use the !g_strcmp0 form

This comment has been minimized.

@ailin-nemui

ailin-nemui Jan 6, 2018

Contributor

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Nov 2, 2017

Member

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")
Jari Matilainen
Allow selection of what kind of activity targets to ignore
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

This comment has been minimized.

Contributor

ailin-nemui commented Jan 4, 2018

@LemonBoy @dequis is this good to go?

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Jan 5, 2018

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

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jan 6, 2018

ailin-nemui and others added some commits Jan 6, 2018

Update fe-common-core.c
fix mixed decls

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment