Improve readability and accuracy of repeat rule display text
pauldawg - September 21, 2009 - 05:29
| Project: | Date |
| Version: | 6.x-2.4 |
| Component: | Date Repeat API |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
The purpose of this feature request is to improve the wording of the "repeat rule" display text to make it more concise wherever possible, enhance the information provided, and ensure the accuracy, while making it generally easier to read.
- This patch takes care of several fixes and enhancements:
- #460776: Repeating Date Rule Display missing Ordinal word (e.g. “Second”) when displayed in View, e.g. Upcoming
- Remove duplicate "every" in the phrase "every week every Thursday"
- Limit the need for the word "every" in general, preferring words such as daily, weekly, monthly, and annually, or eliminate entirely such as in the following case: "Repeats on the first Sunday of the month" rather than "Repeats every month on the first Sunday"
- Use commas and conjunctions in all lists of repeat rule parts.
- Reuse and clarify wording for ordinal by-day parts, i.e. "first Sunday and Saturday of the month" instead of "first Sunday, first Saturday"
- Change "on" to "during" when referring to months, i.e. "during January, February, and March" instead of "on January, February, March"
- Show the by-month-day dates in the repeat rule, i.e. "Repeats on days 2, 3, and 10 of the month"
- Still remaining to be addressed are:
- Haven't yet gone through the "every (number)" cases (every 2 months, etc.), and these are probably not working correctly yet
- Use consistent formatting for the "Until" date(s); currently the Date format used is hard-coded and does not necessarily matched the preferred date format for the field display.
- Include the start date in the repeat rule if they do not coincide with one another, i.e. "Starts on Tuesday, Jan 1 2009 and repeats on the second Wednesday of the month..." to clearly indicate that the first instance of the date is an exception to the rule.
- Include more information for weekly and monthly recurring dates with no by-day, or by-month-day information, i.e. "Repeats every Thursday" instead of "Repeats every week", or "Repeats on the 15th day of every month" instead of "Repeats monthly".
- Further enhance readability of the by-month-day dates using the "nth" form of the numbers; i.e. "on the 2nd, 3rd and 10th" instead of "on days 2, 3 and 10"
- Ensure that invalid repeat rules are either rejected entirely or are treated the same way by both the _date_repeat_calc function and the date_repeat_rrule_description function; i.e. if the by-day part and the by-month-day part are both in use in an invalid way, the dates shown in the list do not match the wording of the display rule
- Ensure that the existing translations provided are all working correctly and that any additional translations will work correctly
I think I've addressed most of the easy stuff already, and have tested it against several examples. The remainder of the tasks have the need to get some more context about the date field "from" value and "to" value, which doesn't seem to be available within the date_repeat module as it stands now, and some more significant code changes that I thought I might wait for. I am not at the point of creating an official patch for this yet, but I wanted to share what I've done so far.
I've been at Drupal for over 2 years and been programming for decades but new to PHP, and this is my first official code contribution to Drupal, so please be kind. :)
Here is the code for date_repeat.module:
function date_repeat_rrule_description($rrule, $format = 'D M d Y') {
// Empty or invalid value.
if (empty($rrule) || !strstr($rrule, 'RRULE')) {
return;
}
require_once('./'. drupal_get_path('module', 'date_api') .'/date_api_ical.inc');
require_once('./'. drupal_get_path('module', 'date_repeat') .'/date_repeat_calc.inc');
// Make sure there will be an empty description for any unused parts.
$description = array(
'!interval' => '',
'!byday' => '',
'!bymonth' => '',
'!count' => '',
'!until' => '',
'!except' => '',
'!week_starts_on' => '',
);
$parts = date_repeat_split_rrule($rrule);
$exceptions = $parts[1];
$rrule = $parts[0];
$interval = INTERVAL_options();
if ($rrule['INTERVAL'] < 1) {
$rrule['INTERVAL'] = 1;
}
if (empty($rrule['BYDAY']) && empty($rrule['BYMONTHDAY']) && empty($rrule['BYMONTH']) || $rrule['FREQ']=='DAILY' || $rrule['FREQ']=='WEEKLY') {
switch ($rrule['FREQ']) {
case 'WEEKLY':
$description['!interval'] = format_plural($rrule['INTERVAL'], 'weekly', 'every @count weeks') .' ';
break;
case 'MONTHLY':
$description['!interval'] = format_plural($rrule['INTERVAL'], 'monthly', 'every @count months') .' ';
break;
case 'YEARLY':
$description['!interval'] = format_plural($rrule['INTERVAL'], 'annually', 'every @count years') .' ';
break;
default:
$description['!interval'] = format_plural($rrule['INTERVAL'], 'daily', 'every @count days') .' ';
break;
}
}
$days = date_repeat_dow_day_options();
$counts = date_repeat_dow_count_options();
//Sort the by-day results if any into two categories,
//those occurring weekly, and those occurring on the nth ordinal per month, grouped by ordinal
$ordinalResults = array();
$weeklyResults = array();
if (!empty($rrule['BYDAY'])) {
foreach ($rrule['BYDAY'] as $byday) {
$day = substr($byday, -2);
$strCount = substr($byday, 0, 2);
if ($count = intval($strCount)) {
$instance = trim(t($days[$day]));
if (empty($ordinalResults[$count]))
{
$ordinalResults[$count] = array();
}
$ordinalResults[$count][] = $instance;
}
else {
$weeklyResults[] = trim(t($days[$day]));
}
}
$byDayResults = array();
if (!empty($weeklyResults))
{
$byDayResults[] = t('on').' '.trim(t(implodeGrammar($weeklyResults)));
}
if (!empty($ordinalResults))
{
$resultsByOrdinal = array();
for ($i=-5; $i<=5; $i++)
{
if (!empty($ordinalResults[$i]))
{
$ordinal = $i < 0 ? '-'.$i : ($i > 0 ? '+'.$i : '');
$resultsByOrdinal[] = t('the').' '.strtolower(trim(t($counts[$ordinal]))).' '.implodeGrammar($ordinalResults[$i]);
}
}
$byDayResults[] = t('on').' '.trim(t(implodeGrammar($resultsByOrdinal))).' '.t('of the month');
}
$description['!byday'] = implodeGrammar($byDayResults);
}
if (!empty($rrule['BYMONTH'])) {
if (sizeof($rrule['BYMONTH']) < 12) {
$results = array();
$months = date_month_names();
foreach ($rrule['BYMONTH'] as $month) {
$results[] = $months[$month];
}
if (!empty($rrule['BYMONTHDAY'])) {
$description['!bymonth'] = trim(t('!repeats_every_interval on days !month_days of !month_names', array('!repeats_every_interval ' => '', '!month_days' => implodeGrammar($rrule['BYMONTHDAY']), '!month_names' => implodeGrammar($results))));
}
else {
$description['!bymonth'] = trim(t('!repeats_every_interval during !month_names', array('!repeats_every_interval ' => '', '!month_names' => implodeGrammar($results))));
}
}
}
if (!empty($rrule['COUNT'])) {
$description['!count'] = trim(t('!repeats_every_interval !count times', array('!repeats_every_interval ' => '', '!count' => $rrule['COUNT'])));
}
if (!empty($rrule['UNTIL'])) {
$until = date_ical_date($rrule['UNTIL']);
$description['!until'] = trim(t('!repeats_every_interval until !until_date', array('!repeats_every_interval ' => '', '!until_date' => date_format_date($until, 'custom', $format))));
}
if ($exceptions) {
$values = array();
foreach ($exceptions as $exception) {
$values[] = date_format_date(date_ical_date($exception), 'custom', $format);
}
$description['!except'] = trim(t('!repeats_every_interval except on !except_dates', array('!repeats_every_interval ' => '', '!except_dates' => implodeGrammar($values))));
}
if (!empty($rrule['WKST'])) {
$day_names = date_repeat_dow_day_options();
$description['!week_starts_on'] = trim(t('!repeats_every_interval where the week start on !day_of_week', array('!repeats_every_interval ' => '', '!day_of_week' => $day_names[trim($rrule['WKST'])])));
}
return t('Repeats !interval !byday !count !bymonth !until !except.', $description);
}
/**
* Implodes an array of strings using punctuation and/or a conjunction, depending on the number of items in the array
* Two items are joined by a conjunction, whereas three or more are joined by commas with the conjunction preceding the last item
*/
function implodeGrammar($terms, $punctuation = ',', $conjunction = 'and') {
$conjunction = trim(t($conjunction));
$count = count($terms);
if ($count == 1)
{
return $terms[0];
}
elseif ($count == 2)
{
return implode(' '.$conjunction.' ', $terms);
}
else
{
$result = array();
for ($i = 0; $i<$count-1; $i++)
{
$result[] = $terms[$i];
}
$result[] = ' '.$conjunction.' '.$terms[$count-1];
return implode($punctuation.' ', $result);
}
return '';
}
#1
This looks better for "Repeats on the first Saturday of the month until Fri Dec 31 2010 ." Note a space between year and period. [I really wish I didn't have to specify an ending date.]
But "Repeats on days 31 of October until Mon Dec 31 2012 ." is terrible! I would much rather see "Repeats on the 31st of October until Mon Dec 31 2012."
Code style: "{" does not begin a row in Drupal. And we put spaces around operators (even in looping statements)
if ($count == 1) {return $terms[0];
}
elseif ($count == 2) {
return implode(' '.$conjunction.' ', $terms);
}
else {
$result = array();
for ($i = 0; $i < $count - 1; $i++) {
$result[] = $terms[$i];
}
$result[] = ' '.$conjunction.' '.$terms[$count - 1];
return implode($punctuation.' ', $result);
}
Very minor: I have been told that "++$i" is faster than "$i++".
#2
Thanks NancyDru,
I'll take note of your formatting guidelines and will instruct my IDE to do the same! I usually prefer the opening bracket at the end of a line rather the beginning as well, since it saves space. As for the ++i$ preference, I haven't heard of any performance differences here, but have always followed the $i++ construct, as this is the usual style seen in C++ and Java. In actuality, there is of course a functional difference between the two, but it doesn't matter here since we aren't actually doing anything with the result. Do you have any articles you can point me to describing the performance difference?
As for your formatting example, I agree that it is terrible, and didn't actually catch the plural issue. I would personally avoid the lazy "(s)" approach and just check the value instead, so it would read "on day 31 of October" which is grammatically correct, if not quite as elegant as I'd like. However, I would agree that it would be better if is used the "nth" format instead. The reason I haven't yet done this is that I wanted to do it in a way that was more easily translatable. For example, French has a version of this: 1iere (for premiere), 2ieme (for deuxieme), 3ieme (for troisieme). The question is how best to allow for this while still allowing translations to be created. Since I haven't yet looked into the translations approach, I went for an approach that was straightforward, easy to translate, and grammatically correct.
If you have any guidelines on how translations should be done, I'd be very happy to continue with this. Also, if you or KarenS has any thoughts on how we might be able to better integrate the repeat rule with the start date, I'd be very interested in working on that as well. My plan is to roll this out on my own site soon. I'm happy to finally be able to start contributing back to this great community!
Paul
#3
Two additional points:
1) I hadn't noticed the space before the period and will fix that as well.
2) I am thinking that translating might involve more actual hook implementations rather than simple string substitution, in order to support the semantics, structure and syntax of each particular language, although we may be able to do something adequate with externalized formatting strings. I am not yet familiar with the translation support in Drupal so I am hoping someone with translation experience in Drupal will contribute to this.
In case I haven't mentioned it yet, this Date module as well as the Calendar and related modules, has come a long way in the last 2 years and is quite commendable. This should become part of Drupal core at some point, especially as CCK is already there in D7.
#4
I suspect translation is going to be a nightmare here, because dynamic translation is not very good right now. The core t() function should not be used on variable strings, and especially not on partial strings [e.g. t('the')].
As for the difference between $i++ and ++$i, I wouldn't worry much about it. I suspect the difference is not worth remembering which one does what differently. I suspect it is much less than the difference between single and double quotes.
I have to agree and the progress. I had always been told that Calendar was hard to set up and problematic after that. Other than issues with the upcoming block (which I scrapped), and mostly due to my lack of expertise with Views, it was a piece of cake. I suspect it took no more time, and maybe less, than using the Event & ER modules (which are falling into disrepair).
IIRC, once upon a time, someone submitted a format_ordinals function to the Helpers module; you might want to go look for it. I don't know for sure, but it might work in the translation area. No use re-inventing the wheel.
#5
Okay I think I have a patch worth reviewing. I have fixed a couple more issues (including the plural issue and the space before the period) and I am thinking that perhaps I could leave out the translation piece for now and submit the patch for review. This would leave the following remaining items which could be split into other issues. The reason I think we could leave out the translation piece is that based on the previous explanation of the t() function and my observations on how it was used, my sense is that the existing repeat rules were likely not translated properly anyway.
The following need to request information from elsewhere in the date module and could be submitted as one issue or multiple issues:
The following items may be related to translations and could be submitted in a separate issue:
#6
Oh and I also reformatted the file based on what I think are decent rules for formatting. I will double-check them against the Drupal coding standards and send it through the Coder module.
#7
Nancy, have you been able to test this patch? Also, do you know if this patch was created correctly to get picked up by the "test bot", and whether this should indeed be merged with the original issue?
#8
No I haven't yet. Real Life™ has been in the way.
I don't think the test bot works on contribs.
#9
Thanks for the quick reply, Nancy. Wasn't sure exactly how the test bot worked, or whether it worked for Drupal 6.
#10
"Hunk #5 failed at 127."
BTW:
require_once ('./' . drupal_get_path('module', 'date_api') . '/date_api_ical.inc');require_once ('./' . drupal_get_path('module', 'date_repeat') . '/date_repeat_calc.inc');
can be Drupal-ized with:
module_load_include('inc', 'date_api', 'date_api_ical');module_load_include('inc', 'date_repeat', 'date_repeat_calc');
#11
I can't recall if I still had the previous patch there, but I don't think so.
#12
Hi Nancy,
I did the patch based on 6--2, using the patch capability built into Eclipse (Galileo) with CVS. It's my first patch (yet another first) so it's probably my fault if it didn't work. :) As for the require_once, I saw someone else also point this out in another thread. That is the code that is in 6--2, so I can also include that fix as well. If I can find the other comment I'll give a reply to that one as well. Thanks for the hint!
Hoping to see more activity in Date in the next few weeks, as this is one of the most important modules for me right now. I am also troubleshooting a critical bug with the inability to include multiple Date fields in a single View Filter due to incorrect SQL being created, but no one has chimed in on that yet. If you or anyone you know has experience with the Date SQL API, please let me know or send them over to #592796: Date Filters not working in Views when multiple date fields are used.
I'm excited to be a part of the Drupal developer community and hope to increase my experience and start contributing some of my own modules soon.
Paul
#13
I have plenty of modules you are welcome to help on.
#14
Great! I'd be interested in starting with the RealName module if you've got any work there, since I use it on my site. You've got enough open in RealName so I should be able to find something to work on there. Unfortunately I am not having much luck getting by ZendDebugger or XDebug to work in Eclipse, so things are still a little slow-going... if you know of any good resources talking about debugging PHP and Drupal, it would be greatly appreciated. Let's take that conversation offline from here. You can reach me through my contact tab and I'll share my email address with you there if you'd like.
Thanks!
#15
Subscribing, am here because I was wondering why the repeat dates aren't formatted the same as the start or to date. Obviously it's not straight forward, I would go for a long format i.e. full day (with th, nd, st, rd) names, full month names and full four digit years.
Also the repeat dates are above the From date which doesn't seem right. It ought to be From then Repeat until/Except or To.
#16
like OnlineWD, i too am looking for a way to display the repeat rule _after_ the From date. sounds like you've done a lot of work already, so would be more sensible to implement your patch than to start tweaking the same code myself. is this a piece you have included or could easily add?
what i've got now comes out as
Repeats every week until Tue Nov 17 2009 .
Tuesday, 11/10/09, 6:00pm - 9:00pm
if i were writing it manually, i would choose something more like
2 consecutive Tuesdays, 11/10/09 & 11/17/09, 6:00pm - 9:00pm
but i'd be pleased with
Tuesday, 11/10/09, 6:00pm - 9:00pm
Repeats every Tuesday until 11/17/09
#17
@harpa413, @OnlineWD:
My opinion is that there should be better integration between the start date and the repeat rule, but currently this is not how it is designed. The repeat rule is displayed independently from the actual start date, and it is followed by some number of dates that fall into the schedule, but the number of dates displayed is configurable. I am fairly new to this, but so far I haven't been able to figure out how to actually get access to the start date value. Note that it is NOT the same as the first occurrence in the repeating schedule. Try this: schedule an event for a Tuesday and add a repeat rule occurring every Wednesday. According to the IEEE iCal standards upon which this module is largely based, this is a valid condition, and there is no requirement to force a start date to be on the same date as one of repeating days. Unfortunately, in this example, there is no mention of the Tuesday in the repeat rule. If I had my druthers I'd put a lot more into this, but unfortunately I am up against some design issues that I don't know how to get around. If a co-maintainer of this module were able to assist me, I'd happily take some direction and run with it, but so far I haven't found a way to integrate the start date with the repeat rule, and thus I don't know how to accomplish what you are looking to do. If you want to poke around with the code and see what you come up with, please let me know what you find out!
#18
div.field-type-date div.field-item div {display:none;
}
<?phpfunction themename_date_display_range($date1, $date2, $timezone = NULL) {
return '<span class="date-display-start">'. $date1 .'</span>'.
'<span class="date-display-separator"> until ..<br /></span>' .
'<span class="date-display-end">'. $date2 . $timezone. '.</span>';
}
?>
This does me fine, don't need to read the overview - the list of dates and times is sufficient i.e..
Event dates and times:
Thursday 12:00am, November 11th, 2009 until ..
Monday 12:00am, November 11th, 2009.
Thursday 12:00am, December 12th, 2009 until ..
Monday 12:00am, December 12th, 2009.
Thursday 12:00am, December 12th, 2009 until ..
Monday 12:00am, December 12th, 2009.
#19
Applied the patch from #5 and it appears to be working as expected. For future reference patches should be rolled from the Drupal root directory. Not a big deal though.
Couple of things I did notice.
I think this line
if (!empty($rrule['BYMONTH']) || $rrule['FREQ'] == 'MONTHLY') {should be
if (!empty($rrule['BYMONTH']) && $rrule['FREQ'] == 'MONTHLY') {And the formatting used for some of the big arrays is not consistent with Drupal coding standards. http://drupal.org/coding-standards#array
Thanks.
#20
Thanks Joe for reviewing this. I will give this another look later but I believe the line you referenced is correct as-is. I'll either correct it or add comments to explain it and submit another patch from the Drupal root.
Can you give me an example of what you are referring to? I haven't yet added the Coding module and I'll do that as well. It's been on my to-do list. But I also found some other issues with the existing code (such as potential misuse of the t() function, which was pointed out by another user) and I didn't want to introduce any non-contiguous changes, as per the coding best practices.
Thanks again!
#21
I was referring to the formatting of some of the large mutli-dimensional arrays in the patch. As an example your patch has this hunk:
/*** Helper function for FREQ options.
*/
function FREQ_options() {
- return array(
- 'NONE' => t('-- Period'),
- 'DAILY' => date_t('Days', 'datetime_plural'),
- 'WEEKLY' => date_t('Weeks', 'datetime_plural'),
- 'MONTHLY' => date_t('Months', 'datetime_plural'),
- 'YEARLY' => date_t('Years', 'datetime_plural'),
- );
+ return array('NONE' => t('-- Period'), 'DAILY' => date_t('Days', 'datetime_plural'), 'WEEKLY' => date_t('Weeks', 'datetime_plural'), 'MONTHLY' => date_t('Months', 'datetime_plural'), 'YEARLY' => date_t('Years', 'datetime_plural'));
}
The above function should be formatted as such.
<?phpfunction date_repeat_theme() {
return array(
'NONE' => t('-- Period'),
'DAILY' => date_t('Days', 'datetime_plural'),
'WEEKLY' => date_t('Weeks', 'datetime_plural'),
'MONTHLY' => date_t('Months', 'datetime_plural'),
'YEARLY' => date_t('Years', 'datetime_plural')
);
}
?>
And, it looks like in this case the only change made in your patch is removing this formatting. So unless you're actually changing something in the function this change isn't needed.
With regards to the if statement I mentioned above. Without the change I'm getting a bunch of notices from the foreach loop just below it since it is possible for $rrule['BYMONTH'] to not exist and the loop is still executed. It's working in my situation, haven't had a chance to do any extensive testing yet.
#22
Thanks for the clarification. That formatting was performed by Eclipse's auto formatting. It looked good on first glance but I guess I missed something. Still searching for an IDE which will enforce PHP formatting rules correctly, but so far no luck. I will fix the formatting and test that rule by the end of the week. If I recall correctly there was a reason for the OR condition, but I cannot remember what it was. In any case it could be clearer.
#23
#18 - Thanks for sharing!
I think I need to understand theming a little bit more. Didn't even occur to me to theme my way out of this. Hoping to get a copy of Drupal Pro for Christmas!!