Create a binding for the calendar list

RESOLVED FIXED in 1.0b1

Status

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

({dev-doc-needed})

Dependency tree / graph

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted patch [checked in] Fix - v1 β€” β€” Splinter Review
Right now we use the calendar list in many places, each with its own implementation. I'd like to consolidate this. This patch is the first step, which moves the implementation from the main calendar list into a binding. Using this binding for other calendar lists (i.e choose calendar dialog) should be done in a different bug.
Attachment #359264 - Flags: review?(Berend.Cornelius)
(Assignee)

Updated

10 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

10 years ago
Blocks: 428538
(Assignee)

Updated

10 years ago
Whiteboard: [needs review]

Comment 1

10 years ago
Comment on attachment 359264 [details] [diff] [review]
[checked in] Fix - v1

Big patch. I tested it and could not detect any deficiencies. Looks like a good job. Maybe you could take yourself the time to consolidate the event handlers for the key-pressed up and key-pressed-down events.
r=berend
Attachment #359264 - Flags: review?(berend.cornelius09) → review+
Due to limited time I'd rather leave the patch as is. It might look quite duplicated, but the common parts don't make sense on their own.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/90197594f3b6>

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Correction: rev 43ba72c39c9f
Depends on: 480383
Depends on: 480933
Depends on: 480930

Comment 4

10 years ago
There was a little copy&paste error in this patch. If you have a color assigned to at least one category, the color won't be displayed anymore and Thunderbird has problems to start up. A log message "aCalendar is not defined" will be created.

The fix is simple. Line 576 in /calendar/base/content/calendar-views.js says
let ruleString = '.category-color-box[categories~="' + aCalendar.id + '"] {} ';
but should say
let ruleString = '.category-color-box[categories~="' + aCatName + '"] {} ';

After changing this, everything works fine again.
(In reply to comment #4)
Maybe you could provide a patch for corresponding Bug 480933?
Posted patch Bustage fixes - v1 (obsolete) β€” β€” Splinter Review
This patch takes care of all three regression bugs (bug 480383 bug 480930 bug 480933) and also bug 349987.
Attachment #364991 - Flags: review?(ssitter)
Comment on attachment 364991 [details] [diff] [review]
Bustage fixes - v1

Creating a new profile seems to work now but still displays the following assertion:
[[[
Error: [Exception... "Calendar list has no composite calendar yet"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://calendar/content/widgets/calendar-list-tree.xml :: get_compositeCalendar :: line 67"  data: no] 

STACK: 
1: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calUtils.js:1218] notifyFunc
2: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calUtils.js:1221] calListenerBag_notify
3: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js:574] anonymous
4: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js:642] anonymous
5: [null:0] null
6: [chrome://calendar/content/calendar-management.js:104] initHomeCalendar
7: [chrome://calendar/content/calendar-management.js:87] loadCalendarManager
8: [chrome://calendar/content/calendar-chrome-startup.js:43] commonInitCalendar
9: [chrome://calendar/content/calendar.js:52] calendarInit
10: [chrome://sunbird/content/calendar.xul:1] onload

Source File: file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calUtils.js
Line: 1218
]]]

The changes in the categories pane of the preference dialog break the following workflow:
Edit category, click the color picker and choose a color -> use color checkbox is enabled, press OK to save the changes -> changes are not saved

Changing a category color doesn't update the colors in the view for me.

The following is logged when closing the main window:
[[[
Error: categoryPrefBranch.removeObserver is not a function
Source file: chrome://calendar/content/calendar-views.js
Line: 555
]]]
Attachment #364991 - Flags: review?(ssitter) → review-
Posted patch Bustage fixes - v2 (obsolete) β€” β€” Splinter Review
Attachment #364991 - Attachment is obsolete: true
Attachment #365010 - Flags: review?(ssitter)
Attachment #365010 - Attachment is obsolete: true
Attachment #365021 - Flags: review?(ssitter)
Attachment #365010 - Flags: review?(ssitter)
Comment on attachment 365021 [details] [diff] [review]
[checked in] Bustage fixes - v3

I'm not sure about the eval() part but the rest looks OK. r=ssitter
Attachment #365021 - Flags: review?(ssitter) → review+
Another regression that seem to be caused by this patch:
[*] Setting a calendar to read-only throws:
    Error: aPrefName is not defined
    Source File: chrome://calendar/content/calendar-task-tree.xml
    Line: 903
[*] Changing calendar color updates the events in the view with the new color but not box next to the calendar name in the list of calendars
[*] Pressing SPACE key in the calendar list doesn't toggle the visibility anymore
Comment on attachment 365021 [details] [diff] [review]
[checked in] Bustage fixes - v3

I replaced eval with a call to (new Function(arg,body))(arg)

Pushed to comm-central  <http://hg.mozilla.org/comm-central/rev/c92e3a7a0e1c>
Attachment #365021 - Attachment description: Bustage fixes - v3 → [checked in] Bustage fixes - v3
(Assignee)

Updated

10 years ago
Whiteboard: [needs review]
(In reply to comment #11)
> Another regression that seem to be caused by this patch:
> [*] Setting a calendar to read-only throws:
>     Error: aPrefName is not defined
>     Source File: chrome://calendar/content/calendar-task-tree.xml
>     Line: 903
Couldn't reproduce, but fixed in code.

> [*] Changing calendar color updates the events in the view with the new color
> but not box next to the calendar name in the list of calendars
This worked for me?

> [*] Pressing SPACE key in the calendar list doesn't toggle the visibility
> anymore
Fixed.
Attachment #365147 - Flags: review?(ssitter)
Comment on attachment 365147 [details] [diff] [review]
[checked in] Second set of bustage fixes - v1

r=ssitter but I'd add a comment to the handler like |check for key=" " because keycode="VK_SPACE" doesn't work|

The calendar color update issue still exists for me. I tested with Sunbird, maybe there is a different behavior compared to Lightning?
Attachment #365147 - Flags: review?(ssitter) → review+
Comment on attachment 365147 [details] [diff] [review]
[checked in] Second set of bustage fixes - v1

I found the color change issue. It happens only when changing between colors, but not when the color was previously the default color. Fix is in the pushed patch.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/78b9ab3d6c7e>
Attachment #365147 - Attachment description: Second set of bustage fixes - v1 → [checked in] Second set of bustage fixes - v1
(Assignee)

Updated

10 years ago
Attachment #359264 - Attachment description: Fix - v1 → [checked in] Fix - v1
Depends on: 482116
Depends on: 482110
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.