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

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

Merged
merged 3 commits into from
May 1, 2020

Conversation

medinae
Copy link
Contributor

@medinae medinae commented Apr 29, 2020

Before

image

After

image

@shazow
Copy link
Owner

shazow commented Apr 29, 2020

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
Copy link
Contributor Author

medinae commented Apr 30, 2020

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 medinae changed the title Ignored people still show up when they /me emote Ignored people still show up when they /msg and /me emote Apr 30, 2020
@medinae
Copy link
Contributor Author

medinae commented Apr 30, 2020

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

@shazow
Copy link
Owner

shazow commented Apr 30, 2020

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
Copy link
Contributor Author

medinae commented Apr 30, 2020

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
Copy link
Owner

shazow commented Apr 30, 2020

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
Copy link
Contributor Author

medinae commented Apr 30, 2020

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

@shazow
Copy link
Owner

shazow commented May 1, 2020

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
Copy link
Contributor Author

medinae commented May 1, 2020

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

@shazow
Copy link
Owner

shazow commented May 1, 2020

Yea, that whole test could use a simpler rewrite.

@shazow
Copy link
Owner

shazow commented May 1, 2020

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
Copy link
Owner

shazow commented May 1, 2020

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

@shazow shazow merged commit 6fe0b40 into shazow:master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants