Closed Bug 456102 Opened 16 years ago Closed 16 years ago

Minimonth should generally be able to show free-busy states

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

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

Details

Attachments

(1 file, 1 obsolete file)

Currently the freebusy states are only displayed in the minimonth of the left pane in calendar mode and taskmode. Decathlon pointed to the need to implement this useful feature also for the minimonth in the today-pane (see bug 429687 comment #28).
The code to display freebusy states is currently hardwired to the one minimonth and should therefor be overworked.
Attached patch patch v. #1 (obsolete) — Splinter Review
This patch brings the minimonth together with the minimonth-busy-listener in "calendar-minimonth-busy.js" that then can be removed from the repository. The free-busy states are meant to be controlled by the newly introduced "freebusy" attribute at the minimonth, so that optionally all of these widgets can be facilitated with this new feature. Of course one should be aware that this brings a certain performance loss along. For this reason I only set the free-busy attribute in the today-pane depending on whether the minimonth is selected as the control to display the date - and not the miniday. 
I found the minimonth code still unclear and allowed myself to improve it in same places and bring the Sunbird code and lightning code around the minimonth a bit closer together.
Along with that I fixed the buggy scrollhandler over the year-popup.
When I tested this patch I also fixed "Bug 414336 -  Last day in minimonth view is always not bold, independent whether events exist or not" along with it. I hope that's Ok. I thought it's no use to fix that bug separately in code that is bound to be deprecated. As I stated in that bug the cause of the issue is that the "lasteDate" property must be exclusive of the set items range. While I fixed that I also converted the returned date to a datetime object, although I am aware that the minimonth internally still uses jsdates. I think we should change that, too some day or is there any reason to keep the jsdates?
I am asking for review already now but I would like to have this patch tested beforehand.
Attachment #339515 - Flags: ui-review?(christian.jansen)
Attachment #339515 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Keywords: qawanted
Attachment #339515 - Flags: ui-review?(christian.jansen) → ui-review+
Comment on attachment 339515 [details] [diff] [review]
patch v. #1

ui=christian
Comment on attachment 339515 [details] [diff] [review]
patch v. #1

>+   <binding id="minimonth-header" extends="xul:box">
>+   <script type="application/x-javascript" src="chrome://calendar/content/calendar-management.js"/>

script directly in a binding? It should be in a <resources> block afaik, but aside from that, why does calendar-management.js need to be included? afaik it installs a load handler, which would be called more often when included with the binding too.



>          this.mScrollYearsHandler =  {
>+             binding: this,
>+             handleEvent: function scrollYears_handleEvent(event) {
>+                 this.binding.scrollYears(event);
>+             }
>          };
Just thinking...this might cause a circular reference, we should probaly do this.mScrollYearsHandler.binding = null; in the destructor.

>+  <binding id="minimonth" xbl:inherits="onchange,onmonthchange,onpopuplisthidden,readonly" extends="xul:box">
I don't think xbl:inherits works on the binding element, shouldn't this go in the <content> element?

>+    <content class="minimonth-mainbox" orient="vertical">
What do you need the class for? You could probably rather use a tag selector for the css rules.

>+          if (this.hasAttribute("freebusy")) {
>+              this.setAttribute("freebusy", this.getAttribute("freebusy"));
>+          }
Kind of strange...you could either provide an extra method that can be called from both the setAttribute override and the constructor or at least add a comment why this is needed.

>+                var n = parseInt(box.getAttribute("busy") || 0) +
When using parseInt, always use the second parameter (10, in most cases).
>+    // calICompositeObserver methods
We need to be careful when the object implements both calICompositeObserver and calIObserver, I think some notifications can be called twice (i.e once for all calICompositeObservers which superclasses calIObserver and once for the calendar (calIObserver) itself.




>+          }
>+          else {
>+              var ret = XULElement.prototype.setAttribute.call (this, aAttr, aVal);
>+          }
>+          return ret;
please use } else {
Also you define ret in the else block, this should be defined at the top of the function.




>-      <method name="switchMonth">
>+      <method name="switchMonth">43
Typo, change not needed.

>+      <handler event="click" button="0"><![CDATA[
>+        this.setAttribute("selected", "true");
>+        this.minimonthParent.onSelectDay(this)
>+      ]]></handler>
Maybe we should use the event system for this, i.e fireEvent().


>+            <hbox id="calMinimonthBox">
>               <spacer flex="1"/>
>-              <minimonth id="ltnMinimonth" onchange="ltnMinimonthPick(this);" flex="2"/>
>+              <minimonth id="calMinimonth" onchange="ltnMinimonthPick(this);" flex="2" freebusy="true"/>
>               <spacer flex="1"/>
>             </hbox>
<hbox id="calMinimonthBox" pack="center"> and remove the spacers and flex attributes


>+  <script type="application/x-javascript" src="chrome://calendar/content/calendar-management.js"/>
>+  <script type="application/x-javascript" src="chrome://calendar/content/calendar-statusbar.js"/>
Be careful when adding calendar-management, I believe it installs a load handler to set up all the calendar list management stuff.



I'd like to see a second patch for this with the review comments fixed, some issues like including calendar-management.js in lots of dialogs sounds a bit scary to me.

r- for now (sorry!)
Attachment #339515 - Flags: review?(philipp) → review-
Attached patch patch v. #2Splinter Review
I worked in the comments of Philipp. I also added a "removeAttribute" function that was missing and added an internal "_setFreeBusy" function. "getCompositeCalendar" was moved to calUtils so I do not unnecessarily have ot add the calendar tree stuff to the dialogs.

>Maybe we should use the event system for this, i.e fireEvent().
I left the code as it is. The according event is fired from the parental minimonth already shortly after.
Attachment #339515 - Attachment is obsolete: true
Attachment #340331 - Flags: review?(philipp)
Comment on attachment 340331 [details] [diff] [review]
patch v. #2

>+           year.setFullYear(Math.max(1, compFullYear - parseInt(years.length / 2) + 1));
parseInt needs a second argument.

>+      ]]></destructor>
>+
>+    // calIOperationListener methods
>+      <method name="onOperationComplete">
C-style comment outside of methods. Needs to be an xml comment.


>+      // calIObserver methods
>+      <method name="onStartBatch">
Same here


>+    // calICompositeObserver methods
>+      <method name="onCalendarAdded">
And here

>+      <method name="_setFreeBusy">
>+        <parameter name="aFreeBusy"/>
>+        <body><![CDATA[
>+            if (aFreeBusy === true) {
Why so strict? == should be sufficient here...

>+var gCompositeCalendar = null;
>+function getCompositeCalendar() {
Unless gCompositeCalendar is used elsewhere, go ahead and make it a member of the getCompositeCalendar function

>+}
>\ No newline at end of file
Please make sure your files have newlines at the end!!


r=philipp
Attachment #340331 - Flags: review?(philipp) → review+
I worked in the comments of philipp and pushed the patch to comm-central:
http://hg.mozilla.org/comm-central/rev/fe63791a49aa
->fixed
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
(In reply to comment #6)
> I worked in the comments of philipp and pushed the patch to comm-central:
> http://hg.mozilla.org/comm-central/rev/fe63791a49aa
> ->fixed

Are you sure this file belongs to this checkin? 
http://hg.mozilla.org/comm-central/diff/fe63791a49aa/calendar/locales/en-US/updater/updater.ini
Yeah, I also wonder...
Berend, please fix the updater.ini file mentioned in Comment #7 or explain why it was changed although it doesn't belong to the patch.

This checkin seems to have caused Bug 459908 in addition, please take a look.
(In reply to comment #9)
> Berend, please fix the updater.ini file mentioned in Comment #7 or explain why
> it was changed although it doesn't belong to the patch.
I remember having changes to updater.ini in my srcdir, too when building some days ago. They were caused because dist/bin/.../updater.ini has been a symlink into srcdir and has been changed due to IIRC bug 451912. Removing the symlink fixd the problem. Thus I think it just slipped in...
Someone undo that and re-mark as RESOLVED, please.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
did that: <http://hg.mozilla.org/comm-central/rev/4cbde1901dff>
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
checked in lightning build 20081015052201 -> VERIFIED
Status: RESOLVED → VERIFIED
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.