search

shazow / ssh-chat

Chat over SSH.
https://shazow.net/posts/ssh-how-does-it-even/
MIT License
5.59k stars 408 forks source link

Ignored people still show up when they `/msg` and `/me` emote #349

Closed medinae closed 4 years ago

medinae commented 4 years ago

Before

image

After

image

shazow commented 4 years ago

Hiya, thanks for this! Are you certain this doesn't break anything else?

Also we need to fix /ignore with the other kinds of messages, like PrivateMsg. Ideally the fix would apply across the board. :)

medinae commented 4 years ago

Hello, Tests are passing and as you can see in the PR description, everything related to emote is OK.

For the second part of our answer, you mean we have the same issue with emote and private msgs? Shouldn't we do it in another PR?

medinae commented 4 years ago

@shazow Private msg fixed as well. Please review. Also, please check the PR description.

shazow commented 4 years ago

That's great, thank you!

My only concern is the (in)validity of this comment: https://github.com/shazow/ssh-chat/pull/349/files#diff-927aa726acccc0fd011d2817854c3452L141-L143

I'm going to take some time making sure it's not going to break in unexpected ways and then merge it, otherwise it looks good. :)

medinae commented 4 years ago

Yes make sense. ''To allow the sender to see the emote'' is curious. But I roughly tested it, and in particular that part. Works well.

Yeah would be nice if you double check. Let me know. Thanks.

shazow commented 4 years ago

I think my plan was to use the From() interface to differentiate between private and public messages, but I don't see any code that actually does that (maybe it was removed?). Would suck if private messages leak with this change so I'm trying to be extra careful. :)

medinae commented 4 years ago

but I don't see any code that actually does that Yep, haven't found a specific usage either.

shazow commented 4 years ago

Hm, the test is failing for me locally (I think travisci is borked):

$ make test
go test -race -test.timeout 5s ./...
ok      github.com/shazow/ssh-chat  (cached)
--- FAIL: TestIgnore (0.00s)
    room_test.go:119: Got: "** user1 crying\r\n"; Expected: "user1: hello\r\n"
    room_test.go:156: test is broken
FAIL
medinae commented 4 years ago

I'll check today. Probably something to fix in the tests.

shazow commented 4 years ago

Yea, that whole test could use a simpler rewrite.

shazow commented 4 years ago

I found the broken part of the test on my end. Will merge shortly.

The "crying" emote was rendering for the other user whose screen buffer was compared later with a different string, so I added a flush of that before.

shazow commented 4 years ago

Strange that it's not failing for you, wonder if it's running the wrong codebase? What version of Go are you using?