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

vague666 · 2017-01-09T10:59:39Z

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

ailin-nemui · 2017-01-09T11:04:19Z

don't like this very much, we shoulld think of some better way. It's like a quick hack :-)

vague666 · 2017-01-09T11:10:32Z

I'm not sure there is a better way. Anything else would probably involve major changes to the codebase as I see it.

LemonBoy · 2017-01-09T12:45:42Z

Perhaps by modifying activity_hide_targets syntax to allow for some finer-grained control over the filtering (eg: Freenode/* matches everything but Freenode/# matches only channels and Freenode/@ might just match the non-channels if this option makes sense), just my 2cents.

vague666 · 2017-01-09T13:12:05Z

That change wouldn't be too taxing, sure... it just needs more cowbell

LemonBoy · 2017-01-09T20:09:34Z

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

@@ -470,16 +470,23 @@ gboolean strarray_find_dest(char **array, const TEXT_DEST_REC *dest)
 		return TRUE;

 	if (dest->server_tag != NULL) {
-		char *tagtarget = g_strdup_printf("%s/%s", dest->server_tag, "*");
+		char *tagtarget = g_strdup_printf("%s/%s", dest->server_tag, "#");


All the possible tagtarget values are in the form server_tag/{#,@,*,target} so it might make sense to have a strarray_find_prefix function so that you find the first entry matching server_tag/ (the first one is the winner in case of duplicates) and then parse the part after the slash.
(please drop the previous commit and rebase this against master)

You'll have to explain the git steps one by one :) Close the PR? revert? I do not know!

You can git reset to the commit that diverged from master and then cherry-pick the changes on a pristine copy of the code 🔨

Can you expand what you mean with strarray_find_prefix when used on server_tag?

All the entries in the list for a given tag start with tagname/ so it makes sense to do a prefix search on the list and then check whether the part after the slash is a @,#,* or a valid target name.
One more detail jumped to my mind, if the user sets the list to Freenode/@,Freenode/# should we respect both even though that's better expressed as Freenode/*?

vague666 · 2017-01-09T20:17:48Z

I should also make before git commit and git push
I thought the change was so simple there was no chance for errors :P

…ypes of targets Syntax: * Ignore activity in all windows # Ignore activity in all channels @ Ignore activity in all queries = Ignore activity in all dcc chats #chan|nick Ignore activity in named target(channel, query, dcc chat) tag/* Ignore all activity on network 'tag' tag/# Ignore activity in all channels on network 'tag' tag/@ Ignore activity in all queries on network 'tag' tag/= Ignore activity in all dcc chats on network 'tag' tag/#chan|nick Ignore activity in named channel/query/dcc chat on network 'tag'

LemonBoy

We're getting there! I'm really looking forward to see this merged 🎉

LemonBoy · 2017-01-13T12:10:43Z

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

+			GSList *targets = NULL, *iterator = NULL;
+			gboolean found = FALSE;
+
+			if (type != NULL) {


Is it possible for type to be NULL? If so when?
You didn't check for it being NULL above.

It might be a left-over from an earlier attempt

LemonBoy · 2017-01-13T12:12:02Z

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

-	if (strarray_find(array, "*") != -1)
+	const char *type = module_find_id_str("WINDOW ITEM TYPE", dest->window->active->type);
+
+	if ((strarray_find(array, "*") != -1) || // we ignore all targets


Put the check for * in its own line please (and comments on the line above the one they refer to)

LemonBoy · 2017-01-13T12:14:00Z

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

+	const char *type = module_find_id_str("WINDOW ITEM TYPE", dest->window->active->type);
+
+	if ((strarray_find(array, "*") != -1) || // we ignore all targets
+		(g_ascii_strcasecmp(type, "CHANNEL") == 0 && strarray_find(array, "#") != -1) || // we ignore all channels


Having a small helper function taking a TEXT_DEST_REC and returning an enum depending on whether it is a channel, a dcc query or a regular one would make the code easier to read and avoid many string comparisons.

LemonBoy · 2017-01-13T12:16:40Z

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

 		return TRUE;
+	else if (dest->server_tag != NULL) {
+		char *prefix = g_strdup_printf("%s/", dest->server_tag);
+		if (strarray_find_prefix(array, prefix)) {


Apply the early exit pattern here, if the prefix isn't there then we have no business to do (and avoid a whole level of indentation)

LemonBoy · 2017-01-13T12:20:48Z

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

+						targets = g_slist_append(targets, g_strdup("@"));
+				}
+
+				for (iterator = targets; iterator; iterator = iterator->next) {


The whole targets list is quite wasteful as you just need 4 bits to represent the set of all the possible checks to do (*, #, =, dest->target)

I think it makes it easier to build the list for which types to check for

LemonBoy · 2017-01-13T20:05:11Z

src/fe-common/core/window-items.h

+	WITEM_TYPE_PRIVMSG = 4,
+	WITEM_TYPE_DCCCHAT = 8,
+	WITEM_TYPE_OTHER = 16
+} WindowType;


WI_TYPE ?

LemonBoy · 2017-01-13T20:08:08Z

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

 		return TRUE;
+	else if (dest->server_tag != NULL) {


If server_tag is NULL we can just exit

LemonBoy · 2017-01-13T20:22:08Z

src/core/misc.c

+	item = g_ascii_strdown(item, -1);
+	index = 0;
+	for (tmp = array; *tmp != NULL; tmp++, index++) {
+		if (g_str_has_prefix(g_ascii_strdown(*tmp, -1), item))


g_ascii_strncasecmp

LemonBoy · 2017-01-13T20:23:06Z

src/core/misc.c

@@ -168,6 +168,24 @@ int strarray_find(char **array, const char *item)
 	return -1;
 }

+gboolean strarray_find_prefix(char **array, const char *item)


Please make this match the prototype of strarray_find

LemonBoy · 2017-01-13T20:29:56Z

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

@@ -462,25 +463,57 @@ 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);
+	WindowType type = window_item_get_type(dest->window->active);


Either make it const or split the declaration and the initializer.

LemonBoy · 2017-01-13T20:30:42Z

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


-		tagtarget = g_strdup_printf("%s/%s", dest->server_tag, dest->target);
-		ret = strarray_find(array, tagtarget);
+	GSList *targets = NULL, *iterator = NULL;


Declarations at the top please

LemonBoy · 2017-01-13T20:32:35Z

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

+
+	g_slist_foreach(targets, (GFunc)g_free, NULL);
+	g_slist_free(targets);
+	g_free(prefix);


Get rid of prefix when it is not needed anymore

LemonBoy · 2017-01-13T20:33:50Z

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


+	// we ignore all targets


If the array is empty then we could return early

LemonBoy

Looks good

LemonBoy · 2017-01-13T20:57:39Z

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

-		g_free(tagtarget);
-		if (ret != -1)
-			return TRUE;
+	g_return_val_if_fail(dest->server_tag != NULL, FALSE);


The g_return macros are only used at the top of the function to do some basic checks on the parameters, the explicit if block was fine.

I guess I misunderstood your 'use !' comment

LemonBoy · 2017-01-13T21:01:01Z

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

-		tagtarget = g_strdup_printf("%s/%s", dest->server_tag, dest->target);
-		ret = strarray_find(array, tagtarget);
+	// create a list of types to look for
+	targets = g_slist_append(targets, g_strdup("*"));


There's no need to strdup anything since the string literals have a ∞ lifetime and dest->target is definitely alive when it's feed into printf never to be used again.

LemonBoy · 2017-01-13T21:01:17Z

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

+{
+        g_return_val_if_fail(item != NULL, WI_TYPE_OTHER);
+
+	const char *type = module_find_id_str("WINDOW ITEM TYPE", item->type);


LemonBoy · 2017-01-13T21:01:42Z

src/fe-common/core/window-items.h

+	WI_TYPE_PRIVMSG = 4,
+	WI_TYPE_DCCCHAT = 8,
+	WI_TYPE_OTHER = 16
+} WindowType;


WI_TYPE maybe ?

I pulled that name from SettingType

LemonBoy · 2017-01-13T21:01:54Z

src/core/misc.c

+
+	index = 0;
+	for (tmp = array; *tmp != NULL; tmp++, index++) {
+		//if (g_str_has_prefix(g_ascii_strdown(*tmp, -1), item))


Ah yes, forgot :)

LemonBoy

Let's keep fighting the good fight

ailin-nemui · 2017-10-18T09:19:08Z

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

@@ -461,26 +462,63 @@ void fe_common_core_finish_init(void)

 gboolean strarray_find_dest(char **array, const TEXT_DEST_REC *dest)
 {
+	const WindowType type = dest->window->active ? window_item_get_type(dest->window->active) : WI_TYPE_OTHER;


remove the whole WindowType

use item = window_item_find_window(dest->window, dest->server, dest->target)

ailin-nemui · 2017-10-18T09:21:33Z

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

+	else if (type & WI_TYPE_OTHER)
+		return FALSE;
+	// we ignore all channels
+	else if (type & WI_TYPE_CHANNEL && strarray_find(array, "#") != -1)


use

static int channel_type = module_get_uniq_id_str("WINDOW ITEM TYPE", "CHANNEL"); ... if (item->type == channel_type && ...

ailin-nemui · 2017-10-18T09:26:24Z

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

 	if (strarray_find(array, "*") != -1)
 		return TRUE;
-
-	if (strarray_find(array, dest->target) != -1)
+	// exit if not a channel or query


instead of strarray_find,, strarray_find_prefix and GSLists, rewrite the code like this:

loop once over array

in each iteration, check if anything that needs to match, does match

example check:

if (item != NULL && item->type == channel_type && g_strcmp0(current_element, "#"))

ailin-nemui · 2017-10-18T09:29:15Z

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

@@ -138,6 +138,21 @@ void window_item_set_active(WINDOW_REC *window, WI_ITEM_REC *item)
 	}
 }

+WindowType window_item_get_type(WI_ITEM_REC *item)


ailin-nemui · 2017-10-18T09:29:24Z

src/fe-common/core/window-items.h

+	WI_TYPE_PRIVMSG = 4,
+	WI_TYPE_DCCCHAT = 8,
+	WI_TYPE_OTHER = 16
+} WindowType;


superseded

LemonBoy · 2017-10-25T19:26:28Z

Superseded by #779

LemonBoy requested changes Jan 9, 2017

View reviewed changes

Jari Matilainen added 2 commits January 13, 2017 01:04

New function to find matching prefix in char* array

adc5779

LemonBoy requested changes Jan 13, 2017

View reviewed changes

Jari Matilainen added 5 commits January 13, 2017 12:22

Changed return type to gboolean to match function definition

befa2c2

Make the prefix matching case insensitive

17021e1

Helper function for getting window item type

f272976

Reworked code to use helper function for window item type

9460034

Minor optimization

7d07e22

vague666 changed the title ~~Add setting to select what type of activity targets to ignore, channe…~~ Allow selection of what kind of activity targets to ignore Jan 13, 2017

LemonBoy reviewed Jan 13, 2017

View reviewed changes

enum name change and early exit if no server_tag

7a9d9bd

LemonBoy reviewed Jan 13, 2017

View reviewed changes

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

// we ignore all targets

Copy link

Member

LemonBoy Jan 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the array is empty then we could return early

LemonBoy reviewed Jan 13, 2017

View reviewed changes

Jari Matilainen added 2 commits January 13, 2017 20:47

Minor changes as per review

11bc71a

I blame other languages

7c540a0

LemonBoy reviewed Jan 13, 2017

View reviewed changes

LemonBoy previously requested changes Jan 13, 2017

View reviewed changes

Jari Matilainen added 6 commits January 13, 2017 21:06

Minor changes as per review

e80b659

any other way to do that? (gpointer) dest->target?

e63e0b6

Hopefully the final changes!

9288eed

Hopefully the final changes!

56da627

Move assertion to callpoint to work around assertion fail errors

2c7d585

Fixed typo

5135eee

ailin-nemui requested changes Oct 18, 2017

View reviewed changes

ailin-nemui added the needs changes label Oct 18, 2017

LemonBoy closed this Oct 25, 2017

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

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

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

Conversation

vague666 commented Jan 9, 2017 • edited Loading

ailin-nemui commented Jan 9, 2017

vague666 commented Jan 9, 2017

LemonBoy commented Jan 9, 2017

vague666 commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vague666 commented Jan 9, 2017

LemonBoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LemonBoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LemonBoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LemonBoy commented Oct 25, 2017

vague666 commented Jan 9, 2017 •

edited

Loading