363961 - Allow display of several months in left minimonth pane

Status: NEW

(Calendar :: Calendar Frontend, enhancement)

Description

[Thomas Landauer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Sunbird 0.3

In the left date pane (minimonth) there's always just 1 month displayed, even if there were room for more.

Reproducible: Always

Comment 1

[Thomas Landauer]

Attachment: Screenshot of the problem (monitor in portrait format)

Comment 2

[Damian Szczepanik]

I don't think we will support it, some users have them panel with tasks so this should be rather extension

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

We should support this. The amount of months shown should depends on the available space.

Comment 4

[cmtalbert]

Confirming as per Michiel's comment. The new UI designs for 0.5 have already proposed some solutions to this problem.

Comment 5

[seshu.parvataneni]

I agree to this feature. It would be very useful to have such kind of a view. It will be useful in planning events. A brief description of the feature is explained here:
http://wiki.mozilla.org/Calendar:Calendar_View#Mini_Month_Pane

Comment 7

[Canis Lupus Campestris]

What about 0.9? New calendar view allows to do it IMHO.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 248745 [details]
Screenshot of the problem (monitor in portrait format)

Christian, is this something we want to do?

Comment 10

[Christian Jansen]

I think it would make sense to have the option to show more than 1 month. iCal, Outlook offers same functionality.

Comment 11

[Christian Jansen]

Comment on attachment 248745 [details]
Screenshot of the problem (monitor in portrait format)

I've reviewed the problem, and yes we should use the space more useful. +1 for showing more than 1 month.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This should be fairly easy to do. The following hints may help:

* To make a pane resizable, there must be a <splitter/> element between the two boxes.

* There are "overflow" and "underflow" events (use onoverflow="foo" on the xul element) that help find out if there is enough space or not.

* In the views we have something similar where we show the full month name when there is enough room and only the abbreviated month name if there is little room. Maybe it helps to look there.

If someone wants to step up here and needs help, feel free to contact me.

Comment 14

[santom]

Attachment: patch for bugid 363961

Comment 15

[santom]

Attachment: patch for bugid 363961

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 598286 [details]
patch for bugid 363961

>+var total_hieght = null;
Looks like the total_height variable isn't used, please remove.

>+function doresize(){
>+    var height = document.getElementById("minimonth-pane").boxObject.height;
>+    var count= parseInt(height/calender_height);
Instead of using a global variable, do you think you could dynamically calculate calendar_height here? Also, instead of using parseInt I think it would be better to use Math.floor().


>+    var length = document.getElementById("calMinimonthBox1").childNodes.length;
>+    if(count>1){
>+        var myNode = document.getElementById("calMinimonthBox1");
>+        while (myNode.firstChild) {
>+            myNode.removeChild(myNode.firstChild);
>+        }
The minimonths retrieve all items for that month to mark the bold days, so it would be better to re-use the minimonths that are already there.


>+        for(var j =1;j<=count;j++){
>+            var now = new Date();
>+            var node = document.createElement("minimonth");
>+            node.setAttribute("id","calMinimonth"+j);
>+            node.setAttribute("onchange","minimonthPick(this.value);");
>+            node.setAttribute("freebusy","true");
>+            document.getElementById("calMinimonthBox1").appendChild(node);
>+            node.showMonth(new Date(now.getFullYear(), now.getMonth()+(j-1), 1));
I haven't actually tested the patch, this is just from code inspection, but could you make it so the current month is in the middle? If its an even number of months then just above the middle.

Another thing to think about (maybe you have already solved this), the user can change the month using the arrow and dropdowns. To avoid confusion with different months shown, as soon as the user has chosen a different month in one of the pickers, then the other pickers should adapt so that the minimonths are still in sequence.

>+    if(count == 1){
>+        var myNode = document.getElementById("calMinimonthBox1");
>+            if(myNode.childNodes.length>1)
>+               { myNode.removeChild(myNode.getElementsByTagName("minimonth")[1]);
>+                }
>+                //alert(document.getElementById("calendar-panel").height );
>+        }
Could you explain this section further?


>               <hbox id="calMinimonthBox" pack="center">
>+                <vbox id="calMinimonthBox1">
>+                    <minimonth id="calMinimonth" onchange="minimonthPick(this.value);" freebusy="true"/>
>+                </vbox>
>               </hbox>
You can solve this with one box instead of two:
<vbox id="minimonth-box" pack="center" align="center">
  <minimonth id="minimonth-1" onchange.../>
</vbox>


I'm setting review- so you can take care of the comments above, but you are going in the right direction. Great work on getting started, I am pleasantly surprised you finished this patch so quickly!

Next time you upload a patch, please set review to ? and then enter :Fallen in the textbox that shows up.

Comment 17

[santom]

Attachment: new patch for bugid 363961

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 598537 [details]
new patch for bugid 363961

We've discussed this on IRC and a new patch is coming up.

Comment 19

[santom]

Attachment: updated patch

Comment 20

[santom]

Attachment: update bug

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hey Santom,

I've now tested your patch and the following issues caught my eye:

Issue #1:

1. Make 2 months show (i.e Apr May)
2. Click on May first in the second month
3. Click on a day in April not shown in May (i.e Apr 11th)
--> Two different days are selected

Issue #2:

1. Make 2 months show (i.e Apr May)
2. Resize the months pane so that its barely there, i.e the smallest possible
--> May doesn't go away and overlaps into the calendar list

Issue #3:
1. Make 2 months show (i.e May Jun)
2. Select June 1st in the calendar view
--> June 1st is only selected in the May minimonth, not in the June minimonth.

Issue #4 (somewhat optional):
1. Start resizing the pane, make it larger
--> The minimonth only show up when resizing is complete. It would be better if it were visible already while dragging. This will improve user experience, since the user will quickly notice why resizing makes sense.

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 599600 [details]
update bug

r- based on previous comment

Comment 24

[Chris Nelson]

I don't see any activity since a new patch 6 months ago.  What's the status of this issue?

Comment 25

[Thorsten]

I would also be very interested in having this feature being implemented. Is there any activity on this issue?

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Given santom didn't answer comment 24, I doubt there will be any more progress from him. If someone wants to pick up the patch attached to the bug and fix the remaining issues, I would be delighted.

Comment 27

[Reid Anderson]

I'd like to take this and work on it.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hi Reid, thanks for picking this up! Build instructions for Lightning can be found here: https://developer.mozilla.org/En/Simple_Thunderbird_build

If you need additional help, please contact me via IRC (irc.mozilla.org, #calendar, my nick is Fallen) or email.

Comment 29

[Reid Anderson]

Attachment: First patch

I think this addresses all the comments made on the previous assignee's final patch.

Comment 30

[Decathlon]

I tested a bit the patch and overall it works but there are issues that must be addressed:

- a pair of errors appear in the Error Console when a minimonth disappears resizing the panel with the splitter. It happens not always and in different ways, I have not found a precise rule, but it's always reproducible  making visible 4 minimonths then changing the number to 3 just after TB has restarted:

Error: this.mRealListener.onOperationComplete is not a function
Source File: file:///C:/Users/.../components/calCompositeCalendar.js
Line: 520

[JavaScript Error: "[Exception... "[JavaScript Error: "this.mRealListener.onOperationComplete is not a function" {file: "file:///C:/Users/.../components/calCompositeCalendar.js" line: 520}]'[JavaScript Error: "this.mRealListener.onOperationComplete is not a function" {file: "file:///C:/Users/.../components/calCompositeCalendar.js" line: 520}]' when calling method: [calIOperationListener::onOperationComplete]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://calendar/modules/calProviderUtils.jsm :: cPB_notifyPureOperationComplete :: line 659"  data: yes]"]

- (this is a particular case of issue #1 in comment 21) you can have two different days selected in two different minimonths: today (current month) and one day in the previous month.
To reproduce: restart TB; change the month displayed in the minimonth to the month previous the current month; select a day in this month; drag the splitter in order to show up another minimonth (the current month)
--> today is selected in the new minimonth along with the day in the first minimonth;

- (this is similar to issue #2 in comment 21) with more than one minimonth, when you move upward the splitter, the minimonths overlap the calendar list as long as you are dragging the splitter. They disappear only when releasing the splitter. Probably the panel or some box needs a "overflow: hidden" style;

- (again similar to issue #2 in comment 21) display more minimonths then drag the splitter upward in order to hide completely the minimonth panel and release the splitter, now drag downward a bit the splitter (or click on the grippy)
--> all the previous minimonths are displayed again, but the splitter and the calendar list are placed after the first minimonth and all the others minimonths overlap the calendar list.

- every new minimonth added by moving downward the splitter doesn't have the background color and the border in the header of the minimonth. The header gets the style only after opening a new window (e.g. a new event or the Error console);

- I'm not sure if this is a correct or bad behavior but when you have a selected day inside a minimonth which is not the first one in the panel and then you add or hide another minimonth, all the sequence of minimonths get repositioned upward in the panel in order to place as first minimonth that one with the selected day inside;

- issue #4 in comment 21 (optional) is still present:
  "start resizing the pane, make it larger
   --> The minimonth only show up when resizing is complete..."
here I agree that would be preferable to see partially the new minimonths while dragging the splitter even if the height is not sufficient to show a complete minimonth;

- the splitter needs a css rule in order to allow to change the look (grippy, tickness, borders etc.) based on the OS and the platform, maybe only a common style in this patch and further details with another bug;

- would be nice if the panel remembered the number of minimonths displayed when Thunderbird restarts;


r- based on these issues.

A quick review of the code (mainly code style) follows.

Comment 31

[Decathlon]

Comment on attachment 8388734 [details] [diff] [review]
First patch

Review of attachment 8388734 [details] [diff] [review]:
-----------------------------------------------------------------

In all the patch you should use the Calendar style guide for the code unless the file you are working on has a completely different style:
https://wiki.mozilla.org/Calendar:Style_Guide
so you should add spaces between parenthesis and the keywords "if", "for", "while" etc., spaces between operators, space beside the semicolon and the bracket etc. (see the link). All the conditional and loop statements in the patch need these changes.

::: calendar/base/content/calendar-views.js
@@ +587,5 @@
>          jsMainDate = new Date(mainDate.year, mainDate.month, mainDate.day);
>      }
>  
> +    // If there is more than one minimonth, make sure we select the correct date
> +    var minimonthNode = document.getElementById("calMinimonthBox1");

You should use "let" instead of "var" for declarations in all the patch.
With "let" the variables are block-scoped (see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let )
so it doesn't allow to use the variable "i" as you did for the index in some "for" loop in the patch. In that case if you use "let" you have to declare the variable "i" before the "for" loop.

@@ +589,5 @@
>  
> +    // If there is more than one minimonth, make sure we select the correct date
> +    var minimonthNode = document.getElementById("calMinimonthBox1");
> +    var childLength = minimonthNode.childNodes.length;
> +    for(i=0;i<childLength;i++) { 

Missing declaration of the variable "i".
There is a trailing space in this line.

@@ +590,5 @@
> +    // If there is more than one minimonth, make sure we select the correct date
> +    var minimonthNode = document.getElementById("calMinimonthBox1");
> +    var childLength = minimonthNode.childNodes.length;
> +    for(i=0;i<childLength;i++) { 
> +        var temp = minimonthNode.getElementsByTagName("minimonth")[i];

a meaningful name for this variable instead of "temp" would be preferable

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +143,5 @@
> +    var count = Math.floor(paneHeight/calender_height);
> +    var numChildren = minimonthBox.childNodes.length;
> +    var now = new Date();
> +    var subby = Math.floor(count/2);
> +    

trailing spaces

@@ +145,5 @@
> +    var now = new Date();
> +    var subby = Math.floor(count/2);
> +    
> +    if(count>1){
> +        for(i=0;i<numChildren && i<count;i++){

Missing declaration of the variable "i".
Since you use the value after the "for" loop, you could use "var" here but would be better declaring "i" with "let" before the "for" block.

@@ +146,5 @@
> +    var subby = Math.floor(count/2);
> +    
> +    if(count>1){
> +        for(i=0;i<numChildren && i<count;i++){
> +            var temp = document.getElementById("calMinimonthBox1").getElementsByTagName("minimonth")[i];

Here again a meaningful name instead of "temp" is preferable, more than the previous case ;-)
You have already declared above the variable minimonthBox = document.getElementById("calMinimonthBox1") so here you can use it instead of calling again the function getElementById

@@ +148,5 @@
> +    if(count>1){
> +        for(i=0;i<numChildren && i<count;i++){
> +            var temp = document.getElementById("calMinimonthBox1").getElementsByTagName("minimonth")[i];
> +            if(i==0) {
> +                temp.addEventListener("monthchange",refreshcal,false);

one space after the commas

@@ +149,5 @@
> +        for(i=0;i<numChildren && i<count;i++){
> +            var temp = document.getElementById("calMinimonthBox1").getElementsByTagName("minimonth")[i];
> +            if(i==0) {
> +                temp.addEventListener("monthchange",refreshcal,false);
> +			}

here there are tabs instead of spaces for indentation

@@ +157,5 @@
> +                temp.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby)));
> +            }
> +        }
> +        for(var j=i;j<numChildren;j++){
> +            var myNode = document.getElementById("calMinimonthBox1");

myNode is minimonthBox, already declared above

@@ +166,5 @@
> +            node.setAttribute("id","calMinimonth"+i);
> +            node.setAttribute("onchange","minimonthPick(this.value)");
> +            node.setAttribute("freebusy","true");
> +            node.setAttribute("iposition",i);
> +            document.getElementById("calMinimonthBox1").appendChild(node);

minimonthBox instead of document.getElementById("calMinimonthBox1")

@@ +167,5 @@
> +            node.setAttribute("onchange","minimonthPick(this.value)");
> +            node.setAttribute("freebusy","true");
> +            node.setAttribute("iposition",i);
> +            document.getElementById("calMinimonthBox1").appendChild(node);
> +            if(count%2==0)

according to the code style, "if-else" statements need the brackets even for one line of code

@@ +168,5 @@
> +            node.setAttribute("freebusy","true");
> +            node.setAttribute("iposition",i);
> +            document.getElementById("calMinimonthBox1").appendChild(node);
> +            if(count%2==0)
> +				node.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby+1)));

tabs instead of spaces

@@ +169,5 @@
> +            node.setAttribute("iposition",i);
> +            document.getElementById("calMinimonthBox1").appendChild(node);
> +            if(count%2==0)
> +				node.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby+1)));
> +            else node.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby)));++i;

a new line for ++i;

@@ +170,5 @@
> +            document.getElementById("calMinimonthBox1").appendChild(node);
> +            if(count%2==0)
> +				node.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby+1)));
> +            else node.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby)));++i;
> +				node.addEventListener("monthchange",refreshcal,false);

tabs instead of spaces

@@ +173,5 @@
> +            else node.showMonth(new Date(now.getFullYear(), now.getMonth()+(i-subby)));++i;
> +				node.addEventListener("monthchange",refreshcal,false);
> +        }
> +    }
> +   

trailing spaces

@@ +175,5 @@
> +        }
> +    }
> +   
> +    if(count == 1){
> +        var myNode = document.getElementById("calMinimonthBox1");

no needs for myNode because you already have minimonthBox

@@ +178,5 @@
> +    if(count == 1){
> +        var myNode = document.getElementById("calMinimonthBox1");
> +        var childLength = myNode.childNodes.length;
> +        while(childLength>1) { 
> +            myNode.removeChild(myNode.getElementsByTagName("minimonth")[1]);childLength--;

childLength--; on a new line

@@ +181,5 @@
> +        while(childLength>1) { 
> +            myNode.removeChild(myNode.getElementsByTagName("minimonth")[1]);childLength--;
> +        }
> +    }
> +    

trailing spaces

@@ +183,5 @@
> +        }
> +    }
> +    
> +    // We always want the current selected date to show in the minimonth
> +    var temp = document.getElementById("calMinimonthBox1").getElementsByTagName("minimonth")[0];

minimonthBox instead of document.getElementById("calMinimonthBox1") and a meaningful name instead of "temp"

@@ +193,5 @@
> +    var month = document.getAnonymousElementByAttribute(this, "anonid", "minimonth-header").getAttribute("month");
> +    var year = document.getAnonymousElementByAttribute(this, "anonid", "minimonth-header").getAttribute("year");
> +    var minimonthBox = document.getElementById("calMinimonthBox1");
> +    var numChildren = minimonthBox.childNodes.length;
> +    var now = new Date(year, month, 1); var eachminimonth;

var eachminimonth; on a new line

@@ +194,5 @@
> +    var year = document.getAnonymousElementByAttribute(this, "anonid", "minimonth-header").getAttribute("year");
> +    var minimonthBox = document.getElementById("calMinimonthBox1");
> +    var numChildren = minimonthBox.childNodes.length;
> +    var now = new Date(year, month, 1); var eachminimonth;
> +    

trailing spaces

@@ +199,5 @@
> +    for(var i=position-1,j=1;i>=0;i--,++j){
> +        eachminimonth = minimonthBox.getElementsByTagName("minimonth")[i];
> +        eachminimonth.showMonth(new Date(now.getFullYear(), now.getMonth()-j, 1));
> +    }
> +    

trailing spaces

@@ +200,5 @@
> +        eachminimonth = minimonthBox.getElementsByTagName("minimonth")[i];
> +        eachminimonth.showMonth(new Date(now.getFullYear(), now.getMonth()-j, 1));
> +    }
> +    
> +    for(var ii=position+1,j=1;i<numChildren;ii++,++j){

here you are checking the "for" loop with two variables that are not used and don't change inside the loop. Should it be "ii" instead of "i"?
Moreover it seems that here you can use a unique index inside the loop (maybe even in the previous loops, but in this case it seems simpler)

@@ +204,5 @@
> +    for(var ii=position+1,j=1;i<numChildren;ii++,++j){
> +        eachminimonth = minimonthBox.getElementsByTagName("minimonth")[ii];
> +        eachminimonth.showMonth(new Date(now.getFullYear(), now.getMonth()+j, 1));
> +    }
> +    

trailing spaces

@@ +213,3 @@
>  window.addEventListener("load", function(e) {
> +    var minimonthBox = document.getElementById("calMinimonthBox1");
> +    document.getElementById("minimonth-pane").setAttribute("minheight",minimonthBox.getElementsByTagName("minimonth")[0].boxObject.height);

maybe you could shorten this line by breaking it in two lines or, better, by using a temporary variable for the second argument of setAttribute, something like
let minimonthBoxHeight = minimonthBox.getElementsByTagName("minimonth")[0].boxObject.height

::: calendar/lightning/content/messenger-overlay-sidebar.xul
@@ +143,5 @@
>                width="200"
>                persist="collapsed width">
>            <modevbox id="minimonth-pane" mode="calendar,task" broadcaster="modeBroadcaster" refcontrol="calendar_toggle_minimonthpane_command">
> +            <vbox id="calMinimonthBox1" align="center" pack="center">
> +                    <minimonth id="calMinimonth" onchange="minimonthPick(this.value)" freebusy="true"/>

here the file has two spaces indentation

@@ +148,3 @@
>              </vbox>
>            </modevbox>
> +          <splitter id="minimonth-splitter" oncommand="doresize()" collapse="before" > <grippy/></splitter>

<grippy/> and </splitter> should be be placed on two different lines with indentation.

Comment 32

[Wayne Mery (:wsmwk)]

Reid indicates he doesn't know when he'll get back to this