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

Implement /away and /back commands #745

Merged
merged 1 commit into from
Dec 17, 2016
Merged

Implement /away and /back commands #745

merged 1 commit into from
Dec 17, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Nov 19, 2016

Fixes #705 as the away message has to be a single argument instead of multiple.

Breaking change: /away will now mark you as away with empty message instead of marking you as back. New command /back is used for that instead. (same as HexChat).

@xPaw xPaw added Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. Type: Feature Tickets that describe a desired feature or PRs that add them to the project. labels Nov 19, 2016
@xPaw xPaw added this to the 2.2.0 milestone Nov 19, 2016
Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

So, I guess my only issue is that I'm not sure why we want to do "/away" not set you as back, when (as far as I can see) that is the irc standard, apart from hexchat.

if (cmd === "away") {
let reason = " ";

if (args.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

So, why do we need to set away if there is no message? That doesn't seem to be part of the standard.

https://en.wikipedia.org/wiki/List_of_Internet_Relay_Chat_commands#AWAY

I'm not sure how I feel about this breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

@YaManicKill, did you mean this because when user types /away, the command that gets sent is network.irc.raw("AWAY", " "); and therefore user is marked as away even with no reason given?

I am actually happy with having a way to get away without giving a specific reason, but I definitely see your point. IMO, we have 2 options:

  • Current option: /away <message> that does what it says, marks you as away, with an optional reason. /back that sets you "unaway".
  • Close-to-the-protocol option: /away message sets you away with a reason, /away sets you back.

I have looked at other popular IRC clients and both options exist. That being said, most IRC clients out there have a terrible UX and I think The Lounge setting a higher and more modern bar on that end is a really good thing.
Overall, I am OK with both options, but I tend to like @xPaw's as it's simpler to use (and it's not entirely unexpected either, while having to type /away to set oneself back is rather counter-intuitive). I don't care to not reproduce the protocol 1:1 because it's a client, not a library.

Copy link
Member

@AlMcKinlay AlMcKinlay Dec 2, 2016

Choose a reason for hiding this comment

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

did you mean this because when user types /away, the command that gets sent is network.irc.raw("AWAY", " "); and therefore user is marked as away even with no reason given?

Yes, I did mean this.

I'm very against it because the standard is that "/away" sets you as unaway. I am very strongly for standards, why have a standard if we are going to ignore it?

https://tools.ietf.org/html/rfc1459#section-5.1

I do agree with you on the UX side of things, but we shouldn't just unilaterally ignore the standard because we don't agree with it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm very against it because the standard is that "/away" sets you as unaway. I am very strongly for standards, why have a standard if we are going to ignore it?

Not exactly. While I am myself very much into standardization (just look at my resume lol), this is a protocol, not a standard. The former ensures machine-to-machine compatibility while the latter is more often designed for humans.
In this case, we are talking about a de facto standard, a convention. And the reason for this debate is because we are seeing 2 of these: one with /away [message]//back and one with /away message//away.
Similarly, if we really wanted The Lounge to talk the same language than the protocol, we would use AWAY and not /away but starting with a slash is a convention so well spread that it makes sense to do it. (As far as I know, starting a command with a slash is not part of the protocol). We also send private messages without starting with PRIVMSG. While it would be terrible to do anything else, that's still a difference between client usage and protocol.

I do agree with you on the UX side of things, but we shouldn't just unilaterally ignore the standard because we don't agree with it.

We're not ignoring the protocol nor the conventions. If there were no other consequences or if /away setting you away was not used by any other IRC clients out there, I'd agree with you. But to me, and that's the main reason why I'm in favor of @xPaw's solution, typing /away to remove the away status is completely counter-intuitive and I would not be surprised if that was why so many other clients are going with the same solution.

In short, I definitely agree with you that following the protocol and conforming to standards are very important but we are not ignoring them right now. However, for a client designed to humans, proper UX should not be affected by these.

Copy link
Member

Choose a reason for hiding this comment

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

Fine. I think it's confusing, personally, but I'm unlikely to ever use it (I'll probably use the auto-away feature once that is in) so I'll bend to this.

Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Mostly the auto-unaway thing, and /back vs. /unaway.

return;
}

network.irc.raw("AWAY");
Copy link
Member

Choose a reason for hiding this comment

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

If I understand properly, this will be executed whatever the input. If so, that would act as an auto-unaway so that if I send a message while away, this marks me as not away anymore.
I don't like this too much: I have seen many times people sending messages while away, kind of a "do not disturb" mode. I don't see why we would force the unaway even when user doesn't ask it.

I have been out of the game a couple months, am I misunderstanding something? :)

Copy link
Member

Choose a reason for hiding this comment

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

I cant reply above... I think /away should set away without message

Copy link
Member Author

Choose a reason for hiding this comment

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

@astorije that's only run on /back command.

Copy link
Member

Choose a reason for hiding this comment

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

Aaah, I did not realize that this file was handling both 🤦

Reason why I was confused by this: Not long ago, all input files were being called when sending something, one by one, and at that time they were all starting with a check that the command being sent was the one from the file. I forgot we improved that to lookup for the exact command instead.
Prior to that change, I think that line would have been run whatever input was being sent.

So I think we're all good here, let me know if I'm in the wrong again lol.

if (cmd === "away") {
let reason = " ";

if (args.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

@YaManicKill, did you mean this because when user types /away, the command that gets sent is network.irc.raw("AWAY", " "); and therefore user is marked as away even with no reason given?

I am actually happy with having a way to get away without giving a specific reason, but I definitely see your point. IMO, we have 2 options:

  • Current option: /away <message> that does what it says, marks you as away, with an optional reason. /back that sets you "unaway".
  • Close-to-the-protocol option: /away message sets you away with a reason, /away sets you back.

I have looked at other popular IRC clients and both options exist. That being said, most IRC clients out there have a terrible UX and I think The Lounge setting a higher and more modern bar on that end is a really good thing.
Overall, I am OK with both options, but I tend to like @xPaw's as it's simpler to use (and it's not entirely unexpected either, while having to type /away to set oneself back is rather counter-intuitive). I don't care to not reproduce the protocol 1:1 because it's a client, not a library.

@@ -15,6 +17,7 @@ $(function() {
"/join",
"/kick",
"/leave",
"/me",
Copy link
Member

Choose a reason for hiding this comment

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

Is this fixing an unrelated bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

@@ -6,6 +6,8 @@ $(function() {
var path = window.location.pathname + "socket.io/";
var socket = io({path: path});
var commands = [
"/away",
"/back",
Copy link
Member

Choose a reason for hiding this comment

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

I know other clients use /back, but I lean more towards /unaway. For one, /back is a very generic word and we might want to keep it for later. But more importantly, it's more consistent with other "toggle" actions we have: /op vs. /deop, /voice vs. /devoice, /connect vs. /disconnect, etc.
Since it's not dictacted by the protocol anyway, we have the freedom to do what we want :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think both might be good to have.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have only one and we agree on it. It doesn't look like we are aliasing a lot of stuff so we might as well avoid doing that here.
I think /back could be used for some other client stuff. I don't have an exact use case for this yet, so it's hard for me to justify :) I'll let you pick whichever you prefer.

@astorije astorije self-assigned this Nov 30, 2016
@astorije
Copy link
Member

<off-topic> I found that rant by accident, I was amazed at how opinionated one can be on other people being away. 😂 </off-topic>

@AlMcKinlay
Copy link
Member

@astorije If I'm correct, there is nothing unresolved from your review?

Just trying to push through the last 2.2.0 things, would be good to get that release out soon.

@astorije
Copy link
Member

astorije commented Dec 16, 2016

@xPaw, @YaManicKill: 1️⃣ /back, 2️⃣ /unaway or 3️⃣ both? :)

@AlMcKinlay
Copy link
Member

AlMcKinlay commented Dec 16, 2016

1️⃣ /back, 2️⃣ /unaway or 3️⃣ both? :)

Well, personally I think that "/back" is much more obvious in terms of how I would speak, but ¯\_(ツ)_/¯

@xPaw
Copy link
Member Author

xPaw commented Dec 16, 2016

/back sounds better, imo.

@astorije
Copy link
Member

All good then, merging!

@astorije astorije merged commit 3d0e1fd into master Dec 17, 2016
@astorije astorije deleted the xpaw/away-command branch December 17, 2016 04:16
@astorije astorije mentioned this pull request Jan 6, 2017
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants