Indicator Date and Time

Merge lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms into lp://staging/indicator-datetime/15.04

  1. lp-1419001-honor-ical-valarms
  2. Merge into trunk.15.04
Proposed by Charles Kerr
Status: Merged
Approved by: Ted Gould
Approved revision: 430
Merged at revision: 408
Proposed branch: lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms
Merge into: lp://staging/indicator-datetime/15.04
Diff against target: 1791 lines (+834/-269)
33 files modified
debian/control (+11/-8)
include/datetime/alarm-queue-simple.h (+6/-12)
include/datetime/alarm-queue.h (+1/-1)
include/datetime/appointment.h (+19/-4)
include/datetime/clock-mock.h (+1/-1)
include/datetime/clock.h (+1/-1)
include/datetime/date-time.h (+5/-1)
include/datetime/planner-snooze.h (+1/-2)
include/datetime/snap.h (+2/-1)
include/datetime/wakeup-timer-mainloop.h (+2/-2)
include/datetime/wakeup-timer-powerd.h (+2/-2)
include/datetime/wakeup-timer.h (+1/-1)
src/actions-live.cpp (+13/-6)
src/actions.cpp (+2/-2)
src/alarm-queue-simple.cpp (+140/-109)
src/appointment.cpp (+9/-3)
src/date-time.cpp (+31/-9)
src/engine-eds.cpp (+153/-54)
src/main.cpp (+7/-5)
src/planner-snooze.cpp (+10/-6)
src/snap.cpp (+9/-7)
tests/CMakeLists.txt (+33/-4)
tests/manual-test-snap.cpp (+5/-5)
tests/print-to.h (+45/-0)
tests/run-eds-test.sh (+57/-0)
tests/test-alarm-queue.cpp (+10/-7)
tests/test-eds-valarms-config-files/.config/evolution/sources/system-proxy.source (+21/-0)
tests/test-eds-valarms-config-files/.local/share/evolution/calendar/system/calendar.ics (+47/-0)
tests/test-eds-valarms.cpp (+101/-0)
tests/test-live-actions.cpp (+0/-6)
tests/test-snap.cpp (+10/-10)
tests/timezone-mock.h (+1/-0)
tests/wakeup-timer-mock.h (+78/-0)
To merge this branch: bzr merge lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Ted Gould (community) Approve
Review via email: mp+255249@code.staging.launchpad.net

Commit message

Improve valarm support to honor calendar events' valarm triggers.

Description of the change

== Description of the Change

Improve our valarm support for better calendar reminders, adding support for (1) multiple valarms per event, and (2) honor the valarms' trigger times, rather than unconditionally reminding when the event is reached.

A use case that exercises both of these features is an airplane flight vevent that has a "pack your bags" valarm set to be triggered the day before and a "go to the airport now" valarm to be triggered a few hours before.

At the code level, the Appointment class has been refactored to own a container of Alarm objects, which correspond to ical valarms. There are changes in the AlarmQueue and in the EDS backend to accommodate this.

The patch also adds EDS tests to confirm that we can get a set of Alarms correctly from an ical file loaded by evolution. A lot of the EDS/dbus-test-runner scaffolding is reused from renato's qtorganizer5-eds code.

== Checklist

> Are there any related MPs required for this MP to build/function as expected? Please list.

No other MPs needed

> Is your branch in sync with latest trunk? (e.g. bzr pull lp:trunk -> no changes)

Yes

> Did the code build without warnings?

Yes

> Did the tests run successfully?

Yes

> Did you perform an exploratory manual test run of your code change and any related functionality?

Yes

> If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

N/A

> What device (or emulator) has your component test plan been executed successfully on?

Mako r164

> What manual tests are relevant for this MP?

indicator-datetime/calendar-event-reminder-time

> Did you include a link to the MR Review Checklist Template to make your reviewer's life easier?

https://wiki.ubuntu.com/Process/Merges/Checklists/indicator-datetime

To post a comment you must log in.
lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms updated
416. By Charles Kerr

in tests/run-eds-test.sh, improve the comments

417. By Charles Kerr

remove some new bits that turned out to be unneeded after all

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:417
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/304/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-amd64-ci/18/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-armhf-ci/18/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/304/rebuild

review: Needs Fixing (continuous-integration)
lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms updated
418. By Charles Kerr

in debian/control, add evolution-data-server to the build-dep now that we're using it for live EDS tests.

419. By Charles Kerr

in new code, use std::array rather than C style arrays

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:417
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/305/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-amd64-ci/19/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-armhf-ci/19/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/305/rebuild

review: Needs Fixing (continuous-integration)
lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms updated
420. By Charles Kerr

in the unit tests, add a PrintTo function for Alarms so that GTest can represent it as a string

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:417
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/306/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-amd64-ci/20/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-armhf-ci/20/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/306/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

Comments and a couple of questions, but I don't see any blockers.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:420
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/307/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-amd64-ci/21/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-armhf-ci/21/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/307/rebuild

review: Needs Fixing (continuous-integration)
lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms updated
421. By Charles Kerr

in DateTime class, make it harder to accidentally mix local and nonlocal timezones by replacing DateTime::DateTime(time_t) with two methods, DateTime::Local(time_t) and DateTime(GTimeZone*, time_t)

422. By Charles Kerr

in new EDS tests, use timezones consistently

423. By Charles Kerr

in new EDS code, use timezones consistently

424. By Charles Kerr

in Actions, sync DateTime API use by calling DateTime::Local(time_t) instead of DateTime::DateTime(time_t)

425. By Charles Kerr

in SimpleAlarmQueue, reduce a lambda capture to only the fields it needs

426. By Charles Kerr

in SimpleAlarmQueue, make the signature for find_next_alarm() and appointment_get_current_alarm() suck less.

427. By Charles Kerr

in SimpleAlarmQueue, add a new method 'bool already_triggered() const' to reduce code overlapl between find_next_alarm() and appointment_get_current_alarm()

428. By Charles Kerr

in EngineEds, make the ECalComponentAlarmAction 'omit' array a constexpr.

429. By Charles Kerr

in the EDS engine, give a better explanation in the comments how we handle alarms with no triggers, and why

430. By Charles Kerr

in tests/run-eds-test.sh, only delete the tmpdir if the test passed.

Revision history for this message
Charles Kerr (charlesk) wrote :

> You know you want to make these into a template :-)

Yes but not today. :-)

> Seems like you only need this here? No need to get references to the parameters.

Fixed r425

>> bool find_next_alarm(Alarm& setme) const
> I think in c++11 it makes more sense to return a tuple
> than to have a return in a parameter. Then you can bring
> it back with std::tie() in the caller to make it not suck

Returning a tuple of {success flag, Alarm} means we'd
still return an Alarm even if nothing was found.

Since we're walking through memory owned by the caller,
we could return a simple const pointer that's nullptr
if no match is found. That would make the API less screwy.

Fixed r426

> Seems like these two (the function above) could be combined
> into a shared function easily, just to reduce the number of
> search functions. If you "find_next_alarm()" and then is_same_minute
> the result I think that would provide more shared code.

They don't really fit, since one needs to loop through all appointments
and the other only walks through one appointment's alarms.
I could extract-method the "is this triggered already?" logic and
have both methods call it though.

Fixed r427

> Is there no #define for this? Scary. But we should probably static
> this array. No reason to build it on the stack each time.

Yeah, it's a strange interface.

Changed to a constexpr in r428.

> Not sure why we're checking the alarm to see if the text is set,
> why don't we always want the data structure to match what is
> being returned in a. Seems we want a sync here, even clearing
> the text if it was no longer in a.

There can be multiple valarms attached to an event, even to trigger
at the same time. e.g., events generated by calendar-app will have
one valarm specifying the sound action nd another specifying the
display action. We're iterating through all the valarms here, so we
have to be careful to pick up display text only if it's actually there.

> Confused at what this is doing, it seems like if the component
> didn't have any alarms, it also wouldn't have any UIDs for alarms
> that we could get text from.

It's not that it has no alarms; it's that the alarms had no triggers
specifying when they were supposed to go off. We need to special-case
that scenario because ubuntu-ui-toolkit used to generate alarms that
had no trigger.

Explained in comments better in r429.

> I think you actually only want to delete the temp directory if the
> test passes. That way it can end up still around so you can figure
> out what happened and/or picked up by Jenkins.

Good idea! Fixed r430.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:420
http://jenkins.qa.ubuntu.com/job/indicator-datetime-ci/308/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/indicator-datetime-vivid-armhf-ci/22/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-datetime-ci/308/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches