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

Flavor Text returns #575

Merged
merged 6 commits into from
Sep 13, 2023
Merged

Flavor Text returns #575

merged 6 commits into from
Sep 13, 2023

Conversation

Cenrus
Copy link
Contributor

@Cenrus Cenrus commented Sep 10, 2023

About The Pull Request

Reimplements flavor text. OOC notes not included

Why It's Good For The Game

It's hard to express your character with the ss13 sprite limitations so some short sentences can do wonders.

Changelog

🆑
add: Flavor text reintroduced
/:cl:

code/modules/mob/living/carbon/human/examine.dm Outdated Show resolved Hide resolved
@@ -343,6 +343,19 @@
else if(isobserver(user))
. += span_info("<b>Traits:</b> [get_quirk_string(FALSE, CAT_QUIRK_ALL)]")

var/flavor_text_link
/// The first 1-FLAVOR_PREVIEW_LIMIT characters in the mob's "flavor_text" DNA feature. FLAVOR_PREVIEW_LIMIT is defined in flavor_defines.dm.
var/preview_text = copytext_char((dna.features["flavor_text"]), 1, FLAVOR_PREVIEW_LIMIT)
Copy link
Member

Choose a reason for hiding this comment

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

Really not a fan of this being stored in DNA.features.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as storing it elsewhere does not replicate the problem I have seen in other servers where when the client disconnects, their flavor text doesnt go with the client which causes weird issues. @Kapu1178

Copy link
Contributor

@RimiNosha RimiNosha Sep 10, 2023

Choose a reason for hiding this comment

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

May be best to store it in mind instead.

Actually that's a stupid idea due to that being transferred with the brain IIRC, just write it to a var on the mob. Maybe it could be reused for giving other mobs extra examine info too, though that might be out of scope.

code/modules/mob/living/carbon/human/examine.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/human/human.dm Outdated Show resolved Hide resolved
Comment on lines 16 to 18
if(istype(user, /mob/dead/new_player))
var/mob/dead/new_player/player = user
player.new_player_panel()
Copy link
Member

Choose a reason for hiding this comment

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

This is rather odd, what's it for?

return button_element(prefs, "Set Examine Text", "pref_act=[type]")

/datum/preference/text/flavor_text/user_edit(mob/user, datum/preferences/prefs)
var/input = tgui_input_text(user, "Describe your character in greater detail.",, serialize(prefs.read_preference(type)))
Copy link
Member

@Kapu1178 Kapu1178 Sep 10, 2023

Choose a reason for hiding this comment

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

Needs a sensible length limit. You also might want to make this a multi-line input and strip out the newline characters after.

@gonenoculer5
Copy link
Contributor

An additional safety measure, should a flavortext contain a link, the link (and associated user and character) needs to be logged in ADMINLOG, so that the content of the link can be verified. Might need to work with Kapu on that. I am requesting this for the sake of administrations sanity. I will be writing policy regarding Flavortext and passing it for administration and the leads approval before this is merged.

@gonenoculer5
Copy link
Contributor

An additional safety measure, should a flavortext contain a link, the link (and associated user and character) needs to be logged in ADMINLOG, so that the content of the link can be verified. Might need to work with Kapu on that. I am requesting this for the sake of administrations sanity. I will be writing policy regarding Flavortext and passing it for administration and the leads approval before this is merged.

@Kapu1178 @francinum @fighterslam

@Kapu1178
Copy link
Member

An additional safety measure, should a flavortext contain a link, the link (and associated user and character) needs to be logged in ADMINLOG, so that the content of the link can be verified. Might need to work with Kapu on that. I am requesting this for the sake of administrations sanity. I will be writing policy regarding Flavortext and passing it for administration and the leads approval before this is merged.

They won't embed. Embedding player-written HTML is never intentional. It is no different than a player putting a link in papercode, or in chat.

@gonenoculer5
Copy link
Contributor

Thats fine. I dont want it to embed, just have the link logged so we can check it.

@Kapu1178
Copy link
Member

Thats fine. I dont want it to embed, just have the link logged so we can check it.

I don't think thats necessary at all. it is the same as papercode/chat.

@gonenoculer5
Copy link
Contributor

gonenoculer5 commented Sep 10, 2023

After discussing with the leads, we'll just make links against the rules for flavortext. 2-3k characters is fine for the text limit imo.

@Kapu1178
Copy link
Member

After discussing with the leads, we'll just make links against the rules for flavortext. 4-6k characters is fine for the text limit.

fuck no 4-6k is INSANE, people do not need to be writing autobiographies

@Cenrus
Copy link
Contributor Author

Cenrus commented Sep 10, 2023

MAX_FLAVOR_LEN is at 4096 at the moment, I haven't changed the value myself

@Kapu1178
Copy link
Member

MAX_FLAVOR_LEN is at 4096 at the moment, I haven't changed the value myself

Ill deal with it later, dw

@Kapu1178
Copy link
Member

@Cenrus Please address remaining reviews

@Kapu1178 Kapu1178 merged commit 0217682 into DaedalusDock:master Sep 13, 2023
12 checks passed
@Cenrus Cenrus deleted the flavor branch September 13, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants