Add an AFK timer

[Asheviere]

No description provided.

[Asheviere]

Currently on LoA/vacation, did plan on picking this back up, will update PR next week when I'm back :)

[scheibo]

Did some more thinking/research on this - I think what we (I?) really want is /away.

https://bisqwit.iki.fi/jutut/away.html has some good arguments against the pre|AFK 'away nick' @Zarel suggested. I think it would be useful if we added an /away command to toggle an 'away' state for a user (I don't know if we'd want to actually support /away messages, because they could potentially be inappropriate). The server can automatically set /away for a user based on inactivity (this change).

My ideal design:

• when a user is /away, their name turns gray and they are sorted to the bottom of the room's user list, but they keep their rank, and clicking on their grayed out name shows a tag/indicator that they are away/AFK.
• if the server detects a user as inactive they can set them to the /away state
• I can manually set /away on my desktop without waiting for the server to timeout my client
• a user can leave the /away state by using the /away command again, or if a user makes a non-passive action (replies publicly in chat, makes an action in battle/starts a challenge etc. simply joining a room or battle as an observer should not alter the away state IMO, neither should something like /help or /dt etc).

I'd want to get some consensus here though, and its a broader change than what you originally envisioned in this PR.

[Zarel]

Your link is for IRC, and there's no reason we'd have to carry over the flaws mentioned in that article.

Also, my suggestion of |AFK was purely protocol-level; I definitely don't want it to be a literal name change.

Anyway, your outline matches my vision, except /back is the command to remove the away state, and the away state is set by /away with or without any description.

[scheibo]

Gotcha. Works for me! @bumbadadabum - LMK if what I laid out is out of scope for what you were planning to do when you get back (no rush!) and I will take on anything you're not interested in :)

[Asheviere]

I dread the client work immensely lol. I'll message you on Discord when I get the time.

[Asheviere]

It's not out of scope though, sounds like a way more solid thought out idea than the basically spur of the moment quick and dirty solution I have here lol.

[Asheviere]

Talked to pre and this is how we plan to implement the protocol for the away state:

For broadcasting to chatrooms: |a|username when a user is marked as away and |b|username when the user is back. We considered putting this into |n| but that would break the format of that protocol message, which would mess with bots.

When a user joins a room, users marked as away will have a ` at the end of their username in the |users| part of the |init| message. We thought ` made the most sense, as it is not allowed in usernames, and using pipes would make the whole message hell to parse. This seemed like the most compact way to indicate people being AFK in this message while retaining backwards compatibility.

Of course, it should also show up on /query userdetails but this uses JSON and would be trivial to add this to.

The final point is whether we show this to users in the top right. My idea for making that work is to change the NAMED field in the |updateuser| to be 0 if a user is a guest, 1 if a user is named, and 2 if a user is away. This still allowed boolean checks for named to work as they did before, while adding an easy way to check whether the user got marked as away without looking at chatrooms.

[scheibo]

As for the client side, I think I need to:

• change the parsing logic to understand the ` and, and this.users needs to store not just the name string but also away status. I assume we'd want to parse the away state out of the ID as soon as possible, but then we're stuck passing the two together everywhere, like to UserList.add etc. It would make more sense to have a User object to pass around/store in this.users, but I don't think we have the full details for that, just the ID + away bit pair?
• observe |a| and |b| in the same way we do joins and leaves (which brings up the question - will we want |A| & |B| along with |a|& |b| and |away| & |back|? How frequent do we expect away to occur?
• change the color in constructItem. Ideally I'd be able to add an 'away' class or something similar the the li, but because the color is an inline style it wouldn't apply thanks to cascade rules so I'd need apply it inline as well.
  • we also need to add something to the id of the li to represent 'away', because we pull it out in the elemComparator
• change the comparator logic to sort away users at the bottom (though still staff above regular users etc)

Probably missing some things, but thats my initial guess at what might be required (I might also need to make some changes for away in non-ChatRooms? Like for when an away user is spectating a battle?). I'd actually probably update the Preact client at the same time, because the code looks to already be there as well and be pretty much identical.

[Zarel]

I don't like |a| and |b|; I think |n| should work fine.

I don't like the ` sigil. | shouldn't require that much parsing complexity.

[Zarel]

PS already hardcodes how many pipes to split on, anyway:

https://github.com/Zarel/Pokemon-Showdown-Client/blob/62c01a0216057730acbe4a50d0c48baa6606f52a/src/client-main.ts#L510-L535

[Asheviere]

But it would 1. Break every ps bot 2. Require you to parse a highly variable amount of pipes. Also how exactly do you want it sent in |n|?

[Zarel]

PS already requires parsing variable amounts of pipes. I linked to the modern way this is achieved in client.

I would probably make the AFK state the third argument of |n|.

[Asheviere]

Yes variable per message type. But not an init message containing anything from 8 to 500 pipes. Also, pipes are used to separate parts of the message, and this usage is not that. It is used here to add a little identifier to the end of usernames to signify they're afk, and it seems weird and out of place to use the same character we used to separate the userlist itself from the other parts of the init. Confusing to read and confusing to parse.

[Asheviere]

Ok talked to Zarel, he wants @ to be banned from usernames, and we could use that in the |init| message.

[Asheviere]

Ok implemented the new protocol.

[scheibo]

I just tested this again locally and took another pass over the code (previously I'd only be reviewing it as it updated). I'm pretty confident this is good to go. Let's ship!

[Zarel]

Looks good to me. I approved this before but it's not showing up?

[HoeenCoder]

Looks good to me, great job guys!