373004 - [proto] all-day option doesn't affect free/busy transparency correctly

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Michael Büttner (no reviews TFN)]

Setting the all-day option in the event dialog automatically sets the free/busy transparency setting [options -> show time as -> ...] to 'free'. this basically means that all-day events by default won't be considered for free/busy calculations. synchronizing these two options is generally a good idea, but unfortunately this default setting doesn't work if the user revokes the all-day option. in this case the free/busy transparency setting still keeps set to 'free'. in this case it should be set to 'busy' as this is most probably what the user expects.

automatically synchronizing those two options currently works just one-way, it would be nice if the other way would also work as expected. the logic should be modified as follows:

if ((all-day enabled) && (free/busy == undefined or busy)) then set free/busy to free
if ((all-day disabled) && (free/busy == undefined or free)) then set free/busy to busy

the relevant source location is http://lxr.mozilla.org/seamonkey/source/calendar/prototypes/wcap/sun-calendar-event-dialog.js#1472

Comment 1

[Berend Cornelius]

I fixed it in the way as Michael had proposed. In the patch attached I also took care that the AllDay-setting of the Invite Attendee's dialog is reflected in the "Show Time As" - menu after that dialog has been closed.

Comment 2

[Berend Cornelius]

Attachment: patch that updates the "Show Time As" menu

mickey: Please verify

Comment 3

[Berend Cornelius]

Attachment: Second patch without whitespace

Oups, found another trailing whitespace

Comment 4

[Michael Büttner (no reviews TFN)]

Comment on attachment 264122 [details] [diff] [review]
Second patch without whitespace

>+!�;47970;47970
>+!�;48827;48827
>+!�;51961;51961
>+!�;81089;81089
i bet this doesn't belong to the patch :-)

>+function setShowTimeAs(allDay)
>+{
>+  // automatically select "show time as free" if this
>+  // event is said to be all-day.
>+  if(allDay) {
>+    gShowTimeAs = "TRANSPARENT";
>+  }
>+  else{
>+    gShowTimeAs = "OPAQUE"
>+  }    
With how the logic currently works, it is always necessary to call updateShowTimeAs() after the call to setShowTimeAs(). Why don't call updateShowTimeAs() in setShowTimeAs() before the function returns. This would reduce the probability that changes to the free/busy state is not reflected in the menu.

besides that, the i decided to prefix each function with three comment lines. in order to have a consistent style in the source files, i'd like to keep this rule if you don't mind.

>+    var isAllDay = getElementValue("event-all-day", "checked");
>+    setShowTimeAs(isAllDay)
>+    updateShowTimeAs();
It is unnecessary to do a full DOM tree lookup in order to find out if this is an all-day event. this could be done more efficient with setShowTimeAs(gStartTime.isDate)

besides all these minor flaws, this patch introduces a subtle bug. if you play close attention to what i had proposed, you see that i check the current state before changing it. i repeat it here again for clarity...

if ((all-day enabled) && (free/busy == undefined or busy)) then set free/busy
to free
if ((all-day disabled) && (free/busy == undefined or free)) then set free/busy
to busy

this logic is not present in the proposed patch. just imagine what happens without this additional check while the user performs these actions:

1) set event to all-day (internal logic sets free/busy option to "free")
2) set free/busy option manually to "busy"
3) enter invite attendee dialog and return

you'd find the free/busy option to be set to "free", which obviously shouldn't be the case. that's why i'm marking this patch r-.

Comment 5

[Berend Cornelius]

Attachment: The new patch

I applied some changes to meet mickeys objections. Concerning his follow up bug I inserted a query that determines if an "allDay change" in the attendees dialog has taken place or not. However this query is inserted into the callback function of the attendees dialog.

Comment 6

[Michael Büttner (no reviews TFN)]

Comment on attachment 264233 [details] [diff] [review]
The new patch

>+function setShowTimeAs(allDay)
>+{
>+  if(allDay) {
Style nit, missing blank after 'if'.

>+    gShowTimeAs = "TRANSPARENT";
>+  }
>+  else{
Style nit, missing blank after 'else'.

>+    gShowTimeAs = "OPAQUE";
>+  }    
Style nit, this line has trailing blanks.

Alternatively this could have been written as
  gShowTimeAs = allDay ? "TRANSPARENT" : "OPAQUE";

>+    if (isAllDay != gStartTime.isDate){
>+        setShowTimeAs(gStartTime.isDate)
>+    }
Style nit, the indentation is wrong.

I would suggest to move the definition of the callback function after the variable 'isAllDay' being defined, since otherwise is it unclear what 'isAllDay' referring to, besides the fact that it's just bad style otherwise.

Since these are all just style nits, I'll fix them before checking this patch in. Thanks for the patch. r=mickey.

Comment 7

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 8

[Andreas Treumann]

Verified in Lightning build 2007070304.