388405 - Unify calendar list between lightning and sunbird

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

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

Ideally, there should only be code for one calendar list that works in both sunbird and lightning. Futhermore, to get rid of the screenshot-checkbox that we are currently using in ltn, it would be better to use a <listbox> with a <listheader>. I at least have the XUL for this working, now I just need the overlay.

I'd prefer using a listbox. This might make implementing bugs like bug 255180 harder, but a tree doesn't have "real" checkboxes. They use images for checkboxes. I'd like to avoid this.

Comment 1

[Stefan Sitter [:ssitter]]

I would suggest to use the calendar list from Sunbird as the base list because this list currently exposes less errors than the calendar list in Lightning.

Comment 2

[Stefan Sitter [:ssitter]]

Philipp, do you plan to reuse the Sunbird solution in Lightning? This should help to fix Bug 373983.

Comment 3

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

I will be using a richlistbox, so that bug will also be fixed (and some others ;)

Comment 4

[Pete Riley]

I understand the benefit of unifying the code between Lightning and Sunbird.  However, since you're changing the code anyway, is it currently impossible to change it in a way that allows for the implementation of, for example, bug 255180 and bug 266249?  Would it be better to first improve the tree control to do everything that you want it to do, and then implement this bug and the other two bugs, all at the same time?  That would be so nice!

To improve the tree control, does this require a bug request to other people at Mozilla, or can you use inheritance or something to create a "calendar tree class" that combines the checkboxes, calendar icons, and calendar names?  I'm just asking.  :)

Comment 5

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

Attachment: Unify Calendar List - v2

I replaced alot of things, please take a close look at this patch to see if something could break. Some testing on mac would be great too, since I also touched the hidden window. I solved bug 350323 a bit differently here: hidden calendars only get shown when an event is added to the respective calendar or modified. Does that sound like a good solution?

Things that can possibly break (i.e areas that have been touched)
  
 * Creating/Editing events or tasks in various ways (clicks, toolbar, keys, context menus)
 * Creating/Properties/Deleting a calendar in various ways (clicks, toolbar, keys, context menus)
 * reload remote calendars from various locations (clicks, toolbar, keys, context menus)
 * export/publish calendar from various locations (clicks, toolbar, keys, context menus)
 * Mac Hidden window (missing overlays? everything disabled as needed?)
 * Calendar/Category colors (especially changing them, is everything refreshed?)
 * All context menus or other popups

I have filed a bunch of bugs I have found while creating this patch. A further folloup bug I have not filed, it might be smart to put commonly used sets from sunbird/base/content/calendar-sets.inc and lightning/content/messenger-overlay-sidebar.xul into a single file and include from both.

Comment 6

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

Attachment: Unify Calendar List - v2 (ignoring whitespaces)

This is the same patch, but ignoring whitespace changes, to make the review easier.

Comment 7

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

Attachment: Screenshots of Sunbird and Lightning

Comment 8

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

Pete, I only saw your comments after submitting the patches, I have a slow line here, sorry :) I do admit, the use of a richlistbox will make at least the tree bug a bit harder to fix. The sorting bug is pretty easy to fix, although its fix currently requires trunk. I'll try and see how this could be done with a tree, but since I have already written the code, would it be ok to fix this in a separate bug? Mickey is on vacation until the 24th, I'll see what I can do until then.

Comment 9

[Pete Riley]

> I'll try and see how this could be done with a tree, but since
> I have already written the code, would it be ok to fix this in
> a separate bug?

Of course.  Maybe there is no need to create another bug for the tree since bug 255180 already exists.

I have another comment about the current behavior of the list of calendars (in 0.5 and 0.7pre).  It seems that there should be a "wait cursor" (hourglass) while Lightning is loading/unloading calendars.  For example, if a calendar takes ten seconds to load or unload, when I click the checkbox, there is NO visual feedback in the UI that Lightning is processing.  Even the checkmark doesn't appear/disappear until after the ten seconds.

I think that this is bad because some people (e.g. my mom) are going to click the checkbox multiple times because they think that nothing is happening.

