search

speedcontrol / nodecg-speedcontrol

NodeCG bundle to help facilitate the running of speedrunning marathons, including overlays.
MIT License
44 stars 34 forks source link

feat: Tag players in Twitch title #146

Closed duncte123 closed 1 year ago

duncte123 commented 1 year ago

A new twitch feature allows you to mention runners in the title of your stream. Their twitch theme settings determine the colour of the runner's name.

I've checked the usage of this function and it is only used for twitch title updates.

Examples: No hover Mouse over name

hoXyy commented 1 year ago

Shouldn't this use the player's Twitch name instead of the actual player name? You might want to use a name on the layout that's not their Twitch name (or the player themselves might want that), the way this is done kinda locks people into using Twitch names everywhere.

duncte123 commented 1 year ago

Shouldn't this use the player's Twitch name instead of the actual player name? You might want to use a name on the layout that's not their Twitch name (or the player themselves might want that), the way this is done kinda locks people into using Twitch names everywhere.

Good catch, hadn't thought of that yet as the current name is usually the same as the twitch name

zoton2 commented 1 year ago

Sorry for the delay in the review.

I was going to mention the Twitch username bit but it seems it's been covered/fixed already. My only concern is we should probably make this something the user can toggle via the configuration so we don't introduce "breaking" changes. I'm also not sure if I like it just going directly in a helper function that may get re-used elsewhere by mistake, but will have to look into it myself.

duncte123 commented 1 year ago

Sorry for the delay in the review.

I was going to mention the Twitch username bit but it seems it's been covered/fixed already. My only concern is we should probably make this something the user can toggle via the configuration so we don't introduce "breaking" changes. I'm also not sure if I like it just going directly in a helper function that may get re-used elsewhere by mistake, but will have to look into it myself.

I can rename the function, currently, it's only used to generate the Twitch title. I'm also happy to add a config item for this

duncte123 commented 1 year ago

I have renamed the function and added a config item that's disabled by default. Are there any other changes you want to see @zoton2?

zoton2 commented 1 year ago

I made a few minor changes/fixes to this feature, and added a README entry for it, can you double check and tell me if this is still OK?

duncte123 commented 1 year ago

I made a few minor changes/fixes to this feature, and added a README entry for it, can you double check and tell me if this is still OK?

LGTM