8 files changed, 169 insertions, 182 deletions

diff --git a/include/datetime/clock-mock.h b/include/datetime/clock-mock.h
index 19a859b..27926ff 100644
--- a/include/datetime/clock-mock.h
+++ b/include/datetime/clock-mock.h
@@ -42,11 +42,11 @@ public:
     DateTime localtime() const { return m_localtime; }
 
     void set_localtime(const DateTime& dt) {
-        const auto old_day = m_localtime.day_of_year();
+        const auto old = m_localtime;
         m_localtime = dt;
-        skewDetected();
-        const auto new_day = m_localtime.day_of_year();
-        if (old_day != new_day)
+        if (!DateTime::is_same_minute(old, m_localtime))
+            minuteChanged();
+        if (!DateTime::is_same_day(old, m_localtime))
             dateChanged();
     }
 
diff --git a/include/datetime/clock.h b/include/datetime/clock.h
index 9e87082..b3e3538 100644
--- a/include/datetime/clock.h
+++ b/include/datetime/clock.h
@@ -35,21 +35,25 @@ namespace datetime {
 
 /**
  * \brief A clock.
- *
- * Provides a signal to notify when clock skew is detected, such as
- * when the timezone changes or when the system resumes from sleep.
  */
 class Clock
 {
 public:
     virtual ~Clock();
     virtual DateTime localtime() const =0;
-    core::Signal<> skewDetected;
+
+    /** \brief A signal which fires when the clock's minute changes */
+    core::Signal<> minuteChanged;
+
+    /** \brief A signal which fires when the clock's date changes */
     core::Signal<> dateChanged;
 
 protected:
     Clock();
 
+    /** \brief Compares old and new times, emits minuteChanged() or dateChanged() signals if appropriate */
+    void maybe_emit (const DateTime& a, const DateTime& b);
+
 private:
     static void onSystemBusReady(GObject*, GAsyncResult*, gpointer);
     static void onPrepareForSleep(GDBusConnection*, const gchar*, const gchar*, const gchar*, const gchar*, GVariant*, gpointer);
@@ -70,12 +74,7 @@ private:
 class Timezones;
 
 /**
- * \brief A live clock that provides the actual system time.
- *
- * This subclass also adds another clock skew detection test:
- * it wakes up every skewTestIntervalSec seconds to see how
- * much time has passed since the last wakeup. If the answer
- * isn't what it expected, the skewDetected signal is triggered.
+ * \brief A live #Clock that provides the actual system time.
  */
 class LiveClock: public Clock
 {
@@ -83,7 +82,6 @@ public:
     LiveClock (const std::shared_ptr<Timezones>& zones);
     virtual ~LiveClock();
     virtual DateTime localtime() const;
-    core::Property<unsigned int> skewTestIntervalSec;
 
 private:
     class Impl;
diff --git a/include/datetime/date-time.h b/include/datetime/date-time.h
index 145e34e..c2429eb 100644
--- a/include/datetime/date-time.h
+++ b/include/datetime/date-time.h
@@ -49,13 +49,14 @@ public:
     std::string format(const std::string& fmt) const;
     int day_of_month() const;
     int64_t to_unix() const;
-    int day_of_year() const;
-    gint64 difference(const DateTime& that) const;
 
     bool operator<(const DateTime& that) const;
     bool operator!=(const DateTime& that) const;
     bool operator==(const DateTime& that) const;
 
+    static bool is_same_day(const DateTime& a, const DateTime& b);
+    static bool is_same_minute(const DateTime& a, const DateTime& b);
+
 private:
     std::shared_ptr<GDateTime> m_dt;
 };
diff --git a/src/clock-live.cpp b/src/clock-live.cpp
index 25623ae..69ebda7 100644
--- a/src/clock-live.cpp
+++ b/src/clock-live.cpp
@@ -24,6 +24,37 @@ namespace unity {
 namespace indicator {
 namespace datetime {
 
+/***
+****
+***/
+
+namespace
+{
+
+void clearTimer(guint& tag)
+{
+    if (tag)
+    {
+        g_source_remove(tag);
+        tag = 0;
+    }
+}
+
+guint calculate_milliseconds_until_next_minute(const DateTime& now)
+{
+    auto next = g_date_time_add_minutes(now.get(), 1);
+    auto start_of_next = g_date_time_add_seconds (next, -g_date_time_get_seconds(next));
+    const auto interval_usec = g_date_time_difference(start_of_next, now.get());
+    const guint interval_msec = (interval_usec + 999) / 1000;
+    g_date_time_unref(start_of_next);
+    g_date_time_unref(next);
+    g_assert (interval_msec <= 60000);
+    return interval_msec;
+}
+
+} // unnamed namespace
+
+
 class LiveClock::Impl
 {
 public:
@@ -38,13 +69,12 @@ public:
             setTimezone(m_timezones->timezone.get());
         }
 
-        m_owner.skewTestIntervalSec.changed().connect([this](unsigned int intervalSec) {setInterval(intervalSec);});
-        setInterval(m_owner.skewTestIntervalSec.get());
+        restart_minute_timer();
     }
 
     ~Impl()
     {
-        clearTimer();
+        clearTimer(m_timer);
 
         g_clear_pointer(&m_timezone, g_time_zone_unref);
     }
@@ -64,70 +94,55 @@ private:
     void setTimezone(const std::string& str)
     {
         g_clear_pointer(&m_timezone, g_time_zone_unref);
-        m_timezone= g_time_zone_new(str.c_str());
-        m_owner.skewDetected();
-    }
-
-private:
-
-    void clearTimer()
-    {
-        if (m_skew_timeout_id)
-        {
-            g_source_remove(m_skew_timeout_id);
-            m_skew_timeout_id = 0;
-        }
-
-        m_prev_datetime.reset();
+        m_timezone = g_time_zone_new(str.c_str());
+        m_owner.minuteChanged();
     }
 
-    void setInterval(unsigned int seconds)
-    {
-        clearTimer();
-
-        if (seconds > 0)
-        {
-            m_prev_datetime = localtime();
-            m_skew_timeout_id = g_timeout_add_seconds(seconds, onTimerPulse, this);
-        }
-    }
+    /***
+    ****
+    ***/
 
-    static gboolean onTimerPulse(gpointer gself)
+    void restart_minute_timer()
     {
-        static_cast<Impl*>(gself)->onTimerPulse();
-        return G_SOURCE_CONTINUE;
-    }
+        clearTimer(m_timer);
 
-    void onTimerPulse()
-    {
-        // check to see if too much time passed since the last check */
+        // maybe emit change signals
         const auto now = localtime();
-        const auto diff = now.difference (m_prev_datetime);
-        const GTimeSpan fuzz = 5;
-        const GTimeSpan max = (m_owner.skewTestIntervalSec.get() + fuzz) * G_USEC_PER_SEC;
-        if (abs(diff) > max)
-            m_owner.skewDetected();
-
-        // check to see if the day has changed
-        if (now.day_of_year() != m_prev_datetime.day_of_year())
+        if (!DateTime::is_same_minute(m_prev_datetime, now))
+            m_owner.minuteChanged();
+        if (!DateTime::is_same_day(m_prev_datetime, now))
             m_owner.dateChanged();
 
-        // update m_prev_datetime
+        // queue up a timer to fire at the next minute
         m_prev_datetime = now;
+        auto interval_msec = calculate_milliseconds_until_next_minute(now);
+        interval_msec += 50; // add a small margin to ensure the callback
+                             // fires /after/ next is reached
+        m_timer = g_timeout_add_full(G_PRIORITY_HIGH,
+                                     interval_msec,
+                                     on_minute_timer_reached,
+                                     this,
+                                     nullptr);
+    }
+
+    static gboolean on_minute_timer_reached(gpointer gself)
+    {
+        static_cast<LiveClock::Impl*>(gself)->restart_minute_timer();
+        return G_SOURCE_REMOVE;
     }
 
 protected:
 
     LiveClock& m_owner;
-    GTimeZone * m_timezone = nullptr;
+    GTimeZone* m_timezone = nullptr;
     std::shared_ptr<Timezones> m_timezones;
 
     DateTime m_prev_datetime;
-    unsigned int m_skew_timeout_id = 0;
+    unsigned int m_timer = 0;
 };
 
 LiveClock::LiveClock(const std::shared_ptr<Timezones>& tzd):
-  p(new Impl(*this, tzd))
+    p(new Impl(*this, tzd))
 {
 }
 
@@ -138,6 +153,10 @@ DateTime LiveClock::localtime() const
     return p->localtime();
 }
 
+/***
+****
+***/
+
 } // namespace datetime
 } // namespace indicator
 } // namespace unity
diff --git a/src/clock.cpp b/src/clock.cpp
index 7c3b8c7..d5293cc 100644
--- a/src/clock.cpp
+++ b/src/clock.cpp
@@ -81,7 +81,7 @@ Clock::onPrepareForSleep(GDBusConnection* /*connection*/,
                          GVariant*        /*parameters*/,
                          gpointer           gself)
 {
-    static_cast<Clock*>(gself)->skewDetected();
+    static_cast<Clock*>(gself)->minuteChanged();
 }
 
 /***
diff --git a/src/date-time.cpp b/src/date-time.cpp
index 3842ac0..40c638f 100644
--- a/src/date-time.cpp
+++ b/src/date-time.cpp
@@ -76,11 +76,6 @@ int64_t DateTime::to_unix() const
     return g_date_time_to_unix(get());
 }
 
-int DateTime::day_of_year() const
-{
-    return m_dt ? g_date_time_get_day_of_year(get()) : -1;
-}
-
 void DateTime::reset(GDateTime* in)
 {
     if (in)
@@ -95,39 +90,6 @@ void DateTime::reset(GDateTime* in)
     }
 }
 
-#if 0
-DateTime& DateTime::operator=(GDateTime* in)
-{
-    reset(in);
-    return *this;
-}
-
-DateTime& DateTime::operator=(const DateTime& in)
-{
-    m_dt = in.m_dt;
-    return *this;
-}
-#endif
-
-gint64 DateTime::difference(const DateTime& that) const
-{
-    const auto dt = get();
-    const auto tdt = that.get();
-
-    gint64 ret;
-
-    if (dt && tdt)
-        ret = g_date_time_difference(dt, tdt);
-    else if (dt)
-        ret = to_unix();
-    else if (tdt)
-        ret = that.to_unix();
-    else
-        ret = 0;
-
-    return ret;
-}
-
 bool DateTime::operator<(const DateTime& that) const
 {
     return g_date_time_compare(get(), that.get()) < 0;
@@ -141,13 +103,36 @@ bool DateTime::operator!=(const DateTime& that) const
 
 bool DateTime::operator==(const DateTime& that) const
 {
-    GDateTime * dt = get();
-    GDateTime * tdt = that.get();
+    auto dt = get();
+    auto tdt = that.get();
     if (!dt && !tdt) return true;
     if (!dt || !tdt) return false;
     return g_date_time_compare(get(), that.get()) == 0;
 }
 
+bool DateTime::is_same_day(const DateTime& a, const DateTime& b)
+{
+    // it's meaningless to compare uninitialized dates
+    if (!a.m_dt || !b.m_dt)
+        return false;
+
+    const auto adt = a.get();
+    const auto bdt = b.get();
+    return (g_date_time_get_year(adt) == g_date_time_get_year(bdt))
+        && (g_date_time_get_day_of_year(adt) == g_date_time_get_day_of_year(bdt));
+}
+
+bool DateTime::is_same_minute(const DateTime& a, const DateTime& b)
+{
+    if (!is_same_day(a,b))
+        return false;
+
+    const auto adt = a.get();
+    const auto bdt = b.get();
+    return (g_date_time_get_hour(adt) == g_date_time_get_hour(bdt))
+        && (g_date_time_get_minute(adt) == g_date_time_get_minute(bdt));
+}
+
 /***
 ****
 ***/
diff --git a/src/formatter.cpp b/src/formatter.cpp
index 88a64df..a271ba0 100644
--- a/src/formatter.cpp
+++ b/src/formatter.cpp
@@ -28,9 +28,18 @@
 #include <langinfo.h> // nl_langinfo()
 #include <string.h> // strstr()
 
+namespace unity {
+namespace indicator {
+namespace datetime {
+
+/***
+****
+***/
+
 namespace
 {
-void clearTimer(guint& tag)
+
+void clear_timer(guint& tag)
 {
     if (tag)
     {
@@ -39,35 +48,12 @@ void clearTimer(guint& tag)
     }
 }
 
-guint calculate_milliseconds_until_next_minute(GDateTime * now)
-{
-    GDateTime * next;
-    GDateTime * start_of_next;
-    GTimeSpan interval_usec;
-    guint interval_msec;
-
-    next = g_date_time_add_minutes(now, 1);
-    start_of_next = g_date_time_new_local(g_date_time_get_year(next),
-                                          g_date_time_get_month(next),
-                                          g_date_time_get_day_of_month(next),
-                                          g_date_time_get_hour(next),
-                                          g_date_time_get_minute(next),
-                                          0.1);
-
-    interval_usec = g_date_time_difference(start_of_next, now);
-    interval_msec = (interval_usec + 999) / 1000;
-
-    g_date_time_unref(start_of_next);
-    g_date_time_unref(next);
-    return interval_msec;
-}
-
-gint calculate_milliseconds_until_next_second(GDateTime * now)
+gint calculate_milliseconds_until_next_second(const DateTime& now)
 {
     gint interval_usec;
     guint interval_msec;
 
-    interval_usec = G_USEC_PER_SEC - g_date_time_get_microsecond(now);
+    interval_usec = G_USEC_PER_SEC - g_date_time_get_microsecond(now.get());
     interval_msec = (interval_usec + 999) / 1000;
     return interval_msec;
 }
@@ -127,11 +113,6 @@ guint calculate_seconds_until_next_fifteen_minutes(GDateTime * now)
 } // unnamed namespace
 
 
-
-namespace unity {
-namespace indicator {
-namespace datetime {
-
 class Formatter::Impl
 {
 public:
@@ -140,57 +121,64 @@ public:
         m_owner(owner),
         m_clock(clock)
     {
-        m_owner->headerFormat.changed().connect([this](const std::string& /*fmt*/){updateHeader();});
-        m_clock->skewDetected.connect([this](){updateHeader();});
-        updateHeader();
+        m_owner->headerFormat.changed().connect([this](const std::string& /*fmt*/){update_header();});
+        m_clock->minuteChanged.connect([this](){update_header();});
+        update_header();
 
         restartRelativeTimer();
     }
 
     ~Impl()
     {
-        clearTimer(m_header_timer);
+        clear_timer(m_header_seconds_timer);
+        clear_timer(m_relative_timer);
     }
 
 private:
 
-    void updateHeader()
+    static bool format_shows_seconds(const std::string& fmt)
     {
+        return (fmt.find("%s") != std::string::npos)
+            || (fmt.find("%S") != std::string::npos)
+            || (fmt.find("%T") != std::string::npos)
+            || (fmt.find("%X") != std::string::npos)
+            || (fmt.find("%c") != std::string::npos);
+    }
+
+    void update_header()
+    {
+        // update the header property
         const auto fmt = m_owner->headerFormat.get();
         const auto str = m_clock->localtime().format(fmt);
         m_owner->header.set(str);
 
-        restartHeaderTimer();
+        // if the header needs to show seconds, set a timer.
+        if (format_shows_seconds(fmt))
+            start_header_timer();
+        else
+            clear_timer(m_header_seconds_timer);
     }
 
-    void restartHeaderTimer()
+    // we've got a header format that shows seconds,
+    // so we need to update it every second
+    void start_header_timer()
     {
-        clearTimer(m_header_timer);
+        clear_timer(m_header_seconds_timer);
 
-        const auto fmt = m_owner->headerFormat.get();
-        const bool header_shows_seconds = (fmt.find("%s") != std::string::npos)
-                                       || (fmt.find("%S") != std::string::npos)
-                                       || (fmt.find("%T") != std::string::npos)
-                                       || (fmt.find("%X") != std::string::npos)
-                                       || (fmt.find("%c") != std::string::npos);
-
-        guint interval_msec;
         const auto now = m_clock->localtime();
-        auto str = now.format("%F %T");
-        if (header_shows_seconds)
-            interval_msec = calculate_milliseconds_until_next_second(now.get());
-        else
-            interval_msec = calculate_milliseconds_until_next_minute(now.get());
-
+        auto interval_msec = calculate_milliseconds_until_next_second(now);
         interval_msec += 50; // add a small margin to ensure the callback
                              // fires /after/ next is reached
-
-        m_header_timer = g_timeout_add_full(G_PRIORITY_HIGH, interval_msec, onHeaderTimer, this, nullptr);
+        m_header_seconds_timer = g_timeout_add_full(G_PRIORITY_HIGH,
+                                                    interval_msec,
+                                                    on_header_timer,
+                                                    this,
+                                                    nullptr);
     }
 
-    static gboolean onHeaderTimer(gpointer gself)
+    static gboolean on_header_timer(gpointer gself)
     {
-        static_cast<Formatter::Impl*>(gself)->updateHeader();
+        static_cast<Formatter::Impl*>(gself)->update_header();
         return G_SOURCE_REMOVE;
     }
 
@@ -198,7 +186,7 @@ private:
 
     void restartRelativeTimer()
     {
-        clearTimer(m_relative_timer);
+        clear_timer(m_relative_timer);
 
         const auto now = m_clock->localtime();
         const auto seconds = calculate_seconds_until_next_fifteen_minutes(now.get());
@@ -215,7 +203,7 @@ private:
 
 private:
     Formatter* const m_owner;
-    guint m_header_timer = 0;
+    guint m_header_seconds_timer = 0;
     guint m_relative_timer = 0;
 
 public:
diff --git a/tests/test-clock.cpp b/tests/test-clock.cpp
index 142ccad..4271374 100644
--- a/tests/test-clock.cpp
+++ b/tests/test-clock.cpp
@@ -46,28 +46,24 @@ class ClockFixture: public TestDBusFixture
     }
 };
 
-/**
- * Confirm that normal time passing doesn't trigger a skew event.
- * that idling changing the clock's time triggers a skew event
- */
-TEST_F(ClockFixture, IdleDoesNotTriggerSkew)
+TEST_F(ClockFixture, MinuteChangedSignalShouldTriggerOncePerMinute)
 {
+    // start up a live clock
     std::shared_ptr<Timezones> zones(new Timezones);
     zones->timezone.set("America/New_York");
     LiveClock clock(zones);
     wait_msec(500); // wait for the bus to set up
 
-    bool skewed = false;
-    clock.skewDetected.connect([&skewed](){
-                    skewed = true;
-                    g_warn_if_reached();
-                    return G_SOURCE_REMOVE;
-                });
-
-    const unsigned int intervalSec = 3;
-    clock.skewTestIntervalSec.set(intervalSec);
-    wait_msec(intervalSec * 2.5 * 1000);
-    EXPECT_FALSE(skewed);
+    // count how many times clock.minuteChanged() is emitted over the next minute
+    const DateTime now = clock.localtime();
+    const auto gnow = now.get();
+    auto gthen = g_date_time_add_minutes(gnow, 1);
+    int count = 0;
+    clock.minuteChanged.connect([&count](){count++;});
+    const auto msec = g_date_time_difference(gthen,gnow) / 1000;
+    wait_msec(msec);
+    EXPECT_EQ(1, count);
+    g_date_time_unref(gthen);
 }
 
 /***
@@ -99,7 +95,7 @@ TEST_F(ClockFixture, TimezoneChangeTriggersSkew)
     g_time_zone_unref(tz_nyc);
 
     /// change the timezones!
-    clock.skewDetected.connect([this](){
+    clock.minuteChanged.connect([this](){
                    g_main_loop_quit(loop);
                });
     g_idle_add([](gpointer gs){
@@ -128,7 +124,7 @@ TEST_F(ClockFixture, SleepTriggersSkew)
     wait_msec(500); // wait for the bus to set up
 
     bool skewed = false;
-    clock.skewDetected.connect([&skewed, this](){
+    clock.minuteChanged.connect([&skewed, this](){
                     skewed = true;
                     g_main_loop_quit(loop);
                     return G_SOURCE_REMOVE;