On Sun, 16 Dec 2012 22:21:26 +1100 Alexander Klenin <[email protected]> wrote:
>[...] > We had quite a lot of discussion with him about the implementation method. > I favored storing a series of snapshots too, while he preferred a > "list of reversible actions" design. > > Main arguments were: storing snapshots is much simpler to implement > and more reliable, > while using a list of actions requires less memory and is similar to > what SynEdit does, so should > integrate better. > (I am of the opinion that the IDE should have a common undo history > for form designer and code editor). Some operations have side effects and there is no simple reverse operation. These are often the ones you want to undo. E.g. deleting a referenced component. Especially if it was a component referenced by other forms. Combining the undo with the code editor would be nice, but I don't know how to solve it when there are multiple source editors involved. > He went on and implemented his approach as a proof of concept, and > after several iterations > the code is working for some cases, but the complexity is indeed rather high. > We have now reached a point where expert opinion is needed: is > Alexander's approach viable, > or should he abandon it and start from scratch? > > Also, please give the opinion on the code quality and architecture of > his patch -- > he is nearing mid-term evaluation :) Well, the requirements of your course and the preferences of some guy of an open source project are two different shoes. That being said, I can name a few things where I see problems: - public variables should not be named FUndoState. The F is commonly used for private variables. - Instead of 0,1,2,3 for Left, Top, Width, Height an enum should be defined. - TUndoList is logically a TUndoListItem. - FForm.FindComponent does not find components on frames. - The procedure SetPropVal checks for specific properties (e.g. 'TColor'). It must not. - FloaToStr/StrToFloat looses precision. - Randomize should only be called once in the whole IDE. - Use increasing numbers instead of random numbers. Mattias -- _______________________________________________ Lazarus mailing list [email protected] http://lists.lazarus.freepascal.org/mailman/listinfo/lazarus
