Merge lp:~charlesk/indicator-datetime/lp-1302004-fix-event-list-culling-and-sorting into lp:indicator-datetime/15.10

[Charles Kerr]

Bring the code up-to-date with the spec wrt which calendar events are shown in the menu, and in what order.

Bring the code up-to-date with the spec wrt which calendar events are shown in the menu, and in what order. Add unit tests to cover event selection and display order.

https://wiki.ubuntu.com/TimeAndDate#Calendar_events

[Renato Araujo Oliveira Filho (renatofilho)]

code looks good.

review: Approve

[lp:~charlesk/indicator-datetime/lp-1302004-fix-event-list-culling-and-sorting]

437. By Charles Kerr on 2016-04-19

sync with trunk

438. By Charles Kerr on 2016-04-20

fix r437 merge error

=== modified file 'include/datetime/date-time.h'
--- include/datetime/date-time.h	2015-04-06 18:16:24 +0000
+++ include/datetime/date-time.h	2016-04-20 16:31:00 +0000
@@ -68,7 +68,9 @@
     int64_t to_unix() const;
 
     bool operator<(const DateTime& that) const;
+    bool operator>(const DateTime& that) const;
     bool operator<=(const DateTime& that) const;
+    bool operator>=(const DateTime& that) const;
     bool operator!=(const DateTime& that) const;
     bool operator==(const DateTime& that) const;
     int64_t operator- (const DateTime& that) const;

=== modified file 'include/datetime/menu.h'
--- include/datetime/menu.h	2014-01-30 19:44:12 +0000
+++ include/datetime/menu.h	2016-04-20 16:31:00 +0000
@@ -21,6 +21,7 @@
 #define INDICATOR_DATETIME_MENU_H
 
 #include <datetime/actions.h>
+#include <datetime/appointment.h>
 #include <datetime/state.h>
 
 #include <memory> // std::shared_ptr
@@ -47,6 +48,11 @@
     Profile profile() const;
     GMenuModel* menu_model();
 
+    static std::vector<Appointment> get_display_appointments(
+        const std::vector<Appointment>&,
+        const DateTime& start,
+        unsigned int max_items=5);
+
 protected:
     Menu (Profile profile_in, const std::string& name_in);
     virtual ~Menu() =default;

=== modified file 'src/date-time.cpp'
--- src/date-time.cpp	2015-04-06 18:16:24 +0000
+++ src/date-time.cpp	2016-04-20 16:31:00 +0000
@@ -233,11 +233,21 @@
     return g_date_time_compare(get(), that.get()) < 0;
 }
 
+bool DateTime::operator>(const DateTime& that) const
+{
+    return g_date_time_compare(get(), that.get()) > 0;
+}
+
 bool DateTime::operator<=(const DateTime& that) const
 {
     return g_date_time_compare(get(), that.get()) <= 0;
 }
 
