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

Chat API Improvements #39

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

PunishedPineapple
Copy link

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

Relevant issues/PRs leading to this DIP:

Also fixed a few wording/spelling issues.
@NadyaNayme
Copy link

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

philpax commented Jun 5, 2022

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

goaaats commented Jun 5, 2022

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

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

philpax commented Jun 22, 2022

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

LGTM

It remains in one of the examples, as it's the primary known use for part of the DIP at this point in time.  
Additional implementation details in reference-level explanation.  
Also adjusted some unresolved questions.
@PunishedPineapple
Copy link
Author

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

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

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

Implementation PR: Dalamud#1116

@PunishedPineapple
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants