410427 - Event Dialog: Allow setting recurrence "UNTIL" without custom dialog

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

When creating a recurring event, you need to set it to be 'custom' if you want to create a limited number of recurrences. But that's the most common case. It's quite uncommon to have it not limited (the only thing I can think of is birthdays)
Limiting recurrence should be easier to find.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Oops, sorry, my bad, returning to the right component, awfully sorry about stomping your target milestone, please send hatemail to this address :(

Comment 2

[Pete Riley]

> When creating a recurring event, you need to set it to be 'custom' if you want
> to create a limited number of recurrences.

I would also like a solution that doesn't force us to use the Customize dialog 90% of the time.  Perhaps there could be a second dropdown that's called "Until" that has a date picker.

And if I want to use the Customize dialog (for a different reason), then the second dropdown ("Until") could be replaced with the blue summary text (so this behavior wouldn't change).

Comment 4

[Decathlon]

Attachment: Screenshot - Proposal with a checkbox and a datepicker

I have a patch, not completed yet, that works more or less as described in comment #2 (see screenshot). Before I try to complete it, could you give me an UI review and any opinion about this solution (to anyone interested in this bug)?

Since the most common case is to set a limited number or recurrences, the checkbox "Until" is already checked when a recurrence type is selected in the "Repeat" drop down menu, so, the user has only to select a date in the datepicker.
When the entry "custom" is selected, the description rule text appears in the dialog instead of the checkbox + datepicker unless the rule set in the Edit Recurrence dialog can be described only with the "until" datepicker. This last case happens only for daily and "every workday" rules, so, only for this two rules, when the user returns form "Edit Recurrence" dialog, the checkbox + datepicker are showed instead of the description rule.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

I like the ui! nice!
To make it even easier to use, maybe you could add a 'forever' option in the drop-down, and remove the checkbox? I find the checkbox a bit hard to read. What exactly does it check?

Repeat [weekly [v]] until [26/07/2010 [v]]

Repeat [weekly [v]] until [forever    [v]]
 (not sure if that's gramatically correct. Alternativly, use 
                    until [the end of times [v]])

Comment 6

[Decathlon]

(In reply to comment #5)

> I find the checkbox a bit hard to read.
> What exactly does it check?

It enables the datepicker where you can select the until date.
If checked, the datepicker is enabled and the showed date is the until date of the rule (second and fourth screenshot); if unchecked, the datepicker is disabled and the rule doesn't have a until date, so it's a "forever" rule (third screenshot).
Though, I agree that this last case could be a bit more clear. Your proposal is a possibility (I hope I will able to do it), another could be make the datepicker disappear if the checkbox is not checked.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 456166 [details]
Screenshot - Proposal with a checkbox and a datepicker

This looks awesome, nice work!

I have two suggestions

* hide the '[x] until [ date picker ]' when there is no repeat
* on forever perhaps hide the date picker in favor of a 'forever' label or fill in the date picker text with 'forever'

Otherwise it's great.  I'll give a ui-r+ for now since it looks good but if you have an updated version please set the ui-review for that as well.  Thanks.

Comment 8

[Decathlon]

Attachment: WIP patch - "forever" option inside the datepicker

(In reply to comment #7)

> I have two suggestions
> 
> * hide the '[x] until [ date picker ]' when there is no repeat

Done.

> * on forever perhaps hide the date picker in favor of a 'forever' label or fill
> in the date picker text with 'forever'

In this WIP patch I've tried the second solution (see the next screenshot) i.e. as proposed by Michiel van Leeuwen in comment #5.
The patch is, let's say, 80% working, a part from eventual bugs, it lacks only the validity check of the until date, but, other than testing the behavior, I also ask for feedback because I would like to know whether the way I've done the implementation of the menu item in the datapicker (an extended datepicker) could be right or it would be better something simpler.

Alternatively (suggested by Michiel van Leeuwen) we can use two radio buttons, one for the datepicker and the other for the "forever" option, something like this:

  Repeat:  [weekly   |v|]    Until:  (o) [dd/mm/yyyy   |v|]
                                     (o) Forever

but it needs more controls on the dialog.

Comment 9

[Decathlon]

Attachment: Screenshot from the WIP patch

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 502334 [details] [diff] [review]
WIP patch - "forever" option inside the datepicker

I like the way you extended the datetimepickers binding. The code from the update method looks copy&pasted. If so, is it possible to refactor that code out?

You could either try calling this.__proto__.__proto__.update.apply(this, arguments); and if that doesn't work, refactor the code in the base binding to a separate method you can call.

Personally I'd prefer calling it "forever" instead of "no end date". Also I think making that string part of the dropdown like you are doing it now is better UI.

Comment 11

[Decathlon]

Philipp, what should be the target milestone for this bug?


(In reply to comment #10)
> Comment on attachment 502334 [details] [diff] [review]
> WIP patch - "forever" option inside the datepicker
> 
>  The code from the
> update method looks copy&pasted. If so, is it possible to refactor that code
> out?

Actually there is a different line:

  if (this.mValue != null && this.mValue != "forever") {
instead of 
  if (this.mValue != null) { 

anyway I found another solution
 
> Personally I'd prefer calling it "forever" instead of "no end date".

I've used "no end date" because is the sentence used in the Edit Recurrence dialog, but I agree that "forever" is better, also because it can be written  faster in the datepicker.

Comment 12

[Decathlon]

Attachment: patch -v1

I don't know when this bug could be scheduled, I think it's late for the beta3, anyway this patch seems ready so I attach it.

Besides the general behavior related to this bug (make a new repeating event or open an event that already exists), the patch requires to verify:
- the behavior related to the Edit Recurrence Dialog (the "custom..." option
  in the repeat drop down menu). Some settings ("day" and "every workday")
  with the until date, should show the until datepicker instead of the text
  description.
- general functioning of the until datepicker: select a date or the "forever"
  option in particular editing with the keyboard;
- until date validity check. In particular the behavior of the warning dialog
  related to the "Save Event" dialog:
  open the dialog, write with the keyboard a forbidden until date then,
  without other actions, save (CTRL+S), or close (CTRL+W) or Save and close
  (CTRL+L) the dialog. Do the warning dialogs behavior correctly?
 
Another thing that requires attention and that I haven't touched with this patch, is what happens when the user changes the start date of a repeating event. Lightning also changes the until date in order to keep the same difference between start and until date but this doesn't happen in the dialog until it stays open. You can see that, when saving the event with CTRL+S, the until date doesn't change in the dialog but in the view you can see an updated set of events.
At the moment the behavior is the same with the recurrence details text where the text doesn't change with the start/end date. I haven't found a specific bug.
 
I've added the access key to the until-datepicker, but it doesn't work. No key seems working even when I make free another key already used for another working control (actually the same happens for the start date and the end date datepickers). Do you have any suggestion?

I've uploaded a build for Windows here:
 http://www.mediafire.com/file/lendb29f7aqfrh5/lightning1.0b3pre-20110209-Bug410427-Win.xpi
I'd appreciate if someone could try it a bit.

Comment 13

[Decathlon]

Comment on attachment 512025 [details] [diff] [review]
patch -v1

I have to retract the review because Markus Adrario tested the patch and he found an error that requires a change.
Other things he pointed out, can be discussed here:
1. a different look for the "forever" option below the minimonth in the
   datepicker (a button, a central position, or in general a prominent look for
   the text);
2. the ability to show in the datepicker e.g. only the year when the recurrence
   is "yearly", the month for a "monthly" recurrence ...

Thanks for the test Markus.

Comment 14

[Decathlon]

Attachment: [after beta][l10n]patch - v2

I've fixed the error that discovered Markus, so I ask for review. Please also read notes in the first review request (comment 12).

Another thing that Markus pointed out is the following: with a repeating rule selected in the dropdown list (e.g. weekly) and "Forever" as until date, what selected date should be showed in the minimonth when clicking on the datepicker?
Another similar doubt: when the rule is "Custom" with a "repeat n times" recurrence and the user selects another rule e.g. weekly, what should be the new date showed in the datepicker?
For now I set the *Start date* of the event for both cases, but maybe the selected day (if not earlier that the start date) or the last occurrence date would be better?

I've also tried to make the "Forever" label's look more prominent with a bold text and a central position. In the patch I've adopted the third option shown in the following screenshot. Instead, the first image is without any style. What's your opinion?

For the request to directly type/show the year or the month name in the datepicker in order to make simpler the input/read of certain recurrence, I think it should be done in another bug. At first glance it would seem that it needs a lot of work and it would need beforehand some decision about the way it should work (for example: only showing a month/year or also allow to enter text interpretable as month/year? Moreover, typing with the keyboard or make the months and years selectable with the buttons in the minimonth?).

Comment 15

[Decathlon]

Attachment: Screenshot: "Forever" label

Different look for the label "Forever"

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 515061 [details] [diff] [review]
[after beta][l10n]patch - v2

Review of attachment 515061 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=philipp

::: calendar/resources/content/datetimepickers/datetimepickers.xml
@@ +337,5 @@
>          ]]>
>        </destructor>
>  
> +      <!-- This property will be overlayed in some extended binding -->
> +      <property name="_mValueValid">

properties without a setter should have readonly="true"

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Decathlon from comment #14)
> Created attachment 515061 [details] [diff] [review]
> [after beta][l10n]patch - v2
> 
> I've fixed the error that discovered Markus, so I ask for review. Please
> also read notes in the first review request (comment 12).
> 
> Another thing that Markus pointed out is the following: with a repeating
> rule selected in the dropdown list (e.g. weekly) and "Forever" as until
> date, what selected date should be showed in the minimonth when clicking on
> the datepicker?
Can you make it so that no date is shown? Would that make sense?


> I've also tried to make the "Forever" label's look more prominent with a
> bold text and a central position. In the patch I've adopted the third option
> shown in the following screenshot. Instead, the first image is without any
> style. What's your opinion?
I could imagine that it makes sense to give the forever field some sort of border that makes it look selected and at the same time reduce the opacity of the datepicker control slightly.

When the datepicker is selected, then put a border around there and make the forever field slightly less opaque.

You could also add some hover styles that bring back some of the opacity to make it clear to the user that that area can be clicked on to "enable".

I'll leave this up to andreasn though.

> For the request to directly type/show the year or the month name in the
> datepicker in order to make simpler the input/read of certain recurrence, I
> think it should be done in another bug. At first glance it would seem that
Agreed.

Comment 18

[Andreas Nilsson (:andreasn)]

Looks like this patch fails to apply cleanly on trunk.

Comment 19

[Decathlon]

Attachment: patch-v2.1 trunk

(In reply to Andreas Nilsson (:andreasn) from comment #18)
> Looks like this patch fails to apply cleanly on trunk.

This one should apply on trunk. Nothing but the changes to make it working on trunk.

Comment 20

[Andreas Nilsson (:andreasn)]

Comment on attachment 515064 [details]
Screenshot: "Forever" label

In general, I like this patch!
* Central placed forever label looks like it's a label for the content above somehow and all the other labels in this dialog goes to the left and has normal font-weight, so I would prefer that for this dropdown as well.
* When I select, for example repeat weekly it defaults the date to todays date so it appears this needs a better default.
* On Linux it looks like the calendar part of the menu and the forever part of the menu have different kinds of borders and I think that is due to the appearance of calendars in these dropdowns in general, so I'll file another bug about that.

So marking minus for now due to the 2 issues above, but I like the direction!

Comment 21

[Decathlon]

Attachment: patch - v3

(In reply to Andreas Nilsson (:andreasn) from comment #20)
> * Central placed forever label looks like it's a label for the content above
> somehow and all the other labels in this dialog goes to the left and has
> normal font-weight, so I would prefer that for this dropdown as well.

I've done int that way, moreover I've given to the "Forever" label the same look of the days in the minimonth for hover, selected and hover+selected status. When "Forever" is selected, no date appears selected in the minimonths apart from the case when the user clicks the today button in the minimonth (don't know whether this should be changed, but it requires to change the minimonth's implementation and eventually I'd prefer do it in another bug).

> * When I select, for example repeat weekly it defaults the date to todays
> date so it appears this needs a better default.

Done. Now when generating a new event, the proposed until-date is coherent with the rule type and creates in total five(?) repetitions of the rule included the first one.
Moreover when converting from a "custom" rule with "n times" recurrence, the proposed date now is the date of the last occurrence.

(In reply to Philipp Kewisch [:Fallen] from bug 702613 comment #4)
> 
> Unless I missed this already happening, could you make the save button be
> disabled when the warning is shown? Also, we should think about what kind of
> feedback the user gets when he tries to close the dialog, gets the save/dont
> save/cancel dialog and then selects save. I assume the dialog just stays
> open then? I think ideally, the warning should flash somehow (either by
> making it go away and come back, or a real visual flash). But maybe that is
> too much for this bug.

Now the Save button is disabled when the warning dialog is showed but it's only a look issue because the user can't interact with it since the warning dialog (modal) is showed.
The global behavior of the dialog is the same. In particular, when the user closes the dialog after _typed_ an earlier until/end date, what happens is:

the "Save event" dialog (modal) appears with the information that the event has non been saved. It proposes to Save/Don't Save/Cancel. User clicks on:
- "Don't save"  -> the "Save event" dialog gets closed and the main dialog
                   too (the user want to get rid of the dialog);
- "Save"        -> the "Save event" dialog gets closed but immediately 
                   appears the warning dialog that notifies a wrong
                   until/end date (the user want to save the event, but in
                   this case he has to be notified that the input date was
                   wrong);
- "Cancel"      -> as above (the user want to continue to edit the event
  
This is the same behavior of the Hotmail calendar (web application).
Google calendar uses a different approach because it highlights the datepicker with a red color background without any warning dialog unless you try to save (for the until-date, instead, Google calendar's behavior has a bug). I think you want to do something similar to Google (as I read in bug 350463 comment 4) but would be better address it in another bug.

IMHO, in general, to avoid to input an earlier date, would be useful a particular style for the day of the _start_ date in the datepicker's minimonth (e.g. a dotted border), or better, all the days earlier the start date should be disabled. When the date is typed with the keyboard, it would need a particular style or color in the datepicker or a warning icon displayed as long as the typed date is not correct.


An important _unresolved_ issue is that the Event Dialog doesn't update the displayed until-date when the user changes the start date (as it does with the end date). The until-date gets updated in the rule only when the event is being saved, but still the displayed date in the dialog doesn't change (this is the actual Lightning's behavior as you can see it in the repeat text link with a "custom" rule with an until-date).
Doing so, the user might think to create the event with an until-date that is different from the stored one.
In this patch I've not changed that because it requires to keep in count the automatic update of the until-date with the start date (function onStartDateChanged()), and this should be done along with a fix for bug 566149 because the two things are strictly related.
I think I have found a way to fix bug 566149 but first I need some informations about how to implement it correctly, so I would prefer to sort out this issue in that bug, also to avoid adding too many things here.

Sorry for the length of this comment.

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Decathlon from comment #21)
> Created attachment 673677 [details] [diff] [review]
> patch - v3
> 
> (In reply to Andreas Nilsson (:andreasn) from comment #20)
> > * Central placed forever label looks like it's a label for the content above
> > somehow and all the other labels in this dialog goes to the left and has
> > normal font-weight, so I would prefer that for this dropdown as well.
> 
> I've done int that way, moreover I've given to the "Forever" label the same
> look of the days in the minimonth for hover, selected and hover+selected
> status. When "Forever" is selected, no date appears selected in the
> minimonths apart from the case when the user clicks the today button in the
> minimonth (don't know whether this should be changed, but it requires to
> change the minimonth's implementation and eventually I'd prefer do it in
> another bug).
I have a minor issue on mac. When a date is selected and you hover the forever label, a border appears but the text disappears. This is due to this mac rule:

menu[_moz-menuactive="true"],
menuitem[_moz-menuactive="true"] {
  color: -moz-mac-menutextselect;
  background-color: Highlight;
}

You are going to have to overwrite the colors for this rule in the mac-specific theme code, or you can also do it generally since it probably won't harm the other Platforms. I think the consensus is that its always a good idea that if you set color then also set background-color and vice versa.



> > 
> > Unless I missed this already happening, could you make the save button be
> > disabled when the warning is shown? Also, we should think about what kind of
> 
> Now the Save button is disabled when the warning dialog is showed but it's
> only a look issue because the user can't interact with it since the warning
> dialog (modal) is showed.
Ah, I think I wrongly assumed its not a modal dialog but some sort of inline message. I was probably thinking of the future where we have a notificationbar in the dialog :-) Since you invested work to change it, lets go with that.

> Sorry for the length of this comment.
No problem, sorry for taking so long to review this.

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 673677 [details] [diff] [review]
patch - v3

Review of attachment 673677 [details] [diff] [review]:
-----------------------------------------------------------------

Finally, r=philipp :-) Just a few minor nits here.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +874,5 @@
> +        }
> +    }
> +    document.getElementById("repeat-deck").selectedIndex = 0;
> +    setElementValue("repeat-until-datepicker",
> +                    hasUntilDate ? gUntilDate.getInTimezone(floating()).jsDate : "forever");

Please use cal.floating() instead. 

Also, wrapping is a bit strange here, maybe its just splinter review though. If possible maybe you can split this into more lines and use variables, but its also fine if you leave it that way.

@@ +2252,5 @@
> +            // the entry-date. the 'disabled' state will be
> +            // revoked if the user turns off the repeat pattern.
> +            disableElementWithLock("todo-has-entrydate", "repeat-lock");
> +        }
> +    };

No semicolon needed here. Also, maybe you can put setUpEntrydateForTask before the let definitions, I recall there being a strict warning if functions are not either toplevel or directly at the start of another function (I find that warning is hokum, but whatever :)

@@ +2337,5 @@
> +                    if (dates) {
> +                        lastOccurrenceDate = dates[dates.length - 1];
> +                    }
> +                    setElementValue("repeat-until-datepicker",
> +                                    (lastOccurrenceDate || proposedUntilDate).getInTimezone(floating()).jsDate);

cal.floating()

@@ +2385,5 @@
>                break;
>          }
>  
> +        if (newUntilDate) {
> +            setElementValue("repeat-until-datepicker", proposedUntilDate.getInTimezone(floating()).jsDate);

cal.floating()

@@ +3482,5 @@
> +    if (untilDate.compare(startDate) < 0) {
> +        // Restore the previous date. Since we are checking an until date,
> +        // a null value for gUntilDate means repeat "forever".
> +        setElementValue("repeat-until-datepicker",
> +                        gUntilDate ? gUntilDate.getInTimezone(floating()).jsDate

cal.floating()

@@ +3485,5 @@
> +        setElementValue("repeat-until-datepicker",
> +                        gUntilDate ? gUntilDate.getInTimezone(floating()).jsDate
> +                                   : "forever");
> +        gWarning = true;
> +        let callback = function() {

Either: function callback() {...}

or: let callback = function() {...};

(semicolon)

Comment 24

[Decathlon]

Attachment: patch - v3 with adjustments

Changes requested in the previous comment.

Philipp, could you please check whether the changes in the file datepickers.css for the MAC need are fine?
I've also added the css rule for the "active" status (the same as minimonth's style) for the "Forever" label that I had missed.

If it's fine for you I'm going to check-in this patch.

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 687738 [details] [diff] [review]
patch - v3 with adjustments

Putting in my queue to test mac, remind me if I don't do this by tomorrow.

Comment 26

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #25)

> Putting in my queue to test mac, remind me if I don't do this by tomorrow.

Philipp? ;-)

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 687738 [details] [diff] [review]
patch - v3 with adjustments

Sorry for the delay. I've tested this on mac and only found one issue (which may also be on other platforms), when you hover the Forever label then the popup size jumps by 2px. If you add the following rule to .datepicker-text-menuItem-class then all is well:

border: 1px solid transparent;

Comment 28

[Decathlon]

(In reply to Philipp Kewisch [:Fallen] from comment #27)

> Sorry for the delay.
No problem. ;)

>  If you add the following rule to
> .datepicker-text-menuItem-class then all is well:

Added in the pushed patch.

Pushed to comm-central changeset 0701728ff277

Comment 29

[Stefan Sitter [:ssitter]]

I got confused because this patch changes the known default behavior from previous Lightning and Sunbird releases. Previously repeating events would repeat forever. Now it repeats only until a certain date that appears to be random (although it follows a rule as mentioned in comments above).

Therefore the user now always needs additional actions to get to the known result. Shouldn't the default behavior be kept as is and the patch should only make it easier to limit the repetition range if necessary?

Comment 30

[Decathlon]

The patch proposes a default until-date that generates five occurrences for every recurrence type, that is the same default value in the Recurrence dialog for the "Create ... appointments" option (initially I proposed a default until-date equal to the start date like the "Repeat until" option in the same dialog).

Obviously any default date proposed is totally arbitrary and certainly will satisfy only a little part of the users, the others will have to change the date. The same would happen for the "forever" case, that, as reported in comment #0, might even be the less common (other than birthdays I could add taxes payments deadlines as events that repeat forever, and a few others).
However, I think Stefan is right, the default "forever" case maybe is little common but, compared with any other default date is less arbitrary and has the "merit" to not change the previous behavior, so I support this change.


Another solution could be to use a preference (obviously without UI) where the user can change the default date by setting the number of repetitions that the new event should have. It could also be very useful when someone needs to create very often the same kind of custom recurrence event. A preference kind of:

newevent.defaultUntilDateBy_n_repetitions.dayWdayWeekBiweekMonthYear=0,0,0,0,0,0

where 0 means a "forever" default value (the actual behavior of the pushed patch with five repetitions would be a preference 5,5,5,5,5,5). A bit hacking but flexible and it would satisfy every need.

Comment 31

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'd vote against a preference, although very flexible I think its a bit unmaintainable since probably noone will use it, and I don't think we are set up to do any tests in that area.

I think defaulting to "forever" wouldn't be a bad idea, but in the end I don't mind if its a date or the forever setting.

Comment 32

[Matthew Mecca [:mmecca]]

(In reply to Stefan Sitter from comment #29)
> Therefore the user now always needs additional actions to get to the known
> result. Shouldn't the default behavior be kept as is and the patch should
> only make it easier to limit the repetition range if necessary?

I agree with Stefan, I think "forever" should remain as the default, especially with repeating tasks - personally I rarely use an until date for these.

Comment 33

[Decathlon]

Attachment: "Forever" default until date

Changed the default until date to "forever".

Philipp, since Lightning doesn't work on trunk, may I push these two patches on Aurora? (This one after the r+ obviously).

Comment 34

[Stefan Sitter [:ssitter]]

What do you mean with "Lightning doesn't work on trunk"? Can you point to the bug report? So far I have not seen major issues, seems to work fine. (Thunderbird 21.0a1 + Lightning 2.3a1, both 2013-02-13)

Comment 35

[Decathlon]

(In reply to Stefan Sitter from comment #34)
> What do you mean with "Lightning doesn't work on trunk"?

Oops. Sorry. I forgot to to update Daily (I had the 19/01 nightly). After the update it works fine.

Comment 36

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 713387 [details] [diff] [review]
"Forever" default until date

r=philipp

Comment 37

[Decathlon]

Pushed to comm-central 
http://hg.mozilla.org/comm-central/pushloghtml?changeset=73e61db3deb7