998281 - Default status new event is available, should be busy

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[jacobmdekker]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140317233339

Steps to reproduce:

Create default event in Lightning calendar. 


Actual results:

Defaut status saved event is Free, should be busy.
See Options > Show time as > Free


Expected results:

Defaut status saved event should be Busy.
Options > Show time as > Busy

Comment 1

[[:MakeMyDay]]

Confirming for events without attendees in 2.6.4 - but this should be rechecked with a current nightly, as there were some changes on status handling in the dialog since then.

Comment 2

[Stefan Sitter [:ssitter]]

Yes, it seems to be broken. The default setting in previous releases was busy for non-all-day events and free for all-day events. The latter could be controlled via advanced preferences. In Lightning 3.2a2 neither free nor busy is selected by default anymore.

Comment 3

[[:MakeMyDay]]

Attachment: ShowTimeAsNotSetByDefaultInEventDialog-V1.diff

This patch simply makes sure the default setting is applied on load if value can be set.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Stefan Sitter from comment #2)
> Yes, it seems to be broken. The default setting in previous releases was
> busy for non-all-day events and free for all-day events. The latter could be
> controlled via advanced preferences. In Lightning 3.2a2 neither free nor
> busy is selected by default anymore.

MakeMyDay, is this pref taken into account? It seems to me that you just make it depend on the allday state? Also, what happens on existing events that don't have a TRANSP value set? These shouldn't be modified just by opening the dialog.

Comment 5

[[:MakeMyDay]]

(In reply to Philipp Kewisch [:Fallen] from comment #4)
> (In reply to Stefan Sitter from comment #2)
> > Yes, it seems to be broken. The default setting in previous releases was
> > busy for non-all-day events and free for all-day events. The latter could be
> > controlled via advanced preferences. In Lightning 3.2a2 neither free nor
> > busy is selected by default anymore.
> 
> MakeMyDay, is this pref taken into account? It seems to me that you just
> make it depend on the allday state? Also, what happens on existing events
> that don't have a TRANSP value set? These shouldn't be modified just by
> opening the dialog.

This is the same function that is applied after returning from the attendee dialog or changing the allday flag.

Regarding the setting on existing events without TRANSP value, you#re right. We should set this only if its realy a newly created event. I will provide an updated patch.

Comment 6

[[:MakeMyDay]]

Attachment: ShowTimeAsNotSetByDefaultInEventDialog-V2.diff

considering Philipps comment, the patch now applies the default behaviour for new events only.

Comment 7

[[:MakeMyDay]]

Attachment: ShowTimeAsNotSetByDefaultInEventDialog-V3.diff

Updated patch header.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8410451 [details] [diff] [review]
ShowTimeAsNotSetByDefaultInEventDialog-V3.diff

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

Looks good, r=philipp

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

comm trees are approval only and the approvers are not around. Setting checkin-needed for now.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8410451 [details] [diff] [review]
ShowTimeAsNotSetByDefaultInEventDialog-V3.diff

Actually, I don't think we need to rush this one in, c-c should be enough as it will be part of TB31 then anyway. If this is pushed after the merge, a=me for aurora. If you disagree please let me know.

Comment 11

[[:MakeMyDay]]

Attachment: ShowTimeAsNotSetByDefaultInEventDialog-V4.diff

Patch with patch header updated

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/d7ea041f90d3

Comment 13

[Decathlon]

It seems this fix causes two minor issues:
- the New Event dialog prompts the "Save event" dialog when closing without any modification;
- now is not possible to create a new event without the TRANSP property (transparent/opaque) because now that
  property is enabled by default and the menu "Show Time as" can only switch between the two options free/busy but
  doesn't allow to remove both (doesn't exist a "Not specified" item for the free/busy status).

Comment 14

[[:MakeMyDay]]

The first one is ugly. To get rid of it, it might be on option to move setting of the default value from the dialog to calIEvent.js, so its already in the item, when passed to the dialog.

Regarding the second one, I guess in general it makes sense to have the property set (even if it's not mandatory, it's also recommended by the RfC) without the ability to remove it for new events. Do you have a specific use case in mind, where it makes sense to not have the TRANSP property set?

Comment 15

[Decathlon]

I think I've not understood this bug, or better, as far as I can remember Lightning has never produced a new event (not an All Day one) with the free/busy time set by default as busy (i.e. the TRANSP property set to OPAQUE). I've just tested with Lightning 1.0, 1.4, 2.6, 3.1 and I've never seen this, but maybe I've missed something.
If it has been decided that a new event should be Busy by default, I think a preference, like the All Day case, would be preferable. Personally I wouldn't like so much a busy value as default.


From the description I can't reproduce with Lightning 2.6.4:

> Steps to reproduce:
> 
> Create default event in Lightning calendar. 
> 
> Actual results:
> 
> Defaut status saved event is Free, should be busy.
> See Options > Show time as > Free

To me the default status saved event results neither Free nor Busy i.e. no TRANSP property in the ics file of the calendar (that personally I prefer).
... See Options > Show time as > Free ... to me results neither Free nor Busy, both deselected.

The only incongruence is the minimonth that gets the day bold, that should mean a busy day, with every new event just created that actually doesn't have (didn't have before the fix) the free/busy status set at all (neither free nor busy).

