Closed Bug 418542 Opened 12 years ago Closed 12 years ago
DAV provider forces calendar visibility on startup, refresh
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.
is bug 418569 a duplicate of this one ?
We should take this for 0.8!
Assignee: nobody → browning
OS: Linux → All
Is the caldav calendar cached? In that case this is bug 412963, but this will definitely not happen for 0.8. Sorry.
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.
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
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.
Not a blocker, but we'll take the patch if it arrives in time.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
... and here it is.
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.
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.
Please, make this patch happened for 0.8, otherwise lightning 0.8 will be at least for me completely unusable
Comment on attachment 304889 [details] [diff] [review] enable/disable observer during refresh/delete r=philipp
Attachment #304889 - Flags: review?(philipp) → review+
Patch checked in on HEAD and MOZILLA_1_8_BRANCH ->FIXED
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
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.
(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.
You need to log in before you can comment on or make changes to this bug.