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

Expand crypt module to cover Notices, Actions & Topics #813

Merged
merged 1 commit into from Sep 20, 2015

Conversation

anthonyryan1
Copy link
Contributor

Improves compatibility with mircryption.

Notes:

  1. I know the use of OnRaw is not suggested, but I cannot find a more appropriate way to intercept the incoming topic message.
  2. There's some duplication here, but when I added functions for the individual pieces of functionality needed to handle outgoing messages, it did not seem to significantly improve readability.

I'm open to all comments and suggestions.

@DarthGandalf
Copy link
Member

OnTopic?

virtual EModRet OnUserAction(CString& sTarget, CString& sMessage) override {
sTarget.TrimLeft(NickPrefix());

if (sMessage.StartsWith("``")) {
Copy link
Member

Choose a reason for hiding this comment

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

if (s.TrimPrefix(..))

@DarthGandalf
Copy link
Member

Looks like you're not open to comments :)

@anthonyryan1
Copy link
Contributor Author

With all due respect, your comment in this issue wasn't exactly meaningful. What about OnTopic?

The other part, was admittedly my fault. I didn't recall seeing your comment about switching the sTarget.TrimLeft(NickPrefix()) with sTarget.TrimPrefix(). If you can elaborate on what you mean regarding the "OnTopic" bit, I can ensure both of your requests are incorporated.

@DarthGandalf
Copy link
Member

OnTopic was response for "but I cannot find a more appropriate way to intercept the incoming topic message."

@DarthGandalf DarthGandalf reopened this Aug 24, 2015
@anthonyryan1
Copy link
Contributor Author

At least when I tested, the raw 332 was not corrected by OnTopic alone. I can chekc if that has changed since my prior testing. OnTopic is used, but without the OnRaw it wasn't working fully.

Perhaps a better solution to that would be to ensure that OnTopic does intercept the 332s as well as what it does currently.

@DarthGandalf
Copy link
Member

Oh, I didn't notice sTarget.TrimLeft before, thanks for fixing it. TrimLeft does something completely different. Speaking about sTarget.TrimLeft, there is a similar one added below in this PR.
My comment was about sMessage.StartsWith("``").

Regarding OnTopic, I see... OnTopic handles cases when it is changed, 332 is for cases if client requests /topic and joins channel.

May you rebase/squash commits?

@anthonyryan1
Copy link
Contributor Author

I hope to get this rebased tomorrow, sorry about the delay.

@anthonyryan1 anthonyryan1 changed the title Expand crypt module to cover Notices, Actions & Topics [WIP] Expand crypt module to cover Notices, Actions & Topics Sep 7, 2015
@anthonyryan1 anthonyryan1 changed the title [WIP] Expand crypt module to cover Notices, Actions & Topics Expand crypt module to cover Notices, Actions & Topics Sep 7, 2015
@anthonyryan1
Copy link
Contributor Author

Rebased & tested.

Let me know if you have any comments of concerns.

@@ -84,22 +86,144 @@ class CCryptMod : public CModule {
return CONTINUE;
}

EModRet OnUserNotice(CString& sTarget, CString& sMessage) override {
sTarget.TrimLeft(NickPrefix());
Copy link
Member

Choose a reason for hiding this comment

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

TrimLeft or TrimPrefix?

Improves compatibility with mircryption.
@anthonyryan1
Copy link
Contributor Author

It should have indeed been TrimPrefix. Somehow that change regressed while rebasing.

DarthGandalf added a commit that referenced this pull request Sep 20, 2015
Expand crypt module to cover Notices, Actions & Topics
@DarthGandalf DarthGandalf merged commit 3032664 into znc:master Sep 20, 2015
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.

None yet

2 participants