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

Add text highlighting #31442

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

VitusVeit
Copy link

@VitusVeit VitusVeit commented Aug 25, 2024

About the PR

This PR adds text highlighting to the ChatBox, the user can now input a series of newline-separated words that will be highlighted on the chat box and bubbles.
A new option is also available that tries to fill in the highlights depending on the player's role and name (disabled by default, can be found in the accessibility options tab, where is also possible to change the highlights color).

The words to be highlighted can also be wrapped in ", making them space-sensitive (or whole-words).
For example when writing "det":

  • "Det come to bar" will be highlighted
  • "Detain him" will not be highlighted.

Why / Balance

Already present in many SS13 servers, this feature greatly improves the readability of the comms and helps departments and individuals know when they are called/needed on the radio.

Resolves #16841

Technical details

This PR is based on #30092, it uses the newly added function "InjectTagAroundString" to change the colors from a list of keywords. The words to be highlighted can be inserted through a TextEdit in the filter panel and will be saved when exiting the game.

Regarding the auto-filling of the highlights, the autofill map is currently being saved in a localization "highlights.ftl" file since adding a new highlights.yml file would need a change in the engine part (as far as I know).

The ChatUIController now uses the CharacterInfoSystem along its "OnCharacterUpdate" event to update the highlights (when the autofill-highlights option is enabled and the player is attached/detached). Then, it uses the info from the character to find the best list of highlights relative to the character's role and name.

Media

Requirements

  • I have read and I am following the Pull Request Guidelines. I understand that not doing so may get my pr closed at maintainer’s discretion
  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

🆑 TheCactus

  • add: Added text highlighting.

@github-actions github-actions bot added the Changes: UI Changes: Might require knowledge of UI design or code. label Aug 25, 2024
@lzk228
Copy link
Contributor

lzk228 commented Aug 25, 2024

the true hero

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 25, 2024

Will some highlights be on by default depending on job? Or always have to be customzied between rounds?

@lzk228
Copy link
Contributor

lzk228 commented Aug 25, 2024

i suggest another implementation for this, as you suggested in #30092 (comment)
separate button for this and add list, not the line
like you write the word, press enter and it adds to list with delete button

@VitusVeit
Copy link
Author

Will some highlights be on by default depending on job? Or always have to be customzied between rounds?

I was thinking about adding default highlightings (eg. for Quartermaster: QM, Cargo) including the person's name. But I didn't add it because I couldn't figure out how to make it work when changing IDs (should it consider the ID you're currently wearing or the character you're controlling?).

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 25, 2024

But I didn't add it because I couldn't figure out how to make it work when changing IDs (should it consider the ID you're currently wearing or the character you're controlling?)

Personally, based on ID seems fine, but then it would kinda have to be based on the permissions of the ID since those change more often than JUST the ID
If anything, a resync button that updates all the highlights based on your current ID/Permissions so it doesnt just modify live when you dont want it to

@lzk228
Copy link
Contributor

lzk228 commented Aug 25, 2024

imo it's fine with empty by default

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 25, 2024

I think empty by default is fine but if that would be the case at some point i would just stop filling it in because it would be annoying

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 25, 2024

Either way, a small Resync button that updates your highlights based on your ID and possibly permissions it has, I think thatd be great

@VitusVeit
Copy link
Author

i suggest another implementation for this, as you suggested in #30092 (comment) separate button for this and add list, not the line like you write the word, press enter and it adds to list with delete button