Different is the case for All Day events where we have a preference (though if we had a bug on the week-view in the All Day event zone where by double clicking Lightning generated a new All Day event without respecting the preference).

Comment 16

[Decathlon]

If I should have to do a patch for this without a preference for normal events, I would propose something that set the free/busy status before opening the dialog, something like:


diff --git a/calendar/base/content/calendar-item-editing.js b/calendar/base/content/calendar-item-editing.js
--- a/calendar/base/content/calendar-item-editing.js
+++ b/calendar/base/content/calendar-item-editing.js
@@ -205,8 +205,13 @@ function setDefaultItemValues(aItem, aCa
     aItem.calendar = aCalendar || getSelectedCalendar();
 
     // Alarms
     cal.alarms.setDefaultValues(aItem);
+
+    // Free/busy status
+    let showTimeAs = (aForceAllday ? Preferences.get("calendar.allday.defaultTransparency", "TRANSPARENT")
+                                   : "OPAQUE");
+    aItem.setProperty("TRANSP", showTimeAs);
 }
 
 /**
  * Creates an event with the calendar event dialog.
diff --git a/calendar/base/content/dialogs/calendar-event-dialog.js b/calendar/base/content/dialogs/calendar-event-dialog.js
--- a/calendar/base/content/dialogs/calendar-event-dialog.js
+++ b/calendar/base/content/dialogs/calendar-event-dialog.js
@@ -466,12 +466,8 @@ function loadDialog(item) {
     updateRepeat(true);
     updateReminder(true);
 
     gShowTimeAs = item.getProperty("TRANSP");
-    // set default value for a new event
-    if (!gShowTimeAs & window.mode == "new" && cal.isEvent(item)) {
-        setShowTimeAs(gStartTime.isDate);
-    }
     updateShowTimeAs();
 }
 
 /**

but it's a hasty patch. Before asking for review I would have to test a lot because there are things which I should have to see deeply.

Comment 17

[[:MakeMyDay]]

Placing this in calendar-item-editing.js seems the better option than doing in calEvent.js as suggested in #14. But I suggest to place it a little above within the isEvent{} section before line 92, because this applies for events only.

I second to add an additional preference here to leave it to the users choice, which status is the individually appropriate one. Will you provide a patch or should I? And do we need a separate bug for this?

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to MakeMyDay from comment #17)
> I second to add an additional preference here to leave it to the users
> choice, which status is the individually appropriate one. Will you provide a
> patch or should I? And do we need a separate bug for this?
I second Decathlon's approach. Doesn't hurt to do this in a separate bug, be sure to mention it here and/or set a depenency. I'll leave the rest for you to figure out.

Comment 19

[Stefan Sitter [:ssitter]]

If you add new preferences please make them more failsafe to use, e.g. xxx.yyy.allday = true|false instead of letting the user specify any arbitrary string.

And maybe we could contact Robert Brand, the author of the "Custom Calendar Defaults" extension <http://www.nadelundhirn.de/wp/tag/custom-calendar-defaults/>. There might be benefit in learning from each other with regard to this feature or maybe consider including parts of the extension in Lightning itself.

Comment 20

[[:MakeMyDay]]

@Decathlon: will you provide a patch as proposed in comment #16? Otherwise I would take this into my queue to get this regression resolved for the upcoming release.

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

In any case, please get this fixed ASAP. Beta 1 is already out, but there will be a second beta some time in the next 6 weeks, and maybe a third.

Comment 22

[Decathlon]

(In reply to MakeMyDay from comment #20)
> @Decathlon: will you provide a patch as proposed in comment #16? Otherwise I
> would take this into my queue to get this regression resolved for the
> upcoming release.

Sorry, I had forgotten this.
Please go ahead with the patch, just keep in count that I hadn't tested so much the code in comment 16.

Comment 23

[[:MakeMyDay]]

Attachment: MakeEventTransparencyPerferenceDependent-V1.diff

This patch removes the changes already checked in for this bug, implements the feature preference dependent instead and addresses also the occured problem when closing a not edited dialog.

With respect to comment #19, I changed also the alreday existing pref for allday event transparency to a boolean type. 

On top of this, the patch disables the transparency related controls if the item is not of type event - RfC 5545 allows the TRANSP property for VEVENTS only. If you want this to be handled in a separate bug, I will remove the section from this patch.

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8444156 [details] [diff] [review]
MakeEventTransparencyPerferenceDependent-V1.diff

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

r-, there are some issues that need taking care of. I will do this locally and upload a new patch, which I will include in b2 but I'd still like a review from you to make sure this is what you wanted.

::: calendar/base/content/calendar-item-editing.js
@@ +208,5 @@
>      cal.alarms.setDefaultValues(aItem);
> +
> +    // Free/busy status for events
> +    let transp = cal.getEventDefaultTransparency(aForceAllday);
> +    // TRANSP must not be set for VTODOS

You say only for vevents, but don't check for it. Probably we should move this into the if (cal.isEvent) block above.

@@ +209,5 @@
> +
> +    // Free/busy status for events
> +    let transp = cal.getEventDefaultTransparency(aForceAllday);
> +    // TRANSP must not be set for VTODOS
> +    if (transp !== null) {

I don't think we need such a strict check here, if (transp) should be enough.

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +1790,5 @@
>  function updateShowTimeAs() {
> +    if (cal.isEvent(window.calendarItem)) {
> +        var showAsBusy = document.getElementById("cmd_showtimeas_busy");
> +        var showAsFree = document.getElementById("cmd_showtimeas_free");
> +    

More whitespaces here.

::: calendar/base/modules/calUtils.jsm
@@ +288,5 @@
>              invitedAttendee = calendar.getInvitedAttendee(aItem);
>          }
>          return invitedAttendee;
>      },
> +    

Some whitespaces here to clean up.

@@ +296,5 @@
> +     * @param boolean isAllDay 
> +     */
> +    getEventDefaultTransparency: function (isAllDay) {
> +        let transp = null;
> +        if (cal.isEvent(aItem)) {

The aItem parameter is missing, or the if block is too much. Given the function name I am assuming you want it to go away.

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: MakeEventTransparencyPerferenceDependent-V2.diff

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8452276 [details] [diff] [review]
MakeEventTransparencyPerferenceDependent-V2.diff

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

::: calendar/base/content/calendar-item-editing.js
@@ +212,4 @@
>  
>      // Alarms
>      cal.alarms.setDefaultValues(aItem);
> +

Ignore this chunk, I forgot to qref.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

I've pushed this to beta so I can do a new beta build with it. 

https://hg.mozilla.org/releases/comm-beta/rev/faa19b33e546

Leaving open for aurora and central and the review.

Comment 28

[[:MakeMyDay]]

Comment on attachment 8452276 [details] [diff] [review]
MakeEventTransparencyPerferenceDependent-V2.diff

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

r=Makemyday with the comment below and your previous comment considered.

::: calendar/base/content/calendar-item-editing.js
@@ +93,5 @@
> +        // Free/busy status is only valid for events, must not be set for tasks.
> +        let transp = cal.getEventDefaultTransparency(aForceAllday);
> +        if (transp && cal.isEvent(aItem)) {
> +            aItem.setProperty("TRANSP", transp);
> +        }

The if clause is obsolete here, because transp is always OPAQUE or TRANSPARENT and we are inside a block, where cal.isEvent(aItem) is true already.

Comment 29

[[:MakeMyDay]]

Attachment: MakeEventTransparencyPreferenceDependent-V3.diff

Updated patch considering my previous comments.

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

I'm going to leave the beta checkin as it is since it works and its just a redundant statement.

https://hg.mozilla.org/comm-central/rev/674da583d779
https://hg.mozilla.org/releases/comm-aurora/rev/0cb152347ca3

Comment 31

[Jan-Åke Larsson]

I am still affected by this. Lightning 3.3.3, Thunderbird 31.6.0

Calendar items I create myself in Lightning are "Free", while invitations (via Outlook) and items created on my phone, synced via Exchange, is "Busy".

Comment 32

[[:MakeMyDay]]

Is this an issue for regular event or all day events? All day events keep 'free' by default, but thete's a preference to toggle this. See calendar.events.defaultTransparency preferences in config editor.

Comment 33

[Jan-Åke Larsson]

Regular events, I am afraid. 

Also, even if I toggle allday event default transparency, newly created events are "Free".

I can change "Show as" just as I expect, but I'd like the default to be "Busy" for my events.

Comment 34

[Jan-Åke Larsson]

That should be:

Also, even if I toggle allday event default transparency, newly created _allday_ events are "Free". (The others are too.)

I can change "Show Time as" just as I expect, but I'd like the default to be "Busy" for my events.

Comment 35

[Jan-Åke Larsson]

I have just tried pressing the "Event" = "new event" button, which gives me "Busy". 

Usually I point-and-drag to create a new event in the calendar view. This gives an event with Time as "Free".

Comment 36

[Stefan Sitter [:ssitter]]

It seems that you see Bug 1045223 that is fixed in Lightning 3.7 and newer.

Comment 37

[Jan-Åke Larsson]

Correct, sorry for the noise.