On 10/19/07, Sara Su <sarasu@...1814...> wrote:

On Fri, 19 Oct 2007, Daniel Genrich wrote:

...

I just took some time to make the "persistent undo" patch (http://people.csail.mit.edu/sarasu/history/) working with todays SVN and to make it also compile with gcc 4.1.2.

Awesome - thank you for taking the time to do this, Genscher.

I've looked superficially at the patch and found some problems:

- please make sure new files bear your copyright, not that of David Yip (or if you're adding to his code, add your name as well)

- you're commenting out parts of code in jabber_whiteboard - why? if you think these parts are unnecessary, just delete them, not comment out. Otherwise leave them alone and make sure persistent undo works with whiteboard and they do not conflict.

- we use #defines in the first lines of all cpp and h files to make sure a file is not compiled/included twice, please add the same thing to your new files

- please remove all debug output

- most importantly, I don't really like this:

// SLS to enable persistent history, we just simulate deletion +void sp_selection_delete_impl(const GSList *items, bool propagate=true, bool propagate_descendants=true) +{ + for (const GSList *i = items ; i ; i = i->next ) { + SPItem *item = (SPItem *) i->data; + item->setHidden(true); + } +}

Preventing deleting of objects is not really a good idea IMHO. If you so this so that the deleted objects can be restored, then the regular in-session undo can do this already, why your persistent undo cannot? I think what you need to do is just serialized the deleted objects (as XML) into your saved history and take them from there when restoring. This will be much cleaner and more consistent.

Thanks everyone for helping incorporate this in the project. As I mentioned to Ted, I feel I should disclose that I've just started an internship at Adobe, where my co-advisor Sylvain has moved. My understanding is there should not be a conflict and I hope this won't affect what we can discuss; however, I will understand if anyone feels differently.

It's not a problem unless at Adobe you will work on something very similar to what you're doing for Inkscape. We already had a contributor from a Corel intern, so don't worry :)