Closed Bug 328073 Opened 20 years ago Closed 19 years ago

recurrence dialog doesn't work well with monthly recurrence

Categories

(Calendar :: Internal Components, defect)

Product:
Calendar
Component:
Internal Components
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: mvl, Assigned: mvl)

References

Details

Attachments

(1 file, 1 obsolete file)

updated patch
16.81 KB, patch
jminta
: first-review+
Details | Diff | Splinter Review
The recurrence dialog ignores the selection made for monthly repeating events.
Attached patch patch v1 (obsolete) — — Splinter Review
First version of the patch. Should mostly work. If nothing else, it's an improvement from what we have now.
Attachment #212616 - Flags: first-review?(jminta)
Comment on attachment 212616 [details] [diff] [review] patch v1 + +function radioGroupSelectItem(radioGroupId, id) +{ My visit url patch brings in applicationUtil.js into lightning and the dialog. Do you think we might be better off doing that here too, so that we can use these same functions that are defined there? + // XXX This code ignores a lot of monthly recurrence rules that + // can come in from external sources. There just is no UI to show them Not just external sources. Right now the dialog allows for repeating on the 23rd, 24th, and 25th days of the month. This would break backwards compatibility and result in dataloss for anyone who has used that feature. We're gonna need to think hard about how to handle that. - if (rule.count == -1) { + if (!rule.isByCount || rule.count == -1) { This conflicts with mickey's patch on Bug 300117. I'm going to try to review that one tonight as well, but depending how that goes, please coordinate with him. + switch (recurtype) { + case 1: This would be a lot easier to read if you gave these cases string values rather than integers, or at least if you added comments out next to them to explain the case we're in. + var el = document.getElementById('monthly-nth-week'); + recRule.setComponent("BYDAY", 1, [el.week*8 + el.day+1]); *8?! I did a double-take when I read that, until I went and looked up icalrecur.c. This definitely wants a comment to explain this encoding. + case 3: + recRule.setComponent("BYDAY", 1, [-(el.day+1)]); This reads like it is intended to be a string with a '-' character and then a number character. I think it'd be better to not abuse js-types and just do [(-1)*(el.day+1)] + document.getElementById("monthly-nth-day").label = window.calendarEvent.startDate.day+"th day of the month"; + + var monthWeekNum = Math.floor(window.calendarEvent.startDate.day / 7) + 1; + var list = ['First', 'Second', 'Third', 'Fourth', 'Fifth']; + var weekdays = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday']; l10n :-( Also, this creates something ugly like '1th day of the month'. English sucks. In general, all of the code you've added to updateDeck needs to move. None of the computations that you're doing will change just because the deck changes. They can all be done when the dialog is loaded. + </hbox> + <spaced flex="1"/> + </vbox> This probably wants to be <spacer>. Also, can you explain briefly what bug you're fixing with this extra <vbox>? + <hbox> + <label id="repeat-numberoftimes-warning" + class="warning-text-class" + value="&newevent.recurnumberoftimes.warning;" + hidden="true"/> + </hbox> Why move this down here? It loses its association with the textbox that it is responding to. This is definitely going to help our recurrence when it lands, but the l10n and dataloss issues are the biggest problems that need to be solved.
Attachment #212616 - Flags: first-review?(jminta) → first-review-
(In reply to comment #2) > + // XXX This code ignores a lot of monthly recurrence rules > that > + // can come in from external sources. There just is no UI to > show them > Not just external sources. Right now the dialog allows for repeating on the > 23rd, 24th, and 25th days of the month. It only pretents it does. There is UI, but it doesn't work. That's the whole point of this bug. Hooking up UI to code, so that the settings are actually saved. > + case 3: > + recRule.setComponent("BYDAY", 1, [-(el.day+1)]); > This reads like it is intended to be a string with a '-' character and then a > number character. I think it'd be better to not abuse js-types and just do > [(-1)*(el.day+1)] Hmm, what i worte is just simple math. I didn't even think about abusing JS types. I'm just trying to create a negative number. > + </hbox> > + <spaced flex="1"/> > + </vbox> > This probably wants to be <spacer>. Also, can you explain briefly what bug > you're fixing with this extra <vbox>? iirc, alignment. But i need to double-check. > + <hbox> > + <label id="repeat-numberoftimes-warning" > + class="warning-text-class" > + value="&newevent.recurnumberoftimes.warning;" > + hidden="true"/> > + </hbox> > Why move this down here? It loses its association with the textbox that it is > responding to. The radiobuttons looked really ugly, with a random gap in it. I'm trying to come up with a better solution, but i didn't want to put too much in this patch. > This is definitely going to help our recurrence when it lands, but the l10n and > dataloss issues are the biggest problems that need to be solved. Like i said, there is no (internal) dataloss.
*** Bug 312513 has been marked as a duplicate of this bug. ***
Component: Sunbird and Calendar-Extension Front End → Base
*** Bug 330520 has been marked as a duplicate of this bug. ***
Attached patch updated patch — — Splinter Review
Patch updated to review comments. I still have the error box at the bottom of the dialog, because it looks really bad when it is in the middle of a radiobutton group. It will create some weird empty space. It's now next to the OK button, where most people will just have clicked. More likely to grab attention.
Attachment #212616 - Attachment is obsolete: true
Attachment #216241 - Flags: first-review?(jminta)
Comment on attachment 216241 [details] [diff] [review] updated patch + // Set label to 'second week of the month' + var monthWeekNum = Math.floor(window.calendarEvent.startDate.day / 7) + 1; + nthstr = props.GetStringFromName("recur"+(monthWeekNum-1)); You add 1 and then subtract 1? This is currently giving me the wrong word-ordinal. I get 'Third Saturday' when it really is the fourth Saturday. The internal logic, when you hit OK also treats it as the Fourth, so I think the -1 just wants to be dropped. + str = props.formatStringFromName("recurNthWeek", [nthstr, daystr], 2); + document.getElementById("monthly-nth-week").label = str; + // Set two values needed to create the real rrule later + document.getElementById("monthly-nth-week").day = window.calendarEvent.startDate.weekday; + document.getElementById("monthly-nth-week").week = monthWeekNum; + Some whitespace to break up these two blocks would be nice. + var monthLength = window.calendarEvent.startDate.endOfMonth.day; + var isLastWeek = (monthLength - window.calendarEvent.startDate.day) < 7; + document.getElementById("monthly-last-week").hidden = !isLastWeek; + str = props.formatStringFromName("recurLast", [daystr], 1); + document.getElementById("monthly-last-week").label = str; + document.getElementById("monthly-last-week").day = window.calendarEvent.startDate.weekday; There's no sense in taking the time to compute/set these labels if the element is hidden. Can you put this in an |if (isLastWeek)| block? + // If this is the last week of the month, set label to 'last week of the month' You don't want to say "last week', because it's actually the last [weekday] of the month. + if (days.length > 0 && days[0] > 0) { + radioGroupSelectItem("monthly-type", "monthly-nth-week"); + } + if (days.length > 0 && days[0] < 0) { + radioGroupSelectItem("monthly-type", "monthly-last-week"); + } Minor perf: Why not pass in a named object to getComponent, so that you don't make the array compute its length? + el = document.getElementById('monthly-last-week'); + dump("day: "+el.day+" byday: "+((-1)*(Number(el.day)+1))+"\n"); + recRule.setComponent("BYDAY", 1, [(-1)*(8+Number(el.day)+1)]); I don't think we need the dump to be checked in. Also, can you point to the source of this magic formula too please? +# For the recurrence dialog +recur1=First +recur2=Second +recur3=Third +recur4=Fourth +recur5=Fifth These are pretty general purpose words that could be used elsewhere, so I'd rather not see them be called 'recur'. +recurNthWeek=%1$S %2$S of the month +recurLast=Last %1$S of the month Can you include a comment here to localizers to tell them what generally will be substituted in for these variables (ie. something from recurX or something from ordinalX and then something from dayX.) +ordinal1=1th Most English numbers that end in 1 have a -st ending, instead of -th. (Same for 21, 31, not 11.) +ordinal22=22th 22nd. This is awesome. r=jminta with those changes.
Attachment #216241 - Flags: first-review?(jminta) → first-review+
*** Bug 326980 has been marked as a duplicate of this bug. ***
patch checked in.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
There is no WINDOWS version in net. Where can I get one?
See bug 331702. Please be patient.
*** Bug 332371 has been marked as a duplicate of this bug. ***
*** Bug 331757 has been marked as a duplicate of this bug. ***
The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!
QA Contact: sunbird → base
*** Bug 346928 has been marked as a duplicate of this bug. ***
Is there now a windows version of the patch and where can I get it?
In the latest version you still cannot create a date like this: on 2nd Sunday in June, recurrence: once in a year. An I wrong? This was possible in version 0.2
(In reply to comment #18) > In the latest version you still cannot create a date like this: > on 2nd Sunday in June, recurrence: once in a year. This is outside the scope of this bug. Please open a new bug to expose the monthly recurrence options for yearly recurrences.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: