271525 - Simplify retrieveAndSaveLocalCalendar

Status: RESOLVED INVALID

(Calendar :: General, defect)

Description

[ago]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Now retrieveAndSaveLocalCalendar (in calendarmanager.js) uses 2 callback
functions which make it relatively complex to read. All this function does is:

1) download
2) reload
3) merge edited item (optional)
4) upload (optional)

It would be better to split in the above components. Moreover download() should
be incorporated directly into reload(), since in no case it should be called by
itself giving a further simplification and a unified reload mechanism.

At this point it is much more readable if the calls to reload/merge/upload 

are made directly in saveItem/deleteItem all other calls to
retrieveAndSaveLocalCalendar in other modules should be replaced by calls to
reload(). No callback is required. So retrieveAndSaveLocalCalendar can be
collapsed into a download method. 

Reproducible: Always
Steps to Reproduce:

Comment 1

[ago]

addCalendar() can also be incorporated into reload, in fact it is a reload
without removeCalendar....

Comment 2

[ago]

checkCalendarURL() should also be modified, again there are callbacks and a call
to calendar.js! From the name I would expect to check the url for syntax error
or the existance of a file or something similar but in fact it can create an
event if there isn't one and invoke the edit event dialog, or create a local
file and download.... 

...All this though not directly, it would be too easy, but indirectly via
callback, so that the code is actually executed within
getRemoteCalendarText()!!! I'd say confusing at best... getRemoteCalendarText
should not take any callback argument at all, and just do what the name says...
Similar for checkCalendarURL (whose name should probably be changed into
checkRemoteCalendar or something). On top of this there is code duplication that
can be separated into download/reload functions outlined above...


I would say confusing... All this  involves a callback and a call to calendar.js
(as a rule calendarManager should not call any method of calendar.js)...
Confusing to say the least. It also uses 

Comment 3

[ago]

And within getRemoteCalendarText not only there is the callback above but also a
call to reloadCalendars... And it does not even Return Calendar Text! It returns
a boolean!!!!!!

Comment 4

[Richard Klein]

I'm making this a blocker to bug 271409 Reorganization of calendarManager.js

Comment 5

[ago]

I guess there is little point playng further with calendarManager since a lot of
work has been done on the new interface calICalendar. I was not aware of it when
I wrote this bug.

Comment 6

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

yeah, calICalendar and calICalendarManager now does the job.

Comment 7

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!