Closed Bug 373004 Opened 16 years ago Closed 16 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: michael.buettner, Assigned: berend.cornelius09)

Details

Attachments

(1 file, 2 obsolete files)

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
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.
mickey: Please verify
Attachment #264121 - Flags: review?(michael.buettner)
Attached patch Second patch without whitespace (obsolete) — Splinter Review
Oups, found another trailing whitespace
Attachment #264121 - Attachment is obsolete: true
Attachment #264122 - Flags: review?(michael.buettner)
Attachment #264121 - Flags: review?(michael.buettner)
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-.
Attachment #264122 - Flags: review?(michael.buettner) → review-
Attached patch The new patchSplinter Review
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.
Attachment #264122 - Attachment is obsolete: true
Attachment #264233 - Flags: review?(michael.buettner)
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.
Attachment #264233 - Flags: review?(michael.buettner) → review+
Whiteboard: [checkin needed after 0.5]
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed after 0.5]
Target Milestone: --- → 0.7
Verified in Lightning build 2007070304.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.