+bool DateTime::operator>=(const DateTime& that) const
+{
+    return g_date_time_compare(get(), that.get()) >= 0;
+}
+
 bool DateTime::operator!=(const DateTime& that) const
 {
     // return true if this isn't set, or if it's not equal

=== modified file 'src/menu.cpp'
--- src/menu.cpp	2016-03-23 20:28:22 +0000
+++ src/menu.cpp	2016-04-20 16:31:00 +0000
@@ -25,6 +25,8 @@
 #include <glib/gi18n.h>
 #include <gio/gio.h>
 
+#include <algorithm>
+#include <iterator>
 #include <vector>
 
 namespace unity {
@@ -56,12 +58,90 @@
     return G_MENU_MODEL(m_menu);
 }
 
+/**
+ * To avoid a giant menu on the PC, and to avoid pushing lower menu items
+ * off-screen on the phone, the menu should the
+ * next five calendar events, if any.
+ *
+ * The list might include multiple occurrences of the same event (bug 1515821).
+ */
+std::vector<Appointment>
+Menu::get_display_appointments(const std::vector<Appointment>& appointments_in,
+                               const DateTime& now,
+                               unsigned int max_items)
+{
+    std::vector<Appointment> appointments;
+    std::copy_if(appointments_in.begin(),
+                 appointments_in.end(),
+                 std::back_inserter(appointments),
+                 [now](const Appointment& a){return a.end >= now;});
+
+    if (appointments.size() > max_items)
+    {
+        const auto next_minute = now.add_full(0,0,0,0,1,-now.seconds());
+        const auto start_of_day = now.start_of_day();
+        const auto end_of_day = now.end_of_day();
+
+        /*
+         * If there are more than five, the events shown should be, in order of priority:
+         * 1. any events that start or end (bug 1329048) after the current minute today;
+         * 2. any full-day events that span all of today (bug 1302004);
+         * 3. any events that start or end tomorrow;
+         * 4. any events that start or end the day after tomorrow; and so on.
+         */
+        auto compare = [next_minute, start_of_day, end_of_day](
+            const Appointment& a,
+            const Appointment& b)
+        {
+            const bool a_later_today = (a.begin >= next_minute) || (a.end <= end_of_day);
+            const bool b_later_today = (b.begin >= next_minute) || (b.end <= end_of_day);
+            if (a_later_today != b_later_today)
+                return a_later_today;
+
+            const bool a_full_day_today = (a.begin <= start_of_day) && (end_of_day <= a.end);
+            const bool b_full_day_today = (b.begin <= start_of_day) && (end_of_day <= b.end);
+            if (a_full_day_today != b_full_day_today)
+                return a_full_day_today;
+
+            const bool a_after_today = (a.begin > end_of_day) || (a.end > end_of_day);
+            const bool b_after_today = (a.begin > end_of_day) || (a.end > end_of_day);
+            if (a_after_today != b_after_today)
+                return a_after_today;
+            if (a.begin != b.begin)
+                return a.begin < b.begin;
+            if (b.end != b.end)
+                return a.end < b.end;
+
+            return false;
+        };
+        std::sort(appointments.begin(), appointments.end(), compare);
+        appointments.resize(max_items);
+    }
+
+    /*
+     * However, the display order should be the reverse: full-day events
+     * first (since they start first), part-day events afterward in
+     * chronological order. If multiple events have exactly the same start+end
+     * time, they should be sorted alphabetically.
+     */
+    auto compare = [](const Appointment& a, const Appointment& b)
+    {
+        if (a.begin != b.begin)
+            return a.begin < b.begin;
+
+        if (a.end != b.end)
+            return a.end < b.end;
+
+        return a.summary < b.summary;
+    };
+    std::sort(appointments.begin(), appointments.end(), compare);
+    return appointments;
+}
 
 /****
 *****
 ****/
 
-
 #define ALARM_ICON_NAME "alarm-clock"
 #define CALENDAR_ICON_NAME "calendar"
 
@@ -141,25 +221,19 @@
 
     void update_upcoming()
     {
-        // The usual case is on desktop (and /only/ case on phone)
-        // is that we're looking at the current date and want to see
-        // "the next five calendar events, if any."
-        //
-        // However on the Desktop when the user clicks onto a different
-        // calendar date, show the next five calendar events starting
-        // from the beginning of that clicked day.
-        DateTime begin;
+        // The usual case is to show events germane to the current time.
+        // However when the user clicks onto a different calendar date,
+        // we pick events starting from the beginning of that clicked day.
         const auto now = m_state->clock->localtime();
         const auto calendar_day = m_state->calendar_month->month().get();
-        if ((profile() == Desktop) && !DateTime::is_same_day(now, calendar_day))
-            begin = calendar_day.start_of_day();
-        else
-            begin = now.start_of_minute();
+        const auto begin = DateTime::is_same_day(now, calendar_day)
+            ? now.start_of_minute()
+            : calendar_day.start_of_day();
 
-        std::vector<Appointment> upcoming;
-        for(const auto& a : m_state->calendar_upcoming->appointments().get())
-            if (begin <= a.begin)
-                upcoming.push_back(a);
+        auto upcoming = get_display_appointments(
+            m_state->calendar_upcoming->appointments().get(),
+            begin
+        );
 
         if (m_upcoming != upcoming)
         {

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2016-03-22 18:51:50 +0000
+++ tests/CMakeLists.txt	2016-04-20 16:31:00 +0000
@@ -57,6 +57,7 @@
 add_test_by_name(test-formatter)
 add_test_by_name(test-live-actions)
 add_test_by_name(test-locations)
+add_test_by_name(test-menu-appointments)
 add_test_by_name(test-menus)
 add_test_by_name(test-planner)
 add_test_by_name(test-settings)

=== modified file 'tests/print-to.h'
--- tests/print-to.h	2015-06-18 04:52:52 +0000
+++ tests/print-to.h	2016-04-20 16:31:00 +0000
@@ -21,6 +21,7 @@
 #define INDICATOR_DATETIME_TESTS_PRINT_TO
 
 #include <algorithm>
+#include <vector>
 
 #include <datetime/appointment.h>
 
@@ -71,6 +72,15 @@
     *os << '}';
 }
 
+void
+PrintTo(const std::vector<Appointment>& appointments, std::ostream* os)
+{
+    *os << '{';
+    for (const auto& appointment : appointments)
+        PrintTo(appointment, os);
+    *os << '}';
+}
+
 } // namespace datetime
 } // namespace indicator
 } // namespace unity

