After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 657955 - Add GnomeWallClock
Add GnomeWallClock
Status: RESOLVED FIXED
Product: gnome-desktop
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Desktop Maintainers
Desktop Maintainers
Depends on: 657958
Blocks: 657074 657959 658011
 
 
Reported: 2011-09-01 16:37 UTC by Colin Walters
Modified: 2011-09-02 19:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Actually delete gnome-tz-monitor.[ch] (8.22 KB, patch)
2011-09-01 16:37 UTC, Colin Walters
rejected Details | Review
Add GnomeWallClock (11.96 KB, patch)
2011-09-01 16:37 UTC, Colin Walters
committed Details | Review
GnomeWallClock: Add support for Linux timerfd() (14.02 KB, patch)
2011-09-01 16:37 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2011-09-01 16:37:02 UTC
See patches.
Comment 1 Colin Walters 2011-09-01 16:37:04 UTC
Created attachment 195404 [details] [review]
Actually delete gnome-tz-monitor.[ch]

The previous commit only removed them from Makefile.am.
Comment 2 Colin Walters 2011-09-01 16:37:07 UTC
Created attachment 195405 [details] [review]
Add GnomeWallClock

Since clocks are a bit tricky, it's useful to centralize this class.
See https://bugzilla.gnome.org/show_bug.cgi?id=655129
See https://bugzilla.gnome.org/show_bug.cgi?id=657074
Comment 3 Colin Walters 2011-09-01 16:37:09 UTC
Created attachment 195406 [details] [review]
GnomeWallClock: Add support for Linux timerfd()

See https://bugzilla.gnome.org/show_bug.cgi?id=655129 for more background
information on timerfd.
Comment 5 Vincent Untz 2011-09-02 01:04:27 UTC
Review of attachment 195405 [details] [review]:

General comments:
 - gtk-doc comments please :-)
 - as discussed on IRC, I'd go with a "changed" signal that would pass the string as argument to the handler, instead of the notify:: on the clock property. This looks nicer and easier to use (to me, at least), but if others disagree and prefer the notify:: way, I won't fight over it.

::: libgnome-desktop/gnome-wall-clock.c
@@ +70,3 @@
+	g_object_unref (tz);
+	
+	g_signal_connect (self->priv->tz_monitor, "changed", G_CALLBACK(on_tz_changed), self);

Missing space between G_CALLBACK and (. This happens also a few lines below, , and for G_OBJECT_CLASS (in dispose & finalize).

@@ +169,3 @@
+		g_source_remove (self->priv->clock_update_id);
+  
+	source = g_timeout_source_new_seconds (1);

(Obviously, we want the other patch to avoid this 1s timeout when we don't show seconds)

@@ +175,3 @@
+	g_source_unref (source);
+
+	format_type = g_settings_get_string (self->priv->desktop_settings, "clock-format");

Shouldn't this be an enum instead of a string? (I didn't look at the gsettings-desktop-schemas patch)

@@ +218,3 @@
+                  const char *key,
+                  gpointer user_data)
+{

It'd be nice to only update the clock if the key is relevant to us (ie, the keys used in update_clock).
Comment 6 Vincent Untz 2011-09-02 01:09:35 UTC
Review of attachment 195406 [details] [review]:

Assuming the gnome-datetime-source.c has been reviewed for glib (so skipping that), everything is good (except for the small detail below). Thanks!

::: configure.ac
@@ +165,3 @@
 
+AC_CACHE_CHECK(for timerfd_create(2) system call,
+    glib_cv_timerfd,AC_COMPILE_IFELSE([AC_LANG_PROGRAM([

Just rename glib_cv_timerfd to something not using the glib namespace.
Comment 7 Colin Walters 2011-09-02 01:13:01 UTC
(In reply to comment #5)
>
> (Obviously, we want the other patch to avoid this 1s timeout when we don't show
> seconds)

We need to wake up once a second to detect wall clock changes (via date&time panel or via resume from suspend).

> @@ +175,3 @@
> +    g_source_unref (source);
> +
> +    format_type = g_settings_get_string (self->priv->desktop_settings,
> "clock-format");
> 
> Shouldn't this be an enum instead of a string? (I didn't look at the
> gsettings-desktop-schemas patch)

Hmm...yes.  I guess gsettings is doing some enum->string translation if you request via _get_string().
Comment 8 Colin Walters 2011-09-02 19:41:10 UTC
Attachment 195405 [details] pushed as fb9ee91 - Add GnomeWallClock
Attachment 195406 [details] pushed as 5f65d7d - GnomeWallClock: Add support for Linux timerfd()