389535 - consolidate implementations for minimonth control

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Michael Büttner (no reviews TFN)]

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.

Comment 1

[Michael Büttner (no reviews TFN)]

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

Comment 2

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

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.

Comment 3

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

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.

Comment 4

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

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.

Comment 5

[Michael Büttner (no reviews TFN)]

(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.

Comment 6

[Simon Paquet [:sipaq]]

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.

Comment 7

[Michael Büttner (no reviews TFN)]

(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).

Comment 8

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

I added a destructor to the recurrence-preview binding that removes the event handler created in the constructor and addressed the style nits.

Comment 9

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

Ups, Bug 391062 already has been fixed for which this patch still contained an ugly workaround.

Comment 10

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

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

Comment 11

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED