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

Add support for the draft/extended-monitor capability #254

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

delthas
Copy link

@delthas delthas commented Jul 27, 2021

Copy link
Member

@dwfreed dwfreed left a comment

Choose a reason for hiding this comment

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

Don't abuse side effects from other functions in clever ways, especially when these functions make it easy to exclude the appropriate users from getting duplicate messages without such abuse. This makes it difficult to refactor code without considering how something abuses something else's side effects, making the code harder to work with overall.

I make no comment about whether I think this functionality is useful/good or not, this is simply to correct implementation issues.

@@ -142,6 +143,7 @@ init_builtin_capabs(void)
CLICAP_CAP_NOTIFY = capability_put(cli_capindex, "cap-notify", NULL);
CLICAP_CHGHOST = capability_put(cli_capindex, "chghost", &high_priority);
CLICAP_ECHO_MESSAGE = capability_put(cli_capindex, "echo-message", NULL);
CLICAP_EXTENDED_MONITOR = capability_put(cli_capindex, "draft/extended-monitor", &high_priority);
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need high priority? It's a draft cap, and anybody supporting it should support 3.2-style CAP LS (which doesn't need priority)

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense 👍

@@ -1637,6 +1637,10 @@ change_nick_user_host(struct Client *target_p, const char *nick, const char *use
sendto_common_channels_local_butone(target_p, CLICAP_CHGHOST, NOCAPS,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of abusing current_serial semantics, this should exclude users with extended-monitor enabled:

Suggested change
sendto_common_channels_local_butone(target_p, CLICAP_CHGHOST, NOCAPS,
sendto_common_channels_local_butone(target_p, CLICAP_CHGHOST, CLICAP_EXTENDED_MONITOR,

Copy link
Author

@delthas delthas Jul 30, 2021

Choose a reason for hiding this comment

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

We still want to sent messages to users sharing a channel and who have the extended-monitor cap but who are not currently monitoring the user. That's why we can't just send to neighbours first, excluding those who have the cap, then send to all those who are monitoring the user.

Copy link
Member

Choose a reason for hiding this comment

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

Err, yes. The correct implementation would end up being more complex than this, my brain just failed to process it correctly.

@@ -1637,6 +1637,10 @@ change_nick_user_host(struct Client *target_p, const char *nick, const char *use
sendto_common_channels_local_butone(target_p, CLICAP_CHGHOST, NOCAPS,
":%s!%s@%s CHGHOST %s %s",
target_p->name, target_p->username, target_p->host, user, host);
struct monitor *monptr = find_monitor(target_p->name, 0);
if(monptr)
sendto_monitor_with_capability_butserial(target_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_CHGHOST, NOCAPS, true, ":%s!%s@%s CHGHOST %s %s",
Copy link
Member

Choose a reason for hiding this comment

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

Eliminate current_serial abuse

Suggested change
sendto_monitor_with_capability_butserial(target_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_CHGHOST, NOCAPS, true, ":%s!%s@%s CHGHOST %s %s",
sendto_monitor_with_capability(target_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_CHGHOST, NOCAPS, ":%s!%s@%s CHGHOST %s %s",

@@ -75,6 +75,7 @@ extern void sendto_match_butone(struct Client *, struct Client *,
extern void sendto_match_servs(struct Client *source_p, const char *mask,
int capab, int, const char *, ...) AFP(5, 6);

extern void sendto_monitor_with_capability_butserial(struct Client *, struct monitor *monptr, int caps, int negcaps, bool skipserial, const char *, ...) AFP(6, 7);
Copy link
Member

Choose a reason for hiding this comment

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

Eliminate current_serial abuse

Suggested change
extern void sendto_monitor_with_capability_butserial(struct Client *, struct monitor *monptr, int caps, int negcaps, bool skipserial, const char *, ...) AFP(6, 7);
extern void sendto_monitor_with_capability(struct Client *, struct monitor *monptr, int caps, int negcaps, const char *, ...) AFP(5, 6);

*/
void
sendto_monitor(struct Client *source_p, struct monitor *monptr, const char *pattern, ...)
_sendto_monitor_with_capability_butserial(struct Client *source_p, struct monitor *monptr, int caps, int negcaps, bool skipserial, const char *pattern, va_list * args)
Copy link
Member

Choose a reason for hiding this comment

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

Eliminate current_serial abuse

Suggested change
_sendto_monitor_with_capability_butserial(struct Client *source_p, struct monitor *monptr, int caps, int negcaps, bool skipserial, const char *pattern, va_list * args)
_sendto_monitor_with_capability(struct Client *source_p, struct monitor *monptr, int caps, int negcaps, const char *pattern, va_list * args)

@@ -89,6 +90,10 @@ m_away(struct MsgBuf *msgbuf_p, struct Client *client_p, struct Client *source_p

sendto_common_channels_local_butone(source_p, CLICAP_AWAY_NOTIFY, NOCAPS, ":%s!%s@%s AWAY",
Copy link
Member

Choose a reason for hiding this comment

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

Exclude extended-monitor

Suggested change
sendto_common_channels_local_butone(source_p, CLICAP_AWAY_NOTIFY, NOCAPS, ":%s!%s@%s AWAY",
sendto_common_channels_local_butone(source_p, CLICAP_AWAY_NOTIFY, CLICAP_EXTENDED_MONITOR, ":%s!%s@%s AWAY",

@@ -89,6 +90,10 @@ m_away(struct MsgBuf *msgbuf_p, struct Client *client_p, struct Client *source_p

sendto_common_channels_local_butone(source_p, CLICAP_AWAY_NOTIFY, NOCAPS, ":%s!%s@%s AWAY",
source_p->name, source_p->username, source_p->host);
struct monitor *monptr = find_monitor(source_p->name, 0);
if(monptr)
sendto_monitor_with_capability_butserial(source_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_AWAY_NOTIFY, NOCAPS, true, ":%s!%s@%s AWAY",
Copy link
Member

Choose a reason for hiding this comment

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

Eliminate current_serial abuse

Suggested change
sendto_monitor_with_capability_butserial(source_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_AWAY_NOTIFY, NOCAPS, true, ":%s!%s@%s AWAY",
sendto_monitor_with_capability(source_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_AWAY_NOTIFY, NOCAPS, ":%s!%s@%s AWAY",

@@ -127,6 +132,14 @@ m_away(struct MsgBuf *msgbuf_p, struct Client *client_p, struct Client *source_p
source_p->username,
source_p->host,
source_p->user->away);
struct monitor *monptr = find_monitor(source_p->name, 0);
if(monptr)
sendto_monitor_with_capability_butserial(source_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_AWAY_NOTIFY, NOCAPS, true,
Copy link
Member

Choose a reason for hiding this comment

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

Adjust the regular away-notify to exclude extended-monitor (due to a GitHub limitation, I can't comment on that line). Eliminate current_serial abuse

Suggested change
sendto_monitor_with_capability_butserial(source_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_AWAY_NOTIFY, NOCAPS, true,
sendto_monitor_with_capability(source_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_AWAY_NOTIFY, NOCAPS,

@@ -139,6 +139,11 @@ me_su(struct MsgBuf *msgbuf_p, struct Client *client_p, struct Client *source_p,
sendto_common_channels_local(target_p, CLICAP_ACCOUNT_NOTIFY, NOCAPS, ":%s!%s@%s ACCOUNT %s",
Copy link
Member

Choose a reason for hiding this comment

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

Exclude extended-monitor

Suggested change
sendto_common_channels_local(target_p, CLICAP_ACCOUNT_NOTIFY, NOCAPS, ":%s!%s@%s ACCOUNT %s",
sendto_common_channels_local(target_p, CLICAP_ACCOUNT_NOTIFY, CLICAP_EXTENDED_MONITOR, ":%s!%s@%s ACCOUNT %s",

@@ -139,6 +139,11 @@ me_su(struct MsgBuf *msgbuf_p, struct Client *client_p, struct Client *source_p,
sendto_common_channels_local(target_p, CLICAP_ACCOUNT_NOTIFY, NOCAPS, ":%s!%s@%s ACCOUNT %s",
target_p->name, target_p->username, target_p->host,
EmptyString(target_p->user->suser) ? "*" : target_p->user->suser);
struct monitor *monptr = find_monitor(target_p->name, 0);
if(monptr)
sendto_monitor_with_capability_butserial(target_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_ACCOUNT_NOTIFY, NOCAPS, true, ":%s!%s@%s ACCOUNT %s",
Copy link
Member

Choose a reason for hiding this comment

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

Eliminate current_serial abuse

Suggested change
sendto_monitor_with_capability_butserial(target_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_ACCOUNT_NOTIFY, NOCAPS, true, ":%s!%s@%s ACCOUNT %s",
sendto_monitor_with_capability(target_p, monptr, CLICAP_EXTENDED_MONITOR | CLICAP_ACCOUNT_NOTIFY, NOCAPS, ":%s!%s@%s ACCOUNT %s",

@emersion
Copy link

emersion commented Nov 9, 2021

@delthas, any plans to update this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants