Closed
Bug 265274
Opened 20 years ago
Closed 20 years ago
Shared calendar for small LANs using flat-files
Categories
(Calendar :: General, enhancement)
Calendar
General
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
|
mvl
:
first-review+
|
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.
Only a couple of lines changed within refreshRemoteCalendarAndRunFunction to call retrieveAndSaveLocalCalendar
Added two new attributes to CalendarObject(), created a new method retrieveAndSaveLocalCalendar(), and modified addCalendar() and reloadCalendar()
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
Updated•20 years ago
|
Attachment #162734 -
Attachment is patch: true
Comment 6•20 years ago
|
||
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 :)
Comment 9•20 years ago
|
||
(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.
Reporter | ||
Comment 10•20 years ago
|
||
I did so not to use a callback which I do not particularly like (I think it makes the code harder to read).
Updated•20 years ago
|
Attachment #162733 -
Attachment is patch: true
Comment 11•20 years ago
|
||
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)
Reporter | ||
Comment 12•20 years ago
|
||
Hmmm I could compare the edited event to the one in the newly reloaded calendar...
Reporter | ||
Comment 13•20 years ago
|
||
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 14•20 years ago
|
||
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
Comment 15•20 years ago
|
||
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.
Reporter | ||
Comment 16•20 years ago
|
||
(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.
Reporter | ||
Comment 17•20 years ago
|
||
> 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
Comment 18•20 years ago
|
||
(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()
Assignee | ||
Comment 19•20 years ago
|
||
(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.
Reporter | ||
Comment 20•20 years ago
|
||
> 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
Reporter | ||
Comment 21•20 years ago
|
||
Forget about my the comment about possible implementation of the wait() function...
Comment 22•20 years ago
|
||
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.
Comment 23•20 years ago
|
||
shouldn't the interval get deleted if a calendar is removed?
Reporter | ||
Comment 24•20 years ago
|
||
> 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... ;)
Reporter | ||
Comment 25•20 years ago
|
||
(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...
Reporter | ||
Comment 26•20 years ago
|
||
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)
Reporter | ||
Comment 27•20 years ago
|
||
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 28•20 years ago
|
||
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.
Comment 29•20 years ago
|
||
If it is not the last patch, and you know, please don't ask for reviews yet.
Reporter | ||
Comment 30•20 years ago
|
||
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?
Comment 31•20 years ago
|
||
(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)
Reporter | ||
Comment 32•20 years ago
|
||
> 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.
Comment 33•20 years ago
|
||
(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.
Reporter | ||
Comment 34•20 years ago
|
||
Attachment #163861 -
Attachment is obsolete: true
Reporter | ||
Comment 35•20 years ago
|
||
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 36•20 years ago
|
||
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 {
Reporter | ||
Comment 37•20 years ago
|
||
> 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...
Comment 38•20 years ago
|
||
(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.
Reporter | ||
Comment 39•20 years ago
|
||
> 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...
Comment 40•20 years ago
|
||
> 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.
Reporter | ||
Comment 41•20 years ago
|
||
> (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
Assignee | ||
Comment 42•20 years ago
|
||
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)
Comment 43•20 years ago
|
||
(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.
Reporter | ||
Comment 44•20 years ago
|
||
> 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.
Reporter | ||
Comment 45•20 years ago
|
||
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...
Reporter | ||
Comment 46•20 years ago
|
||
Attachment #162730 -
Attachment is obsolete: true
Attachment #162733 -
Attachment is obsolete: true
Reporter | ||
Comment 47•20 years ago
|
||
Reporter | ||
Comment 48•20 years ago
|
||
Attachment #164169 -
Attachment is obsolete: true
Reporter | ||
Comment 49•20 years ago
|
||
Reporter | ||
Comment 50•20 years ago
|
||
Attachment #164169 -
Flags: first-review?(mvl)
Reporter | ||
Comment 51•20 years ago
|
||
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)
Reporter | ||
Comment 52•20 years ago
|
||
Attachment #166447 -
Attachment is obsolete: true
Assignee | ||
Comment 53•20 years ago
|
||
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)
Reporter | ||
Comment 54•20 years ago
|
||
Comment 55•20 years ago
|
||
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 56•20 years ago
|
||
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.
Reporter | ||
Comment 57•20 years ago
|
||
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
Comment 58•20 years ago
|
||
With all the current work on this, I think it deserves to be confirmed. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 59•20 years ago
|
||
Attachment #166506 -
Attachment is obsolete: true
Comment 60•20 years ago
|
||
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+
Reporter | ||
Comment 61•20 years ago
|
||
Attachment #166777 -
Attachment is obsolete: true
Comment 62•20 years ago
|
||
patch (with some minor changes) checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 63•20 years ago
|
||
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 → ---
Comment 64•20 years ago
|
||
Attachment #167598 -
Flags: first-review?(mvl)
Updated•20 years ago
|
Attachment #167598 -
Flags: first-review?(mvl) → first-review+
Comment 65•20 years ago
|
||
patch checked in
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Attachment #166457 -
Flags: first-review?(mvl)
Comment 66•18 years ago
|
||
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.
Description
•