New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TIMOB-24248] Android: Ti.Calendar Recurring Events are not clearly exposed #9412
Conversation
@@ -42,6 +42,13 @@ | |||
@Kroll.constant public static final int STATE_FIRED = AlertProxy.STATE_FIRED; | |||
@Kroll.constant public static final int STATE_SCHEDULED = AlertProxy.STATE_SCHEDULED; | |||
|
|||
//region recurrence frequency | |||
@Kroll.constant public static final int RECURRENCEFREQUENCY_DAILY = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd prefer RECURRENCE_FREQUENCY_DAILY
to RECURRENCEFREQUENCY_DAILY
(same goes for the rest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the way we currently have them for iOS:
https://docs.appcelerator.com/platform/latest/#!/api/Titanium.Calendar-property-RECURRENCEFREQUENCY_DAILY
We can change them in there too - now would be the time because that would be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd want to "fix" them to match our naming conventions. We'd have to retain the existing definitions and mark them as deprecated for this release, then in 8.0 or later we can remove the ones missing the underscores. So not really a breaking change, but an important deprecation to get in before GA.
@@ -117,7 +120,7 @@ public static String getExtendedPropertiesUri() | |||
event.hasAlarm = !eventCursor.getString(7).equals("0"); | |||
event.status = eventCursor.getInt(8); | |||
event.visibility = eventCursor.getInt(9); | |||
|
|||
event.setRecurrenceRules(eventCursor.getString(10), eventCursor.getInt(11)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this break when pulling up existing events with no recurrence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently it could. I did not get any exception, but it is documented as "implementation-defined"
https://developer.android.com/reference/android/database/Cursor.html#getString(int)
I will add an exception handling.
{ | ||
return recurrenceRule; | ||
RecurrenceRuleProxy[] result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe something a little more compact?
RecurrenceRuleProxy[] result = new RecurrenceRuleProxy[]{};
if (rrule != null) {
result = new RecurrenceRuleProxy[] { new RecurrenceRuleProxy(rrule, calendarID, begin) };
}
setProperty(TiC.PROPERTY_RECURRENCE_RULES, result);
// Currently only saving added recurrenceRules. | ||
String ruleToSave = ((RecurrenceRuleProxy) ((Object[]) getProperty(TiC.PROPERTY_RECURRENCE_RULES))[0]).generateRRULEString(); | ||
ContentValues contentValues = new ContentValues(); | ||
contentValues.put(Events.RRULE, ruleToSave); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of the truncated constant name here: Events.RRULE
versus something like Events.RECURRENCE_RULE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an Android constant:
https://developer.android.com/reference/android/provider/CalendarContract.EventsColumns.html#RRULE
} | ||
|
||
//region Methods for converting native to Kroll values | ||
private void calculateDaysOfTheMonth(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void calculateDaysOfTheMonth() {
private void calculateDaysOfTheYear() { | ||
String days = matchExpression(".*(BYYEARDAY=[0-9]*).*", 10); | ||
if (days != null && frequencyMap.get(frequency).equals(CalendarModule.RECURRENCEFREQUENCY_YEARLY)) { | ||
daysOfTheYear = new int[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
daysOfTheYear = new int[] { Integer.valueOf(days) };
//region values that can be set from a Creation Dictionary | ||
private int frequency = -1; | ||
private int interval = -1; | ||
private int[] daysOfTheMonth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why the choice to use int[]
for these fields that all seem to be either empty or have one single value. Personally I'd have used Integer
type, with null
representing a "no value".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use something like Optional
: https://developer.android.com/reference/java/util/Optional.html or not until Api level 24 is our minimum?
cc @garymathews @jquick-axway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our JavaScript API defines it as an array (link below). So, using an int[]
is fine.
http://docs.appcelerator.com/platform/latest/#!/api/Titanium.Calendar.RecurrenceRule-property-daysOfTheMonth
@ypbnv, we need to double check what iOS returns if this property is not set upon creation. That is, if it returns null or an empty array. We don't document this and these details matter. Once known, we can then initialize the int[]
member variables appropriately (either null or empty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jquick-axway I agree. I used this as a reference:
https://developer.apple.com/documentation/eventkit/ekrecurrencerule/1507410-daysofthemonth?language=objc
Couldn't find the result there. Maybe try it on a device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ypbnv, I just tested it out and confirmed that the following properties never return null on iOS. They return an empty array/dicationary if not assigned upon creation. Personally, I agree with this behavior, because I find most script developers (unlike C/C++ devs) tend to not bother doing null checks.
- daysOfTheMonth
- daysOfTheWeek
- daysOfTheYear
- monthsOfTheYear
- weeksOfTheYear
So, we'll need to match the above behavior on Android.
Also, when you change the docs, would you mind updating the above properties with this info too please? Perhaps add the following sentence to the end of each property description like this...
Will be an empty array if no recurrence of this type has been configured.
Note that the daysOfTheWeek
property is a dictionary and not an array. So, update the docs appropriately.
Any case, I hope this helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are currently working this way on Android. I will add the description in the documentation.
|
||
private String matchExpression(String regEx, int length) { | ||
Pattern pattern = Pattern.compile(regEx); | ||
Matcher matcher = pattern.matcher(rRule); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to use this.rRule
for consistency here.
public String generateRRULEString() { | ||
StringBuilder finalRRule = new StringBuilder(); | ||
// Handle frequency. | ||
if (frequency > -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use this.frequency
to be consistent about field access?
I assume -1
is some magical constant to denote "not specified"? Again I'd personally use Optional<Integer>
or at least Integer
with a null
value to mark whether we have a value or not. But even if you don't do that, you may want to at least extract a constant for -1 to state it's "UNSPECIFIED" or something.
StringBuilder finalRRule = new StringBuilder(); | ||
// Handle frequency. | ||
if (frequency > -1) { | ||
String frequencyPart = "FREQ=" + frequencyMap.keySet().toArray()[frequency - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How in the world is this consistent? The frequency map's key set is presumably not sorted/ordered.
I'm starting to think frequency should be an enum.
} // end of switch | ||
} | ||
// Handle interval. | ||
if (interval > -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, use of a magic -1 constant is not particularly obvious.
} | ||
} | ||
|
||
private void calculateMonthsOfTheYear() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combine with calculateWeeksOfTheYear
since they both really apply to yearly frequencies (and maybe rename to something like calculateYearlyFrequency
)
|
||
private void calculateDaysOfTheWeek() { | ||
// Create a dictionary in the context of a month. | ||
if (frequency == CalendarModule.RECURRENCEFREQUENCY_MONTHLY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two if clauses are exclusive. I think it may make more sense to calculate the necessary fields grouped by the frequency value, rather than to calculate grouped by specific fields.
Basically, initialize days of the week, days of the year, months of the year, etc with the default empty array value (or whatever empty value you decide to use based on Optional/Integer usage); and then override to the real value if the frequency is of a given type in a method that just handles that frequency type.
Get rid of "magic" numbers and possible inconsistency between frequency words and numbers. Make fields access more distinguished. Guard for exceptions when getting recurrence rules for events. Fix some formatting.
@sgtcoolguy Updated the PR. I haven't marked the frequency constants yet. If we are OK to do that on iOS as well I will mark them once I update the documentation. @hansemannn, what are your thoughts about that? @jquick-axway Speaking of documentation - I did not find what are the values for |
The recurrence frequency constants are defined under our "Ti.Calendar" doc here...
iOS defaults to There are no constants for |
private String rRule; | ||
|
||
//region values that can be set from a Creation Dictionary | ||
private Integer frequency = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, frequency
should be an enum, not an int
or Integer
. This way we can easily constrain the value to known constants and provide easy mappings to non-Titanium identifiers.
For example:
public enum TiRecurrenceFrequencyType
{
DAILY(CalendarModule.RECURRENCEFREQUENCY_DAILY, "DAILY"),
WEEKLY(CalendarModule.RECURRENCEFREQUENCY_WEEKLY, "WEEKLY"),
MONTHLY(CalendarModule.RECURRENCEFREQUENCY_MONTHLY, "MONTHLY"),
YEARLY(CalendarModule.RECURRENCEFREQUENCY_YEARLY, "YEARLY");
private final int tiIntId;
private final String rfcStringId;
private TiRecurrenceFrequencyType(int tiIntId, String rfcStringId)
{
this.tiIntId = tiIntd;
this.rfcStringId = rfcStringId;
}
public int toTiIntId()
{
return this.tiIntId;
}
public String toRfcStringId()
{
return this.rfcStringId;
}
public static TiRecurrenceFrequencyType fromTiIntId(int value)
{
for (TiRecurrenceFrequencyType nextObject : TiRecurrenceFrequencyType.values()) {
if ((nextObject != null) && (nextObject.tiIntId == value)) {
return nextObject;
}
}
return null;
}
public static TiRecurrenceFrequencyType fromRfcStringId(String value)
{
for (TiRecurrenceFrequencyType nextObject : TiRecurrenceFrequencyType.values()) {
if ((nextObject != null) && (nextObject.rfcStringId == value)) {
return nextObject;
}
}
return null;
}
}
With the above, you no longer need your frequencyMap
static variable since the RFC frequency string ID mapping is already embedded into the enum. You can also easily convert/validate the JavaScript frequency
field in your RecurrenceRuleProxy
via the enum's fromTiIntId()
like this...
if (creationDictionary.containsKey(TiC.PROPERTY_FREQUENCY)) {
int integerId = TiConvert.toInt(creationDictionary.get(TiC.PROPERTY_FREQUENCY));
this.frequency = TiRecurrenceFrequencyType.fromTiIntId(integerId);
if (this.frequency == null) {
// Given frequency ID from JavaScript is invalid. Log an error.
}
}
if (this.frequency == null) {
// Default to daily if not assigned or given an invalid frequency.
this.frequency = TiRecurrenceFrequencyType.DAILY;
}
You don't have to do this now, but Gary and I want to slowly transition our code to use the above pattern since it's a much more maintainable and less error prone solution. I've already started doing so with Titanium's orientation type handling here: TiDeviceOrientation.java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jquick-axway Updated.
Unit test will be added once we establish the best way to have a properly set calendar on the emulators. All of the additions in this PR rely on having an accessible calendar with events. |
if (creationDictionary.containsKey(TiC.PROPERTY_FREQUENCY)) { | ||
this.frequency = | ||
TiRecurrenceFrequencyType.fromTiIntId(TiConvert.toInt(creationDictionary.get(TiC.PROPERTY_FREQUENCY))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS defaults to DAILY if the frequency was not set. We should do the same. Something like this...
if (creationDictionary.containsKey(TiC.PROPERTY_FREQUENCY)) {
this.frequency = TiRecurrenceFrequencyType.fromTiIntId(TiConvert.toInt(creationDictionary.get(TiC.PROPERTY_FREQUENCY)));
}
if (this.frequency == null) {
this.frequency = TiRecurrenceFrequencyType.DAILY;
}
@Kroll | ||
.getProperty | ||
@Kroll.method | ||
public String getCalendarID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, adding multiple annotations to the same member triggers this clang formatting issue. We currently work-around it by adding the following comments around it like the below. (It's a bummer, I know. Sorry.)
// clang-format off
@Kroll.getProperty
@Kroll.method
public String getCalendarID()
// clang-format on
{
return this.calendarID;
}
apidoc/Titanium/Calendar/Event.yml
Outdated
@@ -86,7 +86,8 @@ methods: | |||
type: Number | |||
constants: Titanium.Calendar.SPAN_* | |||
default: <Titanium.Calendar.SPAN_THISEVENT> | |||
platforms: [iphone, ipad] | |||
platforms: [android, iphone, ipad] | |||
since: {android: "7.0.0", iphone: "3.1.0", ipad: "3.1.0"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Android version to "7.1.0"
apidoc/Titanium/Calendar/Event.yml
Outdated
type: Array<Titanium.Calendar.RecurrenceRule> | ||
osver: {ios: {min: "5.0"}} | ||
platforms: [iphone, ipad] | ||
platforms: [android, iphone, ipad] | ||
since: {android: "7.0.0", iphone: "3.1.0", ipad: "3.1.0"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Android version to "7.1.0"
apidoc/Titanium/Calendar/Event.yml
Outdated
@@ -249,9 +250,13 @@ properties: | |||
|
|||
- name: recurrenceRules | |||
summary: The recurrence rules for the calendar item. (Available in iOS 5.1 and above.) | |||
description: | | |||
On Android only the first element of the recurrenceRules is take into account | |||
due to the way it handles conditions for recurrence rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a comma after "On Android," and the word "take" should be "taken".
On Android, only the first element of the recurrenceRules is taken into account
due to the way it handles conditions for recurrence rules.
int index = 0; | ||
for (String record : value) { | ||
try { | ||
result[index++] = new KrollDict(new JSONObject(record)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at TiConvert.toStringArray()
, an element in the array can be null
. So, this will wrongly remove null entries and reduce the size of the array. Perhaps the below would be better?
for (String record : value) {
KrollDict dictionary = null;
if (record != null) {
try {
dictionary = new KrollDict(new JSONObject(record));
} catch (Exception e) {
e.printStackTrace();
Log.w(TAG, "Unable to parse dictionary.");
}
}
result[index++] = dictionary;
}
Generated by 🚫 dangerJS |
Fix docs. Add clang-format off for proxy methods. Clear tresting code. Fix MONTHLY rules. Fix toStringArray() mothod.
@jquick-axway Updated the PR. Also found few other things that should be updated. |
FR Passed.
Studio Ver: 5.0.0.201712081732 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: Pass
JIRA: https://jira.appcelerator.org/browse/TIMOB-24248
Description:
Implements Ti.Calendar.RecurrenceRule for Android. Due to the way Android handles recurrence rules it is not as complex as iOS and does not support all of the conditions, but provides parity for the most common cases.
Requirements:
Test case: