raidboss: Support for objects in trigger outputStrings #5861
Conversation
Extract the ShortName logic/functions to Util, clean up duplicate code Narrow the typedef for params objects Add helpers to get party member param object Add config option for default display label for players Clean up handling of param objects
Co-authored-by: Adrienne Walker <enne@quisquo.us>
|
@valarnin Sorry to step on your patch, but I wanted to get this in while it was on my mind since there's a bunch of changes after this that would be good to make. |
|
All good, I'm fine with you taking it over. This fell a bit to the wayside due to IRL obligations. |
Yeah, I figured! Did you want to take a look at the changes? |
resources/party.ts
Outdated
| job?: Job; | ||
| id?: string; | ||
| name: string; | ||
| nick: string; |
There was a problem hiding this comment.
Most triggers currently use shortName which is why I included it here to begin with. It seems that nick is taking a dual role/default to shortName logic path here which may be counterintuitive since everything else here should be a concrete value.
It may make sense to include both here instead, and have the default be nick still, so that users have the option of still using shortName for specific triggers?
There was a problem hiding this comment.
I'm not sure what you mean by this, or by concrete value. data.ShortName the function is both "first name" and "nickname substitution". Users don't really know anything about "short name" as a function and I felt like "nick" better summed up what it did.
What would both options be? Would shortName just be firstName? Would nick be firstName if it didn't have a substitution? I'm not sure when you would ever want to pick between these.
There was a problem hiding this comment.
My original thought was that shortName would be firstName, yes. But on further reflection I don't think this is really a requirement.
My thought process originally was that a given field should be "concrete" as in the field shouldn't vary depending on config values, meaning that nick should always be a specified nickname or blank, and shortName would always be firstName. But that's probably not very intuitive to the end user.
There was a problem hiding this comment.
Gotcha.
I think we could leave the nick field blank if there's no nick and then also have the default field be the short name and it would have the same end result for a user who picks "nick". However, I don't think end users will notice this or be able to do anything with that change either?
There was a problem hiding this comment.
Yeah, I think this is fine as-is. As I said in my last comment, I think that keeping nick as nickname ?? firstName is more user-friendly here.
Maybe it also makes sense to add firstName as a separate field but I feel like the user should be able to just set someone's nickname to their first name if they wanted to anyways.
There was a problem hiding this comment.
Also, I do think this is not something that folks writing triggers will deal with explicitly, at least re: party members. (Maybe if there's some other object type, they would.) And so I think we should feel somewhat free to change what this looks like in the future along with the options if that makes sense.
Edit: I could imagine adding some extra options for something to differentiate multiple jobs, e.g. "MNK 2" vs "MNK FirstName"
There was a problem hiding this comment.
Also as an in-general comment, it feels kind of bad to be adding even more code to
popup-text.tswhen that file is already so massive, but I'm not sure if it's feasible to offload the new code to something else.
Agreed, but also it seems like a preexisting problem that can be solved in a different PR.
resources/party.ts
Outdated
| job?: Job; | ||
| id?: string; | ||
| name: string; | ||
| nick: string; |
There was a problem hiding this comment.
I'm not sure what you mean by this, or by concrete value. data.ShortName the function is both "first name" and "nickname substitution". Users don't really know anything about "short name" as a function and I felt like "nick" better summed up what it did.
What would both options be? Would shortName just be firstName? Would nick be firstName if it didn't have a substitution? I'm not sure when you would ever want to pick between these.
Followup to #5861. There's no "output strings" in oopsy, but it does use ShortName in a few places. Move this over to data.party.member(x).toString() in order to potentially support an oopsy option for how to display names.
Followup to #5861. This changes some logic for mit tracking and raise tracking to use full names instead of short names when storing and comparing, only using the shortened version for output. This also creates the party tracker earlier so that it can be passed to all the things that want to use it, and so all of oopsy can indirect through `PartyTracker` to find out short names.
Followup to #5861. There's no "output strings" in oopsy, but it does use ShortName in a few places. Move this over to data.party.member(x).toString() in order to potentially support an oopsy option for how to display names. This also changes some logic for mit tracking and raise tracking to use full names instead of short names when storing and comparing, only using the shortened version for output. This also creates the party tracker earlier so that it can be passed to all the things that want to use it, and so all of oopsy can indirect through `PartyTracker` to find out short names.
Followup to #5861. There's no "output strings" in oopsy, but it does use ShortName in a few places. Move this over to data.party.member(x).toString() in order to potentially support an oopsy option for how to display names. This also changes some logic for mit tracking and raise tracking to use full names instead of short names when storing and comparing, only using the shortened version for output. This also creates the party tracker earlier so that it can be passed to all the things that want to use it, and so all of oopsy can indirect through `PartyTracker` to find out short names.
Followup to #5861. There's no "output strings" in oopsy, but it does use ShortName in a few places. Move this over to data.party.member(x).toString() in order to potentially support an oopsy option for how to display names. This also changes some logic for mit tracking and raise tracking to use full names instead of short names when storing and comparing, only using the shortened version for output. This also creates the party tracker earlier so that it can be passed to all the things that want to use it, and so all of oopsy can indirect through `PartyTracker` to find out short names. 2cc271d
This is #5551, with some additional changes on top to resolve a few conversations.
Closes #5551.