jstepien wrote:

Here's a slightly modified solution. Changes include:

- Changes to DoLoad's interface were reverted.
- Use localtime_r instead of stateful localtime.
- Replaced STR_NETWORK_SERVER_LIST_FILE_DATE with STR_SAVELOAD_DETAIL_FILE_MTIME
- Extracted FileMTime which returns file's modification time.

I've attached a screenshot showing how it changes the load game widget. Notice the "Saved" field in game's details.

Currently it won't compile on Windows but all what's necessary should be # defining POSIX localtime_r 1 as Microsoft's localtime_s 2 with arguments swapped. Unfortunately I haven't got Windows installed to check it.

Attachments

• date_of_file-2.diff (6.80 KiB)
• screenshort.png (25.80 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11288

Zuu wrote:

In general, I wonder if all passing around of mtime along with the file name is needed. But it might be that that is the case that it's good to do that. I need to look closer on the patch for that.

More specifically:
- The doxygen of FileReader class need to be updated to include the added parameter.
- if (NULL != (file = dynamic_cast<FileReader*>(reader))) { <--- It may be a good idea to put the assignment on a separate row for greater readability

I have not yet tried to compile it on Windows. That is still needed to ensure that it works there.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11478

Zuu wrote:

I've been able to apply your patch to current trunk and compile it on windows. The patch does seem to work on windows too.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11484

Zuu wrote:

Sorry, I didn't notice the forum discussion regarding the state of this task. But at least you got some feedback on the current patch. :-)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11485

jstepien wrote:

@zuu, thanks for taking time to review and give it a try.

• The doxygen of FileReader class need to be updated to include the added parameter.

You mean FileReader's constructor's comment, right?

• if (NULL != (file = dynamic_cast<FileReader*>(reader))) { <--- It may be a good idea to put the assignment on a separate row for greater readability

The dynamic_cast is used there to dynamically check whether the passed LoadFilter* is a FileReader*. I haven't looked for such applications of dynamic_cast in OpenTTD's code so I used a convention which is most natural for me in such cases. If you'd find splitting it into two lines a better approach I can fix it.

Sorry, I didn't notice the forum discussion regarding the state of this task.

This one 1? I haven't participated in it. Please attach a link if this issue is discussed anywhere else on the forums.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11486

Zuu wrote:

• The doxygen of FileReader class need to be updated to include the added parameter.

You mean FileReader's constructor's comment, right?

Yes

• if (NULL != (file = dynamic_cast<FileReader*>(reader))) { <--- It may be a good idea to put the assignment on a separate row for greater readability

The dynamic_cast is used there to dynamically check whether the passed LoadFilter* is a FileReader*. I haven't looked for such applications of dynamic_cast in OpenTTD's code so I used a convention which is most natural for me in such cases. If you'd find splitting it into two lines a better approach I can fix it.

It is true that other places in OpenTTD source code have an assignment inside an if-statement. So it is not black and white. I personally would try my best to not code if-statements with side effects to improve readability, but it appears no not be strictly forbidden in OpenTTD.

Sorry, I didn't notice the forum discussion regarding the state of this task.

This one 1? I haven't participated in it. Please attach a link if this issue is discussed anywhere else on the forums.

Yes, that one.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11487

jstepien wrote:

Here's an updated patch with Zuu's comments taken into account.

Attachments

• date_of_file-3.diff (6.85 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11489

jnicholl wrote:

I wasn't able to find any conclusion to this thread, so I made a modified patch that seems to take into account both the forum thread (referred to as [1] above) and also some modifications required to make this work on MinGW. It turns out that although localtime_r does work on MSVC, it does not work on MinGW, and furthermore, localtime_s does not exist on MinGW. However, localtime itself uses thread-local storage instead of static and is thus safe for our purposes (AFAIK). Since I can't guarantee that on any other platforms, there are two implementations provided, selected by # if defined(MINGW32).
I don't have any other platforms easily available for testing, so I can't guarantee I didn't break something. There was also one minor issue with the most recent patch (date_of_file-3.diff) where the return value of localtime_r was being checked incorrectly. I believe I have fixed that also.

I'm not sure if the truncation of the filename is desired, nor do I know what is the desired date format. I chose "%x %X" since that claims to be the locale-specific date format. It's unfortunate that this appears to limit the year display to two digits. A possible alternative would be "%Y-%m-%d %H:%M". "%c" is too verbose for the dialog, I think.

As an aside, while investigating thread-safety issues with localtime, I discovered that when the crash log writes out its buffer, it uses gmtime, which may have similar thread-safety issues on some platforms. I'm not familiar enough to know if this should be a concern or if other threads are stopped at this point.

Attachments

• date_of_file-4.diff (4.32 KiB)
• date_of_file-4.png (555.33 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11748

frosch wrote:

Adjusted some coding style and whitespace issue.
Also fixed initialisation of locale stuff, and changed the sizing of the columns.

There is also some more IRC discussion here: http://irclogs.qmsk.net/channels/openttd/link/1355521386# 1355521386
From that IRC the idea is somewhat to not use the locale stuff, but to use the OTTD ingame date format, and also add support for ingame time formats. (those formats are localised by the translators, so we do not have to rely on strftime format codes, but can add more custom formats)

Attachments

• fs5192.diff (7.74 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11753

jnicholl wrote:

Looks pretty reasonable. Is this patch an intermediate step along the way to ingame time formats? Or is that a future todo?

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192#comment11754

andythenorth closed the ticket.

Reason for closing: Won't implement

Flyspray clean up, no obvious route forward for this, so closing. Thanks.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5192