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 #612

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@vague666
Member

vague666 commented Jan 9, 2017

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

This comment has been minimized.

Contributor

ailin-nemui commented Jan 9, 2017

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

@vague666

This comment has been minimized.

Member

vague666 commented Jan 9, 2017

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

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Jan 9, 2017

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

This comment has been minimized.

Member

vague666 commented Jan 9, 2017

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

@@ -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, "#");

This comment has been minimized.

@LemonBoy

LemonBoy Jan 9, 2017

Member

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)

This comment has been minimized.

@vague666

vague666 Jan 9, 2017

Member

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 9, 2017

Member

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

This comment has been minimized.

@vague666

vague666 Jan 10, 2017

Member

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 10, 2017

Member

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

This comment has been minimized.

Member

vague666 commented Jan 9, 2017

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

Jari Matilainen added some commits Jan 13, 2017

Jari Matilainen
Make activity_hide_targets more functional. Allow ignore of several t…
…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 🎉

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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

This comment has been minimized.

@vague666

vague666 Jan 13, 2017

Member

It might be a left-over from an earlier attempt

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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.

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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)

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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)

This comment has been minimized.

@vague666

vague666 Jan 13, 2017

Member

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

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

WI_TYPE ?

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

If server_tag is NULL we can just exit

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

g_ascii_strncasecmp

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

Please make this match the prototype of strarray_find

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

Declarations at the top please

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

Get rid of prefix when it is not needed anymore

// we ignore all targets

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

If the array is empty then we could return early

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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.

This comment has been minimized.

@vague666

vague666 Jan 13, 2017

Member

I guess I misunderstood your 'use !' comment

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("*"));

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

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.

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

Alignment

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

WI_TYPE maybe ?

This comment has been minimized.

@vague666

vague666 Jan 13, 2017

Member

I pulled that name from SettingType

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

remove it

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

This comment has been minimized.

@LemonBoy

LemonBoy Jan 13, 2017

Member

Remove

This comment has been minimized.

@vague666

vague666 Jan 13, 2017

Member

Ah yes, forgot :)

@LemonBoy

Let's keep fighting the good fight

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

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

remove the whole WindowType

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

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

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

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

use

static int channel_type = module_get_uniq_id_str("WINDOW ITEM TYPE", "CHANNEL");
... if (item->type == channel_type && ...
if (strarray_find(array, "*") != -1)
return TRUE;
if (strarray_find(array, dest->target) != -1)
// exit if not a channel or query

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

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

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

example check:

if (item != NULL && item->type == channel_type && g_strcmp0(current_element, "#"))
@@ -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)

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

remove it

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

This comment has been minimized.

@ailin-nemui

ailin-nemui Oct 18, 2017

Contributor

remove it

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Oct 25, 2017

Superseded by #779

@LemonBoy LemonBoy closed this Oct 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment