Subject: [calsify] AD review of draft-ietf-calext-valarm-extensions-03

From: Barry Leiba <[email-masked]>
Date: Thu, 3 Dec 2020 16:15:38 -0500

Most of my comments are minor, but there are a couple of things to
discuss (particularly how to handle the ABNF).  I'm going to put this
in "revised I-D needed", but let's talk about the things that need
discussion before submitting a revision for the others.

— Section 2 —
There’s no need for the “[1]” citation, and it only messes up idnits.
Please remove it and the “URLs” section at the end.

— Section 3 —

My biggest issue here is with the ABNF, so let’s please have a
discussion about it.

First, some minor things:

1. This is probably a good time to pay attention to BCP 178 (RFC 6648) by
removing the x-prop construction (and x-name elsewhere in the draft).

2. Why is there a need to define new constructs (“alarmcext”,
“audiopropext”, and so on), rather than just re-defining the base ones
(“alarmc”, “audioprop”, …)?  It seems to me that this just confuses
things, and the point here is to make changes that are compatible, but
more readily extensible.

Now, the big one:

3. I have never liked the way iCalendar did its ABNF, to the point
where I think it’s wrong and harmful, causing confusion and not
permitting automated processing of the ABNF.  It relies on normative
comments where that’s not necessary, and ABNF was never intended to be
usef that way.  Comments are meant to be explanatory, and normative
comments are meant to be used only as a last resort, when you can’t
express what you need in the formal language.  I would much rather
limit the comments to a statment that elements can appear in any
order, and then use the ABNF to convey whether the elements are
required and how many times they can appear.

Taking “alarmprop” as an example, here’s a re-write of it in the way
ABNF should be representing it:

OLD
   alarmprop  = *(

                ; the following are REQUIRED,
                ; but MUST NOT occur more than once

                action / trigger /

                ; one set of action properties MUST be
                ; present and MUST match the action specified
                ; in the ACTION property

                actionprops /

                ; the following is OPTIONAL,
                ; and MAY occur more than once

                x-prop / iana-prop

                )
NEW
   alarmprop  = ; the elements herein may appear in any order,
                ; and the order is not significant.

                action
                trigger

                actionprops ; MUST match the action specified
                            ; in the ACTION property

                [*iana-prop]
END

For “audiopropext”:

OLD
   audiopropext  = *(

                   ; 'duration' and 'repeat' are both OPTIONAL,
                   ; and MUST NOT occur more than once each,
                   ; but if one occurs, so MUST the other

                   duration / repeat /

                   ; the following is OPTIONAL,
                   ; but MUST NOT occur more than once

                   attach

                   )
NEW
   audiopropext  = ; the elements herein may appear in any order,
                   ; and the order is not significant.
                   [duration repeat]
                   [attach]
                   )
END

Is there really a good reason not to move in this direction?  The
mechanism of saying that everything can appear zero or more times, and
then limiting that with comments seems very poor.

— Section 6.1 —

      Certain
      kinds of alarm may not provide feedback as to when the calendar
      user sees them, for example email based alerts.

Nit: This sentence isn’t properly ordered (and has a couple of other
nit-level errors).

NEW
      Certain
      kinds of alarms, such as email-based alerts, might not provide
      feedback as to when the calendar user sees them.
END

— Section 7 —

   Clients SHOULD add a "RELATED-TO" property to the new "VALARM"
   component with a value set to the "UID" property value of the
   "VALARM" component being snoozed.  If the "VALARM" component being
   snoozed does not already have a "UID" property, the client SHOULD add
   one.  The "RELATED-TO" property added to the new "VALARM" component
   SHOULD include a "RELTYPE" property parameter with a value set to
   "SNOOZE".

Why the SHOULDs?  This is new function; why are these not MUSTs?

   When the "snooze" alarm is triggered and dismissed the client SHOULD
   remove the corresponding "VALARM" component, or set the
   "ACKNOWLEDGED" property (see Section 6.1).  Alternatively, if the
   "snooze" alarm is itself "snoozed", the client SHOULD remove the
   original "snooze" alarm and create a new one, with the appropriate
   trigger time and relationship set.

Same here about the SHOULDs.  Also, it’s not clear to me how this
relates to the original alarm.  It would be good to say, explicitly,
that the original VALARM is not removed.  But also, is the
“ACKNOWLEDGED” time on the original alarm changed to show the time
that the snooze was dismissed?  If not, why not?  And is there a
reason to choose between the apparently equal alternatives presented
in the first sentence?  Or is the choice entirely up to the
implementation?

— Section 7.1 —

   This specification adds the "SNOOZE" relationship type for use with
   the "RELTYPE" property defined in Section 3.2.15 of [RFC5545].  This
   is used to relate a "snoozed" "VALARM" component to the original
   alarm that the "snooze" was generated for.

Maybe I’m being too picky here, but I don’t think the RELTYPE of
SNOOZE is used to relate the alarms: the UID property is used for
that.  Maybe “This is used when relating” is better wording?

— Section 8.1 —

      When the property value is set to "ARRIVE", the alarm is triggered
      when the calendar user agent arrives in the vicinity of any of the
      specified locations.  When set to "DEPART", the alarm is triggered
      when the calendar user agent departs from the vicinity of any
      specified locations.

I think we need to more clearly specify “vicinity” here, even if it’s
just to say that the definition is up to the implementation.  As it
is, I don’t know whether I need to depart from my kitchen, from my
house, from my street, from my town, or whatever.

      When the property value is set to "CONNECT", the alarm is
      triggered when the calendar user agent connects to a Bluetooth(R)
      [BTcore]-enabled automobile.  When set to "DISCONNECT", the alarm
      is triggered when the calendar user agent disconnects from a
      Bluetooth(R)-enabled automobile.

Nit: It’s odd to have the citation in the middle of the compound
modifier.  I suggest rewriting to avoid the problem:

NEW
      When the property value is set to "CONNECT", the alarm is
      triggered when the calendar user agent connects to an automobile
      that is enabled for Bluetooth(R) [BTcore].  When set to
      "DISCONNECT", the alarm is triggered when the calendar user agent
      disconnects from a Bluetooth(R)-enabled automobile.
END

   Example:  The following is an example of this property:
   PROXIMITY:ARRIVE

I think this example is useless, as it’s incomplete without the geo
location.  And you have Section 8.2 immediately after, so why not just
remove this?

— Section 11 —
Throughout this section, please specify the names and/or URIs of the
registries, to make it easier for both IANA and other readers to find
them.  For example, in Section 11.1:

NEW
   This document defines the following new iCalendar properties to be
   added to the Properties registry defined in Section 8.2.3 of [RFC5545]
   and located here:
   <https://www.iana.org/assignments/icalendar#properties>
END

-- 
Barry