I don't know if this should belong to this bug or if I (or anyone) should create a new bug to handle this in all areas of Lightning/Sunbird instead of only in the calendar list.

Comment 10

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

Sorry, but no, this is not the bug to ask for other improvements. The patch is already long enough as it is. Trying to put more into it won't do any good, but will only delay the fixing of this bug.

Comment 11

[Christian Jansen]

Comment on attachment 273327 [details]
Screenshots of Sunbird and Lightning

I appreciate that you dropped the faked check boxes. From an UI-perspective this patch seems to be ok. 

One nit to fix: Please assure that the calendar box provides in lightning a Label, like we have for Tasks.

With that change r=christian


Calendars
+----------------+
| [ ] Private    |
| [X] Work       |
...

Comment 12

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

Comment on attachment 273322 [details] [diff] [review]
Unify Calendar List - v2

Canceling review for now, I have managed to make a tree that works. I just need to fix some remaining issues.

Comment 13

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

Attachment: Unify Calendar List - v4

This patch now uses a tree, therefore has the header Christian asked about. The only problem that may be is that it doesn't really look coorect on a mac. I have no mac to test this though. The checkbox on a mac is drawn using OS styles, so all that is needed is setting -moz-appearance: checkbox. I'm not sure this cycles correctly in a tree though. We might need to use screenshots on a mac.

I do think this needs to be tested on a mac before being checked in, but in any case the only thing that should need to change then is the CSS. I resolved a couple of tree-specific issues, I'll gather the list of bug#'s soon.

Comment 14

[Michael Büttner (no reviews TFN)]

Unfortunately I need to say that the patch doesn't work on a Mac. Everything except the checkbox inside of the tree-control just looks fine. The checkbox itself won't be displayed, I have the impression that moz-appearance: checkbox is gracefully ignored on a Mac (at least in this context).

I won't have much time to dive into why this doesn't work, but I'd like to take the rest of the patch as it consolidates a hell of a lot of code. Wouldn't it be a good idea to just still use an image instead of a native control until we have some time to track down this particular piece of the puzzle?

Furthermore, one issue I found was that the default calendar is not selected while switching to calendar mode for the first time.

Comment 15

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

I think its ok to use a checkbox image on the mac for now. I think it has to do with the OS styles and only works in html:input elements. Mossop filed bug 389526 to create a skin global image, although I'm not too sure this will be fixed, since native styles are pretty nice if they can be used...

I'll investigate the default calendar being selected issue.

Comment 16

[Michael Büttner (no reviews TFN)]

Some more fly-by comments... I tested this patch now on Windows, Branch build, on Sunbird and Lightning. In general, everything works just fine and I'd be glad if we could take this patch in the 0.7 timeframe.

Lightning & Sunbird, 1) switch off calendar 2) select it 3) create new event (e.g. by dragging in the view). The calendar gets automatically switched back on, but only after the mouse enters the calendar list. Holding the mouse still in the view leaves the calendar switched off. Probably this should just get switched back on immediately.

Lightning, some command names are not consistent. For example, the calendar view context menu refers to a command 'calendar_create_event_command', but the actual name of the command is 'calendar_new_event_command'. This renders those locations unusable, of course.

Sunbird, there are stippled lines around the entries in the calendar list. I know, they are also present in the unifinder, but is this really intended? It looks a bit weired.

Sunbird, reload doesn't work if only WCAP calendars are configured. As soon as an ICS calendar has been added, it works as advertised. I remember that Daniel fixed this with Bug 389397, but it doesn't seem to work with Sunbird. But probably this doesn't have anything to do with this patch.

Comment 17

[Michael Büttner (no reviews TFN)]

Comment on attachment 274261 [details] [diff] [review]
Unify Calendar List - v4

>+++ calendar/base/content/calendar-calendars-list.xul	2007-07-28 09:41:26.398608000 +0200
This file looks fine for me. As a side note, it's now the second time within two days when I was thinking that it's a good idea to unify the commands for Sunbird and Lightning. Do we want to address this in this patch or open another one? It's probably a good idea to file another one, as I see this one is already rather large.

>+        gCompositeCalendar.prefPrefix = 'calendar-main';
What's this prefix used for? As far as I know it was used to distinguish between the calendars in Sunbird and Lightning, but isn't this something we can get rid of if even the calendar manager is not unified? Please note my comments on Bug #388985 (by the way, this patch fixes this other bug).

>+function getSelectedCalendar() {
>+    var tree = document.getElementById("list-calendars-tree");
>+    return (tree.currentIndex > -1) &&
>+           calendarListTreeView.mCalendarList[tree.currentIndex];
>+}
First, is there a reason for naming the tree 'list-calendars-tree' instead of 'calendar-list-tree', or something like that? I would suggest that we indicate the hierarchy order from left to right, high-level names are placed at the left, more specific stuff finds its place to the right. But that's personal preference and I'm sure that I didn't follow this rule in my own code. :-) The same argument applies here and elsewhere (e.g. ...-tree-color).

>+function reloadCalendars() {
>+    getCompositeCalendar().refresh();
>+}
Is it necessary to have a top-level function that just wraps a function call to a public available service? Other code can call both versions (call composite calendar directly or reloadCalendars) with both versions being correct. I don't have strong feelings about this, but it strikes me as being superfluously.

>+function loadCalendarManager() {
>+function unloadCalendarManager() {
Both functions are just fine for me.

>+function calendarListInitColors() {
>+function calendarListUpdateColor(aCalendar) {
These functions mix the initialization of the calendar list with the initialization of the views. I suggest have an explicit initialization function in calendar-views.js and split the parts from these function to where they really belong (calendar list vs views). These functions are the only callers of updateStyleSheetForObject(), which I see as a clear indicator of view related code being mixed up with code that is responsible for something else.

>+var calendarListTreeView = {
All of the code in this section is just fine. I'm asking myself why we're fighting against anonymous function? Is there any clear benefit? I feel that it just adds visual clutter, and doesn't buy us anything in the debugger. I know that we religiously fighting for this, but it strikes me as being superfluously. Feel free to tell me that I'm totally on crack.

>+    onCalendarRegistered: function cMO_onCalendarRegistered(aCalendar) {
>+        // Enable new calendars by default
>+        calendarListTreeView.addCalendar(aCalendar);
>+        getCompositeCalendar().addCalendar(aCalendar);
It's probably just fine for now, but don't we want to persist this visibility of distinct calendars? We can address this in a spin-off bug as it's out of scope for this bug. Reading this line just reminded me of this. Would you please be so kind to file a new one on this?

>+        document.getElementById("calendar_new_event_command")
>+                .removeAttribute("disabled");
>+        document.getElementById("calendar_new_todo_command")
>+                .removeAttribute("disabled");
>+        document.getElementById("calendar_delete_calendar_command")
>+                .removeAttribute("disabled");
I was thinking whether or not it would be better to make the commands being self-contained? Introducing new commands would place the burden to know that this particular piece of code controls the commands. I suggest to make it similar to the function onSelectionChanged() in messenger-overlay-sidebar.js and the 'disabledwhennoeventsselected'-attribute.

>+        // Since the calendar hasn't been removed yet, <= 1 is correct.
>+        if (calendars.length <= 1) {
>+            // If there are no calendars, you can't create events/tasks or
>+            // delete calendars
>+            document.getElementById("calendar_new_event_command")
>+                    .setAttribute("disabled", true);
>+            document.getElementById("calendar_new_todo_command")
>+                    .setAttribute("disabled", true);
>+            document.getElementById("calendar_delete_calendar_command")
>+                    .setAttribute("disabled", true);
>+        }
See my previous comment, same applies here of course.

>+++ calendar/base/themes/pinstripe/calendar-management.css	2007-07-28 09:41:26.588881600 +0200
>+treechildren::-moz-tree-cell(list-calendars-tree-checkbox) {
>+    -moz-appearance: checkbox;
>+    width: 14px;
>+    height: 12px;
>+    margin: 2px 0px 2px 1px;
>+    padding: 0;
>+}
The version of this rule in pinstripe/winstripe differ. Is this intended?

>+treechildren::-moz-tree-cell(color-default) {
>+    background-color: #a8c2e1;
>+}
What's the reason for hard-coding the color? Can't we use any of the pre-defined colors?

> function ltnOnLoad(event)
> {
>+    // Load the Calendar Manager
>+    loadCalendarManager();
Just for reference purposes, this line is what fixes Bug #388985 (or hides it, depending on point of view).

>+++ calendar/lightning/content/messenger-overlay-sidebar.xul	28 Jul 2007 07:27:40 -0000
Besides the mixed up command names and the possibility to unify the commands and I didn't find anything to complain about in this file.

Unfortunately I need to minus the patch per this and previous comments.

Comment 18

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

Attachment: Unify Calendar List - v5

Fixed the default calendar selection issue. I modified the pinstripe CSS in a way I believe it might work, but again I have no mac to test it.

Comment 19

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

Comment on attachment 274803 [details] [diff] [review]
Unify Calendar List - v5

Oops, I totally missed the last two comments, I guess I should have reloaded before :)

Comment 20

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

Attachment: Unify Calendar List - v5

(In reply to comment #16)
> The calendar gets automatically switched back
> on, but only after the mouse enters the calendar list.
Fixed

> Lightning, some command names are not consistent.
Fixed

> Sunbird, there are stippled lines around the entries in the calendar list.
Fixed
> 
> Sunbird, reload doesn't work if only WCAP calendars are configured. As soon as
> an ICS calendar has been added, it works as advertised. I remember that Daniel
> fixed this with Bug 389397, but it doesn't seem to work with Sunbird. But
> probably this doesn't have anything to do with this patch.
Yes, I also believe this doesn't have to do with the patch. Please test withough the patch and file a bug if appropriate

(In reply to comment #17)
> ... it's a good idea to unify the commands for Sunbird and Lightning...
Filed, see dependancies


> 
> >+        gCompositeCalendar.prefPrefix = 'calendar-main';
> What's this prefix used for? As far as I know it was used to distinguish
> between the calendars in Sunbird and Lightning, but isn't this something we can
> get rid of if even the calendar manager is not unified? Please note my comments
> on Bug #388985 (by the way, this patch fixes this other bug).
I'm not quite sure, It seemed to me to be for the calendar prefs. For example, if you have the calendar in the composite, doing |getCalendarManger().getCalendarPref(cal, "calendar-main-in-composite");| will return true. I don't really mind either way, but if we want to remove the prefPrefix, this should happen in an extra bug and also clean up the composite calendar code.

> First, is there a reason for naming the tree 'list-calendars-tree' instead of
> 'calendar-list-tree', or something like that?
Fixed

> >+function reloadCalendars() {
Moved directly into command sets

> >+function calendarListInitColors() {
> >+function calendarListUpdateColor(aCalendar) {
> These functions mix the initialization of the calendar list with the
> initialization of the views.
Since this is also the way it was in the old calendar management, I would like to have it taken care of in a separate bug. If this is ok with you, I will file one.

> I'm asking myself why we're fighting against anonymous functions?
Looking at venkman, this gives me nice names to look at. At least in objects, venkman will know what the function name is and call it [onAddItem] or such, but with the non-anonymous function and its prefix, I also have an idea which object that function belongs to. (i.e cMO_onAddItem). Even though it clutters a bit, I think its pretty :)

> It's probably just fine for now, but don't we want to persist this visibility
> of distinct calendars?
Bug filed, see dependancies

> I was thinking whether or not it would be better to make the commands being
> self-contained?
Not a bad idea, but also something that can be done in a spin-off bug, if thats ok with you. I started adding this to my files, but then noticed, there are many other commands that might need to be disabled when no calendars exist. These were quite many (new event/todo, modify/delete event/todo, export, reload remote, publish, even paste, ......) so I think its sensible to put this in a bug where we can discuss what we want disabled, if we want to allow deleting all calendars, if we should pop up the calendar wizard if no calendars exist and a such command is executed, etc.

> >+treechildren::-moz-tree-cell(list-calendars-tree-checkbox) {
> The version of this rule in pinstripe/winstripe differ. Is this intended?
Yes, the checkboxes are different between pin/winstripe. The new version will use an image on the mac and the constructed checkbox on windows.

> What's the reason for hard-coding the color? Can't we use any of the
> pre-defined colors?
What do you mean with pre-defined? This is the color that calendars had by default even before this patch. See bug 306244. The real issue is that calendars don't get a color if you don't set one. I'm not sure there is a bug on that.

Comment 21

[Michael Büttner (no reviews TFN)]

I'm fine with addressing the above mentioned issues in spin-off bugs as this patch is already quite large and should therefore land sooner than later. Regarding the prefPrefixes I'm also fine with leaving this at it is for now. I'm going to try this patch on a Mac (the still waiting for the build) before giving r+, otherwise the patch looks fine for me.

Comment 22

[Michael Büttner (no reviews TFN)]

Comment on attachment 274827 [details] [diff] [review]
Unify Calendar List - v5

>+treechildren::-moz-tree-image(calendar-list-tree-checkbox) {
>+    list-style-image: url(chrome://calendar/skin/unifinder/checkbox-unchecked.gif);
>+}
>+
>+treechildren::-moz-tree-image(calendar-list-tree-checkbox, checked) {
>+    list-style-image: url(chrome://calendar/skin/unifinder/checkbox-checked.gif);
>+}
We just had a debug session in order to make this patch work on Mac. The references to these files are not correct as Philipp just found out by himself. Apart from that the patch works just fine in Lightning & Sunbird on Mac and Windows. r=mickey with this issue being addressed. Thanks for this one, Philipp.

Comment 23

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

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 24

[Stefan Sitter [:ssitter]]

This caused the following regression in Sunbird: Changing date format and timezone in the preferences causes the following errors:

  Error: currentView is not a function
  Source File: chrome://calendar/content/calendar-management.js  Line: 615

  Error: currentView is not a function
  Source File: chrome://calendar/content/calendar-management.js Line: 624

Comment 25

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

Attachment: Regression fix

I don't know *why* this fixes it, but it does. Both getMinimonth() and currentView() are in the same file, but using venkman it shows that getMinimonth is defined, while currentView() is not in those lines.

Mickey, Stefan, do you have an idea why?

Comment 26

[Michael Büttner (no reviews TFN)]

Comment on attachment 276749 [details] [diff] [review]
Regression fix

>-                var currentView = currentView();
>+                var currentView = window.currentView();
Shame on me that I didn't spot this in the original patch. Anyway, this fix is not the right way, even if it works. Your local variable 'currentView' is hiding the function with the same name. So, that's why it works if you fully qualify the function name (with window.currentView). Besides that, even after having this fixed (by changing the name of the local variable) another exception gets thrown, see below for details. It happens because the multiday view is called but we're still in mail mode.

Error: this.mStartDate has no properties
Source File: chrome://calendar/content/calendar-multiday-view.xml
Line: 2510

Comment 27

[Yannick Baril]

This caused the following regression in Sunbird:
All calendar are selected on start. I must unselected the unwanted each time i restart.

Also I don't know if its related to this patch but the disappearing event get back worst than ever. I'm not able to see one of my 12 .ics calendar on the main window and Sunbird is not responding during what seems to be the reload of calendars. Events appear in the event list.

sunbird calendar: 20070815

Comment 28

[Stefan Sitter [:ssitter]]

(In reply to comment #27)
> All calendar are selected on start. I must unselected the unwanted
> each time i restart.

Already filed as Bug 390523

Comment 29

[Stefan Sitter [:ssitter]]

This check-in probably also regressed Bug 392387.

Comment 30

[Stefan Sitter [:ssitter]]

(In reply to comment #25)
Philipp, Michael, I filed Bug 392388 to track this regression.

Comment 31

[Stefan Sitter [:ssitter]]

This also regressed the feature from Bug 362930 – Selected calendar will not be restored after restart. Philipp, do you want me to file a new bug or do you want it to handle it with Bug 390523 too?

Comment 32

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

I'll handle that with bug 390523

Comment 33

[Martin Schröder [:mschroeder]]

VERIFIED with Lightning 0.7pre (2007082803) on WinXP and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070828 Calendar/0.7pre.