Closed
Bug 418854
Opened 16 years ago
Closed 16 years ago
Undo is not possible for task/event modification
Categories
(Calendar :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: thetux.moz, Assigned: dbo)
Details
(Keywords: dataloss, regression)
Attachments
(2 files)
2.55 KB,
patch
|
cmtalbert
:
review-
|
Details | Diff | Splinter Review |
5.96 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
Steps to Reproduce : ==================== 1) Create an event or task 2) Modify this event/task 3) Click on "Edit | Undo" Actual Results: =============== Undo does not work. Expected Results: ================= It should be possible to undo every modification. Build Date & Platform: ====================== Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13pre) Gecko/20080220 Calendar/0.8pre 20080220
Flags: blocking-calendar0.8?
Comment 1•16 years ago
|
||
Regression range: Works in Lightning 0.8pre (2007-12-15-03) Fails in Lightning 0.8pre (2007-12-16-04) Checkins during regression range: http://tinyurl.com/3x7qmy Bug 408237, Bug 407574, Bug 405418 Could this be caused by the fix for Bug 408237?
Keywords: regression
Comment 2•16 years ago
|
||
Confirming on mac as well. Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13pre) Gecko/20080220 Calendar/0.8pre
Hardware: PC → All
Comment 3•16 years ago
|
||
Confirmed as regession from Bug 408237, removing the change from Bug 408237 makes Undo work again. Or maybe Bug 408237 only revealed a new error. The Undo command calls modifyItem() that exits the method early after comparing the generation attribute, e.g. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/calendar/providers/storage/calStorageCalendar.js&rev=1.132&mark=517#515 or http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/calendar/providers/memory/calMemoryCalendar.js&rev=1.67&mark=241#234 Unfortunately the errors reported by reportError() show up nowhere.
Comment 4•16 years ago
|
||
This needs to be fixed before we release 0.8
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Comment 5•16 years ago
|
||
Daniel, do you think temporary reverting Bug 408237 might be a solution for now?
Comment 6•16 years ago
|
||
(In reply to comment #5) > Daniel, do you think temporary reverting Bug 408237 might be a solution for > now? Stefan, Daniel is away on vacation until the beginning of March. I'd say we backout the patch for bug 408237 temporarily for the release and fix this correctly after 0.8 is out of the door. Philipp, Clint, what do you think?
Comment 7•16 years ago
|
||
I think we should just fix this issue. Temporary fixes are always a bad plan. Especially when the temporary fixes checks in obviously wrong code. If you really want to fix the 'temporary', just remove the check at the links in comment 3 and add a bunch of code comments. Don't put the broken code back in.
Comment 8•16 years ago
|
||
The question for really fixing this bug is: what should happen with the generation number on undo? Should the complete old item be put back, including the generation, or should the generation be increased? The current transaction code assumes the former (by not doing anything with generation), while the providers assume the latter (by making sure the generation always increases). One of them has to be changed. I think that the generation should be increased on undo, and a check should be made that the old generation is exactly one less then the currently stored generation. I'm not sure though where this should happen, mainly due to the possibility of redo. For redo, the same should happen, except for the first 'do'. But the first 'do' goes through the same codepath as 'redo'...
Comment 9•16 years ago
|
||
Guys, we need to get some traction on this. Either we go one of the routes that mvl is discussing in comment 8 *right now* or we backout the patch from bug 408237 now, reapply it after 0.8 is out and fix the bug early in the 0.9 timeframe. Can you please comment on this issue ASAP, so that we have a decision by tomorrow (Monday, 3rd).
Reporter | ||
Comment 10•16 years ago
|
||
Taking correct code out of the programm is a bad idea (and a waste of resources). Since we have limited developer resources it is even more worse.
Comment 11•16 years ago
|
||
As we stated in our last status meeting from February 27th we want to remove this from the blocking list. Maybe Daniel can take care of it when he is back from holiday next week.
Flags: wanted-calendar0.8?
Flags: blocking-calendar0.8-
Flags: blocking-calendar0.8+
Comment 12•16 years ago
|
||
Wow, that a real bad decision. Sorry to be so hard. But undo is a mayor feature and very, very important. (Not having undo is actually a dataloss bug.) You can't just leave it broken. No discussion really possible.
Flags: blocking-calendar0.8- → blocking-calendar0.8?
Comment 13•16 years ago
|
||
Well, of course this needs to be fixed before the release. IMO the question only is, how we fix it. Do we - go the temporary fix road by backing out the patch from bug 408237 now, reapplying it after 0.8 is out and fixing the bug for real early in the 0.9 timeframe? - do we fix the bug right now by going one of the routes that mvl proposed in comment 8, having in mind that this might turn up other serious regressions very late in the game? From a process perspective, I would favor option 2. From a risk perspective, option 1 is clearly better. Therefore I would propose to choose option 1.
Comment 14•16 years ago
|
||
I backed out the patch of bug 408237 and gave this Andreas to test. He found that Undo-Redo worked again without complaints. So I will check this in.
Comment 15•16 years ago
|
||
Please, NO!!! You are putting in code with known problems that might very well cause other regressions. I urge you to consider one of the other fixes I proposed earlier. (And I think that the real fix won't be that hard, if you spend a few minutes thinking about how to do it exactly)
Comment 16•16 years ago
|
||
Yes, this blocks the final 0.8 release. Also per the confcall decision, backing out Daniel's patch will break iTip. We will release the RC1 without this, but we should really get this fixed for the final.
Flags: wanted-calendar0.8?
Flags: blocking-calendar0.8?
Flags: blocking-calendar0.8+
Comment 17•16 years ago
|
||
A possible real solution: - Return the modified items from the providers - Store that return item, instead of the original new item (because it is not the same: the generation changed) - in the transaction manager, copy the generation of the old item to the new item before do and undo I _think_ that this should work. didn't do any testing yet.
Comment 18•16 years ago
|
||
mvl, it would be really great, if you could hook up a first WIP patch for your proposal. Then Philipp or Daniel (when he returns next week from vacation) can take a look at it and we can ask Andreas to test it thoroughly.
Comment 19•16 years ago
|
||
This way you will save some time to avoid regressions that may turn up.
Comment 20•16 years ago
|
||
a work-in-progress patch. It does what I described in comment 17. (the first two items are already implemented). This patch makes undo work for me, at least with storage calendars. There is still a problem though: the generation decreases when you do multiple undos. Older transactions still store the generation as it was at the time of executing them. But at undo, that's no longer the current generation. Undo still works, because of the storage provider only comparing the oldItem as passed in, not as what's really currently stored. I don't know what other providers do.
Comment 21•16 years ago
|
||
Comment on attachment 307550 [details] [diff] [review] WIP patch Although this patch has known problems, it does improve the situation. If the problems with Itip are not too big, I think we should go for this for now.
Attachment #307550 -
Flags: review?(philipp)
Comment 22•16 years ago
|
||
Comment on attachment 307550 [details] [diff] [review] WIP patch Since I was not on the confcall, I don't have an idea how this could break itip. Maybe Clint can bring more light into this? From a code standpoint, I'd like to see the XXX comment a bit more elaborate and if possible with a bug reference. Just looking at code, r+ with the above taken care.
Attachment #307550 -
Flags: review?(philipp) → review?(ctalbert)
Assignee | ||
Comment 23•16 years ago
|
||
The WIP patch doesn't work for me when performing multiple undos on the same item (ics file). I can imagine this is due to the independent transaction objects, e.g. if one transaction is undone, it updates its own items (implementing an calIOperationListener), but leaves the prior and next transaction objects untouched (one transaction object's mItem is the next transaction object's mOldItem...). Moreover I spotted that calMemoryCalendar's deleteItem() always checks against the generation of it's stored item, I think this is another problem.
Assignee | ||
Comment 24•16 years ago
|
||
My understanding of the generation attribute has always been that it serves as a version number when modifying an item, somehow similar to the SEQUENCE number. Quite helpful due to an item's value semantics, to check whether a modification is performed on that latest version of an item. The only feasible solution I can imagine (keeping the generation for that purpose) is to also allow generation downgrades, thus related transaction objects stick to their original generation. For the purpose of version safety, it's IMO sufficient to check only the oldItem's generation against the stored one of an item. So this patch modifies - storage/memory to act like that; and increment only in case of a direct modification (cloned oldItem of same generation) - removes caldav's check: IMO not needed since etag checking is performend in caase of modifyItem() (neither wcap nor gdata implement generation) Rudimental tests work well for me for storage and ics.
Assignee | ||
Comment 25•16 years ago
|
||
Looking further on the transaction code, I can think of race conditions due the async nature of some providers: Even if we could update related items of other transaction objects, multiple transactions would need to be queued until a store has been performed (onOperationComplete).
Comment 26•16 years ago
|
||
Comment on attachment 307550 [details] [diff] [review] WIP patch From comment 23, it looks like this doesn't completely solve the problem so we're going to r- on it. Thanks for the patch.
Attachment #307550 -
Flags: review?(ctalbert) → review-
Assignee | ||
Comment 27•16 years ago
|
||
mvl, do you have cycles left for this or should I pick someone else for review?
Comment 28•16 years ago
|
||
Comment on attachment 308719 [details] [diff] [review] allow generation downgrades Looks good, r=philipp
Attachment #308719 -
Flags: review?(mvl) → review+
Assignee | ||
Comment 29•16 years ago
|
||
Checked in on HEAD, MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Comment 30•16 years ago
|
||
Checked with Sunbird and Lightning builds 20080330 -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•