=== added file 'tests/test-menu-appointments.cpp'
--- tests/test-menu-appointments.cpp	1970-01-01 00:00:00 +0000
+++ tests/test-menu-appointments.cpp	2016-04-20 16:31:00 +0000
@@ -0,0 +1,263 @@
+/*
+ * Copyright 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *   Charles Kerr <[e-mail]>
+ */
+
+#include "glib-fixture.h"
+#include "print-to.h"
+
+#include <datetime/appointment.h>
+#include <datetime/menu.h>
+
+#include <vector>
+
+using MenuAppointmentFixture = GlibFixture;
+
+using namespace unity::indicator::datetime;
+
+namespace
+{
+    Appointment create_appointment(
+        const Appointment::Type& type,
+        const std::string& uid,
+        const std::string& summary,
+        const DateTime& begin,
+        const DateTime& end)
+    {
+        Appointment a;
+        a.type = type;
+        a.uid = uid;
+        a.summary = summary;
+        a.begin = begin;
+        a.end = end;
+        return a;
+    }
+}
+
+TEST_F(MenuAppointmentFixture, DisplayEvents)
+{
+    const auto airport = create_appointment(
+        Appointment::UBUNTU_ALARM,
+        "uid-airport",
+        "Pick Aunt Mabel up at the airport",
+        DateTime::Local(2016,12,24,10,0,0),
+        DateTime::Local(2016,12,24,10,0,0)
+    );
+
+    const auto christmas_eve_candle_service = create_appointment(
+        Appointment::EVENT,
+        "uid-christmas-eve-candle-service",
+        "Christmas Eve Candle Service",
+        DateTime::Local(2016,12,24,22,0,0),
+        DateTime::Local(2016,12,24,23,0,0)
+    );
+
+    const auto christmas = create_appointment(
+        Appointment::EVENT,
+        "uid-christmas",
+        "Christmas",
+        DateTime::Local(2016,12,25,0,0,0),
+        DateTime::Local(2016,12,26,0,0,0)
+    );
+
+    const auto santa = create_appointment(
+        Appointment::UBUNTU_ALARM,
+        "uid-santa",
+        "Time to set out cookies and milk for Santa",
+        DateTime::Local(2016,12,25,1,0,0),
+        DateTime::Local(2016,12,25,1,0,0)
+    );
+
+    const auto bike = create_appointment(
+        Appointment::UBUNTU_ALARM,
+        "uid-bike",
+        "Remember to put out the bike, it's in the garage",
+        DateTime::Local(2016,12,25,1,0,0),
+        DateTime::Local(2016,12,25,1,0,0)
+    );
+
+
+    const auto christmas_lunch = create_appointment(
+        Appointment::EVENT,
+        "uid-christmas-lunch",
+        "Christmas Lunch at Grandma's",
+        DateTime::Local(2016,12,25,12,0,0),
+        DateTime::Local(2016,12,25,14,0,0)
+    );
+
+    const auto boxing_day = create_appointment(
+        Appointment::EVENT,
+        "uid-boxing-day",
+        "Boxing Day",
+        DateTime::Local(2016,12,26,0,0,0),
+        DateTime::Local(2016,12,27,0,0,0)
+    );
+
+    const auto new_years_eve = create_appointment(
+        Appointment::EVENT,
+        "uid-new-years-eve",
+        "New Years' Eve",
+        DateTime::Local(2016,12,31,0,0,0),
+        DateTime::Local(2017,1,1,0,0,0)
+    );
+
+    const auto nye_party = create_appointment(
+        Appointment::EVENT,
+        "uid-new-years-party",
+        "New Year Party at Ted's",
+        DateTime::Local(2016,12,31,19,0,0),
+        DateTime::Local(2017, 1, 1, 2,0,0)
+    );
+
+    const auto new_years_day = create_appointment(
+        Appointment::EVENT,
+        "uid-new-years-day",
+        "New Years' Day",
+        DateTime::Local(2017,1,1,0,0,0),
+        DateTime::Local(2017,1,2,0,0,0)
+    );
+
+    const auto weekday_wakeup_alarm = create_appointment(
+        Appointment::UBUNTU_ALARM,
+        "wakeup-alarm",
+        "Wake Up",
+        DateTime::Local(2017,1,3,6,0,0),
+        DateTime::Local(2017,1,3,6,0,0)
+    );
+
+    const auto dentist_appointment = create_appointment(
+        Appointment::EVENT,
+        "dentist",
+        "Dentist",
+        DateTime::Local(2017,1,5,14,0,0),
+        DateTime::Local(2017,1,5,15,0,0)
+    );
+
+    const auto marcus_birthday = create_appointment(
+        Appointment::EVENT,
+        "uid-mlk",
+        "Marcus' Birthday",
+        DateTime::Local(2017,1,8,0,0,0),
+        DateTime::Local(2017,1,9,0,0,0)
+    );
+
+    const auto mlk_day = create_appointment(
+        Appointment::EVENT,
+        "uid-mlk",
+        "Martin Luther King Day",
+        DateTime::Local(2017,1,16,0,0,0),
+        DateTime::Local(2017,1,17,0,0,0)
+    );
+
+    const auto rodney_party = create_appointment(
+        Appointment::EVENT,
+        "uid-rodney",
+        "Rodney's Party",
+        DateTime::Local(2017,1,30,19,0,0),
+        DateTime::Local(2017,1,30,23,0,0)
+    );
+
+    const auto pub_with_pawel = create_appointment(
+        Appointment::UBUNTU_ALARM,
+        "uid-pawel",
+        "Meet Pawel at the Pub",
+        DateTime::Local(2017,2,4,19,0,0),
+        DateTime::Local(2017,2,4,19,0,0)
+    );
+
+    const struct
+    {
+        const char* const description;
+        DateTime start;
+        std::vector<Appointment> appointments;
+        std::vector<Appointment> expected_display_appointments;
+        int max_items;
+    }
+    tests[] =
+    {
+        {
+            "test presentation order: full-day events come before part-day events",
+            DateTime::Local(2016,12,25,6,0,0),
+            std::vector<Appointment>({christmas, christmas_lunch, boxing_day, new_years_eve}),
+            std::vector<Appointment>({christmas, christmas_lunch, boxing_day, new_years_eve}),
+            5
+        },
+        {
+            "test presentation order: part-day events in chronological order",
+            DateTime::Local(2016,12,24,0,0,0),
+            std::vector<Appointment>({airport, christmas_eve_candle_service, christmas, santa, christmas_lunch}),
+            std::vector<Appointment>({airport, christmas_eve_candle_service, christmas, santa, christmas_lunch}),
+            5
+        },
+        {
+            "test presentation order: multiple events with the same start+end sorted alphabetically",
+            DateTime::Local(2016,12,25,0,0,0),
+            std::vector<Appointment>({christmas, bike, santa, christmas_lunch}),
+            std::vector<Appointment>({christmas, bike, santa, christmas_lunch}),
+            5
+        },
+        {
+            "test culling priority: today's part-day events outrank today's full-day events",
+            DateTime::Local(2016,12,25,1,0,0),
+            std::vector<Appointment>({christmas, santa, christmas_lunch}),
+            std::vector<Appointment>({santa, christmas_lunch}),
+            2
+        },
+        {
+            "test culling priority: events later today outrank both tomorrow's full-day and part-day events",
+            DateTime::Local(2016,12,24,0,0,0),
+            std::vector<Appointment>({christmas_eve_candle_service, christmas, santa}),
+            std::vector<Appointment>({christmas_eve_candle_service}),
+            1
+        },
+        {
+            "test edge cases: confirm <max events works ok",
+            DateTime::Local(2016,12,24,0,0,0),
+            std::vector<Appointment>({christmas, christmas_lunch}),
+            std::vector<Appointment>({christmas, christmas_lunch}),
+            5
+        },
+        {
+            "test edge cases: confirm 0 events works ok",
+            DateTime::Local(2016,12,24,0,0,0),
+            std::vector<Appointment>({}),
+            std::vector<Appointment>({}),
+            5
+        },
+        {
+            "test edge cases: confirm max-events of 0 doesn't crash",
+            DateTime::Local(2016,12,24,0,0,0),
+            std::vector<Appointment>({christmas, bike, santa, christmas_lunch}),
+            std::vector<Appointment>({}),
+            0
+        }
+    };
+
+    for (const auto& test : tests)
+    {
+        g_debug("running test: %s", test.description);
+
+        // run the test...
+        ASSERT_EQ(test.expected_display_appointments, Menu::get_display_appointments(test.appointments, test.start, test.max_items));
+
+        // ...and again with a reversed vector to confirm input order doesn't matter
+        auto reversed = test.appointments;
+        std::reverse(reversed.begin(), reversed.end());
+        ASSERT_EQ(test.expected_display_appointments, Menu::get_display_appointments(reversed, test.start, test.max_items));
+    }
+}
+