Chat fixes #358
Chat fixes #358
Conversation
I was having a bunch of issues with PMs, so it might work inconsistently. The chat code was some of the first code I rewrote and it has not aged well. |
| _otherCharacterEventNotifiers = otherCharacterEventNotifiers; | ||
| _chatEventNotifiers = chatEventNotifiers; | ||
| } | ||
|
|
||
| public override bool HandlePacket(TalkOpenServerPacket packet) | ||
| { | ||
| return Handle(packet, packet.PlayerId); | ||
| } | ||
| var member = _partyDataProvider.Members.FirstOrDefault(member => member.CharacterID == packet.PlayerId); |
There was a problem hiding this comment.
I've been trying to use optional pattern - OptionalFirst(x => x.CharacterID == packet.PlayerId).MatchSome(x => ...) - instead of linq's XOrDefault methods with comparison to null. More functional and all.
There was a problem hiding this comment.
Can you share an example of what you would do here? I'm not familiar with that I guess
There was a problem hiding this comment.
It's used all over the code base - if you search for OptionalFirst solution-wide you'll see the pattern.
There isn't much more to it than the example I gave. It's exactly like SingleOrDefault in that you pass a predicate to the method that provides a match. The result is an Option<T>, which is sort of like OneOf except it's either "Some" or "None". The chained call MatchSome(x => ...) does the action defined in the predicate when the optional has a value.
It's used as a way to avoid the possibility of NullReferenceException being thrown due to a null value. It's a clear way to mark intent and force the caller to handle the return value regardless of whether there is one.
There was a problem hiding this comment.
@MrDanOak has fixed this now using SingleOrNone and MatchSome :)
There was a problem hiding this comment.
Looks good - if you guys sanity check this one more time to make sure the bugs in 352 are all no longer reproducible then I'm cool with merging this
|
Can you add some details to the PR description about why the fixes work? You have the "what" in the commit history, but I'd like a clear line between "I changed this" and "this is the effect" |
|
@ethanmoffat PR description updated and can you show an example of how you'd replace the |
…lt for GroupChatHandler
Solution: adjust coordinates (and child control offsets) so chat tabs are *only* the click area (and not the full panel size). Fixes bug where right-clicking would always try to dispatch to the sys tab, which wouldn't have name data for the PM target.
Fixes bug where relogging and attempting to PM a character you'd previously PM'd would not show the chat tab for the PM session.
ethanmoffat
force-pushed
the
chat-fixes
branch
from
d2a5e30 to
8d0d3c2
Compare
Closes #352
I couldn't replicate this. The SFX played for me
This was a bug where we were always overwriting the render character with the main character. Now we always find one in
_characterRendererProvider.CharacterRendererswith the matching player idAdded sound effect to chat controller when sending message
We were only looking at party members in the local map repository instead of the party repository before. Now it shows messages from members no matter where they are in game.
I believe this was a render order bug? Seemed like some kinda z-index kind of thing. Adding the
Tabclickable area before the close buttons fixed it. I also adjusted the dimensions a bit to match the gfx.