-
Notifications
You must be signed in to change notification settings - Fork 379
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 NickPrefix command to view and set the nickprefix. #1382
Conversation
modules/crypt.cpp
Outdated
void OnListKeysCommand(const CString& sCommand) { | ||
if (BeginNV() == EndNV()) { | ||
if (BeginNV() == EndNV() || BeginNV()->first.Equals(NICK_PREFIX_KEY)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're relying on comparison order between @
and letters. Why are you sure this always works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICK_PREFIX_KEY is actually also a "key", so we want to ignore it. If there are no other keys, NICK_PREFIX_KEY will be the first entry. Unless I misunderstood your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think it's the first entry? Maybe it's the last entry? Or somewhere in the middle?
modules/crypt.cpp
Outdated
SetNV(NICK_PREFIX_KEY, sPrefix); | ||
PutModule("Setting Nick Prefix to " + sPrefix); | ||
if (sStatusPrefix.CaseCmp(sPrefix, std::min(sp, np)) == 0) | ||
PutModule("WARNING: overlap with Status Prefix (" + sStatusPrefix + "), this Nick Prefix will not be used!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not? I see SetNV above... But what's used instead then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either "*" or ".", see NickPrefix(). Thought I'd allow setting it in case one changes it's status prefix instead afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking to me or to the user who tried to set such prefix?
I know about that function. But the current behavior would be surprising, and the message which user sees is misleading.
modules/crypt.cpp
Outdated
@@ -501,25 +504,36 @@ class CCryptMod : public CModule { | |||
} | |||
} | |||
|
|||
void OnNickPrefixCommand(const CString& sCommand) { | |||
CString sPrefix = sCommand.Token(1, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does a prefix with spaces inside mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, thought space could be used but seems they can't, messages from remote don't arrive at the client.
modules/crypt.cpp
Outdated
CString sStatusPrefix = GetUser()->GetStatusPrefix(); | ||
size_t sp = sStatusPrefix.size(); | ||
size_t np = sPrefix.size(); | ||
SetNV(NICK_PREFIX_KEY, sPrefix); | ||
PutModule("Setting Nick Prefix to " + sPrefix); | ||
if (sTail.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with just ignoring anything after the first parameter. It's obvious from the message anyway.
modules/crypt.cpp
Outdated
if (sPrefix.empty()) { | ||
PutModule("Nick Prefix: " + NickPrefix()); | ||
} else if (sPrefix.size() > 1 && sPrefix.StartsWith(":")) { | ||
PutModule("You cannot use : followed by other symbols as Nick Prefix."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just disable anything starting with :
, even if it's :abc
or :
modules/crypt.cpp
Outdated
CString sPrefix = sCommand.Token(1); | ||
|
||
if (sPrefix.empty()) { | ||
/* if (sPrefix.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove...
The test fails |
94e2323
to
afcafed
Compare
Codecov Report
@@ Coverage Diff @@
## master #1382 +/- ##
==========================================
+ Coverage 40.4% 40.45% +0.04%
==========================================
Files 109 109
Lines 21297 21313 +16
==========================================
+ Hits 8606 8623 +17
+ Misses 12691 12690 -1
Continue to review full report at Codecov.
|
Hides the internal keyword from ListKeys and adds the NickPrefix comand to view and optionally set another prefix.