Chat API Improvements - Githubissues

PunishedPineapple commented 2 years ago

There are a few issues with Dalamud's chat log API as it currently exists, as well as room to improve how much information message-processing event handlers in plugins receive. This prospective DIP addresses these.

PunishedPineapple commented 2 years ago

Relevant issues/PRs leading to this DIP:

NadyaNayme commented 2 years ago

I'll let the people smarter than me discuss any actual pro/cons of the proposed changes but as an affected party with probably the most to gain from these changes - it selfishly has my full support.

Being able to ignore plugin-sourced messages means I (and other plugin devs) no longer have to worry about Tidy Chat eating their game messages - ever. Providing masked types (whether as a replacement of XivChatType or as its own thing like XivMaskedChatType) also means I can remove the bit of Chat2 code I stole to get channel detection working as expected in Tidy Chat. Also I would never have in a million years figured out how to solve that issue without having been told by Anna and I can imagine future devs would suffer the same struggle. Hell - even just documenting this behavior for future devs would be largely beneficial even if no changes are made to XivChatType at all.

philpax commented 2 years ago

This is a fantastic DIP! Well done - really detailed and covers everything in detail. I've had a read through and it looks great to me; I've also been bit by some of the inconsistent behaviours that currently exist (last one was senderId), so I can sympathise with the pain.

We still need to set up the GitHub roles, but in lieu of that, I'm tagging the Dalamud team directly: @goaaats, @daemitus, @Aireil, @Caraxi. Would appreciate your feedback 🙂

goaaats commented 2 years ago

Thanks for the DIP, great work. Some general points:

How do context menus on chat messages work? Did we ever figure that out? I remember there being issues with Dalamud breaking this in the past. Would be good to address this, whether or not it ends up in the API - it should at least be correct, and chat messages modified by Dalamud should retain the correct context menus.
Untangling XivChatType is critical
I think being able to set ContentID is dangerous and not a very useful feature to most plugins - in fact, I can't really come up with one that would need it. Could devs that might need it please note their interest?

PunishedPineapple commented 2 years ago

How do context menus on chat messages work? Did we ever figure that out? I remember there being issues with Dalamud breaking this in the past.

My limited (an evening and a half) testing showed that some context menu items are enabled by having a properly formed player link payload in the sender, and other menu items by having a proper entry in the RaptureLogModule's content ID store for that message's index. I have not RE'd the context menu itself, and honestly don't know how to; I've just seen what the game seems to be doing for messages and the effects. I would greatly appreciate Anna's knowledge on this, since I doubt anyone else knows more about how that works.

philpax commented 2 years ago

@ascclemens / @kalilistic can we get your input on the context menus? This DIP seems pretty uncontroversial otherwise, would be great to get it in for the next API version / .NET6 upgrade

anna-is-cute commented 2 years ago

LGTM

PunishedPineapple commented 2 years ago

I mostly removed from the DIP the parts about adding facilities to set the sender information (i.e., content ID) required for some context menus. The way for an API user to recieve the printed message index was left in. Setting sender information will either belong to ClientStructs or just require manual invocation. A note would be added to the PrintChat documentation about what controls available context menu entries.

I think that most of the other unresolved questions would be answerable during implementation, but if people have comments before that, it could be nice to have a stronger direction going in.

PunishedPineapple commented 1 year ago

Implementing this, it looks like it won't be feasible to get localized channel names automatically from the sheets. Since it will have to be done manually, should the channel names be localized for only the client language, or should it just be made a localizeable string like all the other text for whatever language Dalamud has set?.

PunishedPineapple commented 1 year ago

Actually, I'm not sure that CheapLoc can gracefully handle arbitrary channels when exporting strings either. Maybe the best option might just be an attribute on each XivChatType value with nice names in only the supported game client languages. But if anyone has any thoughts...

PunishedPineapple commented 1 year ago

Implementation PR: Dalamud#1116

PunishedPineapple commented 1 year ago

Now people want access to unedited chat messages (see https://discord.com/channels/581875019861328007/860813266468732938/1074943640163602462).

My thought on this at the moment is to get the currently-specified changes in, and then later add additional versions of the existing events that provide the unedited sender/message, since ensuring an unmodified SeString likely requires copying it for each event subscriber invocation (seems wasteful for event subscribers that don't care about the unaltered message).

Other option I can think of is implementing read only counterparts of all SeString payload types and SeString, which also sounds a bit gross.

I guess let me know what I should do.

goatcorp / DIPs

Chat API Improvements #39