Yes, I currently have a separate branch where I'm trying to make a similar UI panel but I'm having some issues with the xaml files (I'm really bad at writing UI code). For now, I limited myself to copying the /tg/'s fancy chat settings with a single LineEdit.

@VitusVeit
Copy link
Author

Also regarding the LineEdit being empty by default, I understand that it can feel repetitive to insert it every round so I found some solutions:

  1. Choose the highlights in the character's creation menu
  2. Fill in the highlights with the chosen character's information when joining the round
  3. Save the highlights on the client's computer so that it's kept when exiting the game

I personally think number 3 is the best solution as it's (relatively) easy to implement and it's also the way /tg/ station handles the text highlighting to my knowledge. I'll be waiting for any maintainer's approval but in the meantime, I'll go about trying to implement these solutions, thanks for the feedback :D

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 25, 2024

If third solution is possible to save them depending on the job/department you have, because if you change jobs often you'd have to still update it a lot.
But I guess there is no perfect solution and I'd already be extremely happy with the ideas you presented, hoping to see it in the game sometime soon!

@forgotmyotheraccount
Copy link
Contributor

I've been eagerly awaiting this for a long time, I have a ton of trouble keeping an eye on the chat, especially on high pop. But if it's empty by default I doubt I'd end up using it. If it's easier I'd prefer it just stick with your roundstart job, then on the rare chance I decide to ditch my department it's up to me to update it myself

@VitusVeit
Copy link
Author

A short update about the PR.
In the past few days, I've been working on refactoring the precedent commits (moving all the logic to the UIController). Along with some UI changes, mainly turning the LineEdit into a vertical TextEdit so that the user can now dedicate each line to a single word it simplifies inserting and removing new words until a proper UI list takes its place.

I also found out that the game already stores the player's name and role (as can be seen in the Character Info panel). So I will be working on matching the user role to a list of predefined highlights (eg. for HoS: HoS, Head of Security, Security, Sec; or for Paramedic: Medical, med, para, etc). This feature will be able to be activated in the settings as "Autofill Highlights" or a similar name.

If all goes well these features will be committed in a matter of a week or less.

@murolem
Copy link

murolem commented Aug 29, 2024

Is it possible to make the color configurable? At least for accessibility purposes.

@Dmitry2777
Copy link

It's a very cool thing ! Just for a new role - AI, it will be great!

@VitusVeit
Copy link
Author

Is it possible to make the color configurable? At least for accessibility purposes.

It hasn't been started yet, but I'm planning on adding a color picker in the settings's accessibility tab.

@ScarKy0
Copy link
Contributor

ScarKy0 commented Sep 2, 2024

Since autofill adds Command if youre a command role, will it highlight [Command] whenever someone uses the respective radio channel?

@VitusVeit
Copy link
Author

No, the 'InjectTagAroundString' function has a check to make sure words inside brackets don't get picked up.

@ScarKy0
Copy link
Contributor

ScarKy0 commented Sep 2, 2024

Perfect, really hoping this gets merged soon!

@ScarKy0
Copy link
Contributor

ScarKy0 commented Sep 2, 2024

Oh right, and does it conflict in any way with codewords? Or do these two act independently?

@VitusVeit
Copy link
Author

Perfect, really hoping this gets merged soon!

Thank you!

Oh right, and does it conflict in any way with codewords? Or do these two act independently?

I just ran a test and can confirm the codewords don't get highlighted since the highlights get overwritten by them.

@ScarKy0
Copy link
Contributor

ScarKy0 commented Sep 2, 2024

Perfect, finally the chat will be comprehensible

@VitusVeit
Copy link
Author

VitusVeit commented Oct 5, 2024

Is this already finished and awaiting approval or is it still ongoing? If it is already done, then I think we should discuss it in the discord channel.

Yep, the PR is completed. It's currently awaiting a review and there could be still some bugs but I found none left during testing.

@AverageNotDoingAnythingEnjoyer

Is this already finished and awaiting approval or is it still ongoing? If it is already done, then I think we should discuss it in the discord channel.

Yep, the PR is completed. It's currently awaiting a review and there could be still some bugs but I found none left during testing.

If it's done, maybe you should make a pr review request on wizden's discord?

@ScarKy0
Copy link
Contributor

ScarKy0 commented Oct 29, 2024

I do recommend bothering maintainers about it because otherwise it will never be reviewed :godo:

Copy link
Contributor

@ScarKy0 ScarKy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, do you think you could make the text highlighting enabled by default?

Not a maintainer but hope this review helps.
The code looks all fine to me and I just think those small additions to the autofill could be good.

Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/highlights.ftl Outdated Show resolved Hide resolved
@VitusVeit
Copy link
Author

Additionally, do you think you could make the text highlighting enabled by default?

Originally it was enabled by default, but I disabled it because I felt that even though the feature was useful, it could have still bugged some players if it was activated by default. I'm currently leaving it for a maintainer to decide.

@ScarKy0
Copy link
Contributor

ScarKy0 commented Nov 3, 2024

Originally it was enabled by default, but I disabled it because I felt that even though the feature was useful, it could have still bugged some players if it was activated by default. I'm currently leaving it for a maintainer to decide.

Yeah thats all good, was just wondering about it

@ScarKy0
Copy link
Contributor

ScarKy0 commented Nov 3, 2024

Though what you should do is join the discord (if you arent in it) and request review in the respective channel

@Errant-4 Errant-4 self-assigned this Nov 3, 2024
@Errant-4 Errant-4 added the S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. label Nov 3, 2024
@ScarKy0
Copy link
Contributor

ScarKy0 commented Nov 3, 2024

Be blessed Errant

@Errant-4
Copy link
Member

Errant-4 commented Nov 3, 2024

The only promise I make at this time is that we are discussing it

@ScarKy0
Copy link
Contributor

ScarKy0 commented Nov 3, 2024

The only promise I make at this time is that we are discussing it

Already plenty

@Errant-4
Copy link
Member

Errant-4 commented Nov 3, 2024

And that you can ping me specifically, if it seems like we forgot about it

@github-actions github-actions bot added S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Nov 13, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@SlamBamActionman SlamBamActionman added the S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. label Nov 14, 2024
@github-actions github-actions bot added size/L Denotes a PR that changes 100-1000 lines. and removed S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: UI Changes: Might require knowledge of UI design or code. S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/L Denotes a PR that changes 100-1000 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Text highlighting specific keywords