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

Allow forcing 24h clock and leading zeroes on month and date strings #1401

Conversation

BlackDog86
Copy link
Contributor

@BlackDog86 BlackDog86 commented Oct 29, 2024

Fixes #1400

@BlackDog86 BlackDog86 self-assigned this Oct 29, 2024
@BlackDog86 BlackDog86 added enhancement ready-to-review A pull request is ready to be reviewed labels Oct 29, 2024
@BlackDog86 BlackDog86 force-pushed the 1400-Date-And-Time-Formatting-Changes branch 3 times, most recently from bc01e5c to e0dc32b Compare October 30, 2024 19:18
X2WOTCCommunityHighlander/Config/XComGameData.ini Outdated Show resolved Hide resolved
@@ -1621,6 +1629,26 @@ static function string GetDateString(TDateTime kDateTime, optional bool bShortFo
kTag.IntValue1 = kDateTime.m_iYear;
kTag.IntValue2 = kDateTime.m_iMonth;

// Issue #1400 - Add additional localization tags to allow mods to display days and months with leading zeroes
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an opinionated change, it should be at least gated behind a separate config var.

Copy link
Contributor Author

@BlackDog86 BlackDog86 Nov 2, 2024

Choose a reason for hiding this comment

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

this isn't really a change at all - the default localization file uses this for the display of dates:
m_strMonthDayYearShort="<XGParam:IntValue2/><XGParam:StrValue1/!SeparatorString/><XGParam:IntValue0/><XGParam:StrValue1/!SeparatorString/><XGParam:IntValue1/>"

The default display would therefore not change at all with the code adjustment. A mod would have to specifically remove the original localization line and replace it with XGParam:StrValue2 or XGParam:StrValue3 explicitly in order for this to adjust base-game behaviour at all so I don't think a config gate is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to be a Highlander change then? It this does nothing without mod-supplied localization lines, what's stopping the mod that is adding those lines from adding its own localized tags and using the tag expand handler to fill them with whatever date format it wants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the mod would need to MCO X2StrategyGameRulesetDataStrcutures just to create a couple of tags which is quite an invasive MCO for a very small change (plus there are already some mods existing MCOing other parts of this, creating conflicts. I thought this sort of thing is more or less exactly what CHL is for?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why would MCO be necessary. You can create arbitrary tags and fill them with arbitrary content via the DLC Hook, no MCO required. Mods can always ship their own adjusted version of GetTimeStringSeparated() to use in their tags.

@Iridar Iridar added waiting-on-author A pull request is waiting on changes from the author and removed ready-to-review A pull request is ready to be reviewed labels Nov 2, 2024
@BlackDog86 BlackDog86 force-pushed the 1400-Date-And-Time-Formatting-Changes branch 2 times, most recently from 39e24b7 to c149fa1 Compare November 2, 2024 03:54
@BlackDog86 BlackDog86 added ready-to-review A pull request is ready to be reviewed and removed waiting-on-author A pull request is waiting on changes from the author labels Nov 2, 2024
@BlackDog86
Copy link
Contributor Author

BlackDog86 commented Nov 12, 2024

After a bit of further investigation & for the purposes of clarity, the only place that the 'short' date is actually called for in the base game, is in the memorial screen. However, other mods (such as beat's mission history) also call for it. See below some examples of the display options:
Memorial before:
image
Memorial after, swapping the short date tags in localization:
image

Clock example, base game:
image
Clock example with bforce24hclock and bforce24hclockleadingzero on:
image
Clock example with bforce24hclock and bforce24hclockleadingzero on and using tag swapping to swap the long dates as well (doing this can adjust the format for soldier birthdays, research reports, avenger facility construction dates in 'avenger report' and a couple other places.
image

I should say that the save and load game screens do not use this function and have their own logic for parsing the save file headers to display the dates of the save games. This logic isn't great either in my view and in one of my own mods I MCO a few of the classes responsible in order to display those dates differently (https://steamcommunity.com/sharedfiles/filedetails/?id=3268722967). This is the output of that mod, for example:
image. The base-game code is a bit janky in that area as well but making that better is out of scope for this, which is just making a few small changes for the sake of flexibility.

@BlackDog86 BlackDog86 force-pushed the 1400-Date-And-Time-Formatting-Changes branch from c149fa1 to 5c1871b Compare November 12, 2024 20:44
@Iridar
Copy link
Contributor

Iridar commented Nov 14, 2024

For context to the above discussion, I didn't understand that the following pipeline was in place:

  1. Different parts of the code call the GetDateString() function
  2. Which takes one of the two existing strings that exist solely for the purpose of arranging date
  3. Expands its tags
  4. Returns the formatted date to whatever code caled the function

So I didn't understand why changes to GetDateString() were necessary.

@Iridar Iridar added ready-for-merge the pull request was reviewed and is ready to be merged. and removed ready-to-review A pull request is ready to be reviewed labels Nov 14, 2024
@Iridar Iridar merged commit bc7f1f3 into X2CommunityCore:master Nov 14, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready-for-merge the pull request was reviewed and is ready to be merged.
Projects
None yet
2 participants