Closed Bug 389535 Opened 13 years ago Closed 13 years ago

consolidate implementations for minimonth control

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

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

References

Details

(Whiteboard: [patch in hand])

Attachments

(1 file, 2 obsolete files)

During the development of the prototype event dialog a new binding 'recurrence-calendar' has been introduced which is used to show the preview of recurrence rules. This binding is nearly identical to the existing implementation of the minimonth control used throughout the application. In order to reduce the amount of code it would be a good idea to consolidate these two bindings.
Blocks: 370435
Flags: blocking-calendar0.7+
Just for reference purposes, I'm going to eliminate the implementation at [1] and add the relevant features to the implementation at [2].

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-preview.xml#44
[2] http://lxr.mozilla.org/mozilla1.8/source/calendar/resources/content/datetimepickers/minimonth.xml#34
Attached patch patch v1 (obsolete) — Splinter Review
This patch merges the implementations found out the source locations I mentioned in my previous comment. First, it eliminates the file '...-dialog-recurrence-preview.css' since the relevant rules have been moved to minimonth.css.

I'm introducing a 'readonly' attribute to the minimonth control that adds the necessary functionality, i.e. it removes functionality :-) Specifying the readonly attribute on the minimonth changes the behavior as follows:

- the month/year popups are disabled (no border, no click events)
- the navigation arrows are hidden
- days can't be selected nor be active (by clicking on them)
- day grid is readonly (no different cursor, etc.)

The functionality to highlight the days which are in the set of occurrences are highlighted in much the same way as the days are normally highlighted.
Attachment #275418 - Flags: review?(philipp)
Whiteboard: [patch in hand]
Comment on attachment 275418 [details] [diff] [review]
patch v1

In general, the dialog starts to get pretty slow when a lot of months are displayed. Not only does the calculation take long, but also every click or keypress restarts the calculation. We should optimize this, either here or in a followup bug. Please file one if appropriate.

Also, the size calculations slightly fail. If there is enough room for three calendars, you can resize a bit smaller and there are still three calendars but the last one is clipped.

This might be a question for Christian, but I don't think there should be a hover or active style for a readonly calendar. If the calendar is only for viewing, it should not look like its clickable.


Minor stuff:

>+              <xul:minimonth anonid="minimonth" readonly="true"/>
>+              <xul:minimonth anonid="minimonth" readonly="true"/>
>+              <xul:minimonth anonid="minimonth" readonly="true"/>
I am aware this was the same before, but why do they all have the same anonid? Doesn't this cause problems anywhere?

>+                      if(index < dates.length) {
>+                      while(current.compare(end) < 0) {
Please run the style script, there are some cases.

>+            this.kMonthCell.setAttribute("interactive","true");
>+            this.kYearCell.setAttribute("interactive","true");
Missing space after comma

>+         
>+            var isReadOnly = this.getAttribute("readonly");
You only use this once below. Admittantly inside a double for loop, but is it very expensive to call getAttribute? I'm fine either way.

Almost there. r- for now, but just to have the above questions answered and to get this out of my queue.
Attachment #275418 - Flags: review?(philipp) → review-
The size of the minimonths might be related to bug 390194.

If you feel like it, you could also add a destructor to the recurrence-preview binding that removes the event handler created in the constructor. Otherwise we should file a bug.
(In reply to comment #4)
> The size of the minimonths might be related to bug 390194.
Right, which is originally a regression from bug bug 377401. That's why I didn't touch it. I would like to address this particular issue in its own bug as it is already filed.
Mickey, is this really a blocker for 0.7? This sounds a lot like a bug which can also easily go in shortly after the release.
(In reply to comment #3)
> (From update of attachment 275418 [details] [diff] [review])
> In general, the dialog starts to get pretty slow when a lot of months are
> displayed. Not only does the calculation take long, but also every click or
> keypress restarts the calculation. We should optimize this, either here or
> in a followup bug. Please file one if appropriate.
This is out of scope for this bug as it is just concerned to get the code into shape. I'm going to file a separate bug on that issue once this gets through review.

> Also, the size calculations slightly fail. If there is enough room for three
> calendars, you can resize a bit smaller and there are still three calendars
> but the last one is clipped.
This is a regression from Bug 377401 and is already taken care of in Bug 390194. I don't see that this should hold us off here?

> This might be a question for Christian, but I don't think there should be a
> hover or active style for a readonly calendar. If the calendar is only for
> viewing, it should not look like its clickable.
I don't know what you actually are referring to. I disabled the border styles for the minimonth in the readonly case, anything else that I might have missed?

> >+              <xul:minimonth anonid="minimonth" readonly="true"/>
> >+              <xul:minimonth anonid="minimonth" readonly="true"/>
> >+              <xul:minimonth anonid="minimonth" readonly="true"/>
> I am aware this was the same before, but why do they all have the same anonid?
> Doesn't this cause problems anywhere?
I don't want to break anything else, that's why I left it in (as it was there before this patch touched these line).
Attached patch patch v2 (obsolete) — Splinter Review
I added a destructor to the recurrence-preview binding that removes the event handler created in the constructor and addressed the style nits.
Attachment #275418 - Attachment is obsolete: true
Attachment #278737 - Flags: review?(philipp)
Attached patch patch v3Splinter Review
Ups, Bug 391062 already has been fixed for which this patch still contained an ugly workaround.
Attachment #278737 - Attachment is obsolete: true
Attachment #278739 - Flags: review?(philipp)
Attachment #278737 - Flags: review?(philipp)
Comment on attachment 278739 [details] [diff] [review]
patch v3

(In reply to comment #7)
> > Also, the size calculations slightly fail. If there is enough room for three
> > calendars, you can resize a bit smaller and there are still three calendars
> > but the last one is clipped.
> This is a regression from Bug 377401 and is already taken care of in Bug
> 390194. I don't see that this should hold us off here?
Nope, thats fine to take care of in bug 390194.

> I don't know what you actually are referring to. I disabled the border styles
> for the minimonth in the readonly case, anything else that I might have missed?
Thats exactly what I was looking for, I guess something was messed up last time I reviewed. Works great now.

> I don't want to break anything else, that's why I left it in (as it was there
> before this patch touched these line).
Fine with me



r=philipp
Attachment #278739 - Flags: review?(philipp) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
You need to log in before you can comment on or make changes to this bug.