OpenTTD

Tasklist

FS#5192 - Display Date of File

Attached to Project: OpenTTD
Opened by Andreas (Wurzel) - Sunday, 20 May 2012, 17:35 GMT
Last edited by andythenorth (andythenorth) - Saturday, 02 September 2017, 12:08 GMT
Type Patch
Category Interface
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Hi,

I added the modification date of a file to the load/save GUI. It is one of the tasks from the ToDO List http://wiki.openttd.org/Todo_list.

Some additional discussion: http://www.tt-forums.net/viewtopic.php?f=33&t=61055

This task depends upon

Closed by  andythenorth (andythenorth)
Saturday, 02 September 2017, 12:08 GMT
Reason for closing:  Won't implement
Additional comments about closing:  Flyspray clean up, no obvious route forward for this, so closing. Thanks.
Comment by Jan Stępień (jstepien) - Sunday, 17 June 2012, 16:27 GMT
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.

[1]: http://pubs.opengroup.org/onlinepubs/000095399/functions/localtime.html
[2]: http://msdn.microsoft.com/en-us/library/a442x3ye(v=VS.80).aspx
Comment by Leif Linse (Zuu) - Thursday, 30 August 2012, 18:52 GMT
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.
Comment by Leif Linse (Zuu) - Sunday, 02 September 2012, 11:44 GMT
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.
Comment by Leif Linse (Zuu) - Sunday, 02 September 2012, 11:58 GMT
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. :-)
Comment by Jan Stępień (jstepien) - Sunday, 02 September 2012, 12:18 GMT
@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.

[1]: http://www.tt-forums.net/viewtopic.php?f=33&t=61055
Comment by Leif Linse (Zuu) - Sunday, 02 September 2012, 13:38 GMT
> > - 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.
>
> [1]: http://www.tt-forums.net/viewtopic.php?f=33&t=61055

Yes, that one.
Comment by Jan Stępień (jstepien) - Sunday, 02 September 2012, 18:51 GMT
Here's an updated patch with Zuu's comments taken into account.
Comment by Jeremy Nicholl (jnicholl) - Monday, 10 December 2012, 05:58 GMT
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.
Comment by frosch (frosch) - Saturday, 15 December 2012, 00:07 GMT
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)
Comment by Jeremy Nicholl (jnicholl) - Saturday, 15 December 2012, 00:39 GMT
Looks pretty reasonable. Is this patch an intermediate step along the way to ingame time formats? Or is that a future todo?

Loading...