297934 - Make sunbird use the new views

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

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

Having the views forker increases the amount of maintance work. Not a good idea.
So we should use one set of views, and the lightning views are the obvious
candidate, because they have cool features like dragging to create a new event.

They are missing navigation buttons though, so those should be added.

Comment 1

[Joey Minta]

Attachment: part 1

Part 1 of n

This part makes a few changes to the views to enable them to work with some of
Sunbird's extra features.

Main changes:
Fix/add double-click to create new event
Add tooltips for mouse-ing over items
Add observers to watch for/report changes to the selection

I've done my best to make sure none of this breaks lightning. :-/

Comment 2

[Joey Minta]

Attachment: part 2 (draft)

This patch could likely end up obsolete before the time comes for it, but I'm
paranoid of losing it.	Plus, anyone who wants to help me debug it can grab it.


Makes Sunbird/Calendar use the new views.

Known problems:
Minimonth selection of day is 1 day off for me.  (Timezone issue)
Multiday selection doesn't work perfectly.
The views often show up too big, so I put them in a box with scrollbars for
now.

Comment 3

[Joey Minta]

Comment on attachment 189598 [details] [diff] [review]
part 1

This isn't quite what I want.  Better version coming soon.

Comment 4

[Joey Minta]

Attachment: part 1 v2

This is what I want.  Everything mentioned above, plus a 'tasks in view' option
for month view.  Also sets up the framework for a 'tasks in view' option in
multiday view.

Comment 5

[Joey Minta]

Attachment: part2 v2

This time for real.

This won't work without part 1 v2, but I'd prefer to multi-task in solving any
problems in either patch.

Makes Sunbird/Calendar use the new views.  All of the options in the menubar
(Sunbird version) should work, including: tasks-in-view, workdays-only,
go-to-next, go-to-previous, etc.

Part 3 will remove all of the code made useless by this patch
Part 4 will implement some sort of navigation arrows for the views.

Comment 6

[Joey Minta]

Comment on attachment 189775 [details] [diff] [review]
part2 v2

+CalendarWindow.prototype.refreshView = function() {
+    if(this.currentView == this.monthView)
+	 this.switchToMonthView();
+    else {
+	 this.showCalendar(this.currentView.startDate,
this.currentView.endDate);
+	 if(this.workDaysOnly && this.currentView.supportsDisjointDates)
+	     this.removeNonWorkDays();
+    }

Here, this.showCalendar( should be this.switchViewInternal( but I'm mostly
interested in whether you feel this is the right way to take this patch.

Comment 7

[Joey Minta]

Attachment: part 2 v2-1

Same as before, but with some extra fixes from 3 days of testing.

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 189773 [details] [diff] [review]
part 1 v2

This looks OK to me, probably want vlad to weigh in too.

Comment 9

[Joey Minta]

*** Bug 304130 has been marked as a duplicate of this bug. ***

Comment 10

[Myk Melez [:myk] [@mykmelez]]

*** Bug 304125 has been marked as a duplicate of this bug. ***

Comment 11

[Joey Minta]

Attachment: everything

Well, here it is... This patch contains all the changes I'm hoping to land in this set (parts 1-4 mentioned in previous comments).  There are still some follow up bugs that will need to be addressed.  The basic way I imagined this patch to be reviewed was for dmose to take base/ and lightning/ and mvl to take resources/ and sunbird/.  If desired, I can split this patch into 3 separate ones.  The first would contain the changes necessary to prepare the views for being included in Sunbird (see details below for base/).  This would be reviewed by dmose.  The second part would be the actual implementation of the new views, including the replacement of calendarWindow.js with viewManager.js.  The final part would be to remove the then obsolete code in calendar.js and the css files (This composes about 80kb of the current patch).  Both 2 and 3 would ideally be reviewed by mvl.  However, since the code removal is pretty straight-forward, and the division between parts pretty clear, I felt having it all together was more beneficial for understanding the motivations behind the changes.

Details of the changes:
base/:
My basic goal for this patch was to implement the new views with as little loss of functionality from 0.3a1 as possible.  Accordingly the views had to undergo changes to support some additional features that Sunbird uses.  These should be done in such a way as to minimize noticable impact on Lightning
-Category colors were added via setting an item-category selector.  Lightning doesn't set categories, so it notices nothing here.
-Tooltips for events.  Sunbird sets tooltips for events.  A stub tooltip was created in Lightning
-mSelectionObserver.  Sunbird needs to coordinate the selected items via the views , the unifinder, and the commands (and one day the task list).  As such, the views need to include observers for selection.  These are only activated, however, if the viewController sets them.  Since Lightning does not, it should notice nothing.
-Tasks in View.  Tasks are already displayed in the month-view.  A getter/setter was implemented to allow this option to be toggled.  In the multiday-view, more extensive changes will be needed, so only stub interfaces were created.  findColumnForEvent was, however, causing problems even at this basic level (due to selection coordination), and so it was changed.  A follow-up bug will fully implement this feature.
-I was experiencing an error in relayout() that required a date to be cloned.

lightning/:
-Stubs for tooltips were impemented (see above)

resources/:
-The most substantial change here is the introduction of a viewManager to replace the calendarWindow prototype.  The gCalendarWindow variable has been preserved, and several interfaces remain, in order to ease the transition.  Follow-up bugs will be filed to remove gCalendarWindow and the obsolete interface EventSelection.
-The introduction of the new-views requires that any code asking for the start/end dates of the current view needed to be changed.  This represents the majority of the changes in files other than calendar.js
-viewManager, unlike calendarWindow does not implement calendarPreferences, nor does it hold a reference to a dateFormatter (allowing it to be much more streamlined).  Areas of the code relying on either of these were repaired.
-The preference observer needed to be updated significantly to the new views
-Both calendarPreferences and calendarEventSelection no longer hold references to the calendarWindow/viewManager object
-I wanted to use the enableElement/disableElement functions provided by applicationUtils.js inside viewManager.js.  This required removing references to gDebugEnabled
-tasksInView and workdaysOnly now work off of observers instead of the hacked solution previously used.
-The context menu now, correctly, only is displayed when clicked within the views, not other areas of the UI.

resources/calendar.js
-Significant amounts of obsolete code were removed here, mostly involving click handlers for the old-views.  
-The long, awkward list of obsolete global vars was removed.  
-The newEvent and newTodo commands had to be updated in order to be able to correctly get the proper default dates for new items.

resources/viewManager.js
-I hope the commenting I have added to the code here should be adequate to explain the majority of things going on in the code.
-pseudoStartDate/pseudoEndDate hold the start/end dates of the views, WITHOUT non-workdays being removed.  This allows for proper back/forward navigation.
-Because there should never be more than one controller/manager for the views, this was implemented as a single variable, rather than a prototype.  The downside here is that mVars are not protected, but get/set functions have been provided regardless.  Standard coding practices dictate that they should not generally be accessed directly.

Finally, although I'm asking dmose/mvl for actual reviews, I'd like to point out that especially for a patch of this size/scope, I completely welcome code comments from anyone involved in the project.

Comment 12

[gekacheka]

Quick comments from looking over code (haven't run it)
Comments are mostly about modularity boundaries of viewManager and views.

Nice start, looks like you've done a lot of work here.

Suggest viewManager be prototype, NOT a singleton, so there can be more 
  than one instance in a window. (For example, it should be possible to
  write an extension to view two calendars side by side instead of 
  intermingled, so one window would need to contain two viewManagers.)

Suggest viewManager be more modular, don't hard-code set of views, 
  so can easily add additional views in extensions.  
  - addView/removeView methods, maybe
    addView(name, command, view, ...)
    removeView(name)
  - use a hash table (object) to hold the view info by name.
  - iterate over views rather than call each one explicitly.
  - move view-specific code into view methods, don't switch on view type.
    such as in switchView or moveView.
    (IF it is important that month-view and mutliweek-view share the same
     XUL DOM, it is also possible to move the methods to view-controller
     objects instead of the views themselves, but still out of the 
     viewManager.  But it is not clear to me whether that is important.)

Suggest rename month-view.xul to multiweek-view.xul,
 - now capable of more than just month view
 - makes available functionality clearer, parallel with multiday-view,
   so makes more sense as a base class for other views such as
   a quarter view or a semester view.

Suggest multiweek view and month view be separate views.
  They can both inherit most of their code, either from a shared
  base class or monthview extends multiweekview.
  Differences should be implemented in methods:
  - Calculating view limits:
    multiweek: counts weeks.  Moving to next week changes view limits.
    monthview: based on current calendar month.  Moving to next week
    does not always change view limits.
  - Shade days not in current month:
    multweek:  no
    monthView: yes
  - pageForward/backward
    multiweekView: pageForward should not show the last week of the
      previous view.  (or it always does, but it must be consistent
      one way or the other.)
    monthView: pageForward always goes to the next month.
      If the first day of the next month is the same as the first day of
      the week, then no days of the previous view are shown, otherwise,
      the last week of the previous view is still visible.
  This will also serve as a model for how extensions can implement
  other durations, such as quarter view or semester view.

viewManager.switchView 
  - move date code to the views.
  - multiweek view computation looks like it will erroneously
    start view on week AFTER selected day if 
    selectedDay.weekday < weekStartOffset, such as sun (0) < mon (1).

future work: consolidate code duplicated in views.
  examples: tooltips, category list parsing
future work: multiple item selection

I hope this helps!

Comment 13

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

Some comments from the first reading of the patch, and some testing:

- Don't use the weird gridOccuranceTooltip, just name it itemTooltip. All the boxes you draw are called items too.
- You really should at least cache the dateformater. The constructor looks pretty expensive.
- I agree with gekacheka on the pluggability of the viewmanager, but i'm not sure if it should be done in this patch. The patch is big enough as it is.
- Multiweek looks weird with this patch. It looks like the week view, not like the monthview. Is that what you want?
- You might want to look at the style of the monthview. The events don't have a border, which looks odd.
- Manipulating events now act on the occurance, not on the event. Do we want UI asking for what to act on? It's pretty hard to change the title of all occurances now.
- Changing anything doesn't redraw the monthview. (didn't test other views)
- day and weekviews make room for events after the endtime, but don't draw them. I think the view should always display the whole day, but scroll to 7:00 (or whatever is set) and change the color of the workhours slightly. This might be out of the scope for this bug.

Comment 14

[Joey Minta]

Attachment: everything v2

Here's a new version incorporating many of the comments I received.  This hasn't had quite as rigorous of testing as the previous version, but nonetheless I'm confident in it.  Any further testing/comments anyone can offer would be greatly appreciated

(In reply to comment #12)
> Suggest viewManager be prototype, NOT a singleton, 

Personally, I find this to be very ugly style, since we're only plannnig to use it once, and it's likely to cause more trouble than good, but your point is valid.  This version is done as a prototype.

> Suggest viewManager be more modular, don't hard-code set of views, 
>   so can easily add additional views in extensions.  

Great idea!

>   - addView/removeView methods, maybe
Done.  There's some (hopefully) clear docs on how to construct a view to add.

>   - use a hash table (object) to hold the view info by name.
>   - iterate over views rather than call each one explicitly.
>   - move view-specific code into view methods, don't switch on view type.
>     such as in switchView or moveView.
Done.

>     (IF it is important that month-view and mutliweek-view share the same
>      XUL DOM, it is also possible to move the methods to view-controller
>      objects instead of the views themselves, but still out of the 
>      viewManager.  But it is not clear to me whether that is important.)
I think it is, for the view-control labels.

> 
> Suggest rename month-view.xul to multiweek-view.xul,
>  - now capable of more than just month view
>  - makes available functionality clearer, parallel with multiday-view,
>    so makes more sense as a base class for other views such as
>    a quarter view or a semester view.
This is outside the scope of this bug.  Furthermore, it's not entirely clear to me that we aren't somewhat abusing the month-view to create a multiweek view.  Either way, that's debate for another bug.

> Suggest multiweek view and month view be separate views.
>   - Calculating view limits:
>     multiweek: counts weeks.
We do that now.

>   - Shade days not in current month:
A simple css hack could accomplish this, in my opinion from outside the actual view code.

>   - pageForward/backward
Done.

> viewManager.switchView 
>   - move date code to the views.
If you're refering to the view-control labels, I disagree.  The more logic we place in the views, the less flexible/extensible they are.  Javascript functions are much easier for extensions to alter than xbl bindings.

>   - multiweek view computation looks like it will erroneously
>     start view on week AFTER selected day if 
>     selectedDay.weekday < weekStartOffset, such as sun (0) < mon (1).
Fixed

Thanks for taking the time to put this together, it was very helpful!

(In reply to comment #13)
> - Don't use the weird gridOccuranceTooltip, just name it itemTooltip. All the
> boxes you draw are called items too.
Done.

> - You really should at least cache the dateformater. The constructor looks
> pretty expensive.
Done.

> - I agree with gekacheka on the pluggability of the viewmanager, but i'm not
> sure if it should be done in this patch. The patch is big enough as it is.
We grew about 10k as a result of this, all in viewManager.js.  On the other hand, implementing this later would consist of reviewing an entirely new viewManager

> - Multiweek looks weird with this patch. It looks like the week view, not like
> the monthview. Is that what you want?
This was intentional, but I agree it looks weird.  I changed it in this version.

> - You might want to look at the style of the monthview. The events don't have a
> border, which looks odd.
I consider this a bit outside the scope of this bug.  In an effort to keep the patch as small as possible, I'm trying to simply *implement* the views, not fix them.

> - Manipulating events now act on the occurance, not on the event. Do we want UI
> asking for what to act on? It's pretty hard to change the title of all
> occurances now.
Yes, we do, but it needs to be a different bug.  We need to change all of our edit/delete event commands, the calendarEventSelection, and other areas in order to fully support this.

> - Changing anything doesn't redraw the monthview. (didn't test other views)
Bug in the actual month view.  It was a 1-liner, so I fixed it.  The multiday view fails to initially add events created via the eventDialog, due to (I think) a floating vs. default tz issue.

> - day and weekviews make room for events after the endtime, but don't draw
> them. I think the view should always display the whole day, but scroll to 7:00
> (or whatever is set) and change the color of the workhours slightly. This might
> be out of the scope for this bug.
Yes, another bug.

Thanks for the testing/comments.

Comment 15

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

Comment on attachment 201999 [details] [diff] [review]
everything v2

Just some comment son code and syntax i noticed while looking at this patch:

>Index: mozilla/calendar/base/content/calendar-month-view.xml

>+      <handler event="mouseover"><![CDATA[
>+            onMouseOverGridOccurrence(event);
Can you rename onMouseOverGridOccurrence to onMouseOverItem ?

>+              var categoriesClassList = "";
It isn't really a class anymore.
>+              box.setAttribute("item-category", categoriesClassList);
but an attribute.
(same in multiday-view)

>+              box.occurrence = itd.item;
Do you need to set this? Just item should do it. (You would need to check callers though)

>Index: mozilla/calendar/resources/content/applicationUtil.js
> function debug(text)
> {
>-    if(gDebugEnabled)
>-        dump(text + "\n");
>+    dump(text + "\n");
> }
Can you leave out those changes? (same for debugVar)

>Index: mozilla/calendar/resources/content/publishDialog.js
>+      var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                  .getService(Components.interfaces.nsIPrefService);
>+      document.getElementById( "publish-remotePath-textbox" ).value = opener.getCharPref( prefService.getBranch("calendar.", "publish.path", "" );

That looks like a syntax error.

>Index: mozilla/calendar/resources/content/viewManager.js

>+               if (document.getElementById("toggle_workdays_only").getAttribute("checked")
>+            == 'true')
a tab :)
>+            this.mWorkDaysOnly = true;
>+
>+        // tasksInView is true by default
>+        if (document.getElementById("toggle_tasks_in_view").getAttribute("checked")
>+            != 'true')
>+            this.tasksInView = false; var startDate = viewManager.currentView.startDate;
missing newline?

>+        var endDate = viewManager.currentView.endDate;


I need to think about the whole viewManager some more. It seems to introcude a random split between a xbl  implemented functions and dataObjects methods. The split is based on what the current views happen to share in the multi-day-view. The split will likely not make any sense for other views that might be added later.

Comment 16

[gekacheka]

[Mostly offtopic for this bug, but to object to MVL's renaming suggestions:

The name "gridOccurrenceTooltip" was chosen for the following reasons:

Tooltips appear in 3 different panes, the grid pane, the event list,
and the task list.  There is a <tooltip> object in calendar.xul for
each pane.  They have different onpopupshowing methods to find the
event or task from the DOM under the mouse, which is different for each
pane, so they are not combined into one tooltip object.

"grid": associates the grid tooltip with the GRID PANE, differentiating
  it from the tooltip for the task list or the event list.

"Occurrence": the mouseoverPreview is designed to display
  information specific to the OCCURRENCE under the mouse, not the ITEM
  (task or event) which may have multiple occurrences.

Renaming "gridOccurrenceTooltip" to "itemTooltip" loses its
association with the grid pane, so it is easy to make the mistake of
thinking it is also used for the event list and task list panes.

Renaming "onMouseOverGridOccurrence" to "onMouseOverItem" would also
lose the association with the grid pane.

I tried to keep the original names short, so I omitted the word 'pane'.

  gridOccurrenceTooltip     onMouseOverGridOccurrence
  eventTreeTooltip          onMouseOverEventTree
  taskTreeTooltip           onMouseOVerTaskTree

Maybe adding the word 'pane' would better get across that these
tooltip objects are associated with particular panes as well as with
what they display:

  gridPaneOccurrenceTooltip  onMouseOverGridPaneOccurrence
  eventTreePaneTooltip       onMouseOverEventTreePane
  taskTreePaneTooltip        onMouseOverTaskTreePane

But this seems beyond the scope of the current bug.
]

On topic: mouseoverPreviews don't work because the previous renaming
was incomplete.  Suggest renaming back to gridOccurrenceTooltip for 
now and leave renaming these for another bug.

Comment 17

[Joey Minta]

(In reply to comment #15)
> It isn't really a class anymore.
> >+              box.setAttribute("item-category", categoriesClassList);
> but an attribute.
Done locally. In reply to gekacheka, and this general debate, I'm indifferent. Note that we probably only need one tooltip, and they're all (eventList/grid/tasklist) going to need to be set in much the same way given that tasks will appear in the view now too.  Whatever the final decision is, I'll implement it, or we can debate it later in another bug.  It's mvl's final comment here that I think we all agree is the most important.  I'm holding off submitting another patch until we sort that out.

> 
> >+              box.occurrence = itd.item;
> Do you need to set this? Just item should do it. (You would need to check
> callers though)
Mouseover previews does some strange stuff with this that I didn't want to mess with here.  (see gekacheka's comment)


> >Index: mozilla/calendar/resources/content/applicationUtil.js
> > function debug(text)
> > {
> >-    if(gDebugEnabled)
> >-        dump(text + "\n");
> >+    dump(text + "\n");
> > }
> Can you leave out those changes? (same for debugVar)
If I want to use the functions in applicationUtil.js (and I do), I either have to declare this variable, or remove these checks for it.  (Right now eventDialog.js is the only one that uses them, and does declare the variable.)  I chose to do the latter, but if you really want I'll add the variable instead.


> >Index: mozilla/calendar/resources/content/publishDialog.js
> That looks like a syntax error.
Fixed.
 
> >Index: mozilla/calendar/resources/content/viewManager.js
> 
> >+               if (document.getElementById("toggle_workdays_only").getAttribute("checked")
> >+            == 'true')
> a tab :)
> >+            this.mWorkDaysOnly = true;
> >+
> >+        // tasksInView is true by default
> >+        if (document.getElementById("toggle_tasks_in_view").getAttribute("checked")
> >+            != 'true')
> >+            this.tasksInView = false; var startDate = viewManager.currentView.startDate;
> missing newline?

I think your patch got corrupted somewhere?  I don't see this sort of thing anywhere.

> I need to think about the whole viewManager some more. It seems to introcude a
> random split between a xbl  implemented functions and dataObjects methods. The
> split is based on what the current views happen to share in the multi-day-view.
> The split will likely not make any sense for other views that might be added
> later.
> 
FWIW, I completely agree that something like supportsRemovingWeekends might be inappropriate for these objects, and in this case, it's essentially a temporary measure until the calendar-month-view also supports removing weekends.  Other than that, I don't find the split quite as 'random'.  Essentially, the dataObjects are designed to provide the connection points between the views and the rest of the calendar code.  Other than asking for the selectedDay, no code outside of viewManager calls anything in the views, and the viewManager itself only accesses the views via the dataObjects, so the distinction seems to be: Anything that needs to be accessed outside the views themselves is exposed via a dataObject.

My main argument against embedding these sorts of things in the xml itself is one of flexibility.  Example: As was seen by version 1 of the 'everything' patch, it's entirely possible, right now, to create a 'multiweek' view using calendar-multiday-view, instead of calendar-month-view.  My fear is that if we try to embed things like navigation labels within the xml files, this will not longer be possible, since the views won't know how to label these views as well.

Comment 18

[gekacheka]

1. viewManager modularity
  I agree with MVL that the split between viewData objects and xbl view
  objects calls for further refactoring.

  viewManager.js contains view-specific code that would be nice to get
  out of the viewManager and into view files.  (dayViewData,
  weekViewData, etc.)

  In calendar.xul, the view-title-box hardcodes the controls for every view.
  * The horizontal controls don't fit multiweek view well.
    Vertical controls fit better when weeks are stacked vertically.
    Transposing the week view does not change the move controls.
  * The control moveView parameters (-1) (-2) (-1) * (+1) (+2) (+1)
    don't fit all views very well.  For example,
    - in multiweek view it would be good to have a control that
      advances by the number of weeks (which is controlled by a preference).
    - in a year view it could be good to have controls that advance by, say,
      -1year -1quarter -1month * +1month +1quarter +1year

  Suggest the view navigation controls be part of each view.

  The base multi-day display and multi-week display without the controls
  could still be separate bindings.  The base multiday-display binding
  displays the grid, and a day-view binding contains a multiday-display
  plus the day-view navigation controls.


2. Design issues:

  2a. The year is not displayed in any of the views.
    Month view:     was part of month title
    Multiweek view: was at top of week number column.
    Week view:      had no year.
    Day view:       was in subtitle of day.

    In multiweek/month view, can also put it after the month in the
    grid cells that display a month (first/last shown day of month).

  2b. The header navigation controls are not centered over the grid.

  2c. In day view, "Tuesday" and "Tue" is redundant.

  2d. In day and week view, the left Time column seems too wide.

  2e. In day and week view there can easily be more hours than fit.
      They should be able to scroll, as they do in the prior Sunbird views.

  If it's not convenient to fix these as part of this bug,
  they may be filed as separate bugs.



3. code style nit:

  Please use 2-space indent for new files such as viewManager.js.
  Recommended by mozilla code guidelines (search for 'indent'):
  http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual

  [4-space indentation causes ambiguity when reading if's, and also
    reduces screen space available for code.  (They are particularly a
    problem when the right ends of lines extend past the right side of
    the window/screen, as happens sometimes with long xul javascript.)

    In an ambiguous if, you have to read the code carefully to see where 
    the if-body starts.  

      if (this.isALongTestCondition(withAComplexTestCondition) &&
	  this.testConditionTakesMoreThan(oneLine)) {
	  this.isTheFirstLineOf(theBody);
	  this.isTheSecondLineIn(theBody)
      }

    With 2-space indentation, the beginning of the if-body is clear from
    the indentation:

      if (this.isALongTestCondition(withAComplexTestCondition) &&
	  this.testConditionTakesMoreThan(oneLine)) {
	this.isTheFirstLineOf(theBody);
	this.isTheSecondLineIn(theBody);
      }
  ]

  There are many ambiguous ifs in viewManager.js.
  There are also ambiguous ifs added to the the view xml files,
    which are predominantly 2-space indented.

4. Errors: With the patch, I'm getting many instances of the following
   errors, but they don't seem related to the view changes in patch,
   yet I don't get them without the patch.
   Is anyone else seeing these errors?  May be cause of problems below

  4a. 
    Error: [Exception... 
      "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
       [mozIStorageStatementWrapper.row]"  
      nsresult: "0x80004005 (NS_ERROR_FAILURE)"  
      location: "JS frame ::
	file:///C:/Program%20Files/Sunbird/components/calStorageCalendar.js
	:: anonymous :: line 741"
      data: no]
    Source File:
      file:///C:/Program%20Files/Sunbird/components/calStorageCalendar.js
    Line: 741

  4b. no calendars visible in calendar list.

  4c. No events initially listed in event list.  
    Selecting the default event filter over again displays event list, 
    so may be startup issue.

  4d. no events displayed in multiweek view or month view when
    switching from another view; I do see events after clicking on an
    event in the event list.  

  4e. No events visible after transposing weekview, do see events again
    after navigating back and forth a week.

  4f. Also getting no dates in event listing date columns, maybe due to
    Error: [Exception... 
      "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE)
       [calIDateTime.getInTimezone]"  
      nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" 
      location: "JS frame :: chrome://calendar/content/unifinder.js :: 
        formatUnifinderEventDateTime :: line 230"
      data: no]
    Source File: chrome://calendar/content/unifinder.js
    Line: 230


I hope this helps!

Comment 19

[Joey Minta]

(In reply to comment #18)
> 1. viewManager modularity
>   I agree with MVL that the split between viewData objects and xbl view
>   objects calls for further refactoring.
> 
>   viewManager.js contains view-specific code that would be nice to get
>   out of the viewManager and into view files.  (dayViewData,
>   weekViewData, etc.)
I don't see much difference code-wise from putting these sorts of things into a view-file or into a data-object (unless you also mean (a) or (b) below).  Done in the easiest way, the split will be there regardless.  mvl seemed to be saying something more along the lines of (taking multiweek/month/calendar-month-view as an example):
Each view needs to implement the following functions/properties
-refresh()
-switchTo()*
-switchFrom()*
-pgUp(),pgDown()*
-setNavigation()*
-onselect()
-controller
-tasksInView
-selectedItem/Day
-showDate()
-setDateRange()
-startDate
-endDate
-add/delete/modify item()
Currently, the way this list of functions is divided between the dataObjects and the xml file seems 'random'.  The starred functions are those which month and multiweek implement in different ways.  These are exactly the functions that are included in a data-object.  There are 2 ways to avoid the 'split', either (a)put everything in 1 file, including if/switch for handling the cases where the 2 are distinct, or (b)implement everything, even the common functions, in 2 separate files, one the month and one for the multiweek.  (In essence, we remove the actual calendar-month-view binding and recreate it, with the additional starred functions, in these files.)

I'm opposed to (a) on the grounds the it limits the flexibility of the views.  With hardcoded if/switch statements, extending these views in other ways becomes much more difficult for extension authors/other implementors.  On the other hand, (b) seemed to be what mvl was going for in IRC.  The only real drawback here is code repetiveness.  All of the un-starred functions will be copied verbatim into both files.  Fixes for a bug in one will need to be migrated to the other in the majority of cases.  This, to me, is starting to sound like the horrible case we already have of 2 calendar.xul files.

> 
>   In calendar.xul, the view-title-box hardcodes the controls for every view.
>   * The horizontal controls don't fit multiweek view well.
>     Vertical controls fit better when weeks are stacked vertically.
>     Transposing the week view does not change the move controls.
>   * The control moveView parameters (-1) (-2) (-1) * (+1) (+2) (+1)
>     don't fit all views very well.  For example,
>     - in multiweek view it would be good to have a control that
>       advances by the number of weeks (which is controlled by a preference).
>     - in a year view it could be good to have controls that advance by, say,
>       -1year -1quarter -1month * +1month +1quarter +1year
> 
>   Suggest the view navigation controls be part of each view.
Keep in mind bug 266160.  It's a usability bug I feel has merit.  Also, it's entirely possible for the multiweek's view to use the switchTo call to change the oncommand of the navigation controls to be (-4)(-1)*(+1)(+4), more in line with 0.2, or to a number controlled by a preference.

> 
>   The base multi-day display and multi-week display without the controls
>   could still be separate bindings.  The base multiday-display binding
>   displays the grid, and a day-view binding contains a multiday-display
>   plus the day-view navigation controls.
This deals with the code-split again.  See discussion above.  Note also that the view-controls aren't the only problem, but actually moving the view forward and back is problematic as well.  If we place these sorts of things in a 'view-file', we have the same split as now.

> 
> 
> 2. Design issues:
> 
>   2a. The year is not displayed in any of the views.
>   2b. The header navigation controls are not centered over the grid.
>   2c. In day view, "Tuesday" and "Tue" is redundant.
>   2d. In day and week view, the left Time column seems too wide.
>   2e. In day and week view there can easily be more hours than fit.
>       They should be able to scroll, as they do in the prior Sunbird views.
> 
>   If it's not convenient to fix these as part of this bug,
>   they may be filed as separate bugs.
For the most part, these should be separate.  I can perhaps add the year to the formating of particular labels, and centering seems reasonable to look into here.  However, this is not the place to go in to the views and remove 'Tue' from the day view, lightning still needs it.  2d is the same situation.  For (2e), there is a scrollbar included in the day/week views, for me.  It doesn't show up very often, since these views try to fit everything on the screen, but it can appear.

> 3. code style nit:
> 
>   Please use 2-space indent for new files such as viewManager.js.
>   Recommended by mozilla code guidelines (search for 'indent'):
>   http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual
4-space is the tradition I've always used.  It gives the additional clarity of easily identifiable code-block ends.  With 2-space, I'm constantly trying to figure out which block is ended by a } and which prevoius block later lines fall into.  The majority of new files to the calendar-codebase since 0.2 have used 4-space indenting.  The only ones that haven't have been xml files, whereas all new js files have been 4.


> 4. Errors: With the patch, I'm getting many instances of the following
>    errors, but they don't seem related to the view changes in patch,
>    yet I don't get them without the patch.
>    Is anyone else seeing these errors?  May be cause of problems below
I believe I've seen something like 4d and 4e in lightning.  Testing there is a good way to determine if bugs are caused by this patch or should be separate.  Anything reproducable in lightning is probably something to be handled separately.  The others, I have never seen, but it does seem possible that most of them could be traced back to the bad timezone.  (I'm not really sure how kDefaultTimezone could end up invalid.  calendarDefaultTimezone() always spits out a timezone or 'floating', which are valid.)  I didn't touch anything about timezones in this patch, and for the most part, the startup order of calendar remains unchanged.

Looking forward to hearing more comments/ideas, especially on point 1.

Comment 20

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

on plugging:
The seperation between the xml and the dataobject isn't random from an implementation point of view, but it is random when looking at tha api. That a pair of views happen to have a common base doesn't need to be reflected in the api.
I wasn't talking about duplicating code. That would be a real bad solution. I was talking about inheritance. For the week and day views, there would be one base file, multiday-view.xml. The real views would inherit from that view, and implement/override the functions that are different between the two views. Code that is not different would be only in the shared base file.
That way, the api that new views will have to implement don't have the split that looks random to them. It's just one api.


On indenting:
I agree that 4 spaces looks weird for ifs, and 2 spaces is harder to look at. Both have problems. We once decided to go with 4 for new files, so we just have to learn to live with it. You can't have it all...

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 201999 [details] [diff] [review]
everything v2

My takeaway from today's IRC discussion is that we've decided there need to be at least some changes from this patch, so I'm removing the r? requests.

Comment 22

[Joey Minta]

Attachment: xbl-ification v1

OK, so this is how things might look with the xbl approach.  The base/ and lightning/ changes remain pretty much the same, only incorporating the comments from the previous round of reviews.

With regard to these new xbl-views:
-For now they're basically all the same, especially in terms of <content>.  I've stuck with the previous idea of keeping all navigation buttons at the top, because I really think bug 266160 is legitimate.  The multiweek view navigation now simply adds 1 week or 2, instead of going forward 1pg or 2pgs.  We probably will need some work here to make this a bit more clear.
-You'll notice there isn't an explicit interface that they implement yet.  However, all views do implement the same functions/attributes.  I hoped to wait and see how this stage went, to figure out what, if any, changes needed to be made to that common set, before actually putting it into an interface.  (Also, we might need to make calISelectionManager/calISelectionObserver interfaces prior to that.)

Regarding gCalendarWindow:
-In order to facilitate the transition, and keep this patch as small/simple/reviewable as possible, gCalendarWindow has remained.  I wrote up a quick currentView object that's added to gCalendarWindow, to re-route all the calls normally made.  I fully recognize that some of the non-crucial aspects of the views (ie, changing multiweek weeks in view, select all, etc) will break during this patch.  mvl seemed to indicate he was willing to accept that type of breakage as well, in an effort to keep the changes here to a minimum.

A follow-up patch (Stage 2) to this will do the following:
-kill gCalendarWindow and all references to it
-tidy up preferences/menus (fix simple bustage)
-create/really implement calIDecoratedView
-implement observers for which view is selected

Follow-up bugs will be filed for the following:
-Allow multiple selection in the views
-Allow tasksInView support for calendar-multiday-view
-Prompt user when attempting to edit a recuring event: 'Edit occurrence or parent?' (with this patch we do a mix of this, which is unclear to users)

Comment 23

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

Some quick comments on the patch:
January 2005 displays wrong for me. jan 1 is on the sunday column, while it really is a saturday. It is caused by a wrong calculation of weekno. this year, jan 1 is week 52 (or 53), but the calculation gives -1.

The style rules in calendar-decorated-views.css, the style rules should be moved to resources/sikin. (not the binding rules, just the rules that define the layout)

How hard would it to make clicking on an item in the unifinder move the view so that the event is visible?

unchecking and then checking weekdays only doesn't work. It still displays weekends.

In general, i like this approach. We will need to look at the actual api the views will implement, but we can do that later.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

jminta wrote:

> -You'll notice there isn't an explicit interface that they implement yet. 
> However, all views do implement the same functions/attributes.  I hoped to wait
> and see how this stage went, to figure out what, if any, changes needed to be
> made to that common set, before actually putting it into an interface. 

mvl wrote:
> We will need to look at the actual api the views will implement, but we can do
> that later.

The (currently implicit) calIDecoratedView goes to the very core of what this patch is all about.  I feel fairly strongly that attempting to do this work separately and later from reviewing this patch is asking for trouble.  A lot of what an interface is has to do with code documentation and explicitly defining the underlying assumptions and model relating to how code works in the specific cases and how it should work in the general case.  It seems to me that trying to review a structural change like this without having done that work is likely to overlook serious structural issues that may turn out to be a big deal later.  I've written up a short doc explaining this in more details at <http://wiki.mozilla.org/User_talk:Dmose:Code_Comments>.

Comment 25

[Joey Minta]

Attachment: xbl-ification+idl

(In reply to comment #23)
> Some quick comments on the patch:
> January 2005 displays wrong for me. jan 1 is on the sunday column, while it
> really is a saturday. It is caused by a wrong calculation of weekno. this year,
> jan 1 is week 52 (or 53), but the calculation gives -1.
Fixed.

> The style rules in calendar-decorated-views.css, the style rules should be
> moved to resources/sikin. (not the binding rules, just the rules that define
> the layout)
Done.

> How hard would it to make clicking on an item in the unifinder move the view so
> that the event is visible?
Not that hard.  Done.

> unchecking and then checking weekdays only doesn't work. It still displays
> weekends.
I think I got this.  It works for me now with the week-view (the only view that currently supports it).

(In reply to comment #24)
> The (currently implicit) calIDecoratedView goes to the very core of what this
> patch is all about.  I feel fairly strongly that attempting to do this work
> separately and later from reviewing this patch is asking for trouble.
A proposed IDL is included in this version.  In an effort to make the IDL, and its implementations as complete as possible, the views now provide an observerID attribute.  This attribute, however, will not be used until the second stage of this patch.

Comment 26

[gekacheka]

Attachment: comments on 2005-11-19 patch (250+ lines)

Overall, nice to see this progress.  

The xbl views look easier to copy and extend, though the control panel
might be extracted as a reusable xbl binding. 

Had trouble with tasks in view toggle, crashed so no events displayed.

The interfaces seem rough yet, need to consider how the list view(s)
and filtering will fit in.

calIDecoratedView.idl
? Issue: should calIDecoratedView extend calICalendarView?
  Or should calICalendarView be modified (combined with calIDecoratedView)?

  calIDecoratedView adds new capabilities:
  + workdaysOnly
  + moveView(by 'pages', view dependent page)
  + observerID (not clear, see below)
  + tasksInView (one of many possible filters?)

  calIDecoratedView adds convenience:
  + goToPage: combines showPage and setting selectedDay.

  calIDecoratedView removes:
  - selectedItem (anticipates multiple selection?)
  - setDateRange (though used internally)
  - setDateList, getDateList (though used internally)
  - supportsDisjointDates, hasDisjointDates

? observerID: the description says there may be "elements" (plural), but
  only one observer seems to be permitted.  How would multiple
  observers be managed?  Should this be more like
  addObserver/removeObserver rather than a single property?

? tasksInView: this is a type of filter.  Maybe there should be a more
  general filter interface, rather than just this property.
  For example, to filter by category, or by privacy, ...

? The filtered list view(s) can use calICalendarViewController, and
  it fits the dateRange capabilities of calICalendarView.
  Would like the list view to have the option to show "events in
  current view", so probably should share some interfaces with
  grid views so selecting the range and filters is parallel.


See attachment for MANY more comments, too many to fit inline comfortably.

I hope this helps!

Comment 27

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

Just one quick thing: The tooltips should be the same for if you are over the grid or over the unifinder or whatever. So no need to name them differently. (Maybe they are not the same now, but they should be).

Comment 28

[gekacheka]

(In reply to comment #27)
> Just one quick thing: The tooltips should be the same for if you are over the
> grid or over the unifinder or whatever. So no need to name them differently.
> (Maybe they are not the same now, but they should be).

Not simply renaming: The methods OnMouseOverGridOcccurence and OnMouseOverTaskTree take different parameters and do different things.  It is not appropriate to simply rename OnMouseOverGridOccurence to OnMouseOverItem, since the name OnMouseOverItem implies it can be used to produce a mouseover preview for any item, but OnMouseOverGridOccurrence may only be used for grid views, where the item can be looked up in a specific way (a specific field on a specific box reached from the hover mouse event's target).  The eventlist/tasklist views do not have such boxes, and the event must be looked up in a different way, so they cannot use the same method as is.

So rather than simply renaming the method, what needs to happen is to remove the grid-view specific code, and the list-view specific code, from mouseoverPreviews.js.   The code to find the item from the mouse event should be moved into the view file (e.g., calendar-multiday-view.xml mouseover handler).  The code to get the tooltip should also be in the handler (so the name of tooltip is not hardcoded in mouseoverPreviews.js, and each application can use the views with its own tooltip object).  Similarly the code for the list views should be moved to the list view files.

Then in mouseoverPreviews.js the onMouseOverGridOccurrence, onMouseOverEventTree, and onMouseOverTaskTree functions can be replaced with a single function named onMouseOverItem that takes a tooltip and an item (not a mouse event) as parameters.

/* Fills tooltipObject with info from item.
   @param tooltipObject: xul <tooltip> object to fill.
   @param item: event or task item.  If it is an occurrence, 
   its calItemBase.parentItem field will be non-null. */ 
function onMouseOverItem(tooltipObject, item);


(One issue with this change is that this duplicates code in calendar-multiday-view.xml and calendar-month-view.xml.  Maybe they could be derived from a common base class to share the handler.  It also duplicates code in unifinder.js and unifinderToDo.js.  Maybe they could become xbl bindings as well, also deriving from a common base class for list views.)

Comment 29

[Joey Minta]

(In reply to comment #26)
I've fixed many of the smaller bugs that gekacheka pointed out locally.  I'm going to hold off on submitting another iteration of the patch until I get more comments from dmose+mvl.  However, I also want to point out that many of the suggestions gekacheka made (all-day events not displayed, workdays-only in month view, context menus, OS-coloring, etc) already have separate bugs filed on them.  These won't be fixed in this patch.

I can't reproduce the error about tasks in view causing storage problems.  Nor can I reproduce any sort of crash.  There was a problem about them not displaying at first, and that part I have done.

Regarding text-coloring: if the text is unreadable, that is a bug in getContrastingTextColor, and again not one I want to fix here.  I feel that leaving events transparent makes them ugly in multiday-views and ghostly in month-views. Therefore, I oppose leaving off the default coloring.

**In general, I'm finding it difficult to balance mvl's goal of simplicity, with gekacheka's assumed goal of fewest regressions.  (The attachment seems to detail the vast majority of regressions that will be caused.)  I tend to agree with mvl's approach more.  We need to get these views in to Sunbird, despite the fallout.  The fact that bugs have already been filed on many of gekacheka's issues underlines the advantages of a single UI.  It's this advantage that I'm most eager to exploit.  As I said in a previous comment, this patch is not the end of things.  I'm fully conscious of the fact that more work will need to be done.  Trying to get it all done at once though is going to make the work too hard for the reviewers.**

Comment 30

[Joey Minta]

Attachment: also includes base bindings

This version includes fixes for several of the problems that gekacheka mentioned as well as incorporating suggestions mvl made on IRC.  The most drastic change is that base bindings are now included, and the 'normal' decorated views extend this binding, which cuts down on code duplication.  mvl was also interested in trying to put together a complete list of regressions this would cause.  Eventually this will be done in a tracking bug, but for now this is everything I can think of:

-Select All is broken (depends on Views need to support multiple selection)
-Bug 304741 (War on Boxes)
-Bug 295387 (All-day events not displayed in multiday view)
-Bug 312830 (Many items in a single day in month-view results in clipping)
-Bug 312736 (No dnd in month/multiweek view) - 0.2 regression, not 0.3a1
-Month/Multiweek views don't support alternative week-start days
-Month/Multiweek views don't support removing days off
-Bug 315960 (View switching code is slow) not sure how much/if this is a regression
-Bug 298349 (Events spanning multiple days only shown on the first day)
-Bug 315954 (Multiday view doesn't display events at end of week)
-Bug 304486 (Moving a floating event via dnd puts it in default timezone)
-Bug 315955 (Month/multiweek views sometimes display incorrect time)
-Prev/Next images don't match views
-Navigation labels not l10n friendly
-Start/End hour preference not honored in day/week views
-Day/Week views don't expand if events are outside the displayed range (depends on war on boxes?)
-Day-name appears twice in day-view (control label and top of column)
-Multiweek dayboxes are colored incorrectly - 0.2 regression, not 0.3a1
-Week numbers not perfectly accurate (depends on Move dateUtils into calDateTime)

Not really regressions but good to solve:
-gCalendarWindow is superfluous
-Inconsistency throughout app for editing recurring events (single instance vs. all instances)

This seems like a lot, and it is.  The list of bugs we solve with this switch is probably just as long, though.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 204362 [details] [diff] [review]
also includes base bindings

In general, this looks good.  Most of my commentary is about fairly minor stuff.  

calIDecoratedView
-----------------
* "impossible" commentary confusing; make clear that it's a code-sharing 
  mechanism
* workdaysOnly: out of curiousity, why is workdaysOnly specified
  in-band on the interface while the actual list of days is specified
  out-of-band in the preferences?  This isn't necessarily wrong, but
  I'm curious about the rationale here...

cal-decorated-base-view.xml
----
* mValue is not declared in calendar-navigation-buttons
* value/mValue needs commentary about how setting is expected to happen
* moveView: since this appears to be overridden by every inheriter, do we 
  really want the default implementation to do anything other than maybe throw 
  NS_ERROR_NOT_IMPLEMENTED?
* moveView: newDay might be a more accurate name than currentDay
* moveView: needs commentary about why it defaults to using 
setDateRange(currentDay, currentDay) rather than showDate

cal-decorated-day-view.xml
--------------------------
* goToDay: why both calling removeNonWorkdays since it's a no-op?
* setNavLabels: should this behave differently if removeNonWorkdays
  has been called? If not, should be able to get away with doing something
  using algorithm similar to the one in cal-decorated-month-view

cal-decorated-month-view.xml
----------------------------
* goToDay: should removeNonWorkdays be called before showDate in order
  to avoid unnecessary calculation and relayout?
* moveView: there seems to be a bracing or indentation error here; this is the error that
  always using braces is designed to prevent
* moveView: algorithm hard to understand, needs high-level commentary.  In 
  particular, I'm not understanding why the selectedDay plays a part in this.

cal-decorated-multi-week-view.xml
--
* goToDay: hard to read; individual clauses of code need comments
* why bother with if() protecting viewElement.tasksInView?
* why is selectedDay a clone of aDate?
* setNavLabels: need comments on clauses & variables (eg sDate)

cal-decorated-week-view.xml
--
same issues as multi-week

calendar-month-view.xml
--
* categories class list in month view: can we do full stripping of any
  chars that are not allowed in class identifiers so as to provide
  defense-in-depth against (eg) HTML/XUL/CSS njection attacks?  Perhaps this code
  should live somewhere more central since it's also used by the multiday view.
* mSelectionObserver: two spaces in assignment
* mSelectionObserver: if you give this function a name rather than having be anonymous, debugging it with Venkman will be easier.
* selectedItem - perhaps check for identity before bothering to unselect & reselect?
* relayout -- should dayCount always be 7, even if non workdays are omitted?

calendar-multiday-view.xml
---
* can you add a comment about why the d2.clone() is necessary?
* findColumnForItem: the task algorithm here doesn't appear to match the 
  commentary in calITodo.idl; do you feel that commentary should be changed?

Comment 32

[Joey Minta]

Attachment: new views, updated to dmose's comments

(In reply to comment #31)
> (From update of attachment 204362 [details] [diff] [review] [edit])

> calIDecoratedView
> -----------------
> * "impossible" commentary confusing; make clear that it's a code-sharing 
>   mechanism
Done.

> * workdaysOnly: out of curiousity, why is workdaysOnly specified
>   in-band on the interface while the actual list of days is specified
>   out-of-band in the preferences?  This isn't necessarily wrong, but
>   I'm curious about the rationale here...
Mostly because the actual non-workdays are specified in preferences, but whether or not they're displayed is a menu-item.

> cal-decorated-base-view.xml
> ----
> * mValue is not declared in calendar-navigation-buttons
> * value/mValue needs commentary about how setting is expected to happen
We now use event.detail, which makes a lot more sense in light of other uses of |event| throughout mozilla code.

> * moveView: since this appears to be overridden by every inheriter, do we 
>   really want the default implementation to do anything other than maybe throw 
>   NS_ERROR_NOT_IMPLEMENTED?
Done.

> * moveView: newDay might be a more accurate name than currentDay
> * moveView: needs commentary about why it defaults to using
> setDateRange(currentDay, currentDay) rather than showDate
Moot points, given above.

> 
> cal-decorated-day-view.xml
> --------------------------
> * goToDay: why both calling removeNonWorkdays since it's a no-op?
Fixed.

> * setNavLabels: should this behave differently if removeNonWorkdays
>   has been called? If not, should be able to get away with doing something
>   using algorithm similar to the one in cal-decorated-month-view
Done.

> cal-decorated-month-view.xml
> ----------------------------
> * goToDay: should removeNonWorkdays be called before showDate in order
>   to avoid unnecessary calculation and relayout?
This depends on how we actually implement removeNonWorkdays in calendar-month-view.  It's mostly here as a place-holder right now.

> * moveView: there seems to be a bracing or indentation error here; this is the
> error that
>   always using braces is designed to prevent
Fixed.

> * moveView: algorithm hard to understand, needs high-level commentary.  In 
>   particular, I'm not understanding why the selectedDay plays a part in this.
I've added some comments initially in the method to explain how I implemented the function.  gekacheka proposed an alternative solution, but I didn't want to create problems with possible double-relayout, etc that could happen by working with a day other than the selected day.

> 
> cal-decorated-multi-week-view.xml
> --
> * goToDay: hard to read; individual clauses of code need comments
Fixed.

> * why bother with if() protecting viewElement.tasksInView?
Because it forces the view to be re-drawn if we switch it.

> * why is selectedDay a clone of aDate?
Removed.

> * setNavLabels: need comments on clauses & variables (eg sDate)
Fixed.

> calendar-month-view.xml
> --
> * categories class list in month view: can we do full stripping of any
>   chars that are not allowed in class identifiers so as to provide
>   defense-in-depth against (eg) HTML/XUL/CSS njection attacks?  Perhaps this
> code
>   should live somewhere more central since it's also used by the multiday view.
This is going to require changes elsewhere in the code (where we set the selectors, and where we prevent double-category entries).  As such, I'd rather do this in a separate bug.  (Fortunately, we haven't had any problems yet.)

> * mSelectionObserver: two spaces in assignment
> * mSelectionObserver: if you give this function a name rather than having be
> anonymous, debugging it with Venkman will be easier.
Fixed.

> * selectedItem - perhaps check for identity before bothering to unselect &
> reselect?
In theory, the selection manager ought to prevent this case from happening.

> * relayout -- should dayCount always be 7, even if non workdays are omitted?
No, but again, since we can't remove non workdays yet, I didn't want to try and predict how that would be implemented and allow for it.

> 
> calendar-multiday-view.xml
> ---
> * can you add a comment about why the d2.clone() is necessary?
Fixed.

> * findColumnForItem: the task algorithm here doesn't appear to match the 
>   commentary in calITodo.idl; do you feel that commentary should be changed?
We don't actually have the ability to display tasks in this view yet.  I think a decision about this can probably be left until we do a proper implementation.  findColumnForItem was changed only because it's called early enough that tasks haven't always been filtered out.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Almost there!

(In reply to comment #32)

> > * moveView: algorithm hard to understand, needs high-level commentary.  In 
> >   particular, I'm not understanding why the selectedDay plays a part in
> >   this.
> I've added some comments initially in the method to explain how I implemented
> the function.  gekacheka proposed an alternative solution, but I didn't want
> to create problems with possible double-relayout, etc that could happen by 
> working with a day other than the selected day.

Isn't that only a problem if you assume that the expected semantics are for the same day in the new month to be selected?  It's not at all clear to me that a user would expect that (though I'd be interested to hear arguments about why that might be true).  gekacheka's suggestion seems a lot simpler.  Whatever we do, we should probably document in the IDL for moveView what selection semantics are expected, even if that's just "left as an implementation detail for the view".

> > * why bother with if() protecting viewElement.tasksInView?
> Because it forces the view to be re-drawn if we switch it.

I guess what I really mean is that I think it makes more sense to have that if check pushed down into the tasksInView setter so that every caller isn't required to write it.

> > calendar-month-view.xml
> > --
> > * categories class list in month view: can we do full stripping of any
> >   chars that are not allowed in class identifiers so as to provide
> >   defense-in-depth against (eg) HTML/XUL/CSS njection attacks?  Perhaps this
> > code
> >   should live somewhere more central since it's also used by the multiday view.
> This is going to require changes elsewhere in the code (where we set the
> selectors, and where we prevent double-category entries).  As such, I'd rather
> do this in a separate bug.  (Fortunately, we haven't had any problems yet.)

OK, if you could file that bug, that would rock.

> > * selectedItem - perhaps check for identity before bothering to unselect &
> > reselect?
> In theory, the selection manager ought to prevent this case from happening.
> 
> > * relayout -- should dayCount always be 7, even if non workdays are omitted?
> No, but again, since we can't remove non workdays yet, I didn't want to try and
> predict how that would be implemented and allow for it.
>
> > * findColumnForItem: the task algorithm here doesn't appear to match the 
> >   commentary in calITodo.idl; do you feel that commentary should be changed?
> We don't actually have the ability to display tasks in this view yet.  I think
> a decision about this can probably be left until we do a proper implementation.
>  findColumnForItem was changed only because it's called early enough that tasks
> haven't always been filtered out.
> 

For the above three items, can you add comments to the code explaining this stuff so that it's clear to the reader?

Comment 34

[gekacheka]

Initialization problem with 20051204 patch: 
the startDate/endDate of multiday views are null for me, probably not getting initialized.  Need to be careful how they are initialized, several methods seem to assume they are already initialized (e.g., numVisibleDates)

Symptom 1: 
  Start sunbird.
  Select an item in any view, such as event list, or day view.

  Error: this.mStartDate has no properties
  Source File: chrome://calendar/content/calendar-multiday-view.xml
  Line: 1975
  (This is in numVisibleDates.)

    view.selectedDay (setter)
      view.showDate
	view.numVisibleDates 
	  view.mStartDate.clone()

Symptom 2:
   Start sunbird.
   Drag a time region to create an event, say in day view.

   Error: [Exception... "Component returned failure code:
                         0x80070057 (NS_ERROR_ILLEGAL_VALUE)
                         [calIDateTime.compare]" 
           nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"
           location: "JS frame ::
	     file:///C:/Program%20Files/Sunbird/components/calEvent.js
             :: anonymous :: line 242"  data: no]
   Source File: file:///C:/Program%20Files/Sunbird/components/calEvent.js
   Line: 242
   (This is in calEvent.getOccurrencesBetween)

   view observer calls to 
     item.getOccurrencesBetween(this.calView.startDate, ...)
   are passing null values for startDate, endDate.

Comment 35

[Joey Minta]

Attachment: new views (20 Dec)

Updated to dmose's last set of comments.  Includes the requested comments and a redone moveView for the decorated month that should hopefully be more clear.  Also includes some changes to fix conflicts with the checkin from Bug 315960.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 206403 [details] [diff] [review]
new views (20 Dec)

OK, r=dmose; nice work!

Comment 37

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

Comment on attachment 206403 [details] [diff] [review]
new views (20 Dec)

This patch look very good. (although i didn't look at all the details)
I guess we should just land it, and try to fix the regressions.
r=mvl.

Comment 38

[Joey Minta]

Patch checked in.  Please file regressions as blocking the New Views regression tracking bug, bug 321164.

RCS file: /cvsroot/mozilla/calendar/base/content/calendar-decorated-base.xml,v
done
Checking in mozilla/calendar/base/content/calendar-decorated-base.xml;
/cvsroot/mozilla/calendar/base/content/calendar-decorated-base.xml,v  <--  calendar-decorated-base.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/calendar/base/content/calendar-decorated-day-view.xml,v
done
Checking in mozilla/calendar/base/content/calendar-decorated-day-view.xml;
/cvsroot/mozilla/calendar/base/content/calendar-decorated-day-view.xml,v  <-- calendar-decorated-day-view.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/calendar/base/content/calendar-decorated-month-view.xml,v
done
Checking in mozilla/calendar/base/content/calendar-decorated-month-view.xml;
/cvsroot/mozilla/calendar/base/content/calendar-decorated-month-view.xml,v  <--  calendar-decorated-month-view.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/calendar/base/content/calendar-decorated-multiweek-view.xml,v
done
Checking in mozilla/calendar/base/content/calendar-decorated-multiweek-view.xml;
/cvsroot/mozilla/calendar/base/content/calendar-decorated-multiweek-view.xml,v  <--  calendar-decorated-multiweek-view.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/calendar/base/content/calendar-decorated-views.css,vdone
Checking in mozilla/calendar/base/content/calendar-decorated-views.css;
/cvsroot/mozilla/calendar/base/content/calendar-decorated-views.css,v  <--  calendar-decorated-views.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/calendar/base/content/calendar-decorated-week-view.xml,v
done
Checking in mozilla/calendar/base/content/calendar-decorated-week-view.xml;
/cvsroot/mozilla/calendar/base/content/calendar-decorated-week-view.xml,v  <--  calendar-decorated-week-view.xml
initial revision: 1.1
done
Checking in mozilla/calendar/base/content/calendar-month-view.xml;
/cvsroot/mozilla/calendar/base/content/calendar-month-view.xml,v  <--  calendar-month-view.xml
new revision: 1.10; previous revision: 1.9
done
Checking in mozilla/calendar/base/content/calendar-multiday-view.xml;
/cvsroot/mozilla/calendar/base/content/calendar-multiday-view.xml,v  <--  calendar-multiday-view.xml
new revision: 1.47; previous revision: 1.46
done
Checking in mozilla/calendar/base/public/Makefile.in;
/cvsroot/mozilla/calendar/base/public/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
RCS file: /cvsroot/mozilla/calendar/base/public/calIDecoratedView.idl,v
done
Checking in mozilla/calendar/base/public/calIDecoratedView.idl;
/cvsroot/mozilla/calendar/base/public/calIDecoratedView.idl,v  <--  calIDecoratedView.idl
initial revision: 1.1
done
Checking in mozilla/calendar/lightning/content/messenger-overlay-sidebar.js;
/cvsroot/mozilla/calendar/lightning/content/messenger-overlay-sidebar.js,v  <--  messenger-overlay-sidebar.js
new revision: 1.26; previous revision: 1.25
done
Checking in mozilla/calendar/lightning/content/messenger-overlay-sidebar.xul;
/cvsroot/mozilla/calendar/lightning/content/messenger-overlay-sidebar.xul,v  <--  messenger-overlay-sidebar.xul
new revision: 1.34; previous revision: 1.33
done
Checking in mozilla/calendar/resources/jar.mn;
/cvsroot/mozilla/calendar/resources/jar.mn,v  <--  jar.mn
new revision: 1.118; previous revision: 1.117
done
Checking in mozilla/calendar/resources/content/calendar.js;
/cvsroot/mozilla/calendar/resources/content/calendar.js,v  <--  calendar.js
new revision: 1.212; previous revision: 1.211
done
Checking in mozilla/calendar/resources/content/calendar.xul;
/cvsroot/mozilla/calendar/resources/content/calendar.xul,v  <--  calendar.xul
new revision: 1.196; previous revision: 1.195
done
Checking in mozilla/calendar/resources/content/calendarManagement.js;
/cvsroot/mozilla/calendar/resources/content/calendarManagement.js,v  <--  calendarManagement.js
new revision: 1.15; previous revision: 1.14
done
Checking in mozilla/calendar/resources/content/calendarWindow.js;
/cvsroot/mozilla/calendar/resources/content/calendarWindow.js,v  <--  calendarWindow.js
new revision: 1.80; previous revision: 1.79
done
Checking in mozilla/calendar/resources/content/menuOverlay.xul;
/cvsroot/mozilla/calendar/resources/content/menuOverlay.xul,v  <--  menuOverlay.xul
new revision: 1.71; previous revision: 1.70
done
Checking in mozilla/calendar/resources/content/mouseoverPreviews.js;
/cvsroot/mozilla/calendar/resources/content/mouseoverPreviews.js,v  <--  mouseoverPreviews.js
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/calendar/resources/skin/classic/calendar.css;
/cvsroot/mozilla/calendar/resources/skin/classic/calendar.css,v  <--  calendar.css
new revision: 1.68; previous revision: 1.67
done
Checking in mozilla/calendar/resources/skin/modern/calendar.css;
/cvsroot/mozilla/calendar/resources/skin/modern/calendar.css,v  <--  calendar.css
new revision: 1.95; previous revision: 1.94
done
Checking in mozilla/calendar/sunbird/base/content/calendar-menubar.inc;
/cvsroot/mozilla/calendar/sunbird/base/content/calendar-menubar.inc,v  <--  calendar-menubar.inc
new revision: 1.17; previous revision: 1.16
done
Checking in mozilla/calendar/sunbird/base/content/calendar-scripts.inc;
/cvsroot/mozilla/calendar/sunbird/base/content/calendar-scripts.inc,v  <--  calendar-scripts.inc
new revision: 1.8; previous revision: 1.7
done
Checking in mozilla/calendar/sunbird/base/content/calendar-sets.inc;
/cvsroot/mozilla/calendar/sunbird/base/content/calendar-sets.inc,v  <--  calendar-sets.inc
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/calendar/sunbird/base/content/calendar.xul;
/cvsroot/mozilla/calendar/sunbird/base/content/calendar.xul,v  <--  calendar.xul
new revision: 1.34; previous revision: 1.33
done
Checking in mozilla/calendar/sunbird/themes/pinstripe/sunbird/calendar.css;
/cvsroot/mozilla/calendar/sunbird/themes/pinstripe/sunbird/calendar.css,v  <--  calendar.css
new revision: 1.14; previous revision: 1.13
done
Checking in mozilla/calendar/sunbird/themes/pinstripe/sunbird/jar.mn;
/cvsroot/mozilla/calendar/sunbird/themes/pinstripe/sunbird/jar.mn,v  <--  jar.mn
new revision: 1.12; previous revision: 1.11
done
Checking in mozilla/calendar/sunbird/themes/winstripe/sunbird/calendar.css;
/cvsroot/mozilla/calendar/sunbird/themes/winstripe/sunbird/calendar.css,v  <--  calendar.css
new revision: 1.31; previous revision: 1.30
done
Checking in mozilla/calendar/sunbird/themes/winstripe/sunbird/jar.mn;
/cvsroot/mozilla/calendar/sunbird/themes/winstripe/sunbird/jar.mn,v  <--  jar.mn
new revision: 1.13; previous revision: 1.12
done