Handle AFK '@-in-names' protocol (take 2) #1270

scheibo · 2019-04-22T16:12:31Z

Rollforward of #1265. Currently this is unchanged from what was committed and then rolled back (thus why this is a draft), but I will be going through everything in git grep users to avoid breaking things.

This reverts commit 5348d13.

scheibo · 2019-04-22T19:29:58Z

Went through everything in git grep users and I think 7ed7e9f covers what was missed (but I'm not a compiler and I can't really guarantee that). Little surprised to see code duplicated in battle.ts which is present in client-chat.js as well?

I've confirmed in the test client that tab completion of usernames now works. Are the additional things I should be testing? Does someone else want to patch this in to help provide some extra assurance it won't break things this time around?

scheibo · 2019-04-26T16:56:44Z

@Zarel

Zarel · 2019-04-26T21:38:57Z

Is the new |j| etc handled correctly in BattleLog and BattleTextParser?

scheibo · 2019-04-26T22:16:13Z

Is the new |j| etc handled correctly in BattleLog and BattleTextParser?

I don't see any |j| etc in BattleTextParser? I had missed BattleLog, but I've now updated it and Battle which were also missing in the original PR.

Zarel · 2019-04-30T01:35:07Z

Have you played through an entire battle on testclient, with and without @ in your name, and confirmed that it works?

Zarel · 2019-04-30T01:35:26Z

(People in the Staff room are pretty willing to help devs test this sort of thing.)

(Once you have, feel free to merge.)

scheibo · 2019-04-30T03:31:56Z

(People in the Staff room are pretty willing to help devs test this sort of thing.)

Doesn't this require me to set up a server + client to allow other people to access it? And they still have to go through a hacky login flow because #1264 has not been resolved?

Easier to just play out battles myself TBH... (I don't have any Pokémon code or even Node for that matter on any of my servers yet...). It'd be great to have a staging environment where we could test out client + server features without pushing to main, but I think a proper set up for that is once again blocked on #1264.

(Once you have, feel free to merge.)

I'm not a collaborator on this repro, but if I were I would be adding a 'Do Not Merge' label currently. I found one bug after playing through a battle (fixed with d701d93), and there's currently another issue where if you are /away and refresh you end up in a mixed state where parts of the UI think your /away and other parts don't (I think due to which UI elements listen to which things to know when to update). Once I've fixed that and gone through another round of poking around looking for edge cases I'll let you know.

scheibo · 2019-05-02T02:15:49Z

and there's currently another issue where if you are /away and refresh you end up in a mixed state where parts of the UI think your /away and other parts don't

I'm mistaken about this. The 'bad UI state' is really a combination of two things, neither which are caused by this PR.

After refreshing chat in the test client, the user list shows '0 users; Only Guests' even though the account youre testing with is not a guest. I've confirmed that on master this is also a thing.
I was worried about the 'UI being in an inconsistent state' because if you have 2 accounts in different tabs open and refresh one of the tabs (you need 2, because if you refresh with just one account open the userlist gets bugged and shows noone as described in 1), the user list shows the away account as not being away (ie. not greyed out), even though clicking on the account shows the user as away. When one account refreshes, the other window just gets a

|l| pre
|j| pre

not any |n| info etc. I believe this is a bug in the server change - leaving/joining seems like it should clear your away state (as should actively making moves in a Pokemon battle - during my testing I noticed that doesn't affect away state either...). I will work with @bumbadadabum to fix the server code, but the client code seems ready to merge as is.

Zarel · 2019-05-04T01:23:03Z

Doesn't this require me to set up a server + client to allow other people to access it? And they still have to go through a hacky login flow because #1264 has not been resolved?

I mean the test would be: Battle on Main, other tester on Main, you on testclient. Bugs that only crop up with an experimental server aren't important; critical bugs are crashes that affect Main like the ones we hit last time.

Zarel · 2019-05-04T01:25:12Z

Once you've tested that, comment and I'll merge immediately.

Zarel

(pending the test in comments)

scheibo · 2019-05-04T02:11:34Z

Oh yeah, I'd tested both against the main server and against a local server already, but I just did it again to be sure:

Poked around the Lobby etc afterwards on the test client and no crashes. I'm significantly more confident in this change than last time, but still find it hard to promise much in an untyped codebase without tests.

scheibo · 2019-05-04T15:59:46Z

src/battle-log.ts

+		case 'name': case 'n': {
+			const user = BattleTextParser.parseNameParts(args[1]);
+			if (toId(args[2]) !== toId(user.name)) {
+				divHTML = '<small>' + BattleLog.escapeHTML(user.name) + ' renamed from ' + BattleLog.escapeHTML(args[2]) + '.</small>';


I should note - this is going to look kind of weird once the server code lands, as args[2] is the ID, which shouldn't have actually changed, so its going to be 'Foo renamed from foo' when Foo@!Away -> Foo.

This should probably be a different message in that case; something like Foo changed status to ...

scheibo added 2 commits April 22, 2019 12:06

Handle AFK '@-in-names' protocol (take 2)

ccd9897

This reverts commit 5348d13.

Fix other references to Room.users

7ed7e9f

scheibo marked this pull request as ready for review April 22, 2019 19:29

Handle the new protocol in BattleLog

78653dc

Zarel approved these changes Apr 30, 2019

View reviewed changes

Fix addJoinLeave post battle

d701d93

scheibo mentioned this pull request May 2, 2019

Add an AFK timer smogon/pokemon-showdown#5002

Merged

Zarel approved these changes May 4, 2019

View reviewed changes

scheibo commented May 4, 2019

View reviewed changes

Zarel merged commit 883cd91 into smogon:master May 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle AFK '@-in-names' protocol (take 2) #1270

Handle AFK '@-in-names' protocol (take 2) #1270

scheibo commented Apr 22, 2019

scheibo commented Apr 22, 2019

scheibo commented Apr 26, 2019

Zarel commented Apr 26, 2019 •

edited

Loading

scheibo commented Apr 26, 2019

Zarel commented Apr 30, 2019

Zarel commented Apr 30, 2019 •

edited

Loading

scheibo commented Apr 30, 2019

scheibo commented May 2, 2019

Zarel commented May 4, 2019 •

edited

Loading

Zarel commented May 4, 2019

Zarel left a comment

scheibo commented May 4, 2019

scheibo May 4, 2019

Zarel May 5, 2019

Handle AFK '@-in-names' protocol (take 2) #1270

Handle AFK '@-in-names' protocol (take 2) #1270

Conversation

scheibo commented Apr 22, 2019

scheibo commented Apr 22, 2019

scheibo commented Apr 26, 2019

Zarel commented Apr 26, 2019 • edited Loading

scheibo commented Apr 26, 2019

Zarel commented Apr 30, 2019

Zarel commented Apr 30, 2019 • edited Loading

scheibo commented Apr 30, 2019

scheibo commented May 2, 2019

Zarel commented May 4, 2019 • edited Loading

Zarel commented May 4, 2019

Zarel left a comment

Choose a reason for hiding this comment

scheibo commented May 4, 2019

scheibo May 4, 2019

Choose a reason for hiding this comment

Zarel May 5, 2019

Choose a reason for hiding this comment

Zarel commented Apr 26, 2019 •

edited

Loading

Zarel commented Apr 30, 2019 •

edited

Loading

Zarel commented May 4, 2019 •

edited

Loading