Core/Server: correct timestamp format for shutdown/restart notification broadcasts #24181
Conversation
…TION_OVERRIDE_WEATHER
|
Can confirm this works, good job. :-) EDIT: Can I just ask, what are the other irrelevant commits you've included in this pull request? I'm not trying to be funny I just can't understand why. |
Those commits are there to keep the PR branch up to date with the 3.3.5 branch (at least for the content of this PR), to avoid file conflicts. |
Hi, thank you for clarifying! :-) |
You're welcome. The commits don't look quite like TC commits, but there may have been some back-and-forth during the creation of his PR, so I am guessing he didn't start off with a fully updated source before creating the PR branch. |
I can't wait for this PR to be merged if it ever happens. I've been wanting to get this sorted for a long time. I'm all about Blizz-like! :-) |
|
Sure, I can see that. I enjoy seeing people making constructive changes toward blizzlike content, especially when the end result gives the player a feeling of being in a game environment with a very close to retail experience. |
That is honestly my aim, people say to me "Why on earth would you focus on silly things like that?" It all counts! |
|
@ForesterDev : would you mind adding the content changes from commit 9009c82 to have all 4 PR tests working as intended? |
src/common/Utilities/Util.cpp
Outdated
| } | ||
| if (secs || (!days && !hours && !minutes)) | ||
| { | ||
| ss << secs; |
There was a problem hiding this comment.
Will this format seconds value as a single digit when between 0 and 9?
There was a problem hiding this comment.
Yes, but for restart/shutdown notifications this situation is impossible, that why I did not process this case
There was a problem hiding this comment.
@Warpten not worth using fmt separately for multiple parts of this function (and you can't wrap it all in one big format)
| ss << days << (shortText ? "d" : " Day(s) "); | ||
| { | ||
| ss << days; | ||
| if (timeFormat == TimeFormat::Numeric) |
There was a problem hiding this comment.
switch (timeFormat)
{
case TimeFormat::Numeric:
ss << ":";
break;
case TimeFormat::ShortText:
ss << "d";
break;
case TimeFormat::FullText:
[[fallthrough]];
default:
if (days == 1)
ss << " Day ";
else
ss << " Days ";
break;
}
|
Thanks for the PR :) |
|
Thank you those who helped, especially @ForesterDev :-) |
|
While this has been resolved, I actually have an idea that we could implement, later on, so obviously we can't make this format work for anything over 59 minutes, else it would look messed up. So I thought maybe we could limit the countdown timer to 59 minutes to solve that matter? |
|
Well, that might be useful on custom servers, because I can't remember seeing retail servers ever announcing an upcoming restart or shutdown in more than 30 or 45 minutes. Whoever chooses to use a warning for more than 59 minutes should consider this a special (custom) case. (At least that is what it looks like to me.) |
Once their automated shutdown countdown kicked in early which resulted in it printing random messages to chat. People were seeing a 53:00 minute warning. Blizzard_CSEU on Twitter even apologized for it.
Think I could make it a feature request like you could configure it in the worldserver.conf? |
|
Feature request ticket sounds fair enough. (I am not a TC member, though. Just speaking from general experience.) |
…on broadcasts (#24181) * Core/SmartScripts: implement SMART_ACTION_OVERRIDE_LIGHT and SMART_ACTION_OVERRIDE_WEATHER * Core/Server: correct timestamp format for shutdown/restart notification broadcasts * remove unexpected changes * move enum from Common to Util * Use enum class instead of enum * Fix width for seconds 0 to 9 (cherry picked from commit 6923158)
Changes proposed:
Target branch(es): 3.3.5/master
Issues addressed: Closes #24174
Tests performed: (Does it build, tested in-game, etc.) Build, tested
