On Thu, 21 Aug 2008, Frank Niessink wrote: > 2008/8/21 Jerome Laheurte <[EMAIL PROTECTED]>: >> So why not use the standard library 'copy' module, which does just >> that, AFAIK ? > > Yeah, that was one option crossing my mind. Then we can implement > __copy__ and __deepcopy__ as needed. > >> Mmmh, I'm not keen on adding an extra keyword argument to a special >> method used by the standard library (I think __getstate__ and >> __setstate__ are used by both copy and pickle). There's something that >> bugs me there: a method that is supposed to return a state, >> independent from any application-specific class, actually returns >> something that include instances of domain objects. I think that >> instead of including a list of children copies, it should include a >> list of children states. As I said earlier, this will need special >> handling for deletion/creation of children, but I think it's worth it. > > Well, __getstate__/__setstate__ are historical accidents. I think I > used the pickle module when I started with TC. We can rename these > methods any way we want. > > Although I agree with your dislike of a method called getstate that > returns a mixture of state and objects, I am not sure that that is the > root issue. I've been thinking for a while now that the whole state > copying is a bad thing. We need to copy the state because of the edit > dialogs. With the current edit dialogs, we don't know what the user is > going to change and she may basically change any property of an > object, including the addition or removal of a whole tree of sub > objects. That's why we need to copy the whole state of domain objects. > Now, if the user cannot edit the whole state of an object, but only > one property at a time, we don't need to copy state. For example, the > change task priority commands don't save state, do and undo are really > each other's inverse operation (as do and undo are meant to be :-):
Actually, the copy stuff works well for 'real' composite objects, when children are actually 'part' of the parent in a 1:n relationship, which is not the case with categories. For notes belonging to a task, it doesn't cause any problem. Though the fact that the __id is changed bugs me (it will cause problems with the synchronization framework, for instance). Anyway, there is a way to fix all this, as well as other issues: let's keep the state saving stuff in SaveStateMixin, except for composite children and other subitems. These should be handled in a different way. I thought about changing the CommandHistory into a stack of CommandHistory objects. When a command is done/undone/redone, it affects the top of the stack. But when entering an item edition dialog, we push an empty CommandHistory on top of the stack. All item creation, deletion, change etc done from the dialog go into this new CommandHistory. When the user exits the dialog, pop the CommandHistory. If he cancelled, just undo() all commands in it. If not, add a special "aggregate" command to the top of the stack, holding a copy of the popped CommandHistory. undo_command and redo_command would loop through all commands in it in the right order and call the corresponding method. Of course, in case of nested edit dialogs, this CommandHistory may contain itself other aggregates commands, etc. Thus: * The bug is fixed * We can provide a 'local' undo/redo mechanism in the editor dialog * We can add functionnality (for instance, edition of categories from within the task/note editor) and it will just work (nested dialogs scenario described earlier) * We can factorize some code by using, for instance, an actual CategoryViewer in the task/note/... editor. There are probably some details to work out, but I think this should work quite well. Cheers Jérôme
