Closed Bug 265274 Opened 20 years ago Closed 20 years ago

Shared calendar for small LANs using flat-files

Categories

(Calendar :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: arusso, Assigned: mostafah)

Details

Attachments

(2 files, 17 obsolete files)

37.45 KB, patch
Details | Diff | Splinter Review
1.24 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10.1

This is a simple patch to allow clients on a small network to share a calendar
without using a didicated server. File locks are implemented using file flags
which are not bullet proof but good enough for small networks and small calendar
files (larger files might be split into a ro file with old events and a rw file
with only active events). Please review the code carefully it is my first
adventure in Mozillaland (and js)...

PS I reload the calendar after any edit, this way if something bad happens at
least the user will NOT see the event he just edited...

Reproducible: Always
Steps to Reproduce:
1. Use the calendar.js and calendarManager.js provided, 
2. Create a calendar, 
3. Edit CalendarManager.rdf > path to point two client to the same file (it must
be on a shared folder with rw permission, symbolic links to the shared file will
not work)
4. Set CalendarManager.rdf > shared to "true"
5. CalendarManager.rdf > autoreload to "5000"



Expected Results:  
That should be it, try to create events on the same calendar using different
clients.
Attached patch calendar.js (obsolete) — Splinter Review
Only a couple of lines changed within refreshRemoteCalendarAndRunFunction to
call retrieveAndSaveLocalCalendar
Attached patch calendarManager.js (obsolete) — Splinter Review
Added two new attributes to CalendarObject(), created a new method
retrieveAndSaveLocalCalendar(), and modified addCalendar() and reloadCalendar()
Attached patch Diff file for calendar.js (obsolete) — Splinter Review
Attached patch Diff file for calendarManager.js (obsolete) — Splinter Review
TBD In calendarManager.retrieveAndSaveLocalCalendar() the equivalent of a wait()
method should be implemented or the maxIter part should be modified so that you
do not get trapped in an endless loop.
Summary: Flatfile-based shared calendar for small LANs → Shared calendar for small LANs using flat-files
Attachment #162734 - Attachment is patch: true
Comment on attachment 162734 [details] [diff] [review]
Diff file for calendarManager.js

>+var gDateModified = new Object();
Aren't you using it as an array? why not declare it as such?

>@@ -108,15 +113,30 @@ function calendarManager( CalendarWindow
> 
>    document.getElementById( "list-calendars-listbox" ).builder.rebuild();
> 
>    /* add active calendars */
>    for( var i = 0; i < this.rootContainer.getSubNodes().length; i++ )
>    {
>-      if( this.rootContainer.getSubNodes()[i].getAttribute( "http://home.netscape.com/NC-rdf#active" ) == "true" )
>+      var calendar= this.rootContainer.getSubNodes()[i]; 
>+      if( calendar.getAttribute( "http://home.netscape.com/NC-rdf#active" ) == "true" )
>       {
>          this.addCalendar( this.rootContainer.getSubNodes()[i] );
>+        var autoreload=calendar.getAttribute( "http://home.netscape.com/NC-rdf#autoreload" );
check your indenting.
In general, i see inconsitent whitespace use in the patch. I'm not sure which
use is correct, but at least try to be consistent. something like 'a= 4' is
wrong anyway
>+        if ( autoreload > 0 ) 
>+        {
>+          function closure(ThisObject,ThisCalendar){
>+            function autoreloadCmd(){
>+              ThisObject.reloadCalendar(ThisCalendar); 
>+            }
>+            return autoreloadCmd;
>+          }
>+          var X=closure(this, calendar);
>+          //var autoreloadCmd="gCalendarWindow.calendarManager.reloadCalendar( gCalendarWindow.calendarManager.getCalendarByName('" + calendar.getAttribute( "http://home.netscape.com/NC-rdf#path" ) + "'));"
Why add code that is commented out? just remove it.
>+          setInterval( X,autoreload); //does not work with "this"
>+          

>@@ -400,16 +430,16 @@ calendarManager.prototype.editServerDial
> /*
> ** Add the calendar so it is included in searches
> */
> calendarManager.prototype.addCalendar = function calMan_addCalendar( ThisCalendarObject )
> {
>    //dump( "\n CALENDAR MANANGER-> add calendar with path "+ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path"+"\n\n" ) );
>-
>-   gICalLib.addCalendar( ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" ),
>-                         ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#type" ));
>-
>+  var path = ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" );
>+  var dateModified = new File( path ).dateModified;
Will that succeed if there is no file, so if you are creating a new file?
>+  gICalLib.addCalendar( path, ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#type" ));
>+  gDateModified[ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#serverNumber")]=dateModified;
Can you try to make this line a bit more readable? Too much nesting for my
taste...
>    //ThisCalendarObject.setAttribute( "http://home.netscape.com/NC-rdf#active", "true" );

>+/*
>+** Save local calendar
>+*/
>+calendarManager.prototype.retrieveAndSaveLocalCalendar = function calMan_retrieveAndSaveLocalCalendar( ThisCalendarObject, ThisCalendarEvent,functionToRun )

Why move the execution of the method into this method? It isn't related to the
management of the list of calendars, so just leave it in the other file, where
it was.

>+{
>+  var path =ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" );
>+
>+  if (ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#shared" )){
>+    var iter =0; 
>+    var timeOut=5000; //millisecs
unfortunatly, that's not enough. Some operations are real slow on large files.

>+      if (iter > maxIters) {
>+        alert("Calendar file '" + path + "' is locked");
You should use the promptservice here, and make it localizable.

>       //now convert it, and put it in the RDF file.
>       var node = this.rootContainer.addNode(name);
>       node.setAttribute("http://home.netscape.com/NC-rdf#active", thisCalendar.name);
>       node.setAttribute("http://home.netscape.com/NC-rdf#serverNumber", nameId);
>       node.setAttribute("http://home.netscape.com/NC-rdf#name", thisCalendar.name);
>+      node.setAttribute( "http://home.netscape.com/NC-rdf#shared",  thisCalendar.shared);
>+      node.setAttribute( "http://home.netscape.com/NC-rdf#autoreload",  thisCalendar.autoreload );
this is like the 10th time you need to add this. I whish it was centralized....
(but that's not for this bug)
> Aren't you using it as an array? why not declare it as such?
Yes good point.

> Why add code that is commented out? just remove it.
Yep, I posted the file I play with... Next time I'll clean it up.

> Will that succeed if there is no file, so if you are creating a new file?
Good point will need to test

> Can you try to make this line a bit more readable? Too much nesting for my
> taste...
No prob

> Why move the execution of the method into this method? It isn't related to the
> management of the list of calendars, so just leave it in the other file, where
> it was.
That is why I personally believe it should be there... "Local" calendars are
also members of the list of calendars... Also for simmetry with
retrieveAndSaveLocalCalendar... De gustibus....
 
> unfortunatly, that's not enough. Some operations are real slow on large files.
That is why I used a variable. Do not take the settings in there too seriously.
Maybe it could even be a parameter. As mentioned though, I believe that the best
approach with large files is to split them up, particularly regarding non
recurring old events (ro)... 

> You should use the promptservice here, and make it localizable.
Yes I was not sure how to do that

> this is like the 10th time you need to add this. I whish it was centralized....
Me too :)
The timeOut might also be a function of the file size...
(In reply to comment #7)
> > Why move the execution of the method into this method? It isn't related to the
> > management of the list of calendars, so just leave it in the other file, where
> > it was.
> That is why I personally believe it should be there... "Local" calendars are
> also members of the list of calendars... Also for simmetry with
> retrieveAndSaveLocalCalendar... De gustibus....

No, i mean the functionToRun part. You are running a function on the calendar.
That isn't part of managing the list of calendars.
I did so not to use a callback which I do not particularly like (I think it
makes the code harder to read). 
Attachment #162733 - Attachment is patch: true
Comment on attachment 162734 [details] [diff] [review]
Diff file for calendarManager.js

>+    //Reload again so if somethng goes wrong the user will not see the edited event, which is a good thing...

I'm not sure if it is a good thing. It works if you added an event, or if you
changed something that is visible in the view you were in. But what if i change
say the url? or add a recurrence in 2 months? I won't notice that it failed,
and assume it succeeded. (yes, not easy to fix, i know)
Hmmm I could compare the edited event to the one in the newly reloaded calendar... 
Attached patch Diff file for calendarManager.js (obsolete) — Splinter Review
New version. Incorporates mvl comments. Most changes were made to
retrieveAndSaveLocalCalendar(). Now it checks the event after a save to make
sure everything went well. Should be safer. Improved lock checks couple of bugs
fixed.
Attachment #162731 - Attachment is obsolete: true
Attachment #162734 - Attachment is obsolete: true
Attachment #163613 - Flags: second-review?(mvl)
Attachment #163613 - Flags: first-review?(mostafah)
Comment on attachment 163613 [details] [diff] [review]
Diff file for calendarManager.js

>--- calendar.orig/content/calendar/calendarManager.js	Tue Sep  7 17:18:50 2004
>+++ calendar/content/calendar/calendarManager.js	Wed Oct 27 19:51:08 2004
>@@ -108,16 +113,29 @@ function calendarManager( CalendarWindow
>+         autoreload = calendar.getAttribute( "http://home.netscape.com/NC-rdf#autoreload" );
>+         if ( autoreload > 0 ) 
>+         {
>+            function closure(ThisObject,ThisCalendar){
>+               function autoreloadCmd(){
>+                 ThisObject.reloadCalendar(ThisCalendar); 
>+               }
>+               return autoreloadCmd;
>+            }
>+            var X=closure(this, calendar);
>+            setInterval( X,autoreload); 
>+         }
>+      }
Is all this nesting really needed? Isn't there a simpler way to do setTimeout?

>@@ -272,13 +295,15 @@ calendarManager.prototype.addServerDialo
>    node.setAttribute("http://home.netscape.com/NC-rdf#name", CalendarObject.name);
>-   
>+   node.setAttribute( "http://home.netscape.com/NC-rdf#shared",  CalendarObject.shared);
>+   node.setAttribute( "http://home.netscape.com/NC-rdf#autoreload",  CalendarObject.autoreload);
>+      
>    var profileFile;
Remove that whitespace change

>@@ -301,15 +326,15 @@ calendarManager.prototype.addServerDialo
>+      profileFile = this.getProfileDirectory(); //Isn't this code useless??
>+      profileFile.append( "Calendar" ); //Isn't this code useless??
>+      profileFile.append("RemoteCalendar"+ next+ ".ics"); //Isn't this code useless??
Then remove it :)

>@@ -400,16 +431,22 @@ calendarManager.prototype.editServerDial
> calendarManager.prototype.addCalendar = function calMan_addCalendar( ThisCalendarObject )
> {
>    //dump( "\n CALENDAR MANANGER-> add calendar with path "+ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path"+"\n\n" ) );
>-
>-   gICalLib.addCalendar( ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" ),
>-                         ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#type" ));
>-
>+   var path = ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" );
>+   var calendarFile = new File( path );
>+   if ( ! calendarFile.exists() ){
>+      calendarFile.create();
>+   }

How sure are you that the path is non-empty?
I'm also not too crazy about the new File() thing. What is it's benifit over
the usual nsILocalFile?

>+   var dateModified = calendarFile.dateModified;
>+    
>+   gICalLib.addCalendar( path, ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#type" ));
>+   var serverNumber=ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#serverNumber");
>+   gDateModified[serverNumber]=dateModified;
Fix whitespaces arount the = here.

>@@ -428,19 +465,113 @@ calendarManager.prototype.removeCalendar

>-calendarManager.prototype.reloadCalendar = function calMan_reloadCalendar( ThisCalendarObject )
>+calendarManager.prototype.reloadCalendar = function calMan_reloadCalendar( ThisCalendarObject, skipRefresh 
Document that changed parameter. Add a comment about this explaining what the
parameters are.

>   //TODO implement reloadCalendar inside gICalLib
>-  gICalLib.removeCalendar( ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" ) );
>-  gICalLib.addCalendar( ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" ),
>-                        ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#type" ));
>-  refreshView();
>+   var path = ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" );
>+   var dateModified = new File( path ).dateModified;
>+   var id = ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#serverNumber");
>+   if (gDateModified[id]< dateModified) {

What happens if the time on the server and on the client differ? Will all this
go wrong? I am afraid it will.

>+      gICalLib.removeCalendar( path );
>+      this.addCalendar( ThisCalendarObject);
>+      if (calMan_reloadCalendar.arguments.length == 1){
>+         refreshView();
huh? so if you pass false for skipReload it will still reload? Bad.
>+      }
>+   }
>+}
>+
>+/*
>+** Save local calendar
>+*/
>+calendarManager.prototype.retrieveAndSaveLocalCalendar = function calMan_retrieveAndSaveLocalCalendar( ThisCalendarObject, ThisCalendarEvent,functionToRun )
>+{
>+  var path =ThisCalendarObject.getAttribute( "http://home.netscape.com/NC-rdf#path" );
>+
>+   if (ThisCalendarObject.getAttribute( ! "http://home.netscape.com/NC-rdf#shared" )){
>+      eval( "gICalLib."+functionToRun+"( ThisCalendarEvent, path )" );
>+   }else{
Whitespaces around else. Check the rest of the file.
Also, check indenting. First line is 2, rest is three.

>+      var lockTimeOut=5000; //millisecs after which a lock expires
>+      var maxWaitTime=10000; //millisecs waited before giving up
>+      var loopWait=500; //millisecs to wait before rechecking lock
whitespace. And in the rest of the file.

>+      var CalendarFile=new File(path);
>+      var LockFile = new File(path + ".lock");
>+      var DummyFile = new File(path  + ".dummy"); //used only to read the filesystem clock
>+            
>+      function compareEvents(Event1,Event2){
>+         function safeStr(str){
>+            str=str.replace(/\r/g,"\n");
>+            str=str.replace(/\n+/g,"\n");
>+            str=str.replace(/\nX-.+\n\s\:.+/g,"");
So, if you change X-MOZILLA-SYNCID you count it as not changed? Or any other X-
property.
>+            return str;
>+         }
>+         return (safeStr(Event1.getIcalString()) == safeStr(Event2.getIcalString()));
>+      }
This is way to nested for my taste.
And what if the event happens to get serialized in a different order?

>+      
>+      //Make TimeOut depend on file size
>+      lockSize =  CalendarFile.size;
>+      lockTimeOut =lockTimeOut +lockSize/1000; //+1 sec per MB
>+      maxWaitTime = maxWaitTime +lockSize/500; //+2 sec per MB
Add a minimum. And a maximum.
>+            
>+       //Get time of server hosting remote file, simulating _nix touch command
>+      if (DummyFile.exists()){
>+        DummyFile.remove();
>+      }
>+      DummyFile.create();
>+      var timeServer=DummyFile.dateModified.getTime();
>+      DummyFile.remove();
>+      
>+      //Check lock
You really should document this loop. I have a hard time figuring out what it
does.

>+      var timeStart=new Date().getTime();
>+      var timeOffset=timeServer-timeStart;
>+      var timeLoop=0;
>+      while(1){
>+         timeNow=new Date().getTime();
>+         if (timeNow > timeLoop+loopWait) {
>+            timeLoop=timeNow;
>+            if (! LockFile.exists() ){
>+               break;
>+            } else if ((T=LockFile.dateModified) && (timeNow + timeOffset > T.getTime() +lockTimeOut)){
Eww. move the assignment out of the if.

>+               //Delete old locks
>+               LockFile.remove();
>+               break;
>+            }
>+         }
>+         //Avoid endless loop
>+         if (timeNow > timeStart + maxWaitTime) {
>+            alert(gCalendarBundle.getString( "unableToWrite" )  + CalendarFile.path);
>+            return null;
>+         }
>+      }
>+      LockFile.create();
>+      
>+      //Do not override other people changes to other events, reload!
>+      this.reloadCalendar( ThisCalendarObject, true ); 
>+      //Save
>+      eval( "gICalLib."+functionToRun+"( ThisCalendarEvent, path )" );
I don't really like event management inside the calendar manager.

>+      //Reload
>+      this.reloadCalendar( ThisCalendarObject );
>+      LockFile.remove();
>+      //Check that edited event is actually in the calendar file
>+      try{
>+         Event=gICalLib.fetchEvent(ThisCalendarEvent.id);
>+      }catch(er) {
>+         Event=null;
>+      }
>+      if (Event != null){
>+         if (functionToRun.match(/delete/)){
>+            alert(gCalendarBundle.getString( "unableToWrite") + CalendarFile.path );  
>+         }else if ( ! compareEvents(Event,ThisCalendarEvent)){
>+            alert(gCalendarBundle.getString( "unableToWrite") + CalendarFile.path );  
>+         }
>+      }
Don't you want to warn if the event is null?
>+   }
>+   gCalendarWindow.clearSelectedEvent( ThisCalendarEvent );
> }
> 
>@@ -588,13 +719,13 @@ calendarManager.prototype.isURLAlreadySu
> calendarManager.prototype.checkCalendarURL = function calMan_checkCalendarURL( Channel )
>-{
>+{
What's this?

>@@ -806,12 +937,14 @@ calendarManager.prototype.getAndConvertA
>          thisCalendar.serverNumber = ArrayOfCalendars[i];
>          thisCalendar.name = getCharPref(prefService.getBranch( "calendar." ), "server"+ArrayOfCalendars[i]+".name", "" );
>+         thisCalendar.shared = getCharPref(prefService.getBranch( "calendar." ), "server"+ArrayOfCalendars[i]+".shared", false );
Can we please cache the prefService.getBranch( "calendar." )

>@@ -820,12 +953,14 @@ calendarManager.prototype.getAndConvertA
Before we start advertising shared files on a lan, think about non-mozilla
clients. If for example apple ical is used, the whole locking falls apart.
otoh, this is better then nothing until the right way using sync is actually
written.
(In reply to comment #15)
> Before we start advertising shared files on a lan, think about non-mozilla
> clients. If for example apple ical is used, the whole locking falls apart.
> otoh, this is better then nothing until the right way using sync is actually
> written.

This is only for for mozilla apps. If one manipulates the shared file with an
other app it falls apart. True. But on this line of reasoning if one opens the
calendar file with winword and saves it, it will fall apart.... So what? There
are calendar apps which are not standard compliant and they might break the
calendar anyway, even if you sync. I like the idea of syncing, if it is fast
enough, the patch above is less robust against external apps manipulation (which
program isn't???), but it is faster than a sync, and more importantly it is
working now... It is a trade-off.
> Is all this nesting really needed? Isn't there a simpler way to do setTimeout?
I need a closure, the closure can be put outside the nest if you like. But a
closure is needed. It doesn't change much.

> Then remove it :)
I am quite sure it is useless, but I would like some confirmation, it is not
part of the code I am testing. I just noticed it reading the code.

> How sure are you that the path is non-empty?
Then gICalLib.addCalendar will have an issue to solve before I get to use File()
:) The error handling should behave as gICalLib.addCalendar, if it skips the
error and allows further execution, I will be as kind to following code (I'll
add a try statement)... 

> I'm also not too crazy about the new File() thing. What is it's benifit over
> the usual nsILocalFile?
No idea, it works and it is simple to use. But if you have any preference for
another i/o library with simila functionality I'll use that instead.

> What happens if the time on the server and on the client differ? Will all this
> go wrong? I am afraid it will.
Nope! It will work! dateModified is always the date of the server and it is
always compared to the current dateModified of the file on the server... The
only assignement is in addCalendar... Local clock is never used...

> huh? so if you pass false for skipReload it will still reload? Bad.
yep, it was a quick hack I forgot to go over it to beautify 

> So, if you change X-MOZILLA-SYNCID you count it as not changed? Or any other
X-property.
It is necessary I am afraid, there is a spurious
"X-MOZILLA-RECUR-DEFAULT-INTERVAL :0" which only comes up after a save/reload
(and possibly other X- settings), if I do not suppress it, ALL comparisons will
evaluate to false, I discussed that with mostafah. Considering that this is a
safe check when the lock has already failed, the probability of an undetected
failure is P(lock fails)*P(same event is edited externally)*P(X- property was
changed)=ridiculously low risk. When a proper .equals() method is implemented I
will use it, for now this is adequate.

> And what if the event happens to get serialized in a different order?
You will get a warning that the save might have failed. Not the end of the
world.... Much better to have a warning which is unjustified, than to have no
warning when one would be justified... Again, when a proper .equals() method is
available, that won't be an issue. PS it never happened in my tests. 

> You really should document this loop. I have a hard time figuring out what it
> does.
Every half seconds it checks if a lock exists, if it does, it checks if the lock
is an old leftover (in which case it deletes the lock and breaks, otherwise it
rechecks after half second), and then it makes sure we do not waste a full day
checking for locks :)... I will improve comments...

> I don't really like event management inside the calendar manager.
The alternative is extensive use of callbacks, which are way harder to read... 

> Don't you want to warn if the event is null?
Null is good if the operation was "delete", but yes I should warn for null and
operation !="delete"... Good point.


> What's this?
Not sure, I did not change that function, probably I accidentally added some
white space or something

> Can we please cache the prefService.getBranch( "calendar." )
pls explain
(In reply to comment #16)
> This is only for for mozilla apps. If one manipulates the shared file with an
> other app it falls apart. True. But on this line of reasoning if one opens the
> calendar file with winword and saves it, it will fall apart.... So what? 

Apple Ical is all about calendering. So it should not break stuff. If you
advertise that you can share a file with other computers, it should not assume
everybody uses mozilla calendar. It should also work with other apps. And if
somebody wishes to use winword to manually type events into the file, it should
still work, as long as the produced events are valid.

But like i said, this is better then nothing att all, until real syncing works.

(In reply to comment #17)
> I need a closure, the closure can be put outside the nest if you like. But a
> closure is needed. It doesn't change much.
Why do you need the closure? Other usages of setTimeout in the mozilla code
don't seem to need it. (but then again, i'm no expert)

> I am quite sure it is useless, but I would like some confirmation, it is not
> part of the code I am testing. I just noticed it reading the code.
Either remove it, or leave it. you decision. But don't just comment about it.


> No idea, it works and it is simple to use. But if you have any preference for
> another i/o library with simila functionality I'll use that instead.
I personally prefer the mozilla api's over third party api's, if only for less
dependency.

> > So, if you change X-MOZILLA-SYNCID you count it as not changed? Or any other
> X-property.
> It is necessary I am afraid, there is a spurious
> "X-MOZILLA-RECUR-DEFAULT-INTERVAL :0" which only comes up after a save/reload
> (and possibly other X- settings), if I do not suppress it, ALL comparisons will
Yes those other X- properties can be important. You can't just ignore them.

> evaluate to false, I discussed that with mostafah. Considering that this is a
> safe check when the lock has already failed, the probability of an undetected
> failure is P(lock fails)*P(same event is edited externally)*P(X- property was
> changed)=ridiculously low risk. When a proper .equals() method is implemented I
> will use it, for now this is adequate.

You can easily write a equals. Just a function that takes tow events, and
compare every property that's important. 
Also, it is called always, not only when the lock fails.

> > You really should document this loop. I have a hard time figuring out what it
> > does.
One more thing about the loop: it will still chew away all processor time while
waiting. You need to find a wait(), or do funkey setTimeout stuff.
> 
> > I don't really like event management inside the calendar manager.
> The alternative is extensive use of callbacks, which are way harder to read... 
Or add a lockCalendar and unlockCalendar to calendarManager, and call those.

> > What's this?
> Not sure, I did not change that function, probably I accidentally added some
> white space or something
I didn't see any whitespace change, i saw nothing. That's why i got confused.
Thinking about it, it could be a \r\n to \n change or something similar.

> > Can we please cache the prefService.getBranch( "calendar." )
> pls explain
Instead of calling getBranch 10 times, call it once and store the result.
var prefBranch = prefService.getBranch( "calendar." ); prefBranch.doSomething()



(In reply to comment #18)
> (In reply to comment #16)
> > > So, if you change X-MOZILLA-SYNCID you count it as not changed? Or any other
> > X-property.
> > It is necessary I am afraid, there is a spurious
> > "X-MOZILLA-RECUR-DEFAULT-INTERVAL :0" which only comes up after a save/reload
> > (and possibly other X- settings), if I do not suppress it, ALL comparisons will
> Yes those other X- properties can be important. You can't just ignore them.
I can't think of any of the X-MOZILLA settings which are important and can't be
discarded. Also note that a js level .equals function will discard X- entries 
anyway.
> Apple Ical is all about calendering. So it should not break stuff. If you
> advertise that you can share a file with other computers, it should not assume
> everybody uses mozilla calendar. 

I disagree. If I share something it is between the same type of applications. If
I can also share with third party applications, that is an additional feature,
very welcome in fact, but it is not a requirement.

> Why do you need the closure? 

Because I am passing objects, not trivial functions and I have to "freeze" the
objects I am passing... Otherwise the setTimeout subsequent calls will use the
objects as of the time the call is executed.

> I personally prefer the mozilla api's over third party api's, if only for less
> dependency.
file.js is in tree, in fact in calendar/content I thought it was mozilla
stuff... Anyway swapping should not be too difficult. I'll have a look.

> Yes those other X- properties can be important. You can't just ignore them.

I doubt "X-MOZILLA-RECUR-DEFAULT-INTERVAL\n :0" is since it always comes out
after save/reload on otherwise absolutely identical events ruining my party... I
could have skipped just that, but I was worried that other X- surprises might be
lurking behind the corner so I killed all X- stuff... If someone can guarantee
that "X-MOZILLA-RECUR-DEFAULT-INTERVAL\n :0" is the only surprise I will be
stricter.

>> You can easily write a equals. Just a function that takes tow events, and
>> compare every property that's important. 

This function will have to skip the X- property then... And I will end up at the
same point... Only real advantage is order change, which never created issues so
far in my application.

> Also, it is called always, not only when the lock fails.

If locks do not fail there is no chance an X- property is overwritten
externally, so if I skip those X- properties and locks work... I do not care. It
becomes a problem if locks fail AND exactly the same record is overwritten
externally AND X- property is changed. Then I will skip a warning I should have
issued. I am more concerned about being hit by lighting :D

> One more thing about the loop: it will still chew away all processor time
while waiting. You need to find a wait(), or do funkey setTimeout stuff.

True but only when you save something and there is a lock. I briefly looked for
a wait() implementation, but did not find anything reasonable. Here for
instance, there are some suggestions
http://www.faqts.com/knowledge_base/entry/edit/index.phtml?aid=1602, but they
are not adequate in my case (in the "while-wait" example execution is
asynchronous and continues immidiately after the tid=... line). I probably have
to trap the "while-wait" code within another while(1) loop and put everything in
a nice wait function... that should do.... PS what is a good place for a wait js
method?

> Or add a lockCalendar and unlockCalendar to calendarManager, and call those.
Good idea
 
> Instead of calling getBranch 10 times, call it once and store the result.
That is not "my" function :) I can do that while I am at it, but I am not going
to test it
Forget about my the comment about possible implementation of the wait() function... 
about the X- properties: It falls in the category 'not perfect, but available
now and better then nothing'. But at least put a big comment about it in the code.
about wait(): You could always write a c++ xpcom interface :)
about file(): It is in the tree, but part of jslib, which is third-party. And
because we already use nsILocalFile in lots of other places, no need to depend
on jslib here.
shouldn't the interval get deleted if a calendar is removed?
> about the X- properties: It falls in the category 'not perfect

Well mostafah suggested to disergard them...

> about wait(): You could always write a c++ xpcom interface :)

I'll have to learn xpcom first... If you do a wait for me I'll do a wait for
you... ;)
(In reply to comment #23)
> shouldn't the interval get deleted if a calendar is removed?

yes but it will require a bit more code since I will need to store the tid for
each calendar, in fact now calendarObject properties are scattered all over the
place (most are accessed via the rdf file directly...) because calendarObject()
is not really used for runtime storage bu it is used only as a blank template...
Which is not very elegant... gTid[] will be yet another global spurious variable
laying around... I think a tide-up is badly needed... But that will touch
several fynctions... For a quick fix gTid[] will do...

Attached patch Diff file for calendarManager.js (obsolete) — Splinter Review
Implements mvl comments
Attachment #163613 - Attachment is obsolete: true
Attachment #163861 - Flags: second-review?(mvl)
Attachment #163861 - Flags: first-review?(mostafah)
Attachment #163613 - Flags: second-review?(mvl)
Attachment #163613 - Flags: first-review?(mostafah)
Sorry forgot a couple of ";" in retrieveAndSaveLocalCalendar (I hate ";") I do
not think is worth another attachment... 

var errorMsg = gCalendarBundle.getString( "unableToWrite" )  + CalendarFile.path
timeLock = LockFile.dateModified

ps ...mvl next one will not be using File()
Comment on attachment 163861 [details] [diff] [review]
Diff file for calendarManager.js

>+   //TODO remove autoreload, for now there is a workaround in reloadCalendar: if(...#active).... 
>    //ThisCalendarObject.setAttribute( "http://home.netscape.com/NC-rdf#active", "false" );
It is common to use //XXX
And I don't understand the comment at all. Remove what? there is no autoreload,
And i think it is not a todo, but might lead to crashes. The calendar will be
gona, and the timeout will acess it.

>+*/
>+//skipRefresh will reparse the file, without refreshing the view
Ehm, why would you want to do that? That will mean the view and the events in
memory are out of sync.

>+         if (calMan_reloadCalendar.arguments.length == 1 || skipRefresh != true){
i think that should be length==2&&skipRefresh. Test for what you really want.

>+calendarManager.prototype.retrieveAndSaveLocalCalendar = function calMan_retrieveAndSaveLocalCalendar( 
ThisCalendarObject, ThisCalendarEvent,functionToRun )
So you don't go for the seperate locking functions? why not?


>+      function compareEvents( Event1, Event2 ){

I don't see a comment here why the removing of all X- properties is safe.

>+      lockTimeOut = lockTimeOut + fileSize / 1000; //+1 sec per MB
>+      maxWaitTime = maxWaitTime + fileSize / 500; //+2 sec per MB

Add a max and a min.
If it is not the last patch, and you know, please don't ask for reviews yet.
I'll take care of removing autoreload

> Ehm, why would you want to do that? That will mean the view and the events in
> memory are out of sync.

Because I do parese + merge + parse. I do not need a refreshView after the first
parse it is a waste of time I only need it in the second parse. 

> i think that should be length==2&&skipRefresh. Test for what you really want.

When this condition is true there is nothing to do... I'd have to put the
refreshView in an else{}... It is less readable...

> So you don't go for the seperate locking functions? why not?

A) For a practical reason: at the moment to keep things in a single function and
in a single file (it makes it easier). B) I still have to think it through. I
think all add/edit/delete operations should be in calendarManager, probably it
is better to provide calendar.add calendar.edit calendar.delete methods... Which
in turn call equivalent gIcalLib methods and appropriate remote calendars
methods, thus avoiding retrieveAndSaveLocalCalendar and
retrieveAndSaveRemoteCalendar... The "extarnal world" should not care where and
how the calendars are saved... Now "knowledge" of the calendar storage is also
required in calendar.js... I do not think this is appropriate... That goes in
the calendarManager reorganization business...

> I don't see a comment here why the removing of all X- properties is safe.

Mostafah says it is... "I can't think of any of the X-MOZILLA settings which are
important and can't be discarded" 

> Add a max and a min.

What exactly is the min for? 
(In reply to comment #30)
> > i think that should be length==2&&skipRefresh. Test for what you really want.
> 
> When this condition is true there is nothing to do... I'd have to put the
> refreshView in an else{}... It is less readable...
Should be !skipRefresh. Sorry.

> > I don't see a comment here why the removing of all X- properties is safe.
> 
> Mostafah says it is... "I can't think of any of the X-MOZILLA settings which are
> important and can't be discarded" 
X- != X-MOZILLA-. If you want to remove the X-MOZILLA stuff, do just that.
And i still would very much like a comment in the code why it is ok.
> 
> > Add a max and a min.
> What exactly is the min for? 

To not fail if the file is currently empty. (or near-empty, and only wait a very
short time)
> Should be !skipRefresh. Sorry.
? That is what is already in the code... And #args == 1 is also correct.

> X- != X-MOZILLA-. 
X-MOZILLA- will be :)

> To not fail if the file is currently empty. (or near-empty, and only wait a
very short time)
What exactly would fail if the file is empty? In this case fileSize will be zero
and being the timeouts a linear relation of the file size, the original values
of lockTimeOut and maxWaitTime are the minima. You can change that if you want a
different minimum. As for the maximum if the filesize is several MB the lock
length are the least of your problems. The linear relationship is correct if
anything the value of the coefficient should be changed, I am not sure what are
reasonable values for the parameters.

= maxWaitTime Those are the variables used in the loop to check the lock, which
is always an empty file. If the calendar is empty or not has nothing to do with
the lock...

PS I would be grateful if you could make an XPCOM wait() function available.
(In reply to comment #32)
> the original values
> of lockTimeOut and maxWaitTime are the minima.
So there is a mimimum :) (Ok, my fault, i missed the + )

> PS I would be grateful if you could make an XPCOM wait() function available.

Thinking about that, it would be kind of wrong, because it will lock the UI.
That is bad. A better way would be to use a timeout. That would not lock the UI,
bit give a slightly more complicated code, but not too much.
Attached patch Diff file for calendarManager.js (obsolete) — Splinter Review
Attachment #163861 - Attachment is obsolete: true
Attached patch Diff file for calendarManager.js (obsolete) — Splinter Review
Attachment #164168 - Attachment is obsolete: true
Attachment #163861 - Flags: second-review?(mvl)
Attachment #163861 - Flags: first-review?(mostafah)
Attachment #164169 - Flags: second-review?(mvl)
Attachment #164169 - Flags: first-review?(mostafah)
Comment on attachment 164169 [details] [diff] [review]
Diff file for calendarManager.js

You need to set the autoreload timers when you add a new calendar, and not only
in the constructor. And also after editing a calendar. (in  the future, there
might be ui to set autoreload)

>+         if ( calMan_reloadCalendar.arguments.length == 1 || skipRefresh != true){
make that !skipRefresh

>+calendarManager.prototype.retrieveAndSaveLocalCalendar = function calMan_retrieveAndSaveLocalCalendar( ThisCalendarObject, ThisCalendarEvent,functionToRun )

You really need to work with setTimeout here, to not lock the entire app when
waiting for the lock.

>+            str = str.replace( /\nX-MOZILLA-.+\n\s\:.+/g, "" );

Comment why this is not a problem. At elast comment that you know what you are
doing

And you still use new File(), instead of nsILocalFile.

>+   //I think deleting calendar and file is a bad idea, particularly if the calendar is shared
>+   //TODO modify GUI and eliminate "Delete Calendar and File" button

Then fix the UI, instead of making the backend inconsisten with the UI. USers
won't understand why the button doesn't work.

>+      var Branch = prefService.getBranch( "calendar." );
Move that into the try{} block. It could fail.
>       try {
> You really need to work with setTimeout here, to not lock the entire app when
> waiting for the lock.

I disagree. I cannot think of a single app that let you save its documents
asynchronously... Having to wait when you save something is absolutely normal
behaviour... If someone else wants to change that, he is welcome...

> Comment why this is not a problem. At elast comment that you know what you are
> doing

I do not "really" understand why it is not a problem, mostafah says it is ok to
ignore X-MOZ settings, on my side it is an act of faith :) so I can add little
info in a comment... Mostafah is probably the best person to add a line there... 

> And you still use new File(), instead of nsILocalFile.

File() is already used throughout in calendarManajer.js, see for instance
calendarManager()... I will change to nsILocalFile but that should be consistent
across all objects/functions of all SB modules... It is not something that
changes the functionality of my patch in any way... It should instead be a
separate bug: let's kill/replace io.js... 

> Then fix the UI, instead of making the backend inconsisten with the UI. USers
> won't understand why the button doesn't work.

Well, I really believe "delete calendar and file" should go, and not only when
the calendar is shared... I have no problem in modifying the interface provided
other people agree... I mentioned that to mostafah in the chat but he did not
seemed too convinced... 

> Move that into the try{} block

There were already several lines using getBranch outside of the try block... See
prefService.getBranch( "calendar." ).clearUserPref()... The catch in try block
does not stop execution, so a problem with getBranch would have caused unhandled
errors with the original code... That is why I assumed that getBranch was a
"safe" operation...  Anyway I can change that... PS not that
getAndConvertAllOldCalendars has much to do with my patch...
(In reply to comment #37)
> I disagree. I cannot think of a single app that let you save its documents
> asynchronously... Having to wait when you save something is absolutely normal
> behaviour... If someone else wants to change that, he is welcome...

This isn't saving, as in after you press the 'save' button. This is after a
change. So pressing 'ok' will suddenly chew away all processortime doing nothing
but waiting.

> File() is already used throughout in calendarManajer.js, see for instance

That is not a reason to add more callers of it. Why create double work, when you
can do it right the first time?

> > Then fix the UI, instead of making the backend inconsisten with the UI. USers
> > won't understand why the button doesn't work.
> 
> Well, I really believe "delete calendar and file" should go, and not only when
> the calendar is shared... I have no problem in modifying the interface provided
> other people agree... I mentioned that to mostafah in the chat but he did not
> seemed too convinced... 

That's because the button should not go when it is a private file. I expect a
file that i do not share to go away when i say it should go away, in the same
way as i expect mailnews do delete a folder if i say so.
Butif you want the button to go away for shared files, make it so, instead of
leaving it and make it not function, without the user knowing that.
> This isn't saving, as in after you press the 'save' button. This is after a
> change. 

Every change is saved to file when you press ok... This is how things are
implemented. So you should wait after every change... If someone wants to
support asynchronous operations and a queue system he/she can be my guest... I
think that would not be appropriate and on top of that it would be ridiculously
complicated for the task at hand... I am not going to do it, I am happy with how
things work now.

> So pressing 'ok' will suddenly chew away all processortime doing nothing

If there is a lock... ...and in a typical application for small networks, it is
quite unlikely that two people save an event at the same time... And even when
that happens, the lock is in most cases resolved within a fraction of a second
(we are talking about LANs here), unless you really have huge calendar files, in
which case locks are the least of your problems... If someone wants to
contribute with an xpcom wait function to help with cpu cycles in such unlikely
circumstances I am more than happy to put it in... 

> That is not a reason to add more callers of it. Why create double work, when
you can do it right the first time?

Because I already wrote the patch using File() to begin with, I can reimplement,
but if a clean-up is needed (and I agree it is needed), the best place is a new
bug. One way could be to rewrite io.js and import it once at module level
instead of using QueryInterface every single time you need to play with a file,
but there might be other approaches (among which using QueryInterface), and I am
happy to implement them, once there is some agreement... My patch is not the
appropriate place for such discussion.  

> That's because the button should not go when it is a private file. 

I disagree on that. It is like an editor that says "Close file and delete it"...
No editor does that... Deletions should always be "complicated"... But if you
really like that option I will just disable the button only when the calendar is
shared...

> in the same way as i expect mailnews do delete a folder if i say so.

In fact mail folders are not deleted when you say so... In TB for instance they
are only moved to another folder...  
> Every change is saved to file when you press ok... This is how things are
> implemented. So you should wait after every change... 

Yes. It is an implemetation detail. The user should not warry about that!
(in fact, i think automatic publishing should be on a timer, and in the
background, so the user doesn't have to wait after every change. The timer would
batch multiple changes)

> I disagree on that. It is like an editor that says "Close file and delete it"...
> No editor does that... Deletions should always be "complicated"...

No. And which editor has profile with data that you edit in it? It is just
something different. Do you want the cookie manager to also only hide cookies
you delete? a texteditor edits external data. A calendar file can be internal data.
I thinkt he best thing would be to remove the change from this patch, and open a
new bug to discuss it.
> (in fact, i think automatic publishing should be on a timer, and in the
> background, so the user doesn't have to wait after every change. 

That requires syncing... That is your ground, but it is not what I am doing :)

> new bug to discuss it.

I agree

Comment on attachment 164169 [details] [diff] [review]
Diff file for calendarManager.js

If mvl gives review+ it doesn't need a second review and can be checked in.
Attachment #164169 - Flags: second-review?(mvl)
Attachment #164169 - Flags: first-review?(mvl)
Attachment #164169 - Flags: first-review?(mostafah)
(In reply to comment #41)
> That requires syncing... That is your ground, but it is not what I am doing :)
waiting 5 seconds before downloading/changing/uploading doesn't require syncing,
but o well.

I'll agree with the blocking wait for this reason: There is no UI to set the
shared attribute yet. This feature should be advertized as experimental with
(known) problems, and is only for advanced users. Normal users won't see the
blocking.
Also, all this is about to be rewritten using new interfaces and all anyway.
> waiting 5 seconds before downloading/changing/uploading doesn't require syncing,

It does if the interval is long enough to be able to edit multiple events before
saving... While the save task waits asynchronously I might edit a second event,
at the same time a separate client edits other two events... Now the only safe
way to "merge" all the four events is syncing... If you write events
immidiately, one by one, there are no possible conflicts to be solved, and the
more complex syncing algorithm is not needed... Without syncing, waiting is not
an option, when syncing is available we can do that. 
 
> This feature should be advertized as experimental with
> (known) problems, and is only for advanced users. Normal users won't see the
> blocking.

If the shared attribute is not set nothing really changes... As for the known
problems the only 2 issues I see are: A) wasted cpu cycles during a lock
situation, B) locks may potentially fail, it is unlikely but you'll at least get
an alert.

> Also, all this is about to be rewritten using new interfaces and all anyway.

I'd like to be in the trunk so when interface changes either it is clear that
the code must be adapted or it is clear that the functionality will have to be
replaced with different tools. 
PS I am going to change a couple of things, hold on... I want also to check if
the event was changed externally since the time the edit dialog was opened...  
Attached patch Diff file for calendar.js (obsolete) — Splinter Review
Attachment #162730 - Attachment is obsolete: true
Attachment #162733 - Attachment is obsolete: true
Attached patch Diff file for calendarManager.js (obsolete) — Splinter Review
Attachment #164169 - Attachment is obsolete: true
Attached patch Diff file for eventDialog.js (obsolete) — Splinter Review
Attached patch Diff file for toDoDialog.js (obsolete) — Splinter Review
Attachment #164169 - Flags: first-review?(mvl)
A few changes,

1) Moved the real code into calendar.js

2) Using nsILocalFile 

3) Added a new check to make sure that the event you edited/deleted wasn't
changed by someone else since the last reload. The check does not require any
additional parsing, but it needs to store the originalEvent (before edits) for
the comparison (which is the reason for the new 3 patched files). This is useful
in any sharing situation where there is no record/table-level centralised
locking (so that the lock starts as soon as you begin to edit a record).

4) Made sharing items compatible also with ToDos

5) Unified deleteToDoCommand, deleteEventCommand, made them compatible with
sharing and fixed a few bugs in there. Now it is easier for instance to have
tasks in the event list... Also there were inconsistencies with how
publishEventAutomatically was handled (upload only when delteting, but download
+ upload when editing/adding...), and deleteToDoCommand required several
downloads/uploads for the same calendar.... All this is fixed.
Attachment #166447 - Flags: first-review?(mostafah)
Attached patch Diff file for calendar.js (obsolete) — Splinter Review
Attachment #166447 - Attachment is obsolete: true
Comment on attachment 166447 [details] [diff] [review]
Diff file for calendar.js

mvl is handling the reviews for this bug.
Attachment #166447 - Flags: first-review?(mostafah)
Attachment #166457 - Flags: first-review?(mvl)
Attached patch Diff for all files (obsolete) — Splinter Review
Comment on attachment 166474 [details] [diff] [review]
Diff for all files

>diff -u6Npr calendar.orig/content/calendar/calendar.js calendar/content/calendar/calendar.js
> function isEvent ( aObject )
> {
>+   //NOTE: it returns true even if aObject is a ToDo...
>    return aObject instanceof Components.interfaces.oeIICalEvent;
> }
You could fix that by also checking for !isToDo

>+function compareItems( aObject1, aObject2 ){
>+   if ( aObject1 == null || aObject2 == null){
General comment: stick to the indenting/whitespace style that is used in the
rest of the file.

>+   try{ //workaround Bug 270644
>+      return (aObject1.lastModified == aObject2.lastModified);
What is the workaround? The try or the lastModified check? I prefer comments on
the line before the actual code. (that's a general comment)
And why is lastModifed ok to check for changes? It assumes the other client
actually updates it etc.


>-function refreshRemoteCalendarAndRunFunction( calendarEvent, Server, functionToRun )
>+//originalEvent is the item before edits were committed, used to check if there were external changes for shared calendar
Try to fit within 80chars width

>+function refreshRemoteCalendarAndRunFunction( calendarEvent, Server, functionToRun, originalEvent )
I found this name misleading. I thought it was actually about remote calendars,
but you say it isn't.

>-function deleteEventCommand( DoNotConfirm )
>+function deleteEventCommand( DoNotConfirm, deleteToDo )
Now this also deletes todos? I prefer a function deleteItems(), that takes an
array of either events or todos, and leave the stuff that actually figures out
the selected events in the old functions.
>+   //process each calendarServer
>+   for (calendarId in calendarsToPublish){
>+   }
Doesn't that loop do about the same as refreshRemoteCalendarAndRunFunction().
Is it really different in functionality? Can we somehow join those?

>--- calendar.orig/content/calendar/calendarManager.js	Fri Nov 12 12:22:24 2004
>+var gAutoreloadTid = new Array();
Tid? What is a Tid? Please expand


>+calendarManager.prototype.reloadCalendar = function calMan_reloadCalendar( ThisCalendarObject, skipRefresh )
>+{
>+         if ( calMan_reloadCalendar.arguments.length == 1 || !skipRefresh ){
Stick the check for number of arguments in the beginning of the function, will
keep this more readable.

> var gOnOkFunction;   // function to be called when user clicks OK
>+var gOriginalEvent;  //Event before edits
Example of inconsitent whitespace, which makes it look ugly.

>+++ calendar/content/calendar/jslib/io/file.js	Thu Nov 18 13:53:33 2004
>-    rv = this.mFileInst.create(JS_FILE_FILE_TYPE, JS_FILE_DEFAULT_PERMS); 
>+    rv = this.mFileInst.create(JS_FILE_FILE_TYPE, JS_FILE_DEFAULT_PERMS); 
huh? what changed here?
>-    rv = this.mFileInst.remove(false); 
>+    rv = this.mFileInst.remove(false); 
and here?
Comment on attachment 166474 [details] [diff] [review]
Diff for all files

Besides those comments, this patch looks fine as a start. It is still
experimental. We can't really advertise real sharing yet, because it entirely
trusts the other clients to be nice too. Apple ical won't obey the locks.
Also the user interaction in case of conflicts isn't ok yet. Just deleting the
users data isn't a nice thing to do.
Attached patch Diff for all files (obsolete) — Splinter Review
Attachment #166448 - Attachment is obsolete: true
Attachment #166449 - Attachment is obsolete: true
Attachment #166450 - Attachment is obsolete: true
Attachment #166451 - Attachment is obsolete: true
Attachment #166457 - Attachment is obsolete: true
Attachment #166474 - Attachment is obsolete: true
With all the current work on this, I think it deserves to be confirmed. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Diff for all files (obsolete) — Splinter Review
Attachment #166506 - Attachment is obsolete: true
Comment on attachment 166777 [details] [diff] [review]
Diff for all files

>+function compareItems( aObject1, aObject2 ){
You persits on using the wrong whitespace/bracing style. 
>+   if ( aObject1 == null || aObject2 == null){
>+      alert((aObject1 == null) +":" +(aObject2 == null))
That's a pretty cryptic alert. the user will not have the slightest idea what
it means if he sees "true:false"
(And it should use nsIPromptService, but calendar is already full of alert()
and prompt())


> function openEventDialog(calendarEvent, mode, onOk, server)
> {
>+  
ditch that blank line

>+function saveItem( calendarEvent, Server, functionToRun, originalEvent )
>+               if (!compareItems( uneditedEvent, originalEvent )){ //event edited externally
Move that comment on a sepate line. (same in other places)
>+                  alert(gCalendarBundle.getString("concurrentEdit"));
>+                  return;
>+         }
>+            }
Funky indenting.
>+         }
>+         gCalendarWindow.clearSelectedEvent( calendarEvent );
>+               }
again.

>+      if ( SelectedItems.length > 1 ){
>                confirmText = confirmDeleteAllEvents;
And again. and the next 15 or so lines
>+            if(isToDo(selectedItem)){
>+               gICalLib.deleteTodo( selectedItem.id );
>+            }else{
>+               gICalLib.deleteEvent( selectedItem.id );
>+            }
>             }
>             catch (ex) {
>-               dump("*** deleteEventCommand failed: "+ ex + "\n");
>+            dump("*** deleteItems failed: "+ ex + "\n");
>             }
And once more
>-  <command id="delete_command_no_confirm" oncommand="deleteEventCommand( true )" disabled="true" disabledwhennoeventsselected="true"/>
>+  <command id="delete_command" oncommand="deleteEvents( )" disabled="true" disabledwhennoeventsselected="true"/>
Why did you rename the function? It's inconsistent now
>Index: calendar/resources/content/calendarManager.js
>+var gAutoreloadTid = new Array(); //stores Timer Id for each autoreload
repeat mode: what is a Tid?
>+calendarManager.prototype.reloadCalendar = function calMan_reloadCalendar( ThisCalendarObject, skipRefresh )
> {
///
>+         if ( calMan_reloadCalendar.arguments.length == 1 || !skipRefresh ){
repeat mode: init skipRefresh at the beginning of the funtion if the length is
1.
>   refreshView();
indenting


Fix those nits, or defend them, and r=mvl
Attachment #166777 - Flags: first-review+
Attachment #166777 - Attachment is obsolete: true
patch (with some minor changes) checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Getting strict javascript warning from code patched here

Warning: assignment to undeclared variable autoreload
Source File: chrome://calendar/content/calendarManager.js
Line: 130
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #167598 - Flags: first-review?(mvl)
Attachment #167598 - Flags: first-review?(mvl) → first-review+
patch checked in
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Attachment #166457 - Flags: first-review?(mvl)
The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!
QA Contact: gurganbl → general
You need to log in before you can comment on or make changes to this bug.