Closed Bug 388418 Opened 13 years ago Closed 12 years ago

[Proto] can not create 'Last day of the month' rule with new edit dialog

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: andreas.treumann, Assigned: michael.buettner)

Details

(Keywords: late-l10n)

Attachments

(3 files, 2 obsolete files)

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
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?
we need to fix that
Flags: blocking-calendar0.7? → blocking-calendar0.7+
Assignee: christian.jansen → michael.buettner
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
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
In my opinion this dataloss bug justifies a late-l10n checkin.
Attached patch missing stringsSplinter Review
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)
(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.
(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.
(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.
sipaq, what's the status on this? IMO we should check this in ASAP, so l10ners could start their work.
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+
Checked strings in on HEAD and MOZILLA_1_8_BRANCH.
Keywords: late-l10n
Target Milestone: --- → 0.7
(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.

(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).
Attached patch patch v1 (obsolete) — Splinter Review
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 on attachment 280601 [details] [diff] [review]
patch v1

UI+= Christian
Attachment #280601 - Flags: ui-review?(christian.jansen) → ui-review+
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.
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?
(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.
Attached patch alternative patch v1 (obsolete) — Splinter Review
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)
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 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+
follow-up fix needed for, e.g.
- set last monday of month and save
- reopen dialog (clear text fine)
- recurrence dialog says last sunday
Attachment #280718 - Flags: review?(daniel.boelzle)
Attached patch final patchSplinter Review
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+
Attachment #280601 - Attachment is obsolete: true
Attachment #280718 - Attachment is obsolete: true
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached patch add-on patchSplinter Review
This add-on patch adds the missing bits and pieces to display the 'Last day of the month' recurrence rule as clear text.
Checked in Lightning (build 2007091606) and Sunbird (build 20070916) -> VERIFIED 
Status: RESOLVED → VERIFIED
(In reply to comment #23)
Were the follow up bugs already filed?
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

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.