Closed
Bug 418542
Opened 16 years ago
Closed 16 years ago
CalDAV provider forces calendar visibility on startup, refresh
Categories
(Calendar :: Provider: CalDAV, defect)
Calendar
Provider: CalDAV
Tracking
(Not tracked)
RESOLVED
FIXED
0.8
People
(Reporter: browning, Assigned: browning)
References
Details
Attachments
(1 file, 1 obsolete file)
3.63 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
The new CalDAV provider refresh() code uses a memory calendar to which it adds items using modifyItem. With the new offline code, if there's an observer around for the modifyItem, ensureCalendarVisible gets called, so calendars which have been (un)checked invisible get re-visible-ized. Thanks to Maxime Delorme for finding the regression.
Attachment #304377 -
Flags: review?(philipp)
Comment 1•16 years ago
|
||
is bug 418569 a duplicate of this one ?
Comment 3•16 years ago
|
||
We should take this for 0.8!
Assignee: nobody → browning
Flags: blocking-calendar0.8?
OS: Linux → All
Comment 4•16 years ago
|
||
Is the caldav calendar cached? In that case this is bug 412963, but this will definitely not happen for 0.8. Sorry.
Assignee | ||
Comment 5•16 years ago
|
||
No, reporter (see latter comments in bug 393817) is not using cached calendars, and I have verified the problem using my own calendars. The symptoms are somewhat similar, but this is a distinct bug.
Comment 6•16 years ago
|
||
Thanks Bruno for pointing to my comment, I confirm that is not using cached calendars And with this bug : my view are overcrowded by 15 calendars, lightning goes to 70% of my CPU, and I can use it anymore If this bug is not correct for 0.8, I will stick to 0.7 So I would suggest to flag it as blocking 0.8
Assignee | ||
Comment 7•16 years ago
|
||
Comment on attachment 304377 [details] [diff] [review] disable, enable observer during refresh() Bah, this patch is not quite right yet and I am withdrawing review request. Looks like addObserver lets us add the same observer more than once, with bad results. Updated patch as soon as I can manage.
Attachment #304377 -
Flags: review?(philipp)
Comment 8•16 years ago
|
||
Not a blocker, but we'll take the patch if it arrives in time.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
Assignee | ||
Comment 9•16 years ago
|
||
... and here it is.
Attachment #304377 -
Attachment is obsolete: true
Attachment #304889 -
Flags: review?(philipp)
Comment 10•16 years ago
|
||
Does this work out even though the provider is async? Is the refresh/delete call synchrounous? If not, some observer calls might be missed. r+ with my questions clarified. I will be away until thursday, if anything else needs to go in for 0.8, please ask berend or maybe Sebo.
Assignee | ||
Comment 11•16 years ago
|
||
This patch effects code that is run after the async i/o to the remote calendar, and just effects the observer on the memory calendar. One hunk is at the end of the REPORT parser, where it is triggered by refresh/addItem/modifyItem, and it just allows an onLoad signal to be observed, the other relevant one is at the end of deleteItem and allows a deleteItem on the memory calendar to be observed. Both sending the onLoad and doing the deletion on the memory calendar are synchronous operations.
Comment 12•16 years ago
|
||
Please, make this patch happened for 0.8, otherwise lightning 0.8 will be at least for me completely unusable
Comment 13•16 years ago
|
||
Comment on attachment 304889 [details] [diff] [review] enable/disable observer during refresh/delete r=philipp
Attachment #304889 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 14•16 years ago
|
||
Patch checked in on HEAD and MOZILLA_1_8_BRANCH ->FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 15•16 years ago
|
||
Thanks Bruno I get the nightly build, and now I can use again lightning. So I can now see what bugs 412963 and bug 412914 really are ;-D Thanks for your great job I was a pleasure to find this regression
Updated•16 years ago
|
Target Milestone: --- → 0.8
Comment 16•16 years ago
|
||
Hmm, this solution looks rather hacky to me, and assumes that the memory provider works synchronously. I am unsure whether it's better to implement local operation listeners (when acting on the memory provider) and forward errors etc to caldav's registered calIObservers. Anyway, I think we are facing the same problem in the caching code.
Assignee | ||
Comment 17•16 years ago
|
||
(In reply to comment #16) > Hmm, this solution looks rather hacky to me, > Agreed, this was a bad solution all the way around. It's about to get rolled back by a patch in bug 420749 and be replaced with a slightly less hacky temporary solution. The problem here is calling ensureCalendarVisible on each add|modifyItem, and I think the proper solution to that is going to be in UI code rather than in providers.
Updated•16 years ago
|
Flags: blocking-calendar0.8-
You need to log in
before you can comment on or make changes to this bug.
Description
•