This gets computed in gtkfilechooserdefault.c:list_mtime_data_func().  Do you think you could cook a little patch to add the time to the generated string?

Hi Federico,
I see from the 2.12.2 "News" history, that you're knowledgable in modifying this code (2.8.6->2.8.7 changes).

Three things scare me about diving in:

(1) I've never programmed anything substantial in C. I'll be happy to try but the problem is, my code will probably look like s*%$ in the eyes of a person with real experience.

And, how do I rebuild the GTK+ library with *my* version of gtkfilechooser? Or, should I build gtkfilechooser as an application? 

(2) I'm not sure, will I need to be creating different code for Windoze versus GNU-Linux/Unix, or does list_mtime_data_func() already have access to the information I need? 

(3) I can't even spell i10n.

(4) After I've got a routine coded, what should I use to test it? Just a do-nothing-else call it and verify the functionality of my GUI and check what gets returned?

Thanks! I'll keep my eye open for your hints.

BTW, right now I'm inclined to actually put in a checkbox for display of file details... there's another open RFE/Bug about showing and sorting according to file size, and I'm sure that people often want to sort the list according to extension as well. Basically, a checkbox to upgrade the FileChooser GUI list to match Nautilus or "ls -al".

Notes to myself: The routine starts at line 7122 of the 2.8.8 source file. Although GTK+ 2.0 tutorial focuses on compiling user programs, the instructions within the download (HACKING) are good. So my "scary #1" is gone. There seems to already be some code there for a "size" column, I might zap the "if 0 {" blocks to cause compilation, see what I get.

I've no idea why they're coded "if 0" with the integer, rather than doing #define of a constant.

Back to my issue (showing modification time):
The statement at line 7149 in my copy GTK+-2.8.8)  
   "time_mtime = gtk_file_info_get_modification_time (info)" 
is setting a 64-bit int with a UNIX time_t value (usually 32 bits).

The statement at line 7155 is setting a date from this value:
   "g_date_set_time (&mtime, (GTime) time_mtime);"
