376086 - Lightning does not support a multiweek view

Status: VERIFIED FIXED

(Calendar :: Lightning Only, enhancement)

Description

[Blair Sutton]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Lightning 0.5 pre (build 2007032905)

In Sunbird one can choose to view their calendar using a multiweek view. This choice is not available in Lightning. This is a very useful feature for users wishing to see their appointments within a multiweek rolling window.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Bas van den Bosch]

jminta wrote an extension for this:
https://addons.mozilla.org/en-US/sunbird/addon/3738

Comment 2

[VoodooChild]

the only thing you have to do with this plugin is to open it as an archive for example in winrar, open install.rdf and change the max. version of thunderbird and lightning.
I dont know how long this will work, but for this moment its working quite nice :D 

Comment 3

[Zvi Har'El]

It has few problems:

1) No way to configure the number of visible weeks (one can do that in Sunbird). This number is always 4, with no previous weeks.
2) The multiweek icon has no tool tip.
3) The context menu (right click) in the multiweek view contains two unfunctional (P) and (N) buttons (presumably Previous and Next ???).

Otherwise it is very nice. I hope it should be integrated into lightning, like it is in sunbird.

Comment 4

[VoodooChild]

1) do it like this:
about:config

a)   calendar.previousweeks.inview   ->  type in the numbers of previous weeks you'd like to see

b)

Comment 5

[VoodooChild]

oups  :-/

b)   calendar.weeks.inview    ->   type in the number of following weeks
you'd like to see

2,3) ... let's wait :-D

Comment 6

[Trevor Morgan]

It's strange that multiweek view is not enabled in lightning
All the code and most of the strings for it are already there, it just needs a few user interface bits to allow it to be selected.

Comment 7

[Christian Jansen]

+1 for having this view. It would be interesting to hear (ok, to read) the why is not in Lightning.

Comment 8

[Zvi Har'El]

(In reply to comment #4)
> 1) do it like this:
> about:config
> 
> a)   calendar.previousweeks.inview   ->  type in the numbers of previous weeks
> you'd like to see
> 
> b)
> 

Setting calendar.previousweeks.inview helps, but this get reset again later, I am not sure in which conditions (I believe when the view is changed). Looging at about:config, you can see that the user-set value has disappeared, and the default of 0 rreturns.

Comment 9

[Simon Paquet [:sipaq]]

Attachment: Patch to enable Multiweek view on Lightning

This patch enables the Multiweek view on Lightning. The patch also covers some whitespace issues in the messenger-overlay-sidebar.js file. I'll also attach a patch showing the changes without any whitespace changes.

Currently this patch has one known issue:
The patch does not implement an equivalent for the "number of weeks"-menuitem in the Sunbird view menu, where you can directly influence the number of weeks that the multiweek view should show. In Lightning, you can only influence this via the preferences dialog.

Personally I consider this to be the better way, since this is not a setting that you change regularly and therefore perfectly positioned in the pref dialog.

Comment 10

[Simon Paquet [:sipaq]]

Attachment: Patch - diff -uw6

Patch without whitespace changes.

Comment 11

[Simon Paquet [:sipaq]]

I should also add, that the patch in this bug also contains the necessary changes to fully fix bug 189416, which has been fixed for Sunbird for quite some time, but never for Lightning.

Comment 12

[Simon Paquet [:sipaq]]

Attachment: Patch v2 (with whitespace changes)

Slightly better patch, suggested by ssitter.

Comment 13

[Simon Paquet [:sipaq]]

Attachment: Patch v2 (without whitespace changes)

Corresponding diff -uw patch

Comment 14

[Michael Büttner (no reviews TFN)]

Comment on attachment 271543 [details] [diff] [review]
Patch v2 (with whitespace changes)

(In reply to comment #9)
> Currently this patch has one known issue:
> The patch does not implement an equivalent for the "number of weeks"-menuitem
> in the Sunbird view menu, where you can directly influence the number of weeks
> that the multiweek view should show. In Lightning, you can only influence this
> via the preferences dialog.
Christian, I'm asking for UI review in order to get this question resolved.

Comment 15

[Christian Jansen]

We should implement the equivalent of Sunbird's "number of weeks"-menuitems. It's a very useful feature.

Regarding toolbar & menu integration I suggest the follow:

Toolbar Integration:

- The button should occur between Week and Month
- It should be called Multiweek

Menu Integration:

Calendar
    ...
    Day
    Week
    Multiweek
    Month
   ----------
    View >
        Workdays Only
        Show Task in Calendar
        Rotate View
        ------------
        Number of Weeks > 2 Weeks
                          3 Weeks
        --------------     ....

The position of the "Number of Weeks >" sub-menu is not perfect, but it is the equivalant to Sunbird. We should decide on the final position, if we restructure the whole menu integration.

Comment 16

[Michael Büttner (no reviews TFN)]

I think a reasonable strategy is to go ahead with this patch as it currently stands and address the configuration of this feature ("number of weeks") in a spin-off bug. Simon, what's your opinion on that?

Comment 17

[Martin Schröder [:mschroeder]]

Bug 353696 is about problems with the preferences for the multiweek view and its menu integration.

Comment 18

[Simon Paquet [:sipaq]]

(In reply to comment #16)
> I think a reasonable strategy is to go ahead with this patch as it currently
> stands and address the configuration of this feature ("number of weeks") in a
> spin-off bug. Simon, what's your opinion on that?

I'm fine with this strategy, especially because the menuitem issue is done in Sunbird via Sunbird-specific code again

http://mxr.mozilla.org/mozilla1.8/source/calendar/resources/content/calendarWindow.js#73

which I do not know how to adjust to make it work in Lightning.

Comment 19

[Michael Büttner (no reviews TFN)]

Comment on attachment 271543 [details] [diff] [review]
Patch v2 (with whitespace changes)

>+    // Set up the commands
>+    var availableViews = document.getElementById("calendar-view-box");
>+    for (var i = 0; i < availableViews.childNodes.length; i++) {
>+        var view = availableViews.childNodes[i];
>+        var command = document.getElementById(view.id+"_command");
>+        if(view.id == type+"-view") {
>+           command.setAttribute("checked", true);
>+        } else {
>+           command.removeAttribute("checked");
>+        }
>+    }
The patch produces loads of exceptions 'command has no properties'. Shouldn't this line above read ...document.getElementById(view.id+"-command")... ???

>+                type="radio"
>+                name="calendarMenuViews"
>+                observes="day-view-command"/>
What's the name for?

>+    <command id="day-view-command"  oncommand="showCalendarView('day')"/>
>+    <command id="week-view-command"  oncommand="showCalendarView('week')"/>
>+    <command id="multiweek-view-command"  oncommand="showCalendarView('multiweek')"/>
>+    <command id="month-view-command"  oncommand="showCalendarView('month')"/>
Wouldn't it be better to condense the 2 blanks into one?

>     <calendar-decorated-day-view id="day-view" flex="1"
>                                  context="calendar-view-context-menu"
>                                  item-context="calendar-item-context-menu"/>
>     <calendar-decorated-week-view id="week-view" flex="1"
>                                   context="calendar-view-context-menu"
>                                   item-context="calendar-item-context-menu"/>
>+    <calendar-decorated-multiweek-view id="multiweek-view" flex="1"
>+                                       context="calendar-view-context-menu"
>+                                       item-context="calendar-item-context-menu"/>
>     <calendar-decorated-month-view id="month-view" flex="1"
>                                    context="calendar-view-context-menu"
>                                    item-context="calendar-item-context-menu"/>
I'd suggest to align those lines...

>+    <toolbarbutton id="calendar-day-view-button"
>+                   mode="calendar"
>+                   type="radio"
>+                   group="calendarViews"
What's the group attribute for?

Unfortunately r- due to the exceptions being thrown.

Comment 20

[Simon Paquet [:sipaq]]

(In reply to comment #19)
> The patch produces loads of exceptions 'command has no properties'. Shouldn't
> this line above read ...document.getElementById(view.id+"-command")... ???

Yes, you're right. I missed the "_" to "-" conversion in that piece of code.
 
> >+                type="radio"
> >+                name="calendarMenuViews"
> >+                observes="day-view-command"/>
> What's the name for?

The name attribute is used for grouping the items with type "radio" together. See http://developer.mozilla.org/en/docs/XUL:menuitem#a-menuitem.type

> >+    <command id="day-view-command"  oncommand="showCalendarView('day')"/>
> >+    <command id="week-view-command"  oncommand="showCalendarView('week')"/>
> >+    <command id="multiweek-view-command"  oncommand="showCalendarView('multiweek')"/>
> >+    <command id="month-view-command"  oncommand="showCalendarView('month')"/>
> Wouldn't it be better to condense the 2 blanks into one?

Will do in the next patch.

> >     <calendar-decorated-day-view id="day-view" flex="1"
> >                                  context="calendar-view-context-menu"
> >                                  item-context="calendar-item-context-menu"/>
> >     <calendar-decorated-week-view id="week-view" flex="1"
> >                                   context="calendar-view-context-menu"
> >                                   item-context="calendar-item-context-menu"/>
> >+    <calendar-decorated-multiweek-view id="multiweek-view" flex="1"
> >+                                       context="calendar-view-context-menu"
> >+                                       item-context="calendar-item-context-menu"/>
> >     <calendar-decorated-month-view id="month-view" flex="1"
> >                                    context="calendar-view-context-menu"
> >                                    item-context="calendar-item-context-menu"/>
> I'd suggest to align those lines...

Will do.

> >+    <toolbarbutton id="calendar-day-view-button"
> >+                   mode="calendar"
> >+                   type="radio"
> >+                   group="calendarViews"
> What's the group attribute for?

It's the same as the "name" attribute for menuitems. See http://developer.mozilla.org/en/docs/XUL:Attribute:group

But don't ask me, why these are different for menuitems and toolbarbuttons. See also bug 189416 comment 39, but be aware that the code positions mentioned here have changed since then.

Comment 21

[Simon Paquet [:sipaq]]

Attachment: Patch v3 (with review comments addressed)

Comment 22

[Simon Paquet [:sipaq]]

Attachment: Patch v4 (including Pinstripe changes)

This patch also adds the necessary Pinstripe changes.

Comment 23

[Michael Büttner (no reviews TFN)]

Comment on attachment 272015 [details] [diff] [review]
Patch v4 (including Pinstripe changes)

>+    // Set up the commands
>+    var availableViews = document.getElementById("calendar-view-box");
You could use getViewDeck() and be application agnostic, but that's more of a theoretical issue since this is Lightning only code.
>+    for (var i = 0; i < availableViews.childNodes.length; i++) {
>+        var view = availableViews.childNodes[i];
>+        var command = document.getElementById(view.id+"-command");
>+        if(view.id == type+"-view") {
Style nit, please add a blank between 'if' and '('.
>+           command.setAttribute("checked", true);
>+        } else {
>+           command.removeAttribute("checked");
>+        }
>+    }
r=mickey with these nits addressed.

Comment 24

[Christian Jansen]

r=chris for this patch.

Comment 25

[Simon Paquet [:sipaq]]

Patch checked in with style nit addressed.

Comment 26

[Sven Giermann]

(In reply to comment #23)
> (From update of attachment 272015 [details] [diff] [review])
> >+    // Set up the commands
> >+    var availableViews = document.getElementById("calendar-view-box");
> You could use getViewDeck() and be application agnostic, but that's more of a
> theoretical issue since this is Lightning only code.

I totally agree with mickey - and even if it's Lightning only code, you should pay attention to what you want to do...
In this line you want to get a reference to the deck, that is holding the different calendar-views; so please use getViewDeck() instead!

(do I have to create a patch for this simple line?)

-    var availableViews = document.getElementById("calendar-view-box");
+    var availableViews = getViewDeck();

Comment 27

[Michael Büttner (no reviews TFN)]

I just double-checked that indeed the getElementById-version has been checked in. Simon, would you mind changing that one line? It doesn't hurt and this version is much cleaner. I should have said "you should use" instead of "you could use" as I wanted you to change that line before checking it in. Sorry for not making myself clear. Please make sure that getViewDeck() is indeed available in messenger-overlay-sidebar.js, otherwise open a new bug on that.

Comment 28

[Simon Paquet [:sipaq]]

Followup-patch as suggested in comment 26 has been checked in.

Comment 29

[Sven Giermann]

Attachment: Additionally provide (disabled) context-menu label for Multiweek view

Maybe I'm too late (as usual), but I recognized that the (disabled) context menu in the calendar view pane for 'Next'/'Previous' are not being set in multiweek view.

This patch addresses this and sets the same label as for week view (this is the same in Sunbird menu 'Go'; no additional label needed).

Comment 30

[Martin Schröder [:mschroeder]]

Reopening for small patch to add missing labels.

Comment 31

[Michael Büttner (no reviews TFN)]

Comment on attachment 274759 [details] [diff] [review]
Additionally provide (disabled) context-menu label for Multiweek view

Thanks for taking care, I didn't even notice this issue. I also took the liberty to remove the "disabled=true" attribute, as otherwise the context menu wouldn't make much sense. r=mickey. Thanks for the patch, Sven.

Comment 32

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 33

[Martin Schröder [:mschroeder]]

Verified with Tb 2.0.0.7pre + Lightning 0.7pre (2007080404) on WinXP.

Comment 34

[Hannes Erven]

Cool thing, thank you folks!

I noticed that the "Multiweek" Button now appears on the Calendar Tool Bar, but it cannot be added to the Mail Toolbar (which is possible with Today, New Event, Week, Day and Month buttons). Is this by design?

Comment 35

[Sven Giermann]

(In reply to comment #34)
> Is this by design?
> 

Yes, it is. You may notice, that you cannot add the other buttons too - you just have them, if you added them before the update.
(See bug 396144)