-
Notifications
You must be signed in to change notification settings - Fork 700
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
Remove join, nick and whois inputs, they are cleanly handled by the server #208
Conversation
👍 |
@@ -17,7 +17,6 @@ $(function() { | |||
"/notice", | |||
"/op", | |||
"/part", | |||
"/query", |
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.
Just wondering, why removing this?
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.
That was an alias for /whois
, and since that's gone, that wouldn't work.
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.
So it was (see src/plugins/inputs/whois.js
, below). I'm guessing @astorije assumed, as I did at first glance, that /query
was an alias for /msg
. That is the common usage among other clients.
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.
Well not exactly an alias for /msg, as some clients don't open a query window for /msg, and just show the message sent/received in the server window and only show a query window when you do /query.
But certanarily, I don't like that, so it's essentially the same thing. The important thing is that it opens a query window, but does /query still do a whois by default, or do you have to do /whois for that?
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 can't recall using any client that auto-whois'd on /query
, no. But I also don't recall using any that didn't open a query window for /msg
, so maybe I have limited experience.
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.
Thanks @xPaw, my bad for forgetting that /query
was merely an alias for /whois
...
I'll test tomorrow and will merge then. |
Tested locally and all of the removed commands (well, apart from When releasing this, I will specify in the release introduction that the My understanding is that usually, people expect that:
Am I correct? We are definitely losing the last option with this PR. Would it be possible to re-add Also, @dgw rightly mentioned that |
Remove join, nick and whois inputs, they are cleanly handled by the server
Fixes #201