but the result contains the date in two formats, plus a couple of flags. It is a big conversion, not a simple chopping off of milliseconds. (So, I can't just subtract to get the 'extra' seconds, the time-of-day which I want.)

Now, deep in the code of "gtkfilesystemwin32.c" (lines 1584 - 1591), I see that someone else has already handled the terrifying conversion of Windoze time to Unix time. So, my scary #2 is also gone! (He coded a double bitwise-left shift, a divide, a little subtraction to adjust from Windoze 1601 to Unix 1970, and included a comment "urgh!". I was right to be scared.)

If I do my own math on the original time_mtime value (MOD 86400 to remove whole days, then divide the leftover seconds into hours/minutes) I can easily create the "hh:mm" data and shove it in the buf, preceeding the date.

But, a slightly nasty add-on issue is: how much new code must I add to sort on the columns correctly? I can't make it an independent column, sorting on *just the time* is nonsense. I can't tell whether the column is being sorted on the Gregorian date "mtime" (I think it is, that's bad), or whether it's sorted on the non-displayed original time_mtime values (that's what I need to do).

I've put the time to the left of the date, within the same column. It's sorted correctly. But I have one problem left:

The times shown are GMT, not local. I've no idea how Nautilus looks up and applies the time zone offset in its display (it even shows the time zone initials). I need to do that TZ Offset adjustment here as well.

Federico, have you got any hints for me?

If you have a time_t, you can use localtime_r() from <time.h> to convert it to your timezone.  That will give you a struct tm, which is already split up into hours/minutes/seconds/etc.  You should use the values that it gives you; there's no need to do "mod 86400" computations or anything like that (they would be incorrect for exotic things like years with leap seconds...).

Right now we use g_date_strftime() to format the GDate value into a string, but that function only handles the date-specific formats of strftime().  We can probably just use raw strftime() to format the time.

To test your code, just compile GTK+ and run the gtk+/tests/testfilechooser program.

You shouldn't need to change any code for sorting.  The mtime_sort_func() function in gtkfilechooserdefault.c already compares the raw time values.

Thanks, I just came to the same conclusion before reading your reply (grrrr, I should've come here first). I was unsure whether it would be OK to use the standard C function localtime:

localtime_value =  localtime (&file_time_raw);

in gnome, since all of this has to work in Windoze as well.
But, I found where Nautilus does it.

BTW, do you prefer separate patches for this RFE (show date with time) versus showing file size? Or roll them both into one? I think separate is a much better way to go, the file size column changes are bigger.

Also: I've written my patches for enhancing 'modified' and showing the file size are simple 'show it unconditionally' patches, which doesn't have any of the other columns, or the GUI for column selection, shown in Nautilus and 'requested' by the author of bug 141155.

I agree that 'extension' and/or 'file type' (I prefer to use 'extension') are very valuable features in Nautilus... I've been sorting header files versus c files quite a lot today. But, if you're not expecting it, the fact that the  file name is chopped up with the 'html' or 'c', or 'lst', or 'txt' etc. shown in another column might be a bit too weird.

Awaiting your instructions, and thanks again!

BTW, I have *NO* idea how a CVS Update looks. (Haven't used a 'Can I just attach the Source for the Routine (with modifications) in here, and leave up to you to convert it into an actual update?

BTW, I'm testing by running a Firefox Nightly and exercising the "Open File" dialog. (I'm installing "my GTK" as the real one.) 

gtk+/tests/testfilechooser operation and comments on stdout look good. (I was hoping that it would exercise some other locales, or try to work with some unicode file names, but, nope.)

Attachment is my version of the routine. We already include <time.h> at line 80, so I didn't need to make changes anywhere else.

I used localtime() instead of localtime_r(), it seems to me that a "requirement for re-entrant code" would have equal bustage with our use of time_mtime and the buf()... But aren't these all allocated on the stack, unique instances with each invocation? (Fix it if you need to, I obviously don't understand this at all.)

The new strftime() result appears exactly as the column nautilus does.

Created attachment 56428 [details]
modified function list_mtime_data_func()

Source code for a complete replacement of the existing function, top to bottom.

function starts at line 7120 of the gtk+-2.8.8 gtkfilechooserdefault.c source file. There might be other changes which make the CVS location a bit different.

To extract a patch from two files:

diff -u -p oldfile.c newfile.c > my-patch.diff

That's the most convenient way to look at changes, although for such a small, self-contained function, posting the function is probably okay :)

The issue of localtime_r is not of reentrancy, but that another thread may want to use localtime() at the same time.  So let's use localtime_r().

Using plain strftime() is a bit tricker than in your function.  Take a look at g_date_strftime() in glib to see what it does to grow the buffer size if needed.

Please remove the spaces around your " %c " string.  I'm pretty sure we'll need to change that format string (need to consult with the internationalization people), but it's better to have it without spaces, as padding is the job of GtkTreeView.

Finally, don't assume that time_t is 32 bits --- it certainly isn't on 64-bit platforms :)

To reduce visual clutter, it would probably make sense to only show the time information for today's and yesterday's files.  What do you think?

See also bug #325064.

(In reply to comment #11)
> To reduce visual clutter, it would probably make sense to only show the time
> information for today's and yesterday's files.  What do you think?
> 
It doesn't save us any width to remove time from older files. And, each row where we leave out the time, the shorter "date" portion will be justified differently, I think it makes the appearance worse (not better).

And, it certainly removes useful information. Example: I open a filechooser, want to re-open the newest file in a Directory I "haven't touched over the weekend", or some other period of days. (Which we can't really take a good guess at, what if Monday was a holiday too?)  

(In reply to comment #10)
> To extract a patch from two files:
> 
> diff -u -p oldfile.c newfile.c > my-patch.diff

I'll do so in the future, and probably with this one. Thanks for the instruction!

> another thread may want to use localtime() at the same time.
A bright light goes on, NOW I understand.

> Using plain strftime() is a bit tricker than in your function.  Take a look at
> g_date_strftime() in glib to see what it does to grow the buffer size if
> needed.

I'll use it. I have a good example of a g_strxxx() call and release of the rturned string in the 'size' code. 

 
> Please remove the spaces around your " %c " string. Done. As you point out, padding is already provided.

> Finally, don't assume that time_t is 32 bits --- it certainly isn't on 64-bit
> platforms :)
I meant to highlight: The time_t **might** result in a 2038 problem if time_t is still 32-bits at that time. But it won't be. I'll just remove the comments about this, I only put them there as a temporary FYI.

Again, thank you for these instructions!!!

See also bug #334718, which is about date localization.

Created attachment 85327 [details] [review]
proposed patch

while we wait for the glib function for formatting a date, this is a patch implementing the feature along the lines of comment #11. I tend to agree with federico: I think that having the time just on the files modified today and yesterday is enough - if I changed something more than three days ago chances are I don't really care about the time it was changed. the patch also removes the (possibly unsafe) usage of strcpy with a fixed length buffer and strings coming from gettext().

Created attachment 85329 [details]
screenshot of the filechooser with the patch applied

this is a screenshot showing the effects of the patch on the file chooser dialog.

Nice patch, Emmanuele :)  Thanks as always.  Feel free to commit.

2007-06-26  Emmanuele Bassi  <ebassi@gnome.org>

        * gtk/gtkfilechooserdefault.c (list_mtime_data_func): Show
        the time of last change in the file chooser, for files
        modified today or yesterday. (#324543)

        * configure.in: Check for localtime_r().

I might be wrong but I think there is a bug here:

+          memcpy ((void *) &tm, (void *) ptm, sizeof (struct tm));

I don't think tm is declared anywhere, should probably be tm_mtime?

yeah, it has already been fixed in trunk.

Ah, thanks!