Closed
Bug 388418
Opened 17 years ago
Closed 17 years ago
[Proto] can not create 'Last day of the month' rule with new edit dialog
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
VERIFIED
FIXED
0.7
People
(Reporter: andreas.treumann, Assigned: michael.buettner)
Details
(Keywords: late-l10n)
Attachments
(3 files, 2 obsolete files)
3.17 KB,
patch
|
sipaq
:
review+
|
Details | Diff | Splinter Review |
8.72 KB,
patch
|
michael.buettner
:
review+
michael.buettner
:
ui-review+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
Details | Diff | Splinter Review |
STEPS TO REPRODUCE: =================== - create a new event at the last day of the month - open the 'edit event' dialog - open the custom recurrence dialog - choose 'monthly' - it's not possible to create a 'Last day of the month' rule RESULT: ======= - it's not possible to create a 'Last day of the month' rule EXPECTED RESULT: ================ - same with the old edit dialog works. REPRODUCIBLE: ============= - always
Comment 1•17 years ago
|
||
Using Sunbird 0.5 it was possible to create events/tasks that repeat on the 'Last day of the month'. Using Sunbird 0.7pre it is not possible to correctly view or edit this events: The Event Dialog incorrectly shows the repeatition rule as "Occurs day -1 of every month effective [...]". Opening the Recurrence Dialog throws the following error: Error: days[val[i] - 1] has no properties Source File: chrome://calendar/content/sun-calendar-event-dialog-recurrence-datepicker.xml Line: 251 --> dataloss, requesting blocking‑calendar0.7
Flags: blocking-calendar0.7?
Assignee | ||
Updated•17 years ago
|
Assignee: christian.jansen → michael.buettner
Comment 3•17 years ago
|
||
If we extend the "The:" drop down lists by the items marked with "->" it should be possible to cover the following two combinations: * Fifth Sunday of the month * Last day of the month The: First Second Third Fourth -> Fifth -> Last Sunday Monday Tuesday Wednesday Thursday Friday Saturday -> Day of the month
Assignee | ||
Comment 4•17 years ago
|
||
Unfortunately we don't have the strings to get this going. First of all, the dialog gets the strings from [1], where 'Fifth' is missing. I searched through the existing strings and found [2], but these are in a properties files which I can only use by introducing an ugly hack (inline javascript in the xul file). Second, there's no such string as 'day of the month' (without any decoration, that is). As far as I can see we have two options here: 1) Break string freeze and get it in. 2) Land a patch that gracefully handles rules that we currently can't display before 0.7 and add the rest of the patch after the release is out of the door. Daniel, that's up to you. I'm postponing this bug until a decision has been made. [1] http://lxr.mozilla.org/mozilla1.8/source/calendar/locales/en-US/chrome/prototypes/sun-calendar-event-dialog.dtd#222 [2] http://lxr.mozilla.org/mozilla1.8/source/calendar/locales/en-US/chrome/calendar/dateFormat.properties#133
Comment 5•17 years ago
|
||
In my opinion this dataloss bug justifies a late-l10n checkin.
Comment 6•17 years ago
|
||
IMO "fifth" occurrence is rather exotic, but I agree we should be able to express last day of month. Thus this patch is for late-l10n and should go in ASAP. I hope I haven't missed anything...
Attachment #279957 -
Flags: review?(bugzilla)
Comment 7•17 years ago
|
||
(In reply to comment #4) > 2) Land a patch that gracefully handles rules that we currently can't display > before 0.7 and add the rest of the patch after the release is out of the door. IMO this is independent from the 1), and makes sense, too, since we still cannot express every rule.
Comment 8•17 years ago
|
||
(In reply to comment #4) > Second, there's no such string as 'day of the month' (without any decoration, > that is). That's because you should not use that. See bug 338167. It's generally not good to try to compose sentences from standalone words. That's likely to lead to localization problems.
Comment 9•17 years ago
|
||
(In reply to comment #8) I agree, I could imagine some problems, too, e.g. with regards to {"first", "second", ...} vs. "last" which may not fit the same order for all those phrases. But IMO we shouldn't spend work into that unless the l10ners force us to do so.
Comment 10•17 years ago
|
||
sipaq, what's the status on this? IMO we should check this in ASAP, so l10ners could start their work.
Comment 11•17 years ago
|
||
Comment on attachment 279957 [details] [diff] [review] missing strings r=sipaq Please add the late-l10n keyword to this bug, CC calendar-l10n@mozilla.bugs and post about the string freeze break in mozilla.dev.l10n if you decide to check this in before 0.7 is out.
Attachment #279957 -
Flags: review?(bugzilla) → review+
Comment 12•17 years ago
|
||
Checked strings in on HEAD and MOZILLA_1_8_BRANCH.
Keywords: late-l10n
Target Milestone: --- → 0.7
Comment 13•17 years ago
|
||
(In reply to comment #9) > But IMO we shouldn't spend work into that unless the l10ners force us > to do so. They already forced us. See bug 338167. In some languages, there are differences in gender for the different days.
Comment 14•17 years ago
|
||
(In reply to comment #13) Right, the event dialog / event summary dialog have the problem with that; should be fixed in a follow-up bug (post 0.7).
Assignee | ||
Comment 15•17 years ago
|
||
This patch adds the two new recurrence rules to the UI. In general, I find it extremely unfortunate to add the 'day of the month' part to the 'The first/second/.../last Monday/Tuesday/...' rule. First, this calls for trouble in terms of localization. Second, it allows to create a rule that will be displayed in the monthday table (second option for monthly) as the rules will be ambiguous. Third, the solution is not general, why don't allow for x 'to the last day of the month' rules? On the contrary we allow for arbitrary days, this is not consistent. Second, the necessary strings for the natural language pattern are missing. I just bail out and don't display anything in that case. I added a comment to that part as we need to add this after 0.7 is out of the door.
Attachment #280601 -
Flags: ui-review?(christian.jansen)
Attachment #280601 -
Flags: review?(daniel.boelzle)
Comment 16•17 years ago
|
||
Comment on attachment 280601 [details] [diff] [review] patch v1 UI+= Christian
Attachment #280601 -
Flags: ui-review?(christian.jansen) → ui-review+
Assignee | ||
Comment 17•17 years ago
|
||
I just had a lengthy discussion with daniel and christian on this topic and I just would like to write down what we agreed on. We're aware that we're currently facing some localization issues. Those will be ironed out after 0.7 has been released. For the time being we take this patch as it currently stands. We also have some issues with the way the rules are constructed. As there's basically no time left in the 0.7 timeframe we need to address this until the 0.9 release. Basically we need to revise the UI for how the rules get constructed. Last but not least, I just didn't see that Daniel added the missing string that is necessary for displaying the 'last day of the month'-rule as natural language pattern. I'll come up with a new version of this patch that addresses this issue.
Comment 18•17 years ago
|
||
actually, there's quite a lot of stuff that currently hurts localization of Calendar. Maybe this should be filed into separate bugs and tracked by a meta bug?
Assignee | ||
Comment 19•17 years ago
|
||
(In reply to comment #18) > actually, there's quite a lot of stuff that currently hurts localization of > Calendar. Maybe this should be filed into separate bugs and tracked by a meta > bug? It would be much appreciated if new bugs are getting filed in case localization problems are encountered. Currently, I'm aware of Bug 359353 but there are certainly more such issues which need to be addressed. I suggest to track such bugs with a prefix in the bug description (such as [l10n], for example) or add this as a tag to the whiteboard.
Assignee | ||
Comment 20•17 years ago
|
||
What was missing from my last patch was the support for the display of the recurrence rule as plain text, since I missed that Daniel already checked in the new string 'last' right before string freeze. Unfortunately, this doesn't work as I just have the following format string: 'the %1$S %2$S of every month' I could obviously use 'last' for %1, but I don't have anything to be substituted for %2 ('day') is missing. In this patch I added what was missing, namely the following format strings: 'the last day of the month' 'the last day of every other month' 'the last day of every %1$S month' Of course this breaks string freeze, which I don't think is worth while the hassle. As far I see we have the following array of options: 1) Release 0.7 without supporting the 'last day of the month' rule. The dialog throws an exception in that case, but it's not a tragedy, since the item won't loose any data. It's just that the recurrence dialog won't work as expected in that case. Don't do anything to the plain text display, this will result in a string: day -1 of every month. 2) Same as one, but don't display anything as plain text in the 'last day of the month' case. 3) Take my first patch, this supports the rule but doesn't display anything as plain text. 4) Take this patch, this supports the rule and displays the correct plain text but breaks string freeze.
Attachment #280718 -
Flags: review?(daniel.boelzle)
Comment 21•17 years ago
|
||
We *cannot* break the string freeze so shortly before the RC1, as this will increase the risk of locales not being ready for RC1 inclusion (especially relevant for Lightning). What we could do, is breaking the string freeze after the RC1 in the four week period before the final release. That would give localizers enough time to fix their locales for the final 0.7 release.
Status: NEW → ASSIGNED
Comment 22•17 years ago
|
||
Comment on attachment 280601 [details] [diff] [review] patch v1 We take the first patch, i.e. option 3: > 3) Take my first patch, this supports the rule but doesn't display anything as > plain text. IMO we shouldn't break string freeze anymore, even not for final 0.7. I think we could go without clear text (although a little ugly in the read-only dialog). r=dbo
Attachment #280601 -
Flags: review?(daniel.boelzle) → review+
Comment 23•17 years ago
|
||
follow-up fix needed for, e.g. - set last monday of month and save - reopen dialog (clear text fine) - recurrence dialog says last sunday
Updated•17 years ago
|
Attachment #280718 -
Flags: review?(daniel.boelzle)
Assignee | ||
Comment 24•17 years ago
|
||
This patch is the one I'm going to check in, it also fixes the problem you mentioned in the previous comment.
Attachment #280892 -
Flags: ui-review+
Attachment #280892 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #280601 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Attachment #280718 -
Attachment is obsolete: true
Assignee | ||
Comment 25•17 years ago
|
||
patch checked in on trunk and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•17 years ago
|
||
This add-on patch adds the missing bits and pieces to display the 'Last day of the month' recurrence rule as clear text.
Reporter | ||
Comment 27•17 years ago
|
||
Checked in Lightning (build 2007091606) and Sunbird (build 20070916) -> VERIFIED
Status: RESOLVED → VERIFIED
Comment 28•17 years ago
|
||
(In reply to comment #23) Were the follow up bugs already filed?
Comment 29•17 years ago
|
||
In 0.7final, with the restored "last day of month" option, if I create a 3-monthly repeating event, it fires on last day of the wrong months. as per https://bugzilla.mozilla.org/show_bug.cgi?id=386516
Comment 30•17 years ago
|
||
Mickey, have you filed the follow up bugs? Has the "add-on patch" to be checked in?
Version: Trunk → unspecified
You need to log in
before you can comment on or make changes to this bug.
Description
•