Hi, thanks for your patch. Please, see my comments inline. Dne 9. dubna 2012 21:35 hjliang <[email protected]> napsal(a): > Hi, > > I have introduce undo/redo function to edit program. > ctrl+z for undo and ctrl+y for redo. > I have tested on qemu, and it seems work. It works for me too but I have a few comments/questions to the code.
First of all, I appreciate that you stick with our coding style and documented (however briefly) new functions you added. If you realize that you forget to add some files to Bazaar and you haven't pushed the branch, it is better to uncommit the last change and add the files. It is better if every revision is at least compilable. I wonder why you decided to limit number of undo operations. You are using lists elsewhere, so why not here? This also leads to another question - in undo_stack_push(), do I understand it right that when the undo stack is full, you discard the last change and replace it with the newest? I like how you created generic listeners for the undo/redo actions. Is there a reason why they weren't merged into one type? After all, the sheet_delete_para_t and sheet_insert_para_t do not differ that much and it would save-up some coding (such as iteration over the listeners). In undo_stack_pop you set the listen_mask to 0 and then you re-set it to both INS and DEL. It would be safer to save the old value and restore it in case this function was called with listen_mask already modified. In my humble opinion, I do not think that it is necessary to create separate type sheet_insert_listener_link_t just to add link_t. Many other structures in HelenOS have link_t as a member to simplify usage within lists. To sum it up: your patch is rather long and thus there were more things to review and comment on. In general, I like it. I might do some modifications prior merging but this will find its way into mainline. Thank you very much. > Besides,I'm wondering how you test a program.Do you use a sophisticated > method? No. Very often, the testing means just starting Qemu and manually testing it in there. However, there is a project for automatic continuous integration testing [1]. Cheers. - Vojta [1] http://trac.helenos.org/ticket/428 > > > > _______________________________________________ > HelenOS-devel mailing list > [email protected] > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel > _______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
