Bug 2227002

Created attachment 1980250 [details]
initial patch proposal

That was painful.

I've done some initial testing of the patch on my end by building a patched RPM and it seems to work fine. Setting the time format to 24h -> 12h -> 24h with intermittent reminders produces appropriate timestamps depending on the setting.

>(In reply to Melker Narikka from comment #0)
> Calendar notifications always use the 12-hour time format. There's an
> additional bug whereby the am/pm suffix is stripped off somewhere along the
> way, making for somewhat tricky debugging.

Scratch the latter part, I guess. Instead of using glib's
g_date_time_format() for time formatting, it seems like
evolution-data-server just calls libc's strftime() instead. My locale
seems to miss said suffix when it comes to libc whereas glib has an
appropriate string stashed away in its gettext files.

Thanks for a bug report. You mentioned upstream and I agree with you, this is supposed to be dealt with there, because it's not Fedora specific. Nonetheless, for this particular project, I can comment here as well, thus we save some resources.

Let alone some coding style glitches and not helpful long code comment, there is one major problem. You read the information from the `org.gnome.desktop.interface`. What if the user does not run GNOME at all? What if the GNOME is not installed on the machine, thus the scheme does not exist? That usually leads to application aborts by the glib2 itself due to non-existent GSettings schema, which is something you do not want to do in a library which can be used anywhere.

Evolution has its own setting for the 24h format and it also provides a plugin/module loaded by the evolution-alarm-notify, which formats the time accordingly. It doe snot work when Evolution is not installed, of course.

Maybe the e_time_format_date_and_time() is not the best choice here, it has its own format specifiers too, as the g_date_time_format() has. On the other hand, the "%x" strftime format is also not the best one, is it?

(In reply to Milan Crha from comment #4)
> Thanks for a bug report. You mentioned upstream and I agree with you, this
> is supposed to be dealt with there, because it's not Fedora specific.
> Nonetheless, for this particular project, I can comment here as well, thus
> we save some resources.
> 
> Let alone some coding style glitches and not helpful long code comment,

The latter was a bit of an oversight left in the code in the digging phase.

> there is one major problem. You read the information from the
> `org.gnome.desktop.interface`. What if the user does not run GNOME at all?
> What if the GNOME is not installed on the machine, thus the scheme does not
> exist? That usually leads to application aborts by the glib2 itself due to
> non-existent GSettings schema, which is something you do not want to do in a
> library which can be used anywhere.

Good catch, you're very much correct. I've made an adjustment where I first probe for its existence and set the property to NULL instead of crashing in case the schema is not found. This should make the NULL guard elsewhere something other than a NOP. I'll post the adjusted patch in the next message. To be clean it might need an extra *_unref() as well.

> Evolution has its own setting for the 24h format and it also provides a
> plugin/module loaded by the evolution-alarm-notify, which formats the time
> accordingly. It doe snot work when Evolution is not installed, of course.
> 
> Maybe the e_time_format_date_and_time() is not the best choice here, it has
> its own format specifiers too, as the g_date_time_format() has. On the other
> hand, the "%x" strftime format is also not the best one, is it?

There are already a number of preferred formats enumerated in the translation files for the formats e_time_format_date_and_time() looks up. Going with just "%x" or combining "%x" and "%X" seems a little unfriendly too, and "%c" is very verbose. I think the formats being looked up there are probably the best bet here. Since e_time_format_date_and_time() called by the default time formatter via e_reminder_watcher_format_time_impl() if no one's handling the time format signal, it should result in some decent looking output.

I did some quick and dirty hacking on e_time_format_date_and_time() to make it use g_date_time_format() instead, still using the same format strings. That makes it output strings that are quite consistent with whatever GNOME is doing and the am/pm suffix works as well. I'll split it off into another patch and I think I'll have to kludge it into e_time_format_time() as well.

Created attachment 1980296 [details]
eds-fix-clock-format-v2.patch

Some style issues probably still persist, as is to be expected.

> I did some quick and dirty hacking on e_time_format_date_and_time() to make it use g_date_time_format() instead, still using the same format strings. That makes it output strings that are quite consistent with whatever GNOME is doing and the am/pm suffix works as well. I'll split it off into another patch and I think I'll have to kludge it into e_time_format_time() as well.

I won't do that. The formats are made two way, the code should be able to parse what it created. I'm afraid it's (close to) impossible when using GDateTime.

(In reply to Milan Crha from comment #7)
> I won't do that. The formats are made two way, the code should be able to
> parse what it created. I'm afraid it's (close to) impossible when using
> GDateTime.

Hmm. Sounds reasonable, I'll abandon that part then.

Apart of some memory leaks (g_settings_schema_source_lookup(), priv::desktop_settings, which I spotted early) and preferable "use_24h_format" instead of "use_24", and quite a few coding style issues [1] (I would comment in the patch, but I do not see how to do it in bugzilla; this should really be done in the gitlab.gnome.org instead, as a merge request), it's still only about "we support GNOME and do not know about anything else".

You said the GDateTime works as expected with the same format, even I'm not sure if you mean "passing non-24 hour format specifier will turn it into 24 hour format when set so in the settings". I do not know how glib does that and how many desktop environments their code support (if they read the desktop settings at all), but what if the reminder watcher will not use e_time_format_date_and_time(), but the GDateTime instead? It can still use the format localized by the translators, that's the easiest thing.

[1] https://wiki.gnome.org/Apps/Evolution/Patches

(In reply to Milan Crha from comment #9)
> Apart of some memory leaks (g_settings_schema_source_lookup(),
> priv::desktop_settings, which I spotted early) and preferable
> "use_24h_format" instead of "use_24", and quite a few coding style issues
> [1] (I would comment in the patch, but I do not see how to do it in
> bugzilla; this should really be done in the gitlab.gnome.org instead, as a
> merge request), it's still only about "we support GNOME and do not know
> about anything else".

Thanks. I think I'll try and get this posted on the GNOME Gitlab tonight. I do wonder which creative ways my mailer might butcher it in.

> You said the GDateTime works as expected with the same format, even I'm not
> sure if you mean "passing non-24 hour format specifier will turn it into 24
> hour format when set so in the settings". I do not know how glib does that
> and how many desktop environments their code support (if they read the
> desktop settings at all), but what if the reminder watcher will not use
> e_time_format_date_and_time(), but the GDateTime instead? It can still use
> the format localized by the translators, that's the easiest thing.

I just meant that "%p" doesn't have a localization when using strftime() from libc: it returns an empty string. GDateTime/glib does have a localization for "%p" for the same locale. A little something like so:

    strftime(buf, sizeof buf, "%p", date_tm) -> ""
    g_date_time_format(date_gd, "%p") -> "IP."

    /* run at 18:57 */
    strftime(buf, sizeof buf, "%I.%M %P") -> "06.57 "
    g_date_time_format(date_gd, "%I.%M %P") -> "06.57 ip."

I hope that makes sense.

The small amount of digging I did in the glib codebase seems to suggest that they do not have any magic way of ascertaining whether to pick 24 or 12 hour formats. It's on the shoulders of the caller to pick the right format string. After grepping through some gettext files I guess one ugly but semi-portable way to check for this would be to look at the preferred format for expressing both date and time and check for "%p"/"%P". That only seems to catch languages spoken on the Indian sub-continent though, which probably isn't ideal. Another method would be to check if strftime() returns an empty string for "%p", which I think was done by gnome-calendar. Seems very kludgy in any case.

But you're correct in that it would be trivial to just use GDateTime instead of e_time_format_date_and_time(), if a little copy-pastey with all the four cases.

> [1] https://wiki.gnome.org/Apps/Evolution/Patches

Thanks for the reference.

I guess gnome-calendar has it taken from the Evolution, which checks whether the current locale supports 12 hour format with the %p.

This can be added into the reminder watcher and be used for the use_24_hour parameter.

> But you're correct in that it would be trivial to just use GDateTime instead of e_time_format_date_and_time(), if a little copy-pastey with all the four cases.

Copy/paste, aka code duplication, is something I'd like to avoid. I understood from the comment #3 that the GDateTime produces correct/expected output out of the box. If not, then forget of the move to the GDateTime, there's no gain in it.

A bit wild idea: as you target specifically GNOME and the GNOME Shell already depends on the evolution-data-server, what about providing a module for the alarm-notify in the GNOME Shell code, which will do the formatting according to the GNOME Settings? That would be probably the cleanest way to achieve the right result.

The code in question is as simple as:
https://gitlab.gnome.org/GNOME/evolution/-/tree/master/src/modules/alarm-notify

I'm closing this in favour of the [1]. Let's continue there. My last comment above still applies, thou.

[1] https://gitlab.gnome.org/GNOME/evolution-data-server/-/merge_requests/128