Code review comment for lp://staging/~charlesk/indicator-datetime/lp-1419001-honor-ical-valarms

> 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.