Created attachment 326117 [details] [review]
Proposed patch (version 2, requires patching evolution code too)

Requires patching the evolution code too.

Created attachment 326118 [details] [review]
Proposed patch (version 2) for the evolution code

Requires patching the evolution-data-server code too.

Review of attachment 326117 [details] [review]:

Thanks for a bug report and patch, but I'm rejecting it. There are couple issues:
a) you leak saved_locale if the locale_fmt is NULL
b) you do not check for NULL and pass the saved_locale to setlocale() and
   free() (you might better use g_strdup() and g_free() here)
c) what if the user has set those locale variables differently intentionally?

Review of attachment 326118 [details] [review]:

I'm setting this to reviewed. The change at line with "case ATTRIB_STRFTIME:" is not needed. Otherwise this is better approach than changing whole e_utf8_strftime, as you had it initially, but still the other concerns apply here.

Thanks very much for reviewing the patch. I'll submit a new patchset shortly.

As for point c), it's impossible: no one would ever want to compose (or read) a sentence in two different languages (would you like to read a sentence such as "Today venerdì 15 April 2016 I submitted a patch to evolution development"). No way !

Honestly, I do not recall when I heard the last time about LC_MESSAGES not matching LC_TIME. Evolution has an option for that particular string, not only available for translators, but users can define their own string in gsettings, which lets then user their native language for the UI, but reply with quotation "header" in one language, line in English. That's for your "No way!".

By the way, the "April" should also honour the LC_TIME, but it's in English. Isn't it weird? (I didn't check the code, I do not know off head.)

It would be also good to use evolution-data-server coding style, and GLib types, not plain 'char' for example. And if you could avoid code duplication, because the only thing you really want to do is to change the locale before calling strftime, otherwise the rest should stay as is, then your wrapper can call the original function, instead of duplicating the code, and do it always, not only for that single format (if I'd pass set of attributes at once then the function would simply fail and use "the wrong" language for the week day name).

Finally, there is a little concern about portability. I think that the construction you use is portable, like to Win32, but I'm not completely sure.

Created attachment 326124 [details] [review]
Reviewed patch for the evolution code

Requires patching the evolution-data-server code too.

Created attachment 326126 [details] [review]
Reviewed patch for the evolution-data-server code

Requires patching of the evolution code too.

I have just submitted the two new patches.

I really believe that we need to keep things simple to use (so, the string should be in only one language by default, without the need to tweak with gsettings which is not trivial for most users).

As for you comment about the month, from what I can see, the current code does not use the formatted month name yet.

Review of attachment 326126 [details] [review]:

I see you didn't do everything I wanted from you. At least these two things should be addressed:
a) do not duplicate the code, instead call the e_utf8_strftime() from within the e_utf8_strftime_matched_lang()
b) the LC_MESSAGES is not portable, you should count with it: https://msdn.microsoft.com/en-us/library/x99tb11d.aspx

Review of attachment 326124 [details] [review]:

::: evolution-3.20.1-orig/mail/em-composer-utils.c
@@ +3025,2 @@
 	{ "{AmPmUpper}", ATTRIB_STRFTIME, { "%p", NULL } },
 	{ "{AmPmLower}", ATTRIB_STRFTIME, { "%P", NULL } },

All the ATTRIB_STRFTIME should be using the MATCH_LANG variant, otherwise you still keep the inconsistency. That is, the name of the attribute could be left the same, just the corresponding case at the bottom should call a different function, but, ideally, with a hidden (gsettings) option with its default to 'false', to use the matched_lang() variant, which is actually match_lc_messages(), not matched_lang(). The default will be 'false', to not use the new function.

Thinking of it, I do not see any other usage of that new function at the moment, thus it might be better to not touch the eds code at all and define such function in the evolution only, at the place where it is currently used. To avoid unnecessary repeated calls for the "does LC_TIME match LC_MESSAGES" test, a caching on the evolution side would be also useful, ideally only within the attribution_format() call.

(In reply to Milan Crha from comment #10)
> Review of attachment 326126 [details] [review] [review]:
> 
> I see you didn't do everything I wanted from you. At least these two things
> should be addressed:
> a) do not duplicate the code, instead call the e_utf8_strftime() from within
> the e_utf8_strftime_matched_lang()

If you call the e_utf8_strftime() from within the e_utf8_strftime_matched_lang(), then it is not possible to free saved_locale (with trivial code) if an error occurs.

> b) the LC_MESSAGES is not portable, you should count with it:
> https://msdn.microsoft.com/en-us/library/x99tb11d.aspx

Thanks for pointing that out. I have now introduced conditional code compilation there.

(In reply to guido from comment #13)
> (In reply to Milan Crha from comment #10)
> > Review of attachment 326126 [details] [review] [review] [review]:
> > 
> > I see you didn't do everything I wanted from you. At least these two things
> > should be addressed:
> > a) do not duplicate the code, instead call the e_utf8_strftime() from within
> > the e_utf8_strftime_matched_lang()
> 
> If you call the e_utf8_strftime() from within the
> e_utf8_strftime_matched_lang(), then it is not possible to free saved_locale
> (with trivial code) if an error occurs.

The e_utf8_strftime_matched_lang() function can be removed completely and the e_utf8_strftime() function can be modified to (always) support this fix. There shouldn't be undesired side-effects.

(In reply to guido from comment #13)
> If you call the e_utf8_strftime() from within the
> e_utf8_strftime_matched_lang(), then it is not possible to free saved_locale
> (with trivial code) if an error occurs.

Are you sure?

   e_utf8_strftime_match_lc_messages()
   {
      possibly change locale;
      res = e_utf8_strftime();
      free whatever is needed from 'change locale';
      return res;
   }

(In reply to guido from comment #14)
> The e_utf8_strftime_matched_lang() function can be removed completely and
> the e_utf8_strftime() function can be modified to (always) support this fix.
> There shouldn't be undesired side-effects.

No, the e_utf8_strftime() is not used only in this single place, then it would be an error. You had it this way at the initial patch, but you replaced it before I rejected it for this reason.

(In reply to Milan Crha from comment #15)
> (In reply to guido from comment #13)
> > If you call the e_utf8_strftime() from within the
> > e_utf8_strftime_matched_lang(), then it is not possible to free saved_locale
> > (with trivial code) if an error occurs.
> 
> Are you sure?
> 
>    e_utf8_strftime_match_lc_messages()
>    {
>       possibly change locale;
>       res = e_utf8_strftime();
>       free whatever is needed from 'change locale';
>       return res;
>    }

What's the point of creating an inconsistency that it might shows up as a bug elsewhere in the code now or in the future ?

It LC_MESSAGES should override LC_TIME for time formatting, then that is is likely to be desirable everywhere throughout the application code. So there is no point of having two functions...

Also, I have now changed e_strftime() too, for exactly the same reason (consistency of behaviour throughout all parts of the application).

> (In reply to guido from comment #14)
> > The e_utf8_strftime_matched_lang() function can be removed completely and
> > the e_utf8_strftime() function can be modified to (always) support this fix.
> > There shouldn't be undesired side-effects.
> 
> No, the e_utf8_strftime() is not used only in this single place, then it
> would be an error. You had it this way at the initial patch, but you
> replaced it before I rejected it for this reason.

I know, it's not only used there (see above) ! I now believe it's an error to use the fix only in the mail message composer...

I have just spotted another time formatting inconsistency in other parts of the application: in the calendar the month is formatted as LC_MESSAGES="C", while the day of the week is formatted as LC_TIME.

(In reply to guido from comment #16)
> What's the point of creating an inconsistency that it might shows up as a
> bug elsewhere in the code now or in the future ?

It's not any inconsistency. The only reason I would accept your proposal is in this particular use case, in the reply credits.

> Also, I have now changed e_strftime() too, for exactly the same reason
> (consistency of behaviour throughout all parts of the application).

No, as I said above, I'll not approve it. The change in the reply credits is special, because it is *in the message body*, everywhere else (in most other places) the e_utf8_strftime() is used properly, for date/time-related values in date/time related columns/fields/places.

P.S.: I need short bug summary for commit messages; please do not fight even for this.

I cannot report a bug and then propose a patch that does not fix it properly !

Please have a look at the new version of the patchset.

The problem is not only in the mailer but also in the composer. Therefore, the new patchset reflects the vision that time and date formatting should be consistent.

Created attachment 326274 [details] [review]
Patch for the evolution data server code (v4)

Created attachment 326275 [details] [review]
Patch for the evolution code (v3)

The problem is wider than originally reported.

Therefore, the title is currently wrong.

I am not fighting, I am just being correct. You are figthing to change the bug title, since you changed it a second time...

If the new title I proposed is too long, then it can be changed to a shorter one, with a similar meaning.

I am getting there, it's just that testing is very slow, due to my machine being slow to re-compile and re-install and also due to the fact, that I am multi-tasking many things...

For some reason, at some point earlier on, I ended up with having the month in English and the name of the day in another language in the composer, so I want to be sure that it does not happen.

It takes some time, also because I don't know the code well, it's the first time I look at it ! But we'll get there soon, trust me !

The latest patchset, always overrides LC_TIME, that's probably wrong and it should be avoided. If LC_TIME is different from LC_MESSAGES, then the other parts of the application should usually honour the choice.

Also, I am now confining the issue to the evolution code, without changing the evolution-data-server code...

Now, it is somewhat more clear to me which parts of the application are using the strftime() functions.

What is not clear yet to me is why the evolution code uses both the e_*_strftime() code from eds and the g_data_strftime() code from Gnome GLib...

Thanks. It sounds to me that we are on the same line finally. Just to summarize my point of view:
- users can have different LC_MESSAGES and LC_TIME on purpose
- the Evolution (and basically any application in general) should honour this
  user decision and show data/time values according to LC_TIME setting and
  the messages (and in general UI) with the LC_MESSAGES
- the date/time values can be part of the UI, but as long as they are
  "separate" date/time values, they should honour LC_TIME
- I agree with you about the exception in the Reply credits line in
  the Evolution, which mixes date/time value with a raw text. Due to the value
  being a mixture, I agree with you that it should follow the LC_MESSAGES for
  the date/time value, rather than use the LC_TIME
- the Reply credits line is the only exception in the Evolution code
- if there happen to be a place in the UI where the date/time value is mixed,
  as you wrote with the calendar, then it should be fixed, but not by
  overriding the LC_TIME, but by using strftime consistently there

(In reply to guido from comment #25)
> What is not clear yet to me is why the evolution code uses both the
> e_*_strftime() code from eds and the g_data_strftime() code from Gnome
> GLib...

Partly historical reasons, partly because it's easier to call the GLib function when the structure in use is the GLib structure, rather than to convert the value and call the eds function.

Created attachment 326332 [details] [review]
Proposed patch (v4) for the evolution code

This version of the patch no longer requires the companion patch for the evolution-data-server code.

(In reply to Milan Crha from comment #26)
> Thanks. It sounds to me that we are on the same line finally. Just to
> summarize my point of view:

[...]

> - if there happen to be a place in the UI where the date/time value is mixed,
>   as you wrote with the calendar, then it should be fixed, but not by
>   overriding the LC_TIME, but by using strftime consistently there

I cannot reproduce the problem, therefore it was probably some sort of transitory problem introduced unintentionally in some way while producing and testing the patch.

A new version of the patch has been posted (v4): it tackles the mail composer only.

Can you test and/or commit the changes ? I tested it and it works fine. Time and date formatting elsewhere (e.g. calendar) follow LC_TIME settings as per original Open Group specifications.

Thanks for the update. This is basically what I was looking for. I made some minor changes, like the function rename, adding to the documentation and fixed the documentation comment for it too.

Created commit 833410a in evo master (3.21.1+)

Thanks. I like very much the new name you gave to the function !