Closed Bug 323093 Opened 19 years ago Closed 18 years ago

24-hours views

Categories

(Calendar :: Internal Components, defect)

Product:
Calendar
Component:
Internal Components
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dmosedale, Assigned: thomas.benisch)

References

Details

Attachments

(16 files, 7 obsolete files)

scrollexample.xul an example which uses a scrollbox for scrolling
3.77 KB, application/vnd.mozilla.xul+xml
Details
patch implements 24-hour by default, working hours and vertical scrollbar
37.66 KB, patch
Details | Diff | Splinter Review
calendar-week-view.png screenshot showing 24-hours by default, work hours and vertical scrollbar
55.44 KB, image/png
Details
patch v4
16.40 KB, patch
dmosedale
: first-review+
dmosedale
: second-review+
Details | Diff | Splinter Review
scroll-container patch
22.39 KB, patch
Details | Diff | Splinter Review
screenshot of the calendar week view
65.18 KB, image/png
Details
resize experiment 1
526 bytes, patch
Details | Diff | Splinter Review
resize experiment 2
1.07 KB, patch
Details | Diff | Splinter Review
handmade scrolling with scrollbox
22.33 KB, patch
Details | Diff | Splinter Review
automatic scrolling with scrollbox
12.10 KB, patch
Details | Diff | Splinter Review
weekview1.png (with patch from comment #74)
72.99 KB, image/png
Details
weekview2.png (with patch from comment #74)
72.46 KB, image/png
Details
weekview3.png (with patch from comment #74)
72.43 KB, image/png
Details
weekview4.png (with patch from comment #74)
74.68 KB, image/png
Details
weekview5.png (with patch from comment #74)
74.29 KB, image/png
Details
simple automatic scrolling by using scrollbox v5
21.63 KB, patch
thomas.benisch
: first-review+
Details | Diff | Splinter Review 
This isn't exactly a dataloss bug, but it's almost equivalent, since it's possible to completely lose track of events and not know it.
I do see the events outside the range, it's just that the grid isn't drawn. So it's really ugly.
But I still think that we should always display all hours, just show the 'workday' in a different color, and scroll it into view.
Assignee: base → dmose
I plan to work on this task. Any objections?
Assignee: dmose → thomas.benisch
As far as I understand the idea is to display 24 hours by default 
and to use a different colour for the working hours.

In the current versions of Lightning and SunBird the start time and 
end time of a day can be configured in the UI, but there's no concept 
of working hours yet. 

(1) I propose to display always 24 hours per day and configure only the
working hours in the UI. The working hours will be displayed in a different 
colour.

(2) An alternative way would be to configure both in the UI, start/end 
time of a day and start/end time of the working hours, but I think this 
is rather confusing for the user.

(1a) If one goes for (1), then there's the question, if I can reuse the
start/end time properties and methods

  calendar.view.defaultstarthour
  calendar.view.defaultendhour
  mStartMin
  mEndMin
  setStartEndMinutes

for the working hours. As a consequence, some code in the
"relayout" methods gets obsolete, e.g. adjusting event boxes to
the start/end time of a day. 

(1b) Also here an alternative approach would be to introduce new 
properties for the working hours, that means in the code both, 
start/end time of a day AND working hours can be configured. 
In the UI one can offer to configure only the working hours.

I favour (1a), but probably someone can advise me, if this is 
the right approach.

(In reply to comment #1)
> I do see the events outside the range, it's just that the grid isn't drawn. So
> it's really ugly.
> But I still think that we should always display all hours, just show the
> 'workday' in a different color, and scroll it into view.
> 

Hmm, I had assumed we wanted to continue with the way the old Sunbird views work.    It would be interesting to know what iCal and Outlook do here.  I generally like to have my window sized such that it displays my "working" hours as well as my "evening" hours.  I'm not sure if this is typical or not.
(In reply to comment #3)
> As far as I understand the idea is to display 24 hours by default 
> and to use a different colour for the working hours.
> 
> In the current versions of Lightning and SunBird the start time and 
> end time of a day can be configured in the UI, but there's no concept 
> of working hours yet. 
> 
> (1) I propose to display always 24 hours per day and configure only the
> working hours in the UI. The working hours will be displayed in a different 
> colour.
> 
> (2) An alternative way would be to configure both in the UI, start/end 
> time of a day and start/end time of the working hours, but I think this 
> is rather confusing for the user.
> 
> (1a) If one goes for (1), then there's the question, if I can reuse the
> start/end time properties and methods
> 
>   calendar.view.defaultstarthour
>   calendar.view.defaultendhour
>   mStartMin
>   mEndMin
>   setStartEndMinutes
> 
> for the working hours. As a consequence, some code in the
> "relayout" methods gets obsolete, e.g. adjusting event boxes to
> the start/end time of a day. 
> 
> (1b) Also here an alternative approach would be to introduce new 
> properties for the working hours, that means in the code both, 
> start/end time of a day AND working hours can be configured. 
> In the UI one can offer to configure only the working hours.
> 
> I favour (1a), but probably someone can advise me, if this is 
> the right approach.

Meanwhile I came to the conclusion, that probably (1b) is the right approach.
Any comments?


(In reply to comment #4)
> (In reply to comment #1)
> > I do see the events outside the range, it's just that the grid isn't drawn. So
> > it's really ugly.
> > But I still think that we should always display all hours, just show the
> > 'workday' in a different color, and scroll it into view.
> > 
> 
> Hmm, I had assumed we wanted to continue with the way the old Sunbird views
> work.    It would be interesting to know what iCal and Outlook do here.  I
> generally like to have my window sized such that it displays my "working" hours
> as well as my "evening" hours.  I'm not sure if this is typical or not.

Probably someone can enlight me, how the old Sunbird views worked.

I had a look at Outlook, and there by default 24 hours per day are displayed.
The start and end time of the working hours can be configured in the UI.
But I haven't found any way to configure the start and end time of a day
in the UI.


(note: it's not needed to quote entire comments. It's easy to look them up)

old sunbird showed only the working hours, and if there are events outside working hours, it also shows those hours.
But i never liked it. It makoes the view jump up and down when browsing though weeks. I think we should always show all hours. (evolution does this, outlook does)

With that in mind, I vote for option 1a. Less code, less options, less codepaths to test.
This seems to touch on a conversation we were having earlier on what to do about one day events that cross the midnight line.  If it was decided that being able to configure day start/end in the UI is the answer to that, then that would affect which options here are acceptable. Is there a bug for that problem that we can link this with?
(In reply to comment #7)
> old sunbird showed only the working hours, and if there are events outside
> working hours, it also shows those hours.
> But i never liked it. It makoes the view jump up and down when browsing though
> weeks. I think we should always show all hours. (evolution does this, outlook
> does)
> 
> With that in mind, I vote for option 1a. Less code, less options, less
> codepaths to test.

iCal shows all 24 hours as well.  Since I have no compelling argument for the old behavior other than "I kinda liked it", I see no reason to swim against the current on this one.

iCal does 1(b) which would seem to put it in line with Outlook.

Doing what people are used to would seem to suggest 1(b), while minimizing prefs and codepaths would suggest 1(a).  I can see why, if 24 hours at a time are being shown, it would be nice to have a visual cue for looking at and/or centering your schedule.  I guess I would lean slightly towards 1(b) as a result, but I don't feel extremely strongly about that.

One possibility would be to try 1(a) for now, and then switch to 1(b) later if people scream too loudly.

For what it's worth, iCal just refers to this as the "Day" not the "Workday".

(In reply to comment #8)
> This seems to touch on a conversation we were having earlier on what to do
> about one day events that cross the midnight line. 

That was bug 298349.

> If it was decided that being able to configure day start/end in the UI is the
> answer to that, then that would affect which options here are acceptable.

Well, I think this bug is the bigger issue, and that is just sort of an implementation detail that we may or may not have to deal with depending on how we decide this one.  Assuming that we go with the 24 hour display approach, then we'd probably want a pref for shift workers about what hour to start & end the 24-hour period with.
One problem with showing the whole 24 hours: The header will almost always be scrolled out of the view. We should fix that first.
1. One downside to the scrolling design is there is no clue whether you have an event before the normal start hour or after the normal end hour.  You have to manually scroll every day to make sure.  So, for example, you might overlook that 7am breakfast meeting if your hours normally start at 9 and you forgot to scroll up to check.  With the Sunbird 0.2 design, which lists the early hours only if there is an event, you can glance at the scrollbar to see if there are any events outside normal hours.  (Especially evident if the scrollbar normally disappears.)  

So if 24hour scrolling is implemented, it would be nice if there was some clue, such as icons that light up in the scrollbar, when there are events scrolled above or below the visible area.  (If they light up in a location in the scroll bar proportional to the location of the offscreen hour, the scroll bar then becomes a graphical non-text overview of the day.)

2. As mvl suggests, Bug 322979 (non-scrolling headers, functionality previously implemented in Bug 242317), should probably be implemented either first or as part of a scrolling redesign.  The design may also accomodate day-break not at midnight (bug 325678).

3. The scrolling approach has long been a desirable design, but in the past it has been impossible to implement a method to scroll to a particular hour in XUL.  See for example bug 133107, where bug 133107 comment 8 suggests is blocked by bug 62536.  This is also discussed in Bug 253777.  But all that was a while ago, maybe there is a way now.  I suggest building a small test example to see if scrolling to an xul element works, and works with elements in xul stacks like in multiday-view, before trying it with the full complexity of multiday-view.

(other references:  

Exploring the code in Bonsai, it looks like restricting view hours seems to have been implemented for dayView.js by Mike Potter on 2002-11-22 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=dayView.js&branch=SUNBIRD_0_2_BRANCH&root=/cvsroot&subdir=mozilla/calendar/resources/content&command=DIFF_FRAMESET&rev1=1.34&rev2=1.35

There seems to be no bugzilla bug.  It looks like it was suggested at 
http://groups.google.com/group/netscape.public.mozilla.calendar/tree/browse_frm/thread/e3801b44eade3edc/1a3c65d798fe9239?rnum=1&hl=en&q=group%3Anetscape.public.mozilla.calendar&_done=%2Fgroup%2Fnetscape.public.mozilla.calendar%2Fbrowse_frm%2Fthread%2Fe3801b44eade3edc%2Fcd51d2f60793838c%3Flnk%3Dst%26q%3Dgroup%3Anetscape.public.mozilla.calendar%26rnum%3D2%26hl%3Den%26#doc_77a7234cbe8c0a99
Ken Pardue:
> most calendar software I've seen shows the
> time from 8am to 5 pm on the calendar, and if anything is before or
> after those times are added to the top or bottom of the calendar
> ... Scrolling [from midnight] is a big turn off ...
Mike Potter:
> This is a known bug, and probably the most annoying bug in the calendar
> program.  I have no idea how to fix it: there is no easy way to scroll a
> XUL box to a certain item.

And the workaround announced at 
http://groups.google.com/group/netscape.public.mozilla.calendar/tree/browse_frm/thread/14df8d5535e5f46c/16254779625478c3?rnum=1&hl=en&q=group%3Anetscape.public.mozilla.calendar&_done=%2Fgroup%2Fnetscape.public.mozilla.calendar%2Fbrowse_frm%2Fthread%2F14df8d5535e5f46c%2F6236af8a2a76b530%3Flnk%3Dst%26q%3Dgroup%3Anetscape.public.mozilla.calendar%26rnum%3D8%26hl%3Den%26#doc_16254779625478c3
Mike Potter:
> There's a new build up today that will only show you certain hours in a
> day, unless you have an event outside of the range you specify.
> This doesn't *exactly* fix the problem of not being able to scroll to a
> certain hour.  Instead, it works around that problem by not showing you
> hours where you don't have events in.
> You can set the start and end dates in the prefs panel. 

)
gekacheka and mvl make very good points.  Given the various fixes necessary to give a reasonable full-scrolling experience, and since the auto-expanding hack looks like it would be a bunch less work, I guess I'd vote for doing the auto-expanding hack in order to ship 0.1, and moving to the full-scrolling experience as part of the war on boxes.  What do folks think?
I personally favour the 24 hours approach, but I will have a look at the
scrolling issues first.
I think everyone's pretty much in agreement that the 24-hour view is the better fix for the long run, but doing the necessary scrolling pre-requisites is almost certain to entrain various box-structure and XUL issues, so going down that route for 0.1 is likely to take too long.
This test example uses a scrollbox and the
nsIScrollBoxObject interface. With the scrollTo() method
it's possible to scroll the content of the scrollbox.
The parameters for the scrollTo() method are calculated
from the boxObject.y property of different boxes.
In this example, on every window onload event the
scrollbox is vertically scrolled to the box with
description "1:00".
When playing around with boxes and scroll bars I noticed,
that there are indeed a lot of problems with scroll bars
and scrolling. Nevertheless I wrote a small example 
(scrollexample.xul, see attachment), which uses a scrollbox 
and the nsIScrollBoxObject interface.
I think in principal this approach can also be used for
the calendar-multiday-view.

I guess the main problem with the 24-hour approach is
the vertical scrolling. Let's therefore restrict in a
first attempt to vertical scrolling. By the way, also Outlook 
only offers a vertical scroll bar. In the 
calendar-multiweek-view one can use a scrollbox instead of
a box for the "childbox". Style will be something like
"overflow-x: hidden; overflow-y:auto;". When the scroll bar
at the scrollbox appears, the layout is messed up. Probably
this can be solved by adding some spacers to "labelbox",
"headerbox" and "childbox". When loading the calendar-multiday-view,
the scrollbox can be scrolled to the first working hour
or to the earliest event of the day/week.

I agree that for now, we should just make the views expand when needed. Scrolling and showing 24 hours can de done later.
This patch displays 24 hours by default and uses a different
color for the working hours. The start and end time of the
working hours can be configured in the UI. At the moment the
start and end time of a day are misused
(Tools/Options/Lightning/Week View).
This patch restricts to a vertical scroll bar only, I didn't get
a working horizontal scroll bar.
When loading the calendar-multiday-view, the scrollbox is
scrolled to the first working hour.
At the moment the whole view looks a bit squeezed, because
the default height of the column-lineboxes is too small.
This can be fixed by using a minimum value for pixels
per minute. But the in the squeezed view all the changes
can be seen, when a scrollbar appears and disappears.
Attachment #211845 - Flags: first-review?(dmose)
I submitted a first patch for review. This patch still requires some minor
improvements (e.g. minimum value for pixels per minute).
I didn't ignore all your comments, but I wanted to present what I have worked
on. Probably this patch is only something for past Lightning 0.1.

Comment on attachment 211845 [details] [diff] [review]
patch implements 24-hour by default, working hours and vertical scrollbar

>diff -x CVS -U 8 -pN -r mozilla_ref/toolkit/content/widgets/scrollbox.xml 

Getting review would be much easier if you could somehow work around the need for the change to this toolkit file. A change to toolkit would need much more testing for regressions, and needs review from different persons. Also, we can't just check it in on the 1.8 branch.


I tried this patch, and it didn't seem to work for me. I still only saw normal work hours. Maybe something with your reuse of old prefs?

I think this patch will need some more iterations before it is ready. I really like the idea, and reaaly want to get it in, but i think it's not safe enough for 0.1 at this stage.
So I think we should go for autoexpand for 0.1, and go for this 24hour patch after 0.1 is released.
(In reply to comment #21)
> Maybe something with your reuse of old prefs?

What i mean: I still have the prefs that used to set the workday. But that pref now means the start of the day to display. The default should be that the old prefs still set the workday (as the UI suggested it did), and the prefs to set the start and end of the view be new prefs, defaulting to 0 and 24.
Comment on attachment 211845 [details] [diff] [review]
patch implements 24-hour by default, working hours and vertical scrollbar

+      <preference id="calendar.view.defaultstartworkhour" name="calendar.view.defaultstartworkhour" type="int"/>
+      <preference id="calendar.view.defaultendworkhour" name="calendar.view.defaultendworkhour" type="int"/>

+pref("calendar.view.defaultstarthour", 0);
+pref("calendar.view.defaultendhour", 24);
+pref("calendar.view.defaultstartworkhour", 8);
+pref("calendar.view.defaultendworkhour", 17);

+defaultStartHour=0
+defaultEndHour=24

These changes don't look sufficient to handle the Sunbird case, and my strong suspicion is that this is the reason why the patch failed for mvl.  I know it's a pain, but we do need to make sure that we keep both platforms in sync as much as possible.  Adding some additional fixes to http://lxr.mozilla.org/mozilla/source/calendar/resources/content/pref/rootCalendarPref.js and http://lxr.mozilla.org/mozilla/source/calendar/resources/content/pref/viewPrefs.xul might help make this patch apply much more smoothly in that case too.
Comment on attachment 211845 [details] [diff] [review]
patch implements 24-hour by default, working hours and vertical scrollbar

The screenshot looks very ince.  In general, this seems like a good longer-term approach to me.  The toolkit change may be the right thing, but it'd be nice to avoid it, since it means that Lightning couldn't work with any versions of Thunderbird that didn't have that fix  (e.g. at least 1.5 and 1.5.0.1) unless we shipped our own fork of scrollbox.xml.  That said, requiring 1.5.0.2 or whatever in the long run might not be such a bad idea, assuming we could convince the powers that be to take that fix into toolkit sooner rather than later.  The other thing this approach probably needs is some sort of UI to indicate when there are events that are scrolled out of view.

Thomas: how much work would it be to put together an autoexpand patch that we could ship 0.1 with in order to buy some more time to sort out the above issues?
(In reply to comment #24)
>
> avoid it, since it means that Lightning couldn't work with any versions of
> Thunderbird that didn't have that fix  (e.g. at least 1.5 and 1.5.0.1) 

Er, I meant 1.5, since 1.5.0.1 was a Firefox-only release.
Whiteboard: [ETA 2/22]
Attached patch patch implements autoexpand (obsolete) — — Splinter Review 
This patch implements autoexpanding of day and week views.

From all events which are displayed in a view, new start and end times for the 
view are calculated. For each view this can be done only, after all events
are available, that means after getItems() at the calendar is called.
As a consequence, if the start and end times change, a second relayout() is
necessary.

The view is also autoexpanded for onAddItem, onModifyItem and onDeleteItem.
If the start and end times change, a whole refresh() is necessary.
Attachment #212775 - Flags: first-review?(dmose)
Comment on attachment 212775 [details] [diff] [review]
patch implements autoexpand

>       <method name="setStartEndMinutes">
>         <parameter name="aStartMin"/>
>         <parameter name="aEndMin"/>
>         <body><![CDATA[
> 	  if (aStartMin < 0 || aStartMin > aEndMin)
>               throw Components.results.NS_ERROR_INVALID_ARG;
>-          if (aEndMin < 0 || aEndMin >= 24*60)
>+          if (aEndMin < 0 || aEndMin > 24*60)
>               throw Components.results.NS_ERROR_INVALID_ARG;

This change looks like it would cause the preferences code to potentially throw an exception when it shouldn't.  Even if this change really is the right thing to do, I don't think we need it for 0.1, and after we switch to 24 hour views, the need should go away completely.

>+      <method name="readjustStartEndMinutes">
>+        <parameter name="aItems"/>
>+        <body><![CDATA[

In general, this looks like a good strategy.  There is a lot of calculation here, and I think we can probably cut code complexity and improve performance a fair amount if we pass in the raw "chunks" to this function instead of the items.  So I'm going to take a run at tweaking the patch to do that.

>+      <method name="readjust">
>+        <body><![CDATA[
>+          if (this.readjustStartEndMinutes(this.getItems())) {
>+              var selectedDay = this.selectedDay;
>+              this.refresh();

It seems to me that we ought to be able to get away with calling relayout here instead of refresh, which should buy some speed as well.

New iteration of the patch coming up.
Attachment #212775 - Flags: first-review?(dmose)
Whiteboard: [ETA 2/22] → [ETA 2/23]
(In reply to comment #27)
> (From update of attachment 212775 [details] [diff] [review] [edit])
> > 	  if (aStartMin < 0 || aStartMin > aEndMin)
> >               throw Components.results.NS_ERROR_INVALID_ARG;
> >-          if (aEndMin < 0 || aEndMin >= 24*60)
> >+          if (aEndMin < 0 || aEndMin > 24*60)
> >               throw Components.results.NS_ERROR_INVALID_ARG;
> 
> This change looks like it would cause the preferences code to potentially throw
> an exception when it shouldn't.  Even if this change really is the right thing
> to do, I don't think we need it for 0.1, and after we switch to 24 hour views,
> the need should go away completely.

Actually this change will make the preferences not to throw. See Bug 327632 and the patch attached to it for details.
Whiteboard: [ETA 2/23] → [ETA 2/24]
(In reply to comment #27)
> (From update of attachment 212775 [details] [diff] [review] [edit])
> >+      <method name="readjustStartEndMinutes">
> >+        <parameter name="aItems"/>
> >+        <body><![CDATA[
> 
> In general, this looks like a good strategy.  There is a lot of calculation
> here, and I think we can probably cut code complexity and improve performance a
> fair amount if we pass in the raw "chunks" to this function instead of the
> items.  So I'm going to take a run at tweaking the patch to do that.

You're right, that you reduce code complexity and improve performance, although
I think that the performance improvement won't be that much. The disadvantage
of this approach is, that you rely on the existence of the column chunk lists, which means that relayout() was already called.

I had the idea, that one calculates all relevant data first and then starts 
drawing. Therefore readjustStartEndMinutes takes a list of items as parameter.
Of course in the current version this idea is only a vision, because when 
switching the views, first relayout() is called (from refresh()), then 
getItems(), after that the start and time of the view is calculated from the
events and if the start and end time differ, relayout() is called a second time.
Finally the event boxes are drawn.

> >+      <method name="readjust">
> >+        <body><![CDATA[
> >+          if (this.readjustStartEndMinutes(this.getItems())) {
> >+              var selectedDay = this.selectedDay;
> >+              this.refresh();
> 
> It seems to me that we ought to be able to get away with calling relayout here
> instead of refresh, which should buy some speed as well.

I think you need to call refresh(). If you call only relayout() on the view,
then the event boxes are not drawn.
Whiteboard: [ETA 2/24] → [ETA 2/28]
(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 212775 [details] [diff] [review] [edit] [edit])
> > >+      <method name="readjustStartEndMinutes">
> > >+        <parameter name="aItems"/>
> > >+        <body><![CDATA[
> > 
> > In general, this looks like a good strategy.  There is a lot of calculation
> > here, and I think we can probably cut code complexity and improve performance a
> > fair amount if we pass in the raw "chunks" to this function instead of the
> > items.  So I'm going to take a run at tweaking the patch to do that.
> You're right, that you reduce code complexity and improve performance, although
> I think that the performance improvement won't be that much. The disadvantage
> of this approach is, that you rely on the existence of the column chunk lists,
> which means that relayout() was already called.

I took a slightly different approach, which was to generate the chunk lists by hand in the cases where they don't already exist.  You're correct that this is still significantly less than ideal on the performance front, but it's a step in the right direction, I believe.

> I had the idea, that one calculates all relevant data first and then starts 
> drawing. Therefore readjustStartEndMinutes takes a list of items as parameter.
> Of course in the current version this idea is only a vision, because when 
> switching the views, first relayout() is called (from refresh()), then 
> getItems(), after that the start and time of the view is calculated from the
> events and if the start and end time differ, relayout() is called a second
> time.

Yeah, it's unfortunate that the existing code structure isn't terribly amenable to this.

> I think you need to call refresh(). If you call only relayout() on the view,
> then the event boxes are not drawn.

You're right; I was misunderstanding what was happening there.  To deal with this, I created a new function call resetColumnsStartEndTimes, which forces all the columns to relayout without actually doing what calendar-multiday-view.relayout does, which is completely drop the columns and the existing chunks.  This should be fairly significant win, I think.

Here's a second iteration through the patch using the strategies I described above.  This also fixes a bug in the first iteration where, with multiple calendars, the onGetResult() calls could race and it was possible to end up with a relayout() call triggered by the second onGetResult blowing away all the events created by the first.

If you search for XXXdmose in the patch, you'll see the one piece that I haven't quite gotten working yet.  If you have time to take that and run with it today, that'd be great.  Otherwise I'll keep going tomorrow...
Attachment #212775 - Attachment is obsolete: true
Just a few thoughts, which came out of a discussion with
mickey.

I think readjustStartEndMinutes() only works, if you call
that for all chunks. I have no idea, how to calculate the
start and end range when calling that function iteratively
for a subset of chunks. 

In the call in onGetResult you may assume, that the start
and end range only increases when called for different
calendars. But for the first calendar this assumption is wrong, 
the start and end range may decrease, e.g. when switching to a
new date range.

Let's assume, that readjustStartEndMinutes() is called
for all chunks only. In this case, the code marked with
XXXdmose would be

    // set the new start and end time for this view
    if (this.mStartMin > earliestChunkStartMin) {
        this.mStartMin = earliestChunkStartMin;
        hasChanged = true;
    } 
    if (this.mEndMin < latestChunkEndMin) {
        this.mEndMin = latestChunkEndMin;
        hasChanged = true;
    }

In onGetResult the code must do something like

    onGetResult: 
    function onGetResult(aCalendar, aStatus, aItemType, aDetail,
                         aCount, aItems) {
        if (!Components.isSuccessCode(aStatus))
            return;

        var allchunks = this.getAllChunks();
        var chunks = this.calView.createChunksForNonDateItems(aItems);
        allchunks.concat(chunks);

        if (this.calView.readjustStartEndMinutes(allchunks, true)) {
            this.calView.resetColumnsStartEndMinutes();
        }

        for each (var item in aItems) {
            this.calView.doAddEvent(item);
        }
    }

That means, in order to get all chunks, the chunks from the view
and the chunks created from aItems are merged.

(In reply to comment #32)
> In the call in onGetResult you may assume, that the start
> and end range only increases when called for different
> calendars. But for the first calendar this assumption is wrong, 
> the start and end range may decrease, e.g. when switching to a
> new date range.

It is correct that readjustStartEndMinutes() can only work iteratively if you assume that calls to it can only need to expand the time-range shown, and never need to shrink it.  Calling setDateRange() calls refresh(), and the first thing that does is call relayout(), which drops all the existing columns entirely.  So when you first arrive in onGetResult(), the columns are brand new and thus at the default (i.e. minimum) size.  So I don't think there's a problem there.   I think there could be a problem if you cause a refresh() to be called before the callbacks for a previous refresh() are finished.  However, that's going to be trouble even without this patch, so I'll file a separate bug on that.
Attached patch autoexpand patch, v3 (obsolete) — — Splinter Review 
This iteration of the patch makes it so that when all chunks have been passed in, the time range can be shrunk or expanded.  I also hoisted some ifs out of the loop, though I suspect that won't make any huge performance difference.  I haven't been able to make this misbehave in the testing I've done so far.  What do you guys think?
Attachment #213429 - Attachment is obsolete: true
Attachment #213517 - Flags: second-review?(thomas.benisch)
Attachment #213517 - Flags: first-review?(jminta)
Whiteboard: [ETA 2/28] → [ETA 2/29]
Whiteboard: [ETA 2/29] → [ETA 3/1]
Comment on attachment 213517 [details] [diff] [review]
autoexpand patch, v3

-using batch mode before getItems and in onOperationComplete ought to improve performance significantly by preventing column relayout for each expansion.

-onAddItem ought to be able to call readjustStartEndMinutes() with the occs from the new item and false, rather than forcing a complete readjust.

-resetColumnsStartEndMinutes() needs to be renamed.  It works with the time-bar as well as the columns.  Furthermore, 'reset' makes me think the defaults are being used again, rather than what is actually happening, where new values are being used.  Maybe "propogate"?

+          var hasChanged = false;
+          const MINS_PER_HOUR = 60;
+          const DAY_START_MIN = 0 * MINS_PER_HOUR;
+          const DAY_END_MIN = 24 * MINS_PER_HOUR;
This should probably be called DAY_END_MAX.

+
+          // find the minimum start time and the maximum end time of
+          // all items, starting at the defaults and expanding as necessary
+          var earliestChunkStartMin = this.mDefaultStartMin;
+          var latestChunkEndMin = this.mDefaultEndMin;
This approach seems like it's going to be slower than necessary for the most common case: getItems().  Anytime that aIsAllChunks is false, we can begin with this.mStartMin/this.mEndMin, instead of the defaults, because the for loop will only expand them and that's all we care about.  With the current code, we still go through all of the logic of this function for every event, even if the first few events max-out the view bounds.  If we use this.mStartMin and the bounds are maxed out, we'll break right away.  We could even do
  if (!aIsAllChunks && this.mStartMin == DAY_START_MIN && this.mEndMin == DAY_END_MAX) {
    return false;
  }
right away, which would be fastest for that case.

+          for each (var chunk in aChunks) {
+
+              if (chunk.startMinute < earliestChunkStartMin) {
+                  earliestChunkStartMin = chunk.startMinute;
+              }
+
+              if (chunk.endMinute > latestChunkEndMin) {
+                  latestChunkEndMin = chunk.endMinute;
+              }
+
+              // break, if the start and end time match the day range
+              if (earliestChunkStartMin < DAY_START_MIN
+                  && latestChunkEndMin > DAY_END_MIN) {
+                  break;
+              }
This last set of comparisons should probably be <= and >=, otherwise this condition will never be true.

+          // don't allow start and end times outside of the day range
+          if (earliestChunkStartMin < DAY_START_MIN) {
+              earliestChunkStartMin = DAY_START_MIN;
+          }
+          if (latestChunkEndMin > DAY_END_MIN) {
+              latestChunkEndMin = DAY_END_MIN;
+          }
Properly done (and as I read the code), these blocks should never be hit.  Each chunk's start/end minutes is required to be between 0 and 24*60, so unless the modulus code pushes us outside these limits (and it doesn't seem to), we ought to be fine removing these.

+                  // here we are doing incremental adjustment, so we 
+                  // can only expand the time shown, never shrink it
+                  if ( this.mStartMin > earliestChunkStartMin ) {
style nit: no spaces around arguments.

Looks good overall.  Most of my comments are small and performance oriented.  r=jminta with those addressed.
Attachment #213517 - Flags: first-review?(jminta) → first-review+
(In reply to comment #35)
> (From update of attachment 213517 [details] [diff] [review] [edit])
> -using batch mode before getItems and in onOperationComplete ought to improve
> performance significantly by preventing column relayout for each expansion.

The problem with doing this is that if somebody has a composite calendar that contains both a local storage calendar and a remote ICS calendar, then nothing gets drawn until the remote calendar is completely loaded.  I could perhaps be talked into making this change anyway, but my gut reaction is that it's not really what we want here.

> -onAddItem ought to be able to call readjustStartEndMinutes() with the occs
> from the new item and false, rather than forcing a complete readjust.

Ah, you're right.  Good catch!

More later...
(In reply to comment #33)
You're right. If someone calls readjustStartEndMinutes(), the 
semantics of this function should be well known.  That is, 
aIsAllChunks == false has the additional meaning of expanding only, 
but this is well documented in the code.

(In reply to comment #35)
> +          var hasChanged = false;
> +          const MINS_PER_HOUR = 60;
> +          const DAY_START_MIN = 0 * MINS_PER_HOUR;
> +          const DAY_END_MIN = 24 * MINS_PER_HOUR;
> This should probably be called DAY_END_MAX.

I think MIN refers to minutes, not minimum.

> +          // find the minimum start time and the maximum end time of
> +          // all items, starting at the defaults and expanding as necessary
> +          var earliestChunkStartMin = this.mDefaultStartMin;
> +          var latestChunkEndMin = this.mDefaultEndMin;
> This approach seems like it's going to be slower than necessary for the most
> common case: getItems().  Anytime that aIsAllChunks is false, we can begin with
> this.mStartMin/this.mEndMin, instead of the defaults, because the for loop will
> only expand them and that's all we care about.  With the current code, we still
> go through all of the logic of this function for every event, even if the first
> few events max-out the view bounds.  If we use this.mStartMin and the bounds
> are maxed out, we'll break right away.  We could even do
>   if (!aIsAllChunks && this.mStartMin == DAY_START_MIN && this.mEndMin ==
> DAY_END_MAX) {
>     return false;
>   }
> right away, which would be fastest for that case.

Joey is right here, so I would write something like

var earliestChunkStartMin = 
    (aIsAllChunks ? this.mDefaultStartMin : this.mStartMin);
var latestChunkEndMin = 
    (aIsAllChunks ? this.mDefaultEndMin : this.mEndMin);

> +          for each (var chunk in aChunks) {
> +
> +              if (chunk.startMinute < earliestChunkStartMin) {
> +                  earliestChunkStartMin = chunk.startMinute;
> +              }
> +
> +              if (chunk.endMinute > latestChunkEndMin) {
> +                  latestChunkEndMin = chunk.endMinute;
> +              }
> +
> +              // break, if the start and end time match the day range
> +              if (earliestChunkStartMin < DAY_START_MIN
> +                  && latestChunkEndMin > DAY_END_MIN) {
> +                  break;
> +              }
> This last set of comparisons should probably be <= and >=, otherwise this
> condition will never be true.

Also here Joey is right, this set of comparisons even must be <= and >=.

> +          // don't allow start and end times outside of the day range
> +          if (earliestChunkStartMin < DAY_START_MIN) {
> +              earliestChunkStartMin = DAY_START_MIN;
> +          }
> +          if (latestChunkEndMin > DAY_END_MIN) {
> +              latestChunkEndMin = DAY_END_MIN;
> +          }
> Properly done (and as I read the code), these blocks should never be hit.  Each
> chunk's start/end minutes is required to be between 0 and 24*60, so unless the
> modulus code pushes us outside these limits (and it doesn't seem to), we ought
> to be fine removing these.

I think, this is just an additional check, which can be omitted, if one relies
on the chunk start/end minutes to be in the range 0 and 24*60.


Apart from my previous comments #37 and #38 I think this patch is fine.

Just a minor issue, I think in various places you used TABS instead of SPACES, 
which looks a bit messy in my editor. What are the guidelines?

In addition, as I'm a newbie, is there anything formal I have to do in order
to approve this patch?

Please make sure to test this patch with events that span multiple days.  That case is always a bit problematic.
Attachment #213517 - Flags: second-review?(thomas.benisch) → second-review+
(In reply to comment #35)
> -resetColumnsStartEndMinutes() needs to be renamed. 

Renamed to propagateStartEndMinutes.

> +                  // here we are doing incremental adjustment, so we 
> +                  // can only expand the time shown, never shrink it
> +                  if ( this.mStartMin > earliestChunkStartMin ) {
> style nit: no spaces around arguments.

Fixed.

(In reply to comment #38)
> (In reply to comment #35)
> > +          const DAY_END_MIN = 24 * MINS_PER_HOUR;
> > This should probably be called DAY_END_MAX.
> I think MIN refers to minutes, not minimum.

Right, this is the day-ending minute.

> Joey is right here, so I would write something like
> var earliestChunkStartMin = 
>     (aIsAllChunks ? this.mDefaultStartMin : this.mStartMin);
> var latestChunkEndMin = 
>     (aIsAllChunks ? this.mDefaultEndMin : this.mEndMin);

Good idea; done.

> > +              if (earliestChunkStartMin < DAY_START_MIN
> > +                  && latestChunkEndMin > DAY_END_MIN) {
> > +                  break;
> > +              }
> > This last set of comparisons should probably be <= and >=, otherwise this
> > condition will never be true.
> Also here Joey is right, this set of comparisons even must be <= and >=.

Fixed.

> I think, this is just an additional check, which can be omitted, if one relies
> on the chunk start/end minutes to be in the range 0 and 24*60.

Omitted.
(In reply to comment #39)
> Apart from my previous comments #37 and #38 I think this patch is fine.
> Just a minor issue, I think in various places you used TABS instead of SPACES, 
> which looks a bit messy in my editor. What are the guidelines?

The guidelines are generally, "in lines of code you touch, be sure to leave them with spaces and not tabs".  Good catch.  A recent upgrade left with me a wonky copy of emacs which is what's causing this.  I'll clean up before I check in.

> In addition, as I'm a newbie, is there anything formal I have to do in order
> to approve this patch?

You pretty much have already done it!  :-)  People often include the notation "r=tbe" or something similar in the bug, but it's not really necessary.
Attached patch patch v4 — — Splinter Review 
Review comments addressed; carrying forward r+ annotations.
Attachment #213517 - Attachment is obsolete: true
Attachment #213666 - Flags: second-review+
Attachment #213666 - Flags: first-review+
Bug 329035 filed about the refresh issue.
Interim fix landed; this no longer blocks 298366 nor 321164.
No longer blocks: lightning-0.1, 321164
Whiteboard: [ETA 3/1]
Comment on attachment 211845 [details] [diff] [review]
patch implements 24-hour by default, working hours and vertical scrollbar

This particular iteration is going to conflict with the CVS trunk.  If you could generate a new patch, that would be great.
Attachment #211845 - Flags: first-review?(dmose)
Before supplying a new 24 hours patch I want to resolve the
scrolling problem first.

In this patch scrolling is implemented by using mickey's
scroll-container. The scrollbar is added manually to the
binding. 

The advantage of this approach is, that the scrollbar
is not part of the box whose content is scrolled.
This is needed if one wants to go for a design as proposed
in http://wiki.mozilla.org/Calendar:Calendar_View.
This would also allow a design with horizontal and vertical
scrollbar, where the headerbox and the timebar are basically 
fixed and only the content of the daybox is scrolled.

The disadvantage of this approach is, that scrolling has
to be programmed manually. In addition, when a window is
resized, the content and the scrollbar have to be adjusted.
This is done, when a resize event was fired, that means
the corresponding code is only executed after the mouse
button has been released. This feels rather slow and 
unnatural to the user.

The alternative to the scroll-container is the usage of the
XUL scrollbox. But with a scrollbox, a fixed headerbox and
a fixed timebar cannot be implemented. But it's possible
to implement a fixed headerbox only.

Please see also the discussion in
http://groups.google.com/group/mozilla.dev.apps.calendar/browse_thread/thread/1e1bc397483cad0a/00c1c18e439a9520#00c1c18e439a9520
and 
http://groups.google.com/group/mozilla.dev.tech.xul/browse_thread/thread/26b5fdc050cb2e11/624b219f02efc80d#624b219f02efc80d

What's your opinion?
dataloss key word
In terms of the advantages vs. disadvantages of UI as you've described it above, this scroll-container sounds like the less painful of the two options for the end user.  What I'd really like, though, is to hear from one of the XUL experts about this, so I've CCed Neil Deakin.

Neil, I'd be interested in your thoughts about the question in comment 48; the  threads linked to there contain lots of useful context about what we're trying to do and why.  Is there any good way to get the user experience we're shooting for without weird UI artifacts?  Ideally, this would be something that would work in 1.5 versions of gecko, but if it were only possible to do this in a 2.0 timeframe, we might be able to deal with that.


I'm not sure what you're asking. Could you summarize the UI you want to create for someone who doesn't know anything about the current calendar UI?

The scrollbars should appear around the scrollable area. Any headers or fixed content should appear outside this area. If you want to adjust the position of the headers, just use the scrollBoxObject methods when the scroll event fires.
Summary: views don't autoexpand to guarantee that out-of-sight events are displayed → 24-hours views
I spent some time talking to Enn (Neil Deakin) and roc (Robert O'Callahan) on IRC this afternoon, and here are some thoughts from that conversation.  Both of them suggested that instead of tweaking the margins in the XBL, it would probably make more sense to tweak the scroll position directly.  This works because a scrollbox is really just a normal box with overflow:hidden.

As far as the resizing problem, roc seemed to think that this might be fixable in Gecko in a 2.0 timeframe.  Here's an edited log of the resulting conversation, which suggests trying a few experiments:

[16:16] <roc> I think the problem is in PresShell::CreateResizeEventTimer
[16:17] <roc> after every resize reflow, we kill the timer and start over
[16:17] <roc> so we only fire a resize event if the user is completely idle for 200ms
[16:17] <dmose> so how is it that resizing HTML content does the right thing?
[16:18] <roc> because we always layout
[16:18] <roc> immediately
[16:18] <dmose> ah
[16:18] <roc> I don't know if we really need this timer stuff
[16:19] <dmose> that does sound like the $20,000 question
[16:19] <roc> experiment #1: replace the call to CreateResizeEventTimer with a direct call to FireResizeEvent
[16:20] <roc> see how that affects things
[16:20] <roc> (that's not 100% safe but it could be made safe)
[16:21] <roc> experiment #2, which we really should do, is replace the call to KillResizeEventTimer from CreateResizeEventTimer with "if (mResizeEventTimer) return;" and add "mResizeEventTimer = nsnull;" to PreShell::sResizeEventCallback
[16:22] <dmose> i presume exp 2 is the way to see if the timer is actually necessary at all?
[16:23] <roc> no
[16:23] <roc> that's experiment #1
[16:23] <roc> experiment #2 is to make the timer more benign
[16:23] <roc> by ensuring we fire every 200ms
[16:24] <roc> instead of requiring the user to be idle for 200ms

So it sounds like the next step would be to try the experiments roc suggests and post the results here...  If you have more technical questions, I'd suggest pinging them directly on IRC, as they're frequently around, and having me act as intermediary in the discussions probably just slows things down.

This is a screenshot of the calendar week view with header, timebar and daybox
marked with different colors.
(In reply to comment #51)
> I'm not sure what you're asking. Could you summarize the UI you want to create
> for someone who doesn't know anything about the current calendar UI?

In principal you'll find a short summary of the problem in 

http://groups.google.com/group/mozilla.dev.tech.xul/browse_thread/thread/26b5fdc050cb2e11/624b219f02efc80d#624b219f02efc80d

Nevertheless I'll try to summarize the UI and the corresponding problems with
scrolling in the following. The screenshot in comment #53 shows the calendar
week view. I marked the header red, the timebar blue and the daybox green.
In the current implementation the whole view can be scrolled in vertical
direction, which has the disadvantage, that the header is not visible for
the user, when displaying the late hours of the day.

The idea is, that depending on the window size the content of the daybox
can be scrolled in vertical and horizontal direction by using scrollbars.
The header and the timebar should always be visible, so that the user
knows which day the column of the daybox belongs to.

Now there's a correlation between the columns of the header and the
columns of the daybox. In addition there's a correlation between
the rows of the timebar and the rows of the daybox. Those columns
and rows should always be aligned. As a consequence, when scrolling
the content of the daybox, also the content of the header must be shifted.
Whereas this problem is trivial when only one dimension is scrollable,
the problem gets non-trivial when scrolling in two dimensions with
fixed header and timebar.

If I summarize the user scenario above, I get two requirements:
1.) content of the daybox must be scrollable by the user by using scrollbars
2.) content of the header and timebar must be scrollable programatically
    in order to align columns and rows

I tried to implement this scenario by two different approaches, first by
using a XUL scrollbox and second by using a hand-made scroll-container.

a) XUL scrollbox approach

   When using a XUL scrollbox and the nsIScrollBoxObject interface it's possible
   to scroll the content of the scrollbox with the scrollTo() method.
   Nevertheless I encountered the following problems:
   
   a1) not all methods of the nsIScrollBoxObject are implemented, e.g.
       ensureIndexIsVisible()

   a2) some methods didn't work, e.g. ensureElementIsVisible()

   a3) flex attribute is not inherited in scrollbox binding

   a4) scrollTo() method only works, if corresponding scrollbars are
       visible

   a1) and a2) can be worked around by using the scrollTo() method.
   a3) requires in principal a fix in the toolkit. As an alternative
   the scrollbox binding can be duplicated in the calendar project
   temporarily. But the real blocker is a4). I set the css style
   for the header and timebar to overflow:hidden, so that no
   scrollbars for those scrollboxes are visible. But this makes it 
   impossible to shift that content programmatically.

b) scroll-container approach

   As already written in comment #48 the advantage of this approach is, 
   that the scrollbar is not part of the box whose content is scrolled.
   This is needed if one wants to go for a design as proposed
   in http://wiki.mozilla.org/Calendar:Calendar_View.

   The disadvantage of this approach is, that scrolling has to be programmed 
   manually. In addition, when a window is resized, the content and the 
   scrollbar have to be adjusted. For more on the related problems with 
   resizing please see comments #48 and #52.


> The scrollbars should appear around the scrollable area. Any headers or fixed
> content should appear outside this area. If you want to adjust the position of
> the headers, just use the scrollBoxObject methods when the scroll event fires. 

As written in detail above, in some designs the scrollbars are not necessarily
around the scrollable area. The problem with adjusting the position of
the headers is, that the scrollTo() method doesn't work for the case, that
the header has no visible scrollbar.

>    a3) requires in principal a fix in the toolkit. As an alternative
>    the scrollbox binding can be duplicated in the calendar project
>    temporarily. 

Or you could use .scrollbox-innerbox { -moz-box-flex: vertical; }

But the real blocker is a4). I set the css style
>    for the header and timebar to overflow:hidden, so that no
>    scrollbars for those scrollboxes are visible. But this makes it 
>    impossible to shift that content programmatically.

Do you have a minimal testcase? Because it works fine for me.

I think we can fix the resize event issues for you.
(In reply to comment #55)
> Or you could use .scrollbox-innerbox { -moz-box-flex: vertical; }

Using vertical didn't work, but e.g. .scrollbox-innerbox { -moz-box-flex: 1; }
in principal does work. The only problem is, that then flex is set to 1 for 
all scroll-innerboxes. What I really want is, that the flex attribute is
inherited by the scroll-innerbox.

> Do you have a minimal testcase? Because it works fine for me.

Uups, it seems that I was wrong. I remember that in the past I didn't get that
working, but now in my testcase it works also for overflow hidden or visible.

(In reply to comment #56)
> I think we can fix the resize event issues for you.

That would be really great. Does that mean, that you'll evaluate experiments
#1 and #2 from comment #52 yourself, or is some action from my side required?

I'd like someone (dmose?) to try those experiments, because I'm completely swamped.
Thomas, could you try to do those experiments and report the results here?  I'm also swamped trying to bear down process-related documents...
(In reply to comment #60)
> Thomas, could you try to do those experiments and report the results here?  I'm
> also swamped trying to bear down process-related documents...

Sure, I will evaluate those experiments.

resize experiment 1 (see comment #52)
resize experiment 2 (see comment #52)
(In reply to comment #61)
> (In reply to comment #60)
> > Thomas, could you try to do those experiments and report the results here?  I'm
> > also swamped trying to bear down process-related documents...
> 
> Sure, I will evaluate those experiments.

I did both resize experiments (see patches in comments #62 and #63) and 
here are the results:

experiment 1:
In principal that works so far, that during resizing the window content 
is updated continuously, that means even if the mouse button has not been
released. But the problem is, that the performance is really poor,
I think it's unacceptable for the user.

experiment 2:
This experiment failed, that means the window content is still only updated
after the mouse button has been released (sResizeEventCallback is only called,
after the mouse button has been released).

I think the conclusion from experiment 1 is, that the timer is necessary.
As proposed by roc the solution of the problem is to ensure that the
event is fired every 200ms instead of requiring the user to be idle
for 200ms. But the proposed changes for experiment 2 didn't solve this
problem.

Any ideas?

I'm not sure why experiment #2 failed. Your patch should have worked. Throw some printfs in there and see what's going on
(In reply to comment #65)
> I'm not sure why experiment #2 failed. Your patch should have worked. Throw
> some printfs in there and see what's going on

When I resize the window while the mouse button is pressed,
then CreateResizeEventTimer() is called and behaves as
expected, that means it returns, because 
mResizeEventTimer != nsnull. So the timer is not destroyed
and no new timer is created. CreateResizeEventTimer() is
called as long as I move around the mouse and keep the
mouse button pressed.
But the problem is, that sResizeEventCallback() is only
called, after the mouse button has been released.
So it looks like the timer is in some kind of idle state.

Some further debugging shows, that sResizeEventCallback() 
is fired by timerManager->FireNextIdleTimer() in 
nsAppShell::Run(). But as long as the mouse button is pressed,
the dowhile loop in nsAppShell::Run() is in the 
nsToolkit::mDispatchMessage() call, which explains why the
timer callback is not fired, but CreateResizeEventTimer()
is called via nsWindow::WindowProc().

So this seems to be a rather delicate problem.

So basically the problem is that timers don't fire while resizing?

Is this true just for Windows, or Linux/Mac too?
(In reply to comment #67)
> So basically the problem is that timers don't fire while resizing?

Right.

> Is this true just for Windows, or Linux/Mac too?

I tested this only on Windows. In principal I can test this also on Linux
(will take some time), but I have no possibility to test this on Mac.

(In reply to comment #68)
> (In reply to comment #67)
> > Is this true just for Windows, or Linux/Mac too?
> 
> I tested this only on Windows. In principal I can test this also on Linux
> (will take some time), but I have no possibility to test this on Mac.

On Linux this problem doesn't appear, that means, that even the patch from
experiment 2 is not required.

(In reply to comment #57)
> (In reply to comment #55)
> > Or you could use .scrollbox-innerbox { -moz-box-flex: vertical; }
> 
> Using vertical didn't work, but e.g. .scrollbox-innerbox { -moz-box-flex: 1; }
> in principal does work. The only problem is, that then flex is set to 1 for 
> all scroll-innerboxes. What I really want is, that the flex attribute is
> inherited by the scroll-innerbox.

There's the additional problem, that the equalsize attribute is not inherited.
So it looks like I have to introduce a new binding which derives from 
xul:scrollbox.

This patch implements handmade scrolling by using a
scrollbox instead of a scroll-container. Due to
the flex and equalsize attributes, which are not
inherited, I introduced a calendar-scrollbox binding,
which is derived from the xul scrollbox.

Whereas this approach works in simple test cases and
for the timebar, it reveals a paint problem for the
daybox. When scrolling the non-visible region of the
box into the visible area, those regions are not
painted. Up to now I haven't found a workaround for
that problem.
This patch implements scrolling by using a scrollbox 
for the daybox, so that the scrollbars appear 
automatically around the scrollable area.

Due to the flex and equalsize attributes, which are not
inherited, I introduced a calendar-scrollbox binding,
which is derived from the xul scrollbox.

Note, that this patch is not finished, but already
in this state shows the same paint problem as
the patch in comment #71. When scrolling the 
non-visible region of the box into the visible area, 
those regions are not painted. Note, that this approach 
works in simple test cases and for the timebar, but not 
for the daybox.
If I summarize the current state, I see only 2 alternatives
for fixing the scrolling issue for the 24 hours view with
fixed headers.

1.) simple automatic scrolling by using scrollbox
    (see patch in comment #18)

    pros:
    + scrolling works automatically
    + fixed headerbox
    + using of scrollbox (or binding derived from scrollbox)
    + no resize/paint problems

    cons:
    - scrollbars only around scrollable area
    - no fixed timebar

2.) handmade scrolling by using scroll-container
    (see patch in comment #48)

    pros:
    + scrollbars can be placed everywhere
    + fixed headerbox
    + fixed timebar

    cons:
    - scrolling programmed manually => more code
    - workarounds for calculating box sizes necessary
    - introduction of a scroll-container
      (although scrollbox is available)
    - resize problem (probably can be fixed)

I think 1.) needs some further explanation. For the childbox a
binding derived from the xul scrollbox will be used (due to the
problem, that the flex and equalsize attributes are not inherited).
The vertical scrollbar will appear around the childbox.
When the vertical scrollbar appears, spacers have to be added
to the label and header box, so that the columns are aligned.
The horizontal scrollbar will be added to mainbox, which has the
disadvantage that the timebar will be scrolled out of the visible
area.

2.) is explained in detail in comment #48.

Taking all the pros and cons into account, I actually favour
alternative 1.) What do you think?

The question there is, how important is horizontal scrolling at all?
At the moment it's not relevant in practice, because the box is clipped
due to the navigation bar. If I remove the navigation bar (just an
experiment), then the whole daybox shrinks to size null.
In addition, other clients (Outlook, Evolution) also don't have
horizontal scrollbars. So in principal one can think about an
alternative 1.b) with no horizontal scrollbar at all.

This patch implements simple automatic scrolling by using a
scrollbox (see alternative 1 in comment #73).
Attachment #226791 - Flags: first-review?(dmose)
As mickey and ssa pointed out, the XUL scrollbar tag on the Mac platform
doesn't work (see #318045). I think this makes alternative 2.) from
comment #73 no longer an option.

Can you attach a few screenshots of alternative 1 or 1b in action?
screenshot of the calendar week view
with patch for simple automatic scrolling by using scrollbox
(see comment #74) applied
screenshot of the calendar week view
with patch for simple automatic scrolling by using scrollbox
(see comment #74) applied
screenshot of the calendar week view
with patch for simple automatic scrolling by using scrollbox
(see comment #74) applied
screenshot of the calendar week view
with patch for simple automatic scrolling by using scrollbox
(see comment #74) applied
screenshot of the calendar week view
with patch for simple automatic scrolling by using scrollbox
(see comment #74) applied
reworked patch due to CVS conflicts
Attachment #226791 - Attachment is obsolete: true
Attachment #227676 - Flags: first-review?(dmose)
Attachment #226791 - Flags: first-review?(dmose)
dmose says:
  Approach 1 looks good from a high level look and from a UI-review standpoint.
  I'm going to have jminta do the actual code review from this point.
Attachment #227676 - Flags: first-review?(dmose) → first-review?(jminta)
Comment on attachment 227676 [details] [diff] [review]
simple automatic scrolling by using scrollbox v2

+          headerdaybox.setAttribute("class", "calendar-header-day-box");
           var labeldaybox = document.getAnonymousElementByAttribute(this, "anonid", "labeldaybox");
+          labeldaybox.setAttribute("class", "calendar-label-day-box");
+          var headertimespacer = document.getAnonymousElementByAttribute(this, "anonid", "headertimespacer");
+          headertimespacer.setAttribute("class", "calendar-header-time-spacer");
+          var innerchildbox = document.getAnonymousElementByAttribute(this, "anonid", "innerchildbox");
+          innerchildbox.setAttribute("class", "calendar-inner-child-box");
Why are we doing all this in js?  and why are we doing at every refresh?  From what I can tell, this is all static, so it should just be included when you define <content>

+      <method name="overUnderFlowHandler">
+        <parameter name="event"/>
+        <body><![CDATA[
+          var propertyName;
+          var orient = this.getAttribute("orient");
+          if (orient == "vertical") {
+            propertyName = "width";
+          } else {
+            propertyName = "height";
+          }
+          var propertyValue;
+          if (event.type == "overflow") {
+            var childbox = document.getAnonymousElementByAttribute(this, "anonid", "childbox");
+            var innerchildbox = document.getAnonymousElementByAttribute(this, "anonid", "innerchildbox");
+            propertyValue = eval("childbox.boxObject." + propertyName) - eval("innerchildbox.boxObject." + propertyName);
eval() is scary and ugly.  As I see it, you can either 1.) save childbox.boxObject and then compute propertyValue in the |if (orient == "vertical")| block, or 2.) use childbox.boxObject[propertyName] down here.

+          } else {
+            propertyValue = 0;
+          }
Please add a comment for what other event types you expect, and why 0 is the appropriate value in that case.

+  <binding id="calendar-scrollbox" extends="xul:scrollbox">
+    <content>
+      <xul:box class="calendar-scrollbox-innerbox" xbl:inherits="orient,align,pack,dir,flex,equalsize">
The whole purpose of this is to get flex to inherit right?  dmose and I were wondering: Can we actually reach into the standard scrollbox and set flex that way?  (document.getAnonymousElementByAttribute(scrollbox, "anonid", "box-we-care-about").flex="1";)  That would save us significant time in xbl resolution.  In either case, please file a bug on toolkit to get flex to inherit.  It's hacky, but forking scrollbox is just as ugly in my mind.

+
+calendar-event-column[orient="horizontal"] {
+  border-top: 1px solid #3F7D91;
+}
+
+calendar-event-column[orient="vertical"] {
+  border-left: 1px solid #3F7D91;
+}
Need some comments in the css why these particular sides are needed.  (For these rules and others.)


The little stuff:
+        <xul:calendar-scrollbox anonid="childbox" flex="1" onoverflow="overUnderFlowHandler(event);" onunderflow="overUnderFlowHandler(event);">
The 80-char barrier is "sacred".  Please don't cross it.  (Here and elsewhere.)

+          var headerscrollbarspacer = document.getAnonymousElementByAttribute(this, "anonid", "headerscrollbarspacer");
+          headerscrollbarspacer.setAttribute(propertyName, propertyValue);
+          var labelscrollbarspacer = document.getAnonymousElementByAttribute(this, "anonid", "labelscrollbarspacer");
+          labelscrollbarspacer.setAttribute(propertyName, propertyValue);
camelCaps are your friend in javaScript.

This looks really solid otherwise and I think the next iteration ought to do it.  We're also going to have a follow-up to get rid of all the slow recalculateStartEndMinutes() cruft, right?  That's going to rock.
Attachment #227676 - Flags: first-review?(jminta) → first-review-
(In reply to comment #84)
> (From update of attachment 227676 [details] [diff] [review] [edit])
> +          headerdaybox.setAttribute("class", "calendar-header-day-box");
>            var labeldaybox = document.getAnonymousElementByAttribute(this,
> "anonid", "labeldaybox");
> +          labeldaybox.setAttribute("class", "calendar-label-day-box");
> +          var headertimespacer = document.getAnonymousElementByAttribute(this,
> "anonid", "headertimespacer");
> +          headertimespacer.setAttribute("class",
> "calendar-header-time-spacer");
> +          var innerchildbox = document.getAnonymousElementByAttribute(this,
> "anonid", "innerchildbox");
> +          innerchildbox.setAttribute("class", "calendar-inner-child-box");
> Why are we doing all this in js?  and why are we doing at every refresh?  From
> what I can tell, this is all static, so it should just be included when you
> define <content>

good point

> +      <method name="overUnderFlowHandler">
> +        <parameter name="event"/>
> +        <body><![CDATA[
> +          var propertyName;
> +          var orient = this.getAttribute("orient");
> +          if (orient == "vertical") {
> +            propertyName = "width";
> +          } else {
> +            propertyName = "height";
> +          }
> +          var propertyValue;
> +          if (event.type == "overflow") {
> +            var childbox = document.getAnonymousElementByAttribute(this,
> "anonid", "childbox");
> +            var innerchildbox = document.getAnonymousElementByAttribute(this,
> "anonid", "innerchildbox");
> +            propertyValue = eval("childbox.boxObject." + propertyName) -
> eval("innerchildbox.boxObject." + propertyName);
> eval() is scary and ugly.  As I see it, you can either 1.) save
> childbox.boxObject and then compute propertyValue in the |if (orient ==
> "vertical")| block, or 2.) use childbox.boxObject[propertyName] down here.
> 
> +          } else {
> +            propertyValue = 0;
> +          }
> Please add a comment for what other event types you expect, and why 0 is the
> appropriate value in that case.

Due to some bugs, e.g. when editing the title of an event in the view,
I have to use a different approach. Unfortunately overflow and underflow events
are not very reliable or too early, that means that the box dimensions are not
correct, when the event is fired. Therefore I now listen at the disabled attribute
of the scrollbar. Apart from that, I don't use eval() anymore.

> +  <binding id="calendar-scrollbox" extends="xul:scrollbox">
> +    <content>
> +      <xul:box class="calendar-scrollbox-innerbox"
> xbl:inherits="orient,align,pack,dir,flex,equalsize">
> The whole purpose of this is to get flex to inherit right?  dmose and I were

You're right, it's all about the flex attribute, which is not inherited
in the scrollbox binding. Meanwhile I filed a bug (343555).

> wondering: Can we actually reach into the standard scrollbox and set flex that
> way?  (document.getAnonymousElementByAttribute(scrollbox, "anonid",
> "box-we-care-about").flex="1";)  That would save us significant time in xbl
> resolution.  In either case, please file a bug on toolkit to get flex to
> inherit.  It's hacky, but forking scrollbox is just as ugly in my mind.

You're proposed fix works, great!!! 
Therefore I omitted the calendar-scrollbox binding.

> +
> +calendar-event-column[orient="horizontal"] {
> +  border-top: 1px solid #3F7D91;
> +}
> +
> +calendar-event-column[orient="vertical"] {
> +  border-left: 1px solid #3F7D91;
> +}
> Need some comments in the css why these particular sides are needed.  (For
> these rules and others.)

I really have difficulties to write some comments on this.
The whole file is undocumented and it's difficult to describe, why one sets
the left border at box xy and the right border at box yz.
So I would be happy if you propose some comments.

> The little stuff:
> +        <xul:calendar-scrollbox anonid="childbox" flex="1"
> onoverflow="overUnderFlowHandler(event);"
> onunderflow="overUnderFlowHandler(event);">
> The 80-char barrier is "sacred".  Please don't cross it.  (Here and elsewhere.)

I used 80 char, but probably the indentation is not always as you prefer.

> +          var headerscrollbarspacer =
> document.getAnonymousElementByAttribute(this, "anonid",
> "headerscrollbarspacer");
> +          headerscrollbarspacer.setAttribute(propertyName, propertyValue);
> +          var labelscrollbarspacer =
> document.getAnonymousElementByAttribute(this, "anonid",
> "labelscrollbarspacer");
> +          labelscrollbarspacer.setAttribute(propertyName, propertyValue);
> camelCaps are your friend in javaScript.

done

> This looks really solid otherwise and I think the next iteration ought to do
> it.  We're also going to have a follow-up to get rid of all the slow
> recalculateStartEndMinutes() cruft, right?  That's going to rock.

This patch is the first step to get 24 hours in the views. You're right,
after everything is working we can remove all the stuff, which was introduced
for autoexpand.

This patch fixes all remarks from comment #84, apart from comments in css files.
Attachment #227676 - Attachment is obsolete: true
Attachment #228050 - Flags: first-review?(jminta)
Comment on attachment 228050 [details] [diff] [review]
simple automatic scrolling by using scrollbox v3

+      <field name="mMinPPM">0.6</field>
This makes it really tough for me to see a significant part of my schedule.  (I only get about 4.5hrs)  Can we make this value a bit smaller?

+        // when a scrollbar appears/disappears in the view,
+        // it's disabled attribute is modified
+        if (event.attrName != "disabled") {
+            return;
+        }
+        if (event.originalTarget.localName != "scrollbar") {
+            return;
+        }
+
Is it possible to do any more bullet-proofing here?  I'm scared that we'll be pretty fragile, if we ever get another scrollbar in the view somewhere.

+        var propertyName;
+        var orient = event.target.getAttribute("orient");
+        if (orient == "vertical") {
+            propertyName = "width";
+        } else {
+            propertyName = "height";
+        }
As far as I can tell, we only use orient in the if check, in which case there's no need to store it in an extra variable, is there?

 calendar-header-container { 
   background: #FFFFFF;
+}
+
+calendar-event-column[orient="horizontal"] {
+  border-top: 1px solid #3F7D91;
+}
+
+calendar-event-column[orient="vertical"] {
+  border-left: 1px solid #3F7D91;
+}
+
+calendar-header-container { 
That gives us 2 rules for calendar-header-container, which I'd rather not have.  Please group them together.

+.calendar-label-day-box[orient="horizontal"] {
+  border-right: 1px solid #3F7D91;
+}
+
+.calendar-label-day-box[orient="vertical"] {
+  border-bottom: 1px solid #3F7D91;
+}
+
Comment here could be "Make sure the box for day-labels appears to end before the scrollbar."

+.calendar-header-time-spacer[orient="horizontal"] {
+  border-bottom: 2px solid #3F7D91;
+}
+
+.calendar-header-time-spacer[orient="vertical"] {
+  border-right: 2px solid #3F7D91;
+}
+
Comment here could be: "Make sure we extend the bold line separating scrollable and non-scrollable areas over the timebar."

+.calendar-header-day-box[orient="horizontal"] {
+  border-bottom: 2px solid #3F7D91;
+  border-right: 1px solid #3F7D91;
+}
+
+.calendar-header-day-box[orient="vertical"] {
+  border-bottom: 1px solid #3F7D91;
+  border-right: 2px solid #3F7D91;
+}
+
"Bold the line separating all-day events from scrollable area."

+.calendar-day-box[orient="horizontal"] {
+  border-right: 1px solid #3F7D91;
+}
+
+.calendar-day-box[orient="vertical"] {
+  border-bottom: 1px solid #3F7D91;
+}
+
"Make sure to have a border between the edge of the views and the scrollbar"

r=jminta with those.
Attachment #228050 - Flags: first-review?(jminta) → first-review+
(In reply to comment #87)
> (From update of attachment 228050 [details] [diff] [review] [edit])
> +      <field name="mMinPPM">0.6</field>
> This makes it really tough for me to see a significant part of my schedule.  (I
> only get about 4.5hrs)  Can we make this value a bit smaller?

I changed that value to 0.4.

> +        // when a scrollbar appears/disappears in the view,
> +        // it's disabled attribute is modified
> +        if (event.attrName != "disabled") {
> +            return;
> +        }
> +        if (event.originalTarget.localName != "scrollbar") {
> +            return;
> +        }
> +
> Is it possible to do any more bullet-proofing here?  I'm scared that we'll be
> pretty fragile, if we ever get another scrollbar in the view somewhere.

I now check in addition, that the parent of the scrollbar is the childbox.

> +        var propertyName;
> +        var orient = event.target.getAttribute("orient");
> +        if (orient == "vertical") {
> +            propertyName = "width";
> +        } else {
> +            propertyName = "height";
> +        }
> As far as I can tell, we only use orient in the if check, in which case there's
> no need to store it in an extra variable, is there?

No, there's no need for an extra variable, done.

>  calendar-header-container { 
>    background: #FFFFFF;
> +}
> +
> +calendar-event-column[orient="horizontal"] {
> +  border-top: 1px solid #3F7D91;
> +}
> +
> +calendar-event-column[orient="vertical"] {
> +  border-left: 1px solid #3F7D91;
> +}
> +
> +calendar-header-container { 
> That gives us 2 rules for calendar-header-container, which I'd rather not have.
>  Please group them together.

done

> +.calendar-label-day-box[orient="horizontal"] {
> +  border-right: 1px solid #3F7D91;
> +}
> +
> +.calendar-label-day-box[orient="vertical"] {
> +  border-bottom: 1px solid #3F7D91;
> +}
> +
> Comment here could be "Make sure the box for day-labels appears to end before
> the scrollbar."
> 
> +.calendar-header-time-spacer[orient="horizontal"] {
> +  border-bottom: 2px solid #3F7D91;
> +}
> +
> +.calendar-header-time-spacer[orient="vertical"] {
> +  border-right: 2px solid #3F7D91;
> +}
> +
> Comment here could be: "Make sure we extend the bold line separating scrollable
> and non-scrollable areas over the timebar."
> 
> +.calendar-header-day-box[orient="horizontal"] {
> +  border-bottom: 2px solid #3F7D91;
> +  border-right: 1px solid #3F7D91;
> +}
> +
> +.calendar-header-day-box[orient="vertical"] {
> +  border-bottom: 1px solid #3F7D91;
> +  border-right: 2px solid #3F7D91;
> +}
> +
> "Bold the line separating all-day events from scrollable area."
> 
> +.calendar-day-box[orient="horizontal"] {
> +  border-right: 1px solid #3F7D91;
> +}
> +
> +.calendar-day-box[orient="vertical"] {
> +  border-bottom: 1px solid #3F7D91;
> +}
> +
> "Make sure to have a border between the edge of the views and the scrollbar"

Thanks for all your comments, I added them unchanged.

> r=jminta with those.

Unfortunately I encountered an additional problem. In some cases, the scrollbar
spacers were not adjusted correctly, e.g. when having the week view in vertical
orientation with no scrollbar ever appeared, and then pressing the rotate button.
I fixed this problem by moving the scrollbar adjustment code into a separate
method 'adjustScrollBarSpacers' and calling this method also at the end
of the view's relayout method.

Due to this fact I ask for an additional review.

This patch fixes all remarks from comment #87.
Attachment #228050 - Attachment is obsolete: true
Attachment #228292 - Flags: first-review?(jminta)
Comment on attachment 228292 [details] [diff] [review]
simple automatic scrolling by using scrollbox v4

      - or top of a multiday view.
     -->
   <binding id="calendar-time-bar">
+
     <content>
       <xul:box xbl:inherits="orient,width,height" flex="1" anonid="topbox">
       </xul:box>
     </content>
Random whitespace changes are bad.

           for each (col in this.mDateColumns) {
                col.column.endLayoutBatchChange();
           }
+
+          // adjust scrollbar spacers
+          this.adjustScrollBarSpacers();
         ]]></body>
       </method>
I don't yet understand why this needs to be in relayout, rather than rotate?  From your description the problem only happens on rotate.

r=jminta with that question answered and/or fixed.
Attachment #228292 - Flags: first-review?(jminta) → first-review+
(In reply to comment #90)
> (From update of attachment 228292 [details] [diff] [review] [edit])
>       - or top of a multiday view.
>      -->
>    <binding id="calendar-time-bar">
> +
>      <content>
>        <xul:box xbl:inherits="orient,width,height" flex="1" anonid="topbox">
>        </xul:box>
>      </content>
> Random whitespace changes are bad.

done

>            for each (col in this.mDateColumns) {
>                 col.column.endLayoutBatchChange();
>            }
> +
> +          // adjust scrollbar spacers
> +          this.adjustScrollBarSpacers();
>          ]]></body>
>        </method>
> I don't yet understand why this needs to be in relayout, rather than rotate? 
> From your description the problem only happens on rotate.

You're right, reorient is a much better place than relayout. Therefore I added
the adjustment of the scrollbar spacers at the end of the reorient method.

This patch fixes all remarks from comment #91.
I carried over the r+ flag from the last patch.

Can you please land this patch? Thanks.
Attachment #228292 - Attachment is obsolete: true
Attachment #228933 - Flags: first-review+
Patch checked in.  Closing this bug as it already has more than enough in it.  We should use other bugs (perhaps bug 133107) to handle ripping out the old view-boundary code and moving to straight 24-views.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 348408 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: