Implement buddy freechat and menuchat#11
Conversation
yungcomputerchair
left a comment
yungcomputerchair
left a comment
There was a problem hiding this comment.
Overall pretty good; the PCID hashmap is awesome and all of the plumbing for cross-shard is there. Just need to add buddy validation, fix the freechat processing, take care of some error handling, and fix the packet name
| if !helpers::validate_menuchat_message(&msg) { | ||
| return Err(FFError::build( | ||
| Severity::Warning, | ||
| format!("Invalid menuchat message\n\t{}: '{}'", player, msg), | ||
| )); | ||
| } |
There was a problem hiding this comment.
For freechat, we don't run validation. Instead we run it through the freechat processor (which does nothing today, but will do things like censor words in the future).
| if !helpers::validate_menuchat_message(&msg) { | |
| return Err(FFError::build( | |
| Severity::Warning, | |
| format!("Invalid menuchat message\n\t{}: '{}'", player, msg), | |
| )); | |
| } | |
| let msg = helpers::process_freechat_message(msg); |
| } | ||
| } | ||
|
|
||
| let login_server = clients.get_login_server().unwrap(); |
There was a problem hiding this comment.
Using unwrap here is very dangerous, because if communication to the login server is lost, this will crash the shard server. We should log a warning and stop if the login server isn't connected.
| let login_server = clients.get_login_server().unwrap(); | |
| if let Some(login_server) = clients.get_login_server() { | |
| ... | |
| } else { | |
| // warn here | |
| } |
| Ok(()) | ||
| } | ||
|
|
||
| pub fn send_buddy_menuchat_message( |
There was a problem hiding this comment.
Most of my comments on the freechat handler apply here too, just not the ones about freechat processing
| buddy_client | ||
| .send_packet(P_FE2CL_REP_SEND_BUDDY_FREECHAT_MESSAGE_SUCC, &response_pkt)?; | ||
|
|
||
| let login_server = clients.get_login_server().unwrap(); |
There was a problem hiding this comment.
See earlier comment about avoiding unwrap() on the login server. Yes, the packet came from the login server, but I believe it's possible for the login server to disconnect before the packet starts processing
There was a problem hiding this comment.
I might be guilty of making this assumption in some parts of my code...
| let response_pkt = sP_FE2CL_REP_SEND_BUDDY_FREECHAT_MESSAGE_SUCC { | ||
| iFromPCUID: player.get_uid(), | ||
| iToPCUID: pkt.iBuddyPCUID, | ||
| szFreeChat: pkt.szFreeChat, |
There was a problem hiding this comment.
Unlike menuchat messages where we fail out if they're invalid, we might modify freechat messages with process_freechat_message, so you should re-encode the modified string here instead of copying from the initial packet
| #[repr(packed(4))] | ||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct sP_LS2FE_REP_SEND_BUDDY_FREECHAT { |
There was a problem hiding this comment.
This packet should definitely be REQ instead of REP since the login server is requesting that the message be sent to the player; you'll need to update in a few spots
2 participants
